[Qemu-devel] [PATCH v2] Fix for qemu-img info to supply FORMAT values for SPARSE extents

[Qemu-devel] [PATCH v2] Fix for qemu-img info to supply FORMAT values for SPARSE extents

[Shwetha Mathangi Chandra Choodamani]

This patch fixes the bug in qemu-img info that wouldn't populate the extent type for default formats.
The extent type has now been set where necessary. This is the second version in the series after inputs
from Fam Zheng(famz@redhat.com).

Signed-off-by: Shwetha Mathangi Chandra Choodamani <saphira.brightscales@gmail.com>
---
 block/vmdk.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index b69988d..d4a37ad 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -499,6 +499,18 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
     VMDK3Header header;
     VmdkExtent *extent;
 
+    char access[11];
+    char type[11];
+    char fname[512];
+    int64_t sectors = 0;
+    int64_t flat_offset;
+    int64_t size;
+    size = bdrv_getlength(file);
+    char *buf;
+    buf = g_malloc0(size + 1);
+    bdrv_pread(file, sizeof(magic), buf, size);
+    sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
+    access, &sectors, type, fname, &flat_offset);
     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
         error_setg_errno(errp, -ret,
@@ -515,6 +527,18 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
                           le32_to_cpu(header.granularity),
                           &extent,
                           errp);
+    while (strcmp(access, "RW")) {
+        while (*buf) {
+            sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
+            access, &sectors, type, fname, &flat_offset);
+            if (*buf == '\n') {
+                buf++;
+                break;
+            }
+            buf++;
+        }
+    }
+    extent->type = g_strdup(type);
     if (ret < 0) {
         return ret;
     }
@@ -523,6 +547,7 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
         /* free extent allocated by vmdk_add_extent */
         vmdk_free_last_extent(bs);
     }
+    g_free(buf);
     return ret;
 }
 
@@ -566,7 +591,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     VmdkExtent *extent;
     BDRVVmdkState *s = bs->opaque;
     int64_t l1_backup_offset = 0;
-
+    char access[11];
+    char type[11];
+    char fname[512];
+    int64_t sectors = 0;
+    int64_t flat_offset;
     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
         error_setg_errno(errp, -ret,
@@ -586,11 +615,9 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
             return ret;
         }
     }
-
     if (!s->create_type) {
         s->create_type = g_strdup("monolithicSparse");
     }
