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&gt; >> --- >> 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