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

[Peter Krempa]

On Wed, Oct 07, 2015 at 09:56:54 -0400, John Ferlan wrote:
> 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.

Well, that seems to be the problem with non-free products.

> 
> 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:

So a commercial product (not that free-ness of it would matter) is
telling us that our code is wrong, which isn't true. If that same
software would actually insist on adding a bug I don't think we should
act on it's behalf. Yes .. that's a speculation, but the point is that
even if you do have access to a tool like that it should help and not
create just additional work.

If we change our code just to shut it up it's not really adding any
value by using it.

Also if somebody sent a patch like this without mentioning any static
analysis the first question that most reviewers would ask is why it
actually is necessary and how does it help. I doubt we'd accept anything
like that.

> (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...

Probably yes. I also would not oppose such change as it actually has
some value in comparison with workarounds like this.

> 
> >>
> >> Signed-off-by: John Ferlan <jferlan at redhat.com>
> >> ---
> >>  src/openvz/openvz_conf.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)

To be clear, NACK to this and workarounds for false positives like this.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151007/14f9f8ce/attachment-0001.sig>