-
     if (le64_to_cpu(header.gd_offset) == VMDK4_GD_AT_END) {
         /*
          * The footer takes precedence over the header, so read it in. The
@@ -694,6 +721,25 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         g_free(s->create_type);
         s->create_type = g_strdup("streamOptimized");
     }
+
+    if (!extent->type) {
+        uint64_t desc_offset = le64_to_cpu(header.desc_offset);
+        char *buf = vmdk_read_desc(file, desc_offset<<9, errp);
+        sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
+        access, &sectors, type, fname, &flat_offset);
+        while (strcmp(access, "RW")) {
+            while (*buf) {
+                sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
+                access, &sectors, type, fname, &flat_offset);
+                if (*buf == '\n') {
+                    buf++;
+                    break;
+                }
+                buf++;
+            }
+        }
+    }
+    extent->type = g_strdup(type);
     extent->has_marker = le32_to_cpu(header.flags) & VMDK4_FLAG_MARKER;
     extent->version = le32_to_cpu(header.version);
     extent->has_zero_grain = le32_to_cpu(header.flags) & VMDK4_FLAG_ZERO_GRAIN;
@@ -711,7 +757,6 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
 {
     char *opt_pos, *opt_end;
     const char *end = desc + strlen(desc);
-
     opt_pos = strstr(desc, opt_name);
     if (!opt_pos) {
         return VMDK_ERROR;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v2] Fix for qemu-img info to supply FORMAT values for SPARSE extents

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

On 04/01/2014 03:49 PM, Shwetha Mathangi Chandra Choodamani wrote:
> This patch fixes the bug in qemu-img info that wouldn't populate the extent type for default formats.
> The extent type has now been set where necessary. This is the second version in the series after inputs
> from Fam Zheng(famz@redhat.com).
> 
> Signed-off-by: Shwetha Mathangi Chandra Choodamani <saphira.brightscales@gmail.com>
> ---
>  block/vmdk.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 4 deletions(-)
> 

> +    bdrv_pread(file, sizeof(magic), buf, size);
> +    sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
> +    access, &sectors, type, fname, &flat_offset);

Odd indentation.  Furthermore, sscanf() has undefined behavior if the
input values overflow the width of the integer variable you are parsing
into.  While you are not the first client of sscanf in the code base, I
strongly recommend that you use something that isn't vulnerable to
undefined behavior on untrusted external input, or at the VERY least,
check the sscanf() return value as well as put a %n parameter at the end
of your format string to ensure that you parsed what you thought you did.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2] Fix for qemu-img info to supply FORMAT values for SPARSE extents

[Fam Zheng]

On Tue, 04/01 17:49, Shwetha Mathangi Chandra Choodamani wrote:
> This patch fixes the bug in qemu-img info that wouldn't populate the extent type for default formats.
> The extent type has now been set where necessary. This is the second version in the series after inputs
> from Fam Zheng(famz@redhat.com).
> 
> Signed-off-by: Shwetha Mathangi Chandra Choodamani <saphira.brightscales@gmail.com>
> ---
>  block/vmdk.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index b69988d..d4a37ad 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -499,6 +499,18 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
>      VMDK3Header header;
>      VmdkExtent *extent;
>  

IIRC VMFS always comes with a description file, so we won't miss the extent
type and this part is not necessary.

> +    char access[11];
> +    char type[11];
> +    char fname[512];
> +    int64_t sectors = 0;
> +    int64_t flat_offset;
> +    int64_t size;
> +    size = bdrv_getlength(file);
> +    char *buf;
> +    buf = g_malloc0(size + 1);
> +    bdrv_pread(file, sizeof(magic), buf, size);
> +    sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
> +    access, &sectors, type, fname, &flat_offset);
>      ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>      if (ret < 0) {
>          error_setg_errno(errp, -ret,
> @@ -515,6 +527,18 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
>                            le32_to_cpu(header.granularity),
>                            &extent,
>                            errp);
> +    while (strcmp(access, "RW")) {
> +        while (*buf) {
> +            sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
> +            access, &sectors, type, fname, &flat_offset);
> +            if (*buf == '\n') {
> +                buf++;
> +                break;
> +            }
> +            buf++;
> +        }
> +    }
> +    extent->type = g_strdup(type);
>      if (ret < 0) {
>          return ret;
>      }
> @@ -523,6 +547,7 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
>          /* free extent allocated by vmdk_add_extent */
>          vmdk_free_last_extent(bs);
>      }
> +    g_free(buf);
>      return ret;
>  }
>  
> @@ -566,7 +591,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>      VmdkExtent *extent;
>      BDRVVmdkState *s = bs->opaque;
>      int64_t l1_backup_offset = 0;
> -
> +    char access[11];
> +    char type[11];
> +    char fname[512];
> +    int64_t sectors = 0;
> +    int64_t flat_offset;
>      ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
>      if (ret < 0) {
>          error_setg_errno(errp, -ret,
> @@ -586,11 +615,9 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>              return ret;
>          }
>      }
> -

Unnecessary blank line change.

>      if (!s->create_type) {
>          s->create_type = g_strdup("monolithicSparse");
>      }
> -

Same here.

>      if (le64_to_cpu(header.gd_offset) == VMDK4_GD_AT_END) {
>          /*
>           * The footer takes precedence over the header, so read it in. The
> @@ -694,6 +721,25 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>          g_free(s->create_type);
>          s->create_type = g_strdup("streamOptimized");
>      }
> +
> +    if (!extent->type) {
> +        uint64_t desc_offset = le64_to_cpu(header.desc_offset);
> +        char *buf = vmdk_read_desc(file, desc_offset<<9, errp);
> +        sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
> +        access, &sectors, type, fname, &flat_offset);
> +        while (strcmp(access, "RW")) {
> +            while (*buf) {
> +                sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
> +                access, &sectors, type, fname, &flat_offset);
> +                if (*buf == '\n') {
> +                    buf++;
> +                    break;
> +                }
> +                buf++;
> +            }
> +        }
> +    }

No. Why do you need to read the description?

Here we already know the extent type by looking at s->create_type, it would be
"monolithicSparse" or "streamOptimized" in the cases that we are trying to fix
in this patch, so no need to parse the description text.

Just set extent->type according to s->create_type.

> +    extent->type = g_strdup(type);


>      extent->has_marker = le32_to_cpu(header.flags) & VMDK4_FLAG_MARKER;
>      extent->version = le32_to_cpu(header.version);
>      extent->has_zero_grain = le32_to_cpu(header.flags) & VMDK4_FLAG_ZERO_GRAIN;
> @@ -711,7 +757,6 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
>  {
>      char *opt_pos, *opt_end;
>      const char *end = desc + strlen(desc);
> -

Blank line change, please drop it.

Fam

>      opt_pos = strstr(desc, opt_name);
>      if (!opt_pos) {
>          return VMDK_ERROR;
> -- 
> 1.7.9.5
> 
>