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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list