Re: [PATCH v3 08/17] block/io: support int64_t bytes in bdrv_aligned_preadv()

[Eric Blake]

On 6/18/20 9:35 AM, Alberto Garcia wrote:
> On Fri 22 May 2020 05:14:36 PM CEST, Eric Blake wrote:
> > >    static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
> > > -    BdrvTrackedRequest *req, int64_t offset, unsigned int bytes,
> > > +    BdrvTrackedRequest *req, int64_t offset, int64_t bytes,
> > >        int64_t align, QEMUIOVector *qiov, size_t qiov_offset, int flags)
>   [...]
> > >        BlockDriverState *bs = child->bs;
> > >        int64_t total_bytes, max_bytes;
> > >        int ret = 0;
> > > -    uint64_t bytes_remaining = bytes;
> > > +    int64_t bytes_remaining = bytes;
> > >        int max_transfer;
> > >    
> > >        assert(is_power_of_2(align));
> > > +    assert(offset >= 0 && bytes >= 0);
> >
> > Use within the function:
> >
> > the new assertion added here does not check for whether offset+bytes is
> > positive; I would suggest we strengthen it to instead be:
> > assert(offset >= 0 && (uint64_t) bytes <= INT64_MAX - offset);
>
> But here you would be making 'bytes' unsigned without asserting that
> it's not negative.

'offset >= 0' proves that offset is nonnegative.  'INT64_MAX - offset' 
is still a signed integer, and is also necessarily non-negative (the 
maximum positive integer minus any other positive integer cannot go 
negative).  We then coerce that into an unsigned comparison, which means 
if bytes was negative, the coercion will result in something larger than 
(unsigned)INT64_MAX, and the overall assert() will fail.  Thus, as 
written, the assertion works just fine at proving both bytes and offset 
were non-negative, and that bytes+offset did not overflow a signed integer.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org