[Qemu-devel] [PATCH 0/7] block: Collection of unrelated simple fixes

[Qemu-devel] [PATCH 0/7] block: Collection of unrelated simple fixes

[Kevin Wolf]

This fixes some misc block layer bugs that Coverity pointed at.

Kevin Wolf (7):
  block: Remove dead code
  block: Fix bdrv_open use after free
  qcow: Fix bdrv_write_compressed error handling
  ide: Fix off-by-one error in array index check
  vmdk: Fix use of uninitialised value
  vmdk: Improve error handling
  vmdk: Fix possible segfaults

 block.c       |    8 ++------
 block/qcow.c  |   30 +++++++++++++++++++-----------
 block/vmdk.c  |   30 ++++++++++++++++++++++--------
 hw/ide/core.c |    2 +-
 4 files changed, 44 insertions(+), 26 deletions(-)

-- 
1.7.6.4

[Qemu-devel] [PATCH 1/7] block: Remove dead code

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 0b7bc06..86d450c 100644
--- a/block.c
+++ b/block.c
@@ -2039,11 +2039,7 @@ const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size)
 {
-    if (!bs->backing_file) {
-        pstrcpy(filename, filename_size, "");
-    } else {
-        pstrcpy(filename, filename_size, bs->backing_file);
-    }
+    pstrcpy(filename, filename_size, bs->backing_file);
 }
 
 int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 1/7] block: Remove dead code

[Robert Wang]

Reviewed-by: wdongxu@linux.vnet.ibm.com

2011/10/26 Kevin Wolf <kwolf@redhat.com>:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index 0b7bc06..86d450c 100644
> --- a/block.c
> +++ b/block.c
> @@ -2039,11 +2039,7 @@ const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
>  void bdrv_get_backing_filename(BlockDriverState *bs,
>                                char *filename, int filename_size)
>  {
> -    if (!bs->backing_file) {
> -        pstrcpy(filename, filename_size, "");
> -    } else {
> -        pstrcpy(filename, filename_size, bs->backing_file);
> -    }
> +    pstrcpy(filename, filename_size, bs->backing_file);
>  }
>
>  int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> --
> 1.7.6.4
>
>
>



-- 
Regards
Robert Wang

Re: [Qemu-devel] [PATCH 1/7] block: Remove dead code

[Stefan Hajnoczi]

On Wed, Oct 26, 2011 at 02:31:16PM +0200, Kevin Wolf wrote:
> @@ -2039,11 +2039,7 @@ const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
>  void bdrv_get_backing_filename(BlockDriverState *bs,
>                                 char *filename, int filename_size)
>  {
> -    if (!bs->backing_file) {
> -        pstrcpy(filename, filename_size, "");
> -    } else {
> -        pstrcpy(filename, filename_size, bs->backing_file);
> -    }
> +    pstrcpy(filename, filename_size, bs->backing_file);
>  }

I think this points to another problem:

bs->backing_file[] is never cleared across bdrv_close()/bdrv_open().

If we open an image file that uses a backing file, then close the
BlockDriverState, and then open a file which does not use a backing file
we're left with the old backing file!

Stefan

Re: [Qemu-devel] [PATCH 1/7] block: Remove dead code

[Kevin Wolf]

Am 27.10.2011 09:37, schrieb Stefan Hajnoczi:
> On Wed, Oct 26, 2011 at 02:31:16PM +0200, Kevin Wolf wrote:
>> @@ -2039,11 +2039,7 @@ const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
>>  void bdrv_get_backing_filename(BlockDriverState *bs,
>>                                 char *filename, int filename_size)
>>  {
>> -    if (!bs->backing_file) {
>> -        pstrcpy(filename, filename_size, "");
>> -    } else {
>> -        pstrcpy(filename, filename_size, bs->backing_file);
>> -    }
>> +    pstrcpy(filename, filename_size, bs->backing_file);
>>  }
> 
> I think this points to another problem:
> 
> bs->backing_file[] is never cleared across bdrv_close()/bdrv_open().
> 
> If we open an image file that uses a backing file, then close the
> BlockDriverState, and then open a file which does not use a backing file
> we're left with the old backing file!

