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

[John Ferlan]

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


John

>>      if (VIR_ALLOC_N(*res, size + 1) < 0)
>>          return BACKING_STORE_ERROR;
>>      memcpy(*res, buf + offset, size);
>>
> 
> 
> Also I've looked on the specs of the qcow2 header format and it seems
> that we don't do exactly the right thing here. To determine that the
> qcow2 image doesn't have a backing store we sholuld use the offset
> rather than size:
> 
>   8 - 15:   backing_file_offset
>         Offset into the image file at which the backing file name
>         is stored (NB: The string is not null terminated). 0 if the
>         image doesn't have a backing file.
> 
>   16 - 19:   backing_file_size
>         Length of the backing file name in bytes. Must not be
>         longer than 1023 bytes. Undefined if the image doesn't have
>         a backing file.
> 
> Also we probably should make sure that the backing file size is less
> than 1024 bytes.
> 
> While I agree that the check is dead code, as we call this function with
> buf_size with a reasonably low value, we probably should improve the
> backing file detection code to match the specs.
> 
> Peter
>