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