Ouch! Care to send a fix?

Did you check if there are more fields in BlockDriverState that should
be cleared?

Kevin

[Qemu-devel] [PATCH 2/7] block: Fix bdrv_open use after free

[Kevin Wolf]

tmp_filename was used outside the block it was defined in, i.e. after it went
out of scope. Move its declaration to the top level.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block.c b/block.c
index 86d450c..9dbb572 100644
--- a/block.c
+++ b/block.c
@@ -571,6 +571,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
               BlockDriver *drv)
 {
     int ret;
+    char tmp_filename[PATH_MAX];
 
     if (flags & BDRV_O_SNAPSHOT) {
         BlockDriverState *bs1;
@@ -578,7 +579,6 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         int is_protocol = 0;
         BlockDriver *bdrv_qcow2;
         QEMUOptionParameter *options;
-        char tmp_filename[PATH_MAX];
         char backing_filename[PATH_MAX];
 
         /* if snapshot, we create a temporary backing file and open it
-- 
1.7.6.4

[Qemu-devel] [PATCH 3/7] qcow: Fix bdrv_write_compressed error handling

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow.c |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index ab36b29..35e21eb 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -736,8 +736,6 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
         return -EINVAL;
 
     out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128);
-    if (!out_buf)
-        return -1;
 
     /* best compression, small window, no zlib header */
     memset(&strm, 0, sizeof(strm));
@@ -745,8 +743,8 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
                        Z_DEFLATED, -12,
                        9, Z_DEFAULT_STRATEGY);
     if (ret != 0) {
-        g_free(out_buf);
-        return -1;
+        ret = -EINVAL;
+        goto fail;
     }
 
     strm.avail_in = s->cluster_size;
@@ -756,9 +754,9 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
 
     ret = deflate(&strm, Z_FINISH);
     if (ret != Z_STREAM_END && ret != Z_OK) {
-        g_free(out_buf);
         deflateEnd(&strm);
-        return -1;
+        ret = -EINVAL;
+        goto fail;
     }
     out_len = strm.next_out - out_buf;
 
@@ -766,19 +764,29 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
 
     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
-        bdrv_write(bs, sector_num, buf, s->cluster_sectors);
+        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
+        if (ret < 0) {
+            goto fail;
+        }
     } else {
         cluster_offset = get_cluster_offset(bs, sector_num << 9, 2,
                                             out_len, 0, 0);
+        if (cluster_offset == 0) {
+            ret = -EIO;
+            goto fail;
+        }
+
         cluster_offset &= s->cluster_offset_mask;
-        if (bdrv_pwrite(bs->file, cluster_offset, out_buf, out_len) != out_len) {
-            g_free(out_buf);
-            return -1;
+        ret = bdrv_pwrite(bs->file, cluster_offset, out_buf, out_len);
+        if (ret < 0) {
+            goto fail;
         }
     }
 
+    ret = 0;
+fail:
     g_free(out_buf);
-    return 0;
+    return ret;
 }
 
 static coroutine_fn int qcow_co_flush(BlockDriverState *bs)
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 3/7] qcow: Fix bdrv_write_compressed error handling

[Paolo Bonzini]

