Re: [libvirt] [PATCH 3/8] virstoragefile: Resolve Coverity DEADCODE
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(a)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