Patchwork [3/6] vpc: Fix bdrv_open() error handling

login
register
mail settings
Submitter Kevin Wolf
Date Jan. 24, 2013, 11:02 a.m.
Message ID <1359025358-5725-4-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/215311/
State New
Headers show

Comments

Kevin Wolf - Jan. 24, 2013, 11:02 a.m.
Return -errno instead of -1 on errors. While touching the
code, fix a memory leak.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vpc.c |   36 +++++++++++++++++++++++++-----------
 1 files changed, 25 insertions(+), 11 deletions(-)
Markus Armbruster - Jan. 24, 2013, 1:09 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Return -errno instead of -1 on errors. While touching the
> code, fix a memory leak.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  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?

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.

>              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;
>          }
>          if (strncmp(footer->creator, "conectix", 8)) {
> +            ret = -EMEDIUMTYPE;

I figure this makes your series depends on Stefan Weil's "block: Fix
error report for wrong file format".  I'd probably order it the other
way, and return -EINVAL here, then change it along with the others in
"block: Use error code EMEDIUMTYPE for wrong format in some block
drivers".  Matter of taste.

>              goto fail;
>          }
>          disk_type = VHD_FIXED;
       }

       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.

> @@ -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?

>              goto fail;
>          }
>  
> @@ -226,8 +233,10 @@ static int vpc_open(BlockDriverState *bs, int flags)
>          s->pagetable = g_malloc(s->max_table_entries * 4);
>  
>          s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
> -        if (bdrv_pread(bs->file, s->bat_offset, s->pagetable,
> -                s->max_table_entries * 4) != s->max_table_entries * 4) {
> +
> +        ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable,
> +                         s->max_table_entries * 4);
> +        if (ret < 0) {
>              goto fail;
>          }
>  
> @@ -265,8 +274,13 @@ static int vpc_open(BlockDriverState *bs, int flags)
>      migrate_add_blocker(s->migration_blocker);
>  
>      return 0;
> - fail:
> -    return err;
> +
> +fail:
> +    g_free(s->pagetable);
> +#ifdef CACHE
> +    g_free(s->pageentry_u8);
> +#endif
> +    return ret;
>  }
>  
>  static int vpc_reopen_prepare(BDRVReopenState *state,
Kevin Wolf - Jan. 25, 2013, 12:51 p.m.
Am 24.01.2013 14:09, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Return -errno instead of -1 on errors. While touching the
>> code, fix a memory leak.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  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
Markus Armbruster - Jan. 25, 2013, 1:37 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 24.01.2013 14:09, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> Return -errno instead of -1 on errors. While touching the
>>> code, fix a memory leak.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  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.

[...]
Kevin Wolf - Jan. 25, 2013, 1:44 p.m.
Am 25.01.2013 14:37, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 24.01.2013 14:09, schrieb Markus Armbruster:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> Return -errno instead of -1 on errors. While touching the
>>>> code, fix a memory leak.
>>>>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>> ---
>>>>  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 [...]
>> 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.

Oh, I completely missed that. I thought you were asking what happened if
we dropped the check like you suggested.

You're right, this needs separate checks for < 0 and < HEADER_SIZE, the
latter probably returning -EINVAL.

Kevin

Patch

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;
         }
         if (strncmp(footer->creator, "conectix", 8)) {
+            ret = -EMEDIUMTYPE;
             goto fail;
         }
         disk_type = VHD_FIXED;
@@ -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;
             goto fail;
         }
 
@@ -226,8 +233,10 @@  static int vpc_open(BlockDriverState *bs, int flags)
         s->pagetable = g_malloc(s->max_table_entries * 4);
 
         s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
-        if (bdrv_pread(bs->file, s->bat_offset, s->pagetable,
-                s->max_table_entries * 4) != s->max_table_entries * 4) {
+
+        ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable,
+                         s->max_table_entries * 4);
+        if (ret < 0) {
             goto fail;
         }
 
@@ -265,8 +274,13 @@  static int vpc_open(BlockDriverState *bs, int flags)
     migrate_add_blocker(s->migration_blocker);
 
     return 0;
- fail:
-    return err;
+
+fail:
+    g_free(s->pagetable);
+#ifdef CACHE
+    g_free(s->pageentry_u8);
+#endif
+    return ret;
 }
 
 static int vpc_reopen_prepare(BDRVReopenState *state,