On 10/26/2011 02:31 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
>   block/qcow.c |   30 +++++++++++++++++++-----------
>   1 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/block/qcow.c b/block/qcow.c
> index ab36b29..35e21eb 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -736,8 +736,6 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
>           return -EINVAL;
>
>       out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128);
> -    if (!out_buf)
> -        return -1;
>
>       /* best compression, small window, no zlib header */
>       memset(&strm, 0, sizeof(strm));
> @@ -745,8 +743,8 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
>                          Z_DEFLATED, -12,
>                          9, Z_DEFAULT_STRATEGY);
>       if (ret != 0) {
> -        g_free(out_buf);
> -        return -1;
> +        ret = -EINVAL;
> +        goto fail;
>       }
>
>       strm.avail_in = s->cluster_size;
> @@ -756,9 +754,9 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
>
>       ret = deflate(&strm, Z_FINISH);
>       if (ret != Z_STREAM_END&&  ret != Z_OK) {
> -        g_free(out_buf);
>           deflateEnd(&strm);
> -        return -1;
> +        ret = -EINVAL;
> +        goto fail;
>       }
>       out_len = strm.next_out - out_buf;
>
> @@ -766,19 +764,29 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
>
>       if (ret != Z_STREAM_END || out_len>= s->cluster_size) {
>           /* could not compress: write normal cluster */
> -        bdrv_write(bs, sector_num, buf, s->cluster_sectors);
> +        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
> +        if (ret<  0) {
> +            goto fail;
> +        }
>       } else {
>           cluster_offset = get_cluster_offset(bs, sector_num<<  9, 2,
>                                               out_len, 0, 0);
> +        if (cluster_offset == 0) {
> +            ret = -EIO;
> +            goto fail;
> +        }
> +
>           cluster_offset&= s->cluster_offset_mask;
> -        if (bdrv_pwrite(bs->file, cluster_offset, out_buf, out_len) != out_len) {
> -            g_free(out_buf);
> -            return -1;
> +        ret = bdrv_pwrite(bs->file, cluster_offset, out_buf, out_len);
> +        if (ret<  0) {
> +            goto fail;
>           }
>       }
>
> +    ret = 0;
> +fail:
>       g_free(out_buf);
> -    return 0;
> +    return ret;
>   }
>
>   static coroutine_fn int qcow_co_flush(BlockDriverState *bs)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH 4/7] ide: Fix off-by-one error in array index check

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 280a117..29305d3 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2039,7 +2039,7 @@ static int ide_drive_pio_post_load(void *opaque, int version_id)
 {
     IDEState *s = opaque;
 
-    if (s->end_transfer_fn_idx > ARRAY_SIZE(transfer_end_table)) {
+    if (s->end_transfer_fn_idx >= ARRAY_SIZE(transfer_end_table)) {
         return -EINVAL;
     }
     s->end_transfer_func = transfer_end_table[s->end_transfer_fn_idx];
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 4/7] ide: Fix off-by-one error in array index check

[Paolo Bonzini]

On 10/26/2011 02:31 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
>   hw/ide/core.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 280a117..29305d3 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2039,7 +2039,7 @@ static int ide_drive_pio_post_load(void *opaque, int version_id)
>   {
>       IDEState *s = opaque;
>
> -    if (s->end_transfer_fn_idx>  ARRAY_SIZE(transfer_end_table)) {
> +    if (s->end_transfer_fn_idx>= ARRAY_SIZE(transfer_end_table)) {
>           return -EINVAL;
>       }
>       s->end_transfer_func = transfer_end_table[s->end_transfer_fn_idx];

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH 5/7] vmdk: Fix use of uninitialised value

[Kevin Wolf]

In error cases, cid is never set.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 6be592f..6cdbfb7 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -208,7 +208,7 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
 static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
 {
     char desc[DESC_SIZE];
-    uint32_t cid;
+    uint32_t cid = 0;
     const char *p_name, *cid_str;
     size_t cid_str_size;
     BDRVVmdkState *s = bs->opaque;
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 5/7] vmdk: Fix use of uninitialised value

[Pavel Borzenkov]

On Wed, Oct 26, 2011 at 4:31 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> In error cases, cid is never set.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

This fix is already in the qemu-trivial queue:
http://repo.or.cz/w/qemu/stefanha.git/commit/8379e46d1fd681b8aa4714382e2cdab05e5d0575

-- 
Pavel

> ---
>  block/vmdk.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 6be592f..6cdbfb7 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -208,7 +208,7 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
>  static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
>  {
>     char desc[DESC_SIZE];
> -    uint32_t cid;
> +    uint32_t cid = 0;
>     const char *p_name, *cid_str;
>     size_t cid_str_size;
>     BDRVVmdkState *s = bs->opaque;
> --
> 1.7.6.4
>
>
>

Re: [Qemu-devel] [PATCH 5/7] vmdk: Fix use of uninitialised value

[Kevin Wolf]

Am 26.10.2011 15:21, schrieb Pavel Borzenkov:
> On Wed, Oct 26, 2011 at 4:31 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> In error cases, cid is never set.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> This fix is already in the qemu-trivial queue:
> http://repo.or.cz/w/qemu/stefanha.git/commit/8379e46d1fd681b8aa4714382e2cdab05e5d0575

Oh, okay, didn't see that. I guess git will just drop my patch when I
rebase after the qemu-trivial one has been applied.

Kevin

[Qemu-devel] [PATCH 6/7] vmdk: Improve error handling

[Kevin Wolf]

Return the right error values in some more places.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 6cdbfb7..fa0e8bd 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -212,8 +212,10 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
     const char *p_name, *cid_str;
     size_t cid_str_size;
     BDRVVmdkState *s = bs->opaque;
+    int ret;
 
-    if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
+    ret = bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE);
+    if (ret < 0) {
         return 0;
     }
 
@@ -239,10 +241,12 @@ static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
     char desc[DESC_SIZE], tmp_desc[DESC_SIZE];
     char *p_name, *tmp_str;
     BDRVVmdkState *s = bs->opaque;
+    int ret;
 
     memset(desc, 0, sizeof(desc));
-    if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) {
-        return -EIO;
+    ret = bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE);
+    if (ret < 0) {
+        return ret;
     }
 
     tmp_str = strstr(desc, "parentCID");
@@ -254,9 +258,11 @@ static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
         pstrcat(desc, sizeof(desc), tmp_desc);
     }
 
