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

[Peter Krempa]

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.

>      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

-------------- 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/aba09f3f/attachment-0001.sig>