[libvirt] [PATCH 3/8] virstoragefile: Resolve Coverity DEADCODE

[Peter Krempa]

On 09/15/14 15:23, John Ferlan wrote:
> 
> 
> On 09/15/2014 04:42 AM, Peter Krempa wrote:
>> On 09/13/14 15:27, John Ferlan wrote:
>>> Coverity complains that the condition "size + 1 == 0" cannot happen.
>>> It's already been determined that offset+size is not larger than
>>> buf_size (and buf_size is smaller than UINT_MAX); and also that
>>
>> buff_size smaller than UINT_MAX isn't guaranteed in this function.
>> Although it will probably never happen the size is declared as size_t
>> and thus equal size to unsigned long long on 64 bit machines.
>>
>>> offset+size didn't overflow.
>>>
>>> So just remove the check
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/util/virstoragefile.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>>> index 5db9184..1a02b18 100644
>>> --- a/src/util/virstoragefile.c
>>> +++ b/src/util/virstoragefile.c
>>> @@ -393,8 +393,6 @@ qcowXGetBackingStore(char **res,
>>>      }
>>>      if (offset + size > buf_size || offset + size < offset)
>>
>> If size is UINT_MAX, then when you add it to offset, which is a small
>> number, then you get something between UINT_MAX and ULLONG_MAX which is
>> still in range of buf_size as it's a size_t.
>>
>>>          return BACKING_STORE_INVALID;
>>> -    if (size + 1 == 0)
>>> -        return BACKING_STORE_INVALID;
>>
>> So you may have this condition theoretically true, while the check above
>> doesn't catch it.
>>
> 
> My previous change was :
> 
> +    if (size == UINT_MAX)
>          return BACKING_STORE_INVALID;
> 
> But it was pointed out by eblake:
> 
> "
> Is this dead code?  After all, we just checked that offset+size is not
> larger than buf_size (and buf_size is smaller than UINT_MAX); and also

I wasn't able to locate the part that guarantees that buf_size is <
UINT_MAX.

> that offset+size didn't overflow.
> 
> "
> 
> Would it be better to use the original change instead?  Just trying to
> get past Coverity issue on this...
> 
> 
> BTW: The remainder of this w/r/t matching spec seems to be outside the
> scope of the Coverity DEADCODE, but if you have a patch I'm more than
> willing to look at it ;-) This code is shared with qcow1GetBackingStore
> and it's not like it's a recent change, so I'm hesitant to change it...

I've sent a patch that should fix the parser according to the docs and
also should fix the dead code here.

> 
> 
> John

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140915/42193d12/attachment-0001.sig>