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

[Kevin Wolf]

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;
> 
> 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.

> 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.

>        checksum = be32_to_cpu(footer->checksum);
>        footer->checksum = 0;
>        if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum)
>            fprintf(stderr, "block-vpc: The header checksum of '%s' is "
>                "incorrect.\n", bs->filename);
> 
> I wonder whether this should fail the open.  Outside the scope of this
> patch.

I think there was a reason why not, but I can't remember the details.

>> @@ -203,19 +208,21 @@ static int vpc_open(BlockDriverState *bs, int flags)
>>  
>>      /* Allow a maximum disk size of approximately 2 TB */
>>      if (bs->total_sectors >= 65535LL * 255 * 255) {
>> -        err = -EFBIG;
>> +        ret = -EFBIG;
>>          goto fail;
>>      }
>>  
>>      if (disk_type == VHD_DYNAMIC) {
>> -        if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
>> -                HEADER_SIZE) != HEADER_SIZE) {
>> +        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
>> +                         HEADER_SIZE);
>> +        if (ret < 0) {
>>              goto fail;
>>          }
>>  
>>          dyndisk_header = (struct vhd_dyndisk_header *) buf;
>>  
>>          if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
>> +            ret = -EINVAL;
> 
> If you keep -EMEDIUMTYPE above, should this be -EMEDIUMTYPE, too?

Good question, I wondered the same when writing the patch.

I decided to stay consistent with vpc_probe() which detects an image as
VHD image if the first magic is right. Then this case means that it's
still a VHD image, but an invalid one.

Kevin