[libvirt] [PATCH v2 2/6] openvz: Resolve Coverity FORWARD_NULL

[John Ferlan]

On 10/07/2015 02:14 AM, Peter Krempa wrote:
> On Fri, Sep 25, 2015 at 12:31:41 -0400, John Ferlan wrote:
>> Coverity complains that strtok_r cannot be called if the first and third
>> parameters are NULL. On entry if the 'value' parameter is not checked for
>> being NULL before the VIR_STRDUP which will set the target to NULL if
>> the source (eg value) is NULL.  Thus add a check for NULL value.
>>
>> NB: Although each of the callers to openvzParseBarrierLimit would only
>> make the call when 'value' was non-NULL, it seems that doesn't satisfy
>> the checker. The NULL check had to be in this funtion.
> 
> Erm, so then file a bug against coverity rather than hacking around it?
> 

Even doing that will take "time" for them to fix and even when fixed
doesn't mean it works in our build environment.

Besides I didn't have success in generating a "small reproducer".

The last time I logged a bug against Coverity was with the VIR_FREE()
ternary operation. That was "fixed"; however, if I remove the change
from the code and build, it seems for 95% of the cases the claim of
RESOURCE_LEAK has gone away, but a few of these type FORWARD_NULL's
now appear. My guess is the ternary in strtok_r is what's causing the
issue. That guess is based on experience of looking at Coverity issues
from trace around the issue:

(5) Event cond_true: 	Condition "1", taking true branch
(6) Event cond_true: 	Condition "(size_t)(void const *)&":"[1] - (size_t)(void const *)":" == 1", taking true branch
(7) Event cond_true: 	Condition "(char const *)":"[0] != 0", taking true branch
(8) Event cond_true: 	Condition "(char const *)":"[1] == 0", taking true branch
(9) Event var_deref_model: 	Passing "&saveptr" to "__strtok_r_1c", which dereferences null "saveptr". [details]

and the "details" for the error that Coverity links one to, where events
(5) - (8) seems to come from:

1137 	#if !defined _HAVE_STRING_ARCH_strtok_r || defined _FORCE_INLINES
1138 	# ifndef _HAVE_STRING_ARCH_strtok_r
1139 	#  define __strtok_r(s, sep, nextp) \
1140 	  (__extension__ (__builtin_constant_p (sep) && __string2_1bptr_p (sep)	      \
1141 			  && ((const char *) (sep))[0] != '\0'			      \
1142 			  && ((const char *) (sep))[1] == '\0'			      \
1143 			  ? __strtok_r_1c (s, ((const char *) (sep))[0], nextp)       \
1144 			  : __strtok_r (s, sep, nextp)))
1145 	# endif
1146 	
1147 	__STRING_INLINE char *__strtok_r_1c (char *__s, char __sep, char **__nextp);
1148 	__STRING_INLINE char *
1149 	__strtok_r_1c (char *__s, char __sep, char **__nextp)
...

Remember that Coverity tries "every" path... I'm still not sure why
some strtok_r calls are "ok" while others aren't. I believe it mostly
has to do with where it's done. Perhaps virStringSplit* type calls
would also solve these...

>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/openvz/openvz_conf.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
>> index fb8768a..e99b83c 100644
>> --- a/src/openvz/openvz_conf.c
>> +++ b/src/openvz/openvz_conf.c
>> @@ -135,10 +135,10 @@ openvzParseBarrierLimit(const char* value,
>>  {
>>      char *token;
>>      char *saveptr = NULL;
>> -    char *str;
>> +    char *str = NULL;
>>      int ret = -1;
>>  
>> -    if (VIR_STRDUP(str, value) < 0)
>> +    if (!value || VIR_STRDUP(str, value) < 0)
> 
> VIR_STRDUP returns 0 if @value was NULL so you can as well as test for <
> 1. Since then the value gets initialized to NULL you don't need the
> first hunk.
> 

If I change to :

    if (VIR_STRDUP(str, value) <= 0)

it doesn't help - so it's something about checking !value that keeps Coverity quiet.
That !value check also must be done in the same function as the strtok_r,
so that lends even more credence to my belief about the ternary.

>>          goto error;
>>  
>>      token = strtok_r(str, ":", &saveptr);
>> @@ -396,7 +396,7 @@ openvzReadFSConf(virDomainDefPtr def,
>>      param = "DISKSPACE";
>>      ret = openvzReadVPSConfigParam(veid, param, &temp);
>>      if (ret > 0) {
>> -        if (openvzParseBarrierLimit(temp, &barrier, &limit)) {
>> +        if (openvzParseBarrierLimit(temp, &barrier, &limit) < 0) {
>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>>                             _("Could not read '%s' from config for container %d"),
>>                             param, veid);
> 
> This change should be mentioned in the commit message since it's
> different from what the current message says.
> 

I forgot about this line... Not that it will matter as I'll just abandon this
change and just filter it in my local build.



John