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

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(a)redhat.com&gt; >> --- >> src/openvz/openvz_conf.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-)