Re: [Qemu-devel] [PATCH 3/6] vpc: Fix bdrv_open() error handling

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 24.01.2013 14:09, schrieb Markus Armbruster:
>> Kevin Wolf <address@hidden> writes:
>> 
>>> Return -errno instead of -1 on errors. While touching the
>>> code, fix a memory leak.
>>>
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>>  block/vpc.c |   36 +++++++++++++++++++++++++-----------
>>>  1 files changed, 25 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/block/vpc.c b/block/vpc.c
>>> index 7948609..9d2b177 100644
>>> --- a/block/vpc.c
>>> +++ b/block/vpc.c
>>> @@ -163,24 +163,29 @@ static int vpc_open(BlockDriverState *bs, int flags)
>>>      struct vhd_dyndisk_header* dyndisk_header;
>>>      uint8_t buf[HEADER_SIZE];
>>>      uint32_t checksum;
>>> -    int err = -1;
>>>      int disk_type = VHD_DYNAMIC;
>>> +    int ret;
>>>  
>>> -    if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
>>> +    ret = bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE);
>>> +    if (ret < 0 ) {
>>>          goto fail;
>>> +    }
>>>  
>>>      footer = (struct vhd_footer*) s->footer_buf;
>>>      if (strncmp(footer->creator, "conectix", 8)) {
>>>          int64_t offset = bdrv_getlength(bs->file);
>>>          if (offset < HEADER_SIZE) {
>>> +            ret = offset;
                 goto fail;
             }
             /* If a fixed disk, the footer is found only at the end of the 
file */
    -        if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, 
HEADER_SIZE)
    -                != HEADER_SIZE) {
    +        ret = bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf,
    +                         HEADER_SIZE);
    +        if (ret < 0) {
                 goto fail;
             }
>> 
>> What if 0 <= bdrv_getlength() < HEADER_SIZE?
>
> Then offset - HEADER_SIZE becomes negative. Not sure what this means, I
> need to check.
>
> offset is signed and the offset parameter of bdrv_pread() is int64_t as
> well. In there, sector_num will become negative as well and is passed as
> int64_t to bdrv_read, bdrv_rw_co, bdrv_rw_co_entry, bdrv_co_do_readv,
> bdrv_check_request. It then is multiplied by BDRV_SECTOR_SIZE and passed
> to bdrv_check_byte_request.
>
> bs->file->growable = 1 because it is opened with bdrv_file_open(),
> therefore bdrv_check_byte_request doesn't complain.
>
> The negative sector number is then passed to the block driver, which can
> do with it whatever it likes. Just some examples:
>
> * raw-posix with aio=threads will store it in RawPosixAIOData.aio_offset
>   and later pass it to preadv/pread. This should return -EINVAL.
>
> * raw-win32 stores it in Offset and OffsetHigh in an OVERLAPPED struct,
>   which both seem to be a DWORD. If I should guess, that's a uint32_t,
>   so we get a position far after EOF. No idea what happens, maybe we're
>   lucky and ReadFile() errors out.
>
> * nbd places it in struct nbd_request.from, which is uint64_t. Pretty
>   large request, but hopefully the server will do something reasonable
>   with it.
>
> So I wouldn't bet that bdrv_pread() handles negative offset correctly
> (let alone consistently) in all cases. We should probably fix this, but
> certainly not in this patch.
>
> I'm strongly for leaving the check in for now.

Sounds like you're thinking about what happens in the bdrv_pread() a few
lines down.  Not reached, because we goto fail, and return a
non-negative number.  That's what worries me.

>> For what it's worth, in other places, we simply bdrv_read() without
>> checking "got enough" first.  As far as I can tell, bdrv_read() returns
>> -EIO when it hits EOF prematurely.
>
> You're thinking of a different case here. But now that you brought it
> up, let me mention that bs->growable means that at least raw-posix reads
> zeros after EOF instead of failing.

Aha.  Didn't realize that growable applies to reads as well.

[...]