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