-    if (bdrv_pwrite_sync(bs->file, s->desc_offset, desc, DESC_SIZE) < 0) {
-        return -EIO;
+    ret = bdrv_pwrite_sync(bs->file, s->desc_offset, desc, DESC_SIZE);
+    if (ret < 0) {
+        return ret;
     }
+
     return 0;
 }
 
@@ -1109,7 +1115,10 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
         /* update CID on the first write every time the virtual disk is
          * opened */
         if (!s->cid_updated) {
-            vmdk_write_cid(bs, time(NULL));
+            ret = vmdk_write_cid(bs, time(NULL));
+            if (ret < 0) {
+                return ret;
+            }
             s->cid_updated = true;
         }
     }
-- 
1.7.6.4

[Qemu-devel] [PATCH 7/7] vmdk: Fix possible segfaults

[Kevin Wolf]

Data we read from the disk isn't necessarily null terminated and may not
contain the string we're looking for. The code needs to be a bit more careful
here.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index fa0e8bd..8caaf0b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -227,6 +227,7 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
         cid_str_size = sizeof("CID");
     }
 
+    desc[DESC_SIZE - 1] = '\0';
     p_name = strstr(desc, cid_str);
     if (p_name != NULL) {
         p_name += cid_str_size;
@@ -243,13 +244,17 @@ static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
     BDRVVmdkState *s = bs->opaque;
     int ret;
 
-    memset(desc, 0, sizeof(desc));
     ret = bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE);
     if (ret < 0) {
         return ret;
     }
 
+    desc[DESC_SIZE - 1] = '\0';
     tmp_str = strstr(desc, "parentCID");
+    if (tmp_str == NULL) {
+        return -EINVAL;
+    }
+
     pstrcpy(tmp_desc, sizeof(tmp_desc), tmp_str);
     p_name = strstr(desc, "CID");
     if (p_name != NULL) {
-- 
1.7.6.4