On 09/15/14 15:23, John Ferlan wrote:
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@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
I wasn't able to locate the part that guarantees that buf_size is < UINT_MAX.
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...
I've sent a patch that should fix the parser according to the docs and also should fix the dead code here.
John
Peter