[Qemu-devel] [PATCH v2 0/6] RESEND - Update filename string sizes in block layer

[Qemu-devel] [PATCH v2 0/6] RESEND - Update filename string sizes in block layer

[Jeff Cody]

[ Note, my prior email to qemu-devel bounced, because I mistyped an address.
  Resending ]

The block layer uses a mixture of 'PATH_MAX' and '1024' string sizes
for filenames (and backing filenames).

This series consolidates all that usage to 'PATH_MAX'.  Since most platforms
(especially the most common platforms for QEMU) have a PATH_MAX larger than
1024 bytes, this series also changes stack allocations of PATH_MAX to be
dynamically allocated.

Note: checkpatch.pl complains about an extra space in a printf in
      patches 1 & 2.  The lines complained about are in the diff context and
      not the actual changes, so I did not fix them up to satisfy checkpatch.

Changes from v2:

    - Change stack allocations to dybnamic (Thanks Kevin)
    - Update qcow/qcow2 ti perform safety checks for platforms that
      have a PATH_MAX < 1024 (thanks John, Kevin).

Jeff Cody (6):
  block: vmdk - make ret variable usage clear
  block: vmdk - move string allocations from stack to the heap
  block: qapi - move string allocation from stack to the heap
  block: move string allocation from stack to the heap
  block: mirror - change string allocation to 2-bytes
  block: update string sizes for filename,backing_file,exact_filename

 block.c                   | 11 ++++---
 block/mirror.c            |  3 +-
 block/qapi.c              |  8 +++--
 block/qcow.c              |  2 +-
 block/qcow2.c             |  3 +-
 block/vmdk.c              | 76 ++++++++++++++++++++++++++++-------------------
 block/vvfat.c             |  4 +--
 include/block/block_int.h |  8 ++---
 qemu-img.c                |  4 +--
 9 files changed, 70 insertions(+), 49 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/6] block: vmdk - make ret variable usage clear

[Jeff Cody]

Keep the variable 'ret' something that is returned by the function it is
defined in.  For the return value of 'sscanf', use a more meaningful
variable name.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vmdk.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 52cb888..dc6459c 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -785,6 +785,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
                               const char *desc_file_path, Error **errp)
 {
     int ret;
+    int matches;
     char access[11];
     char type[11];
     char fname[512];
@@ -796,6 +797,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
+
     while (*p) {
         /* parse extent line in one of below formats:
          *
@@ -805,23 +807,23 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
          * RW [size in sectors] VMFSSPARSE "file-name.vmdk"
          */
         flat_offset = -1;
-        ret = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
-                access, &sectors, type, fname, &flat_offset);
-        if (ret < 4 || strcmp(access, "RW")) {
+        matches = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
+                         access, &sectors, type, fname, &flat_offset);
+        if (matches < 4 || strcmp(access, "RW")) {
             goto next_line;
         } else if (!strcmp(type, "FLAT")) {
-            if (ret != 5 || flat_offset < 0) {
+            if (matches != 5 || flat_offset < 0) {
                 error_setg(errp, "Invalid extent lines: \n%s", p);
                 return -EINVAL;
             }
         } else if (!strcmp(type, "VMFS")) {
-            if (ret == 4) {
+            if (matches == 4) {
                 flat_offset = 0;
             } else {
                 error_setg(errp, "Invalid extent lines:\n%s", p);
                 return -EINVAL;
             }
-        } else if (ret != 4) {
+        } else if (matches != 4) {
             error_setg(errp, "Invalid extent lines:\n%s", p);
             return -EINVAL;
         }
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/6] block: vmdk - move string allocations from stack to the heap

[Jeff Cody]

Functions 'vmdk_parse_extents' and 'vmdk_create' allocate several
PATH_MAX sized arrays on the stack.  Make these dynamically allocated.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vmdk.c | 64 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index dc6459c..043a750 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -784,7 +784,7 @@ static int vmdk_open_sparse(BlockDriverState *bs,
 static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
                               const char *desc_file_path, Error **errp)
 {
-    int ret;
+    int ret = 0;
     int matches;
     char access[11];
     char type[11];
@@ -792,12 +792,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     const char *p = desc;
     int64_t sectors = 0;
     int64_t flat_offset;
-    char extent_path[PATH_MAX];
+    char *extent_path = g_malloc0(PATH_MAX);
     BlockDriverState *extent_file;
     BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
-
     while (*p) {
         /* parse extent line in one of below formats:
          *
@@ -814,18 +813,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         } else if (!strcmp(type, "FLAT")) {
             if (matches != 5 || flat_offset < 0) {
                 error_setg(errp, "Invalid extent lines: \n%s", p);
-                return -EINVAL;
+                ret = -EINVAL;
+                goto exit;
             }
         } else if (!strcmp(type, "VMFS")) {
             if (matches == 4) {
                 flat_offset = 0;
             } else {
                 error_setg(errp, "Invalid extent lines:\n%s", p);
-                return -EINVAL;
+                ret = -EINVAL;
+                goto exit;
             }
         } else if (matches != 4) {
             error_setg(errp, "Invalid extent lines:\n%s", p);
-            return -EINVAL;
+            ret = -EINVAL;
+            goto exit;
         }
 
         if (sectors <= 0 ||
@@ -840,7 +842,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         {
             error_setg(errp, "Cannot use relative extent paths with VMDK "
                        "descriptor file '%s'", bs->file->filename);
-            return -EINVAL;
+            ret = -EINVAL;
+            goto exit;
         }
 
         path_combine(extent_path, sizeof(extent_path),
@@ -849,7 +852,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
                         bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
         if (ret) {
-            return ret;
+            goto exit;
         }
 
         /* save to extents array */
@@ -860,7 +863,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
                             0, 0, 0, 0, 0, &extent, errp);
             if (ret < 0) {
                 bdrv_unref(extent_file);
-                return ret;
+                goto exit;
             }
             extent->flat_start_offset = flat_offset << 9;
         } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
@@ -874,13 +877,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             g_free(buf);
             if (ret) {
                 bdrv_unref(extent_file);
-                return ret;
+                goto exit;
             }
             extent = &s->extents[s->num_extents - 1];
         } else {
             error_setg(errp, "Unsupported extent type '%s'", type);
             bdrv_unref(extent_file);
-            return -ENOTSUP;
+            ret = -ENOTSUP;
+            goto exit;
         }
         extent->type = g_strdup(type);
 next_line:
@@ -893,7 +897,9 @@ next_line:
             p++;
         }
     }
-    return 0;
+exit:
+    g_free(extent_path);
+    return ret;
 }
 
 static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
@@ -1797,10 +1803,15 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
     int ret = 0;
     bool flat, split, compress;
     GString *ext_desc_lines;
-    char path[PATH_MAX], prefix[PATH_MAX], postfix[PATH_MAX];
+    char *path = g_malloc0(PATH_MAX);
+    char *prefix = g_malloc0(PATH_MAX);
+    char *postfix = g_malloc0(PATH_MAX);
+    char *desc_line = g_malloc0(BUF_SIZE);
+    char *ext_filename = g_malloc0(PATH_MAX);
+    char *desc_filename = g_malloc0(PATH_MAX);
     const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
     const char *desc_extent_line;
-    char parent_desc_line[BUF_SIZE] = "";
+    char *parent_desc_line = g_malloc0(BUF_SIZE);
     uint32_t parent_cid = 0xffffffff;
     uint32_t number_heads = 16;
     bool zeroed_grain = false;
@@ -1916,33 +1927,27 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
         }
         parent_cid = vmdk_read_cid(bs, 0);
         bdrv_unref(bs);
-        snprintf(parent_desc_line, sizeof(parent_desc_line),
+        snprintf(parent_desc_line, BUF_SIZE,
                 "parentFileNameHint=\"%s\"", backing_file);
     }
 
     /* Create extents */
     filesize = total_size;
     while (filesize > 0) {
-        char desc_line[BUF_SIZE];
-        char ext_filename[PATH_MAX];
-        char desc_filename[PATH_MAX];
         int64_t size = filesize;
 
         if (split && size > split_size) {
             size = split_size;
         }
         if (split) {
-            snprintf(desc_filename, sizeof(desc_filename), "%s-%c%03d%s",
+            snprintf(desc_filename, PATH_MAX, "%s-%c%03d%s",
                     prefix, flat ? 'f' : 's', ++idx, postfix);
         } else if (flat) {
-            snprintf(desc_filename, sizeof(desc_filename), "%s-flat%s",
-                    prefix, postfix);
+            snprintf(desc_filename, PATH_MAX, "%s-flat%s", prefix, postfix);
         } else {
-            snprintf(desc_filename, sizeof(desc_filename), "%s%s",
-                    prefix, postfix);
+            snprintf(desc_filename, PATH_MAX, "%s%s", prefix, postfix);
         }
-        snprintf(ext_filename, sizeof(ext_filename), "%s%s",
-                path, desc_filename);
+        snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
 
         if (vmdk_create_extent(ext_filename, size,
                                flat, compress, zeroed_grain, opts, errp)) {
@@ -1952,7 +1957,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
         filesize -= size;
 
         /* Format description line */
-        snprintf(desc_line, sizeof(desc_line),
+        snprintf(desc_line, BUF_SIZE,
                     desc_extent_line, size / BDRV_SECTOR_SIZE, desc_filename);
         g_string_append(ext_desc_lines, desc_line);
     }
@@ -2007,6 +2012,13 @@ exit:
     g_free(backing_file);
     g_free(fmt);
     g_free(desc);
+    g_free(path);
+    g_free(prefix);
+    g_free(postfix);
+    g_free(desc_line);
+    g_free(ext_filename);
+    g_free(desc_filename);
+    g_free(parent_desc_line);
     g_string_free(ext_desc_lines, true);
     return ret;
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 3/6] block: qapi - move string allocation from stack to the heap

[Jeff Cody]

Rather than declaring 'backing_filename2' on the stack in
bdrv_quiery_image_info(), dynamically allocate it on the heap.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/qapi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a6fd6f7..e51bade 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -175,7 +175,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
 {
     int64_t size;
     const char *backing_filename;
-    char backing_filename2[1024];
+    char *backing_filename2 = NULL;
     BlockDriverInfo bdi;
     int ret;
     Error *err = NULL;
@@ -211,13 +211,14 @@ void bdrv_query_image_info(BlockDriverState *bs,
 
     backing_filename = bs->backing_file;
     if (backing_filename[0] != '\0') {
+        backing_filename2 = g_malloc0(1024);
         info->backing_filename = g_strdup(backing_filename);
         info->has_backing_filename = true;
-        bdrv_get_full_backing_filename(bs, backing_filename2,
-                                       sizeof(backing_filename2), &err);
+        bdrv_get_full_backing_filename(bs, backing_filename2, 1024, &err);
         if (err) {
             error_propagate(errp, err);
             qapi_free_ImageInfo(info);
+            g_free(backing_filename2);
             return;
         }
 
@@ -231,6 +232,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
             info->backing_filename_format = g_strdup(bs->backing_format);
             info->has_backing_filename_format = true;
         }
+        g_free(backing_filename2);
     }
 
     ret = bdrv_query_snapshot_info_list(bs, &info->snapshots, &err);
-- 
1.9.3

[Qemu-devel] [PATCH v2 4/6] block: move string allocation from stack to the heap

[Jeff Cody]

Rather than allocate PATH_MAX bytes on the stack, use g_strndup() to
dynamically allocate the string, and add an exit label for cleanup.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index cbe4a32..39cd7a6 100644
--- a/block.c
+++ b/block.c
@@ -2207,7 +2207,7 @@ int bdrv_commit(BlockDriverState *bs)
     int n, ro, open_flags;
     int ret = 0;
     uint8_t *buf = NULL;
-    char filename[PATH_MAX];
+    char *filename = NULL;
 
     if (!drv)
         return -ENOMEDIUM;
@@ -2222,13 +2222,14 @@ int bdrv_commit(BlockDriverState *bs)
     }
 
     ro = bs->backing_hd->read_only;
-    /* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
-    pstrcpy(filename, sizeof(filename), bs->backing_hd->filename);
+    /* filename must be NUL-terminated. */
+    filename = g_strndup(bs->backing_hd->filename, PATH_MAX - 1);
     open_flags =  bs->backing_hd->open_flags;
 
     if (ro) {
         if (bdrv_reopen(bs->backing_hd, open_flags | BDRV_O_RDWR, NULL)) {
-            return -EACCES;
+            ret = -EACCES;
+            goto exit;
         }
     }
 
@@ -2307,6 +2308,8 @@ ro_cleanup:
         bdrv_reopen(bs->backing_hd, open_flags & ~BDRV_O_RDWR, NULL);
     }
 
+exit:
+    g_free(filename);
     return ret;
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 5/6] block: mirror - change string allocation to 2-bytes

[Jeff Cody]

The backing_filename string in mirror_run() is only used to check
for a NULL string, so we don't need to allocate 1024 bytes (or, later,
PATH_MAX bytes), when we only need to copy the first 2 characters.

We technically only need 1 byte, as we are just checking for NULL, but
since backing_filename[] is populated by bdrv_get_backing_filename(), a
string size of 1 will always only return '\0';

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/mirror.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 9019d1b..4056164 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -378,7 +378,8 @@ static void coroutine_fn mirror_run(void *opaque)
     int64_t sector_num, end, sectors_per_chunk, length;
     uint64_t last_pause_ns;
     BlockDriverInfo bdi;
-    char backing_filename[1024];
+    char backing_filename[2]; /* we only need 2 characters because we are only
+                                 checking for a NULL string */
     int ret = 0;
     int n;
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 6/6] block: update string sizes for filename, backing_file, exact_filename

[Jeff Cody]

The string field entries 'filename', 'backing_file', and
'exact_filename' in the BlockDriverState struct are defined as 1024
bytes.

However, many places that use these values accept a maximum of PATH_MAX
bytes, so we have a mixture of 1024 byte and PATH_MAX byte allocations.
This patch makes the BlockDriverStruct field string sizes match usage.

This patch also does a few fixes related to the size that needs to
happen now:

    * the block qapi driver is updated to use PATH_MAX bytes
    * the qcow and qcow2 drivers have an additional safety check
    * the block vvfat driver is updated to use PATH_MAX bytes
      for the size of backing_file, for systems where PATH_MAX is < 1024
      bytes.
    * qemu-img uses PATH_MAX rather than 1024.  These instances were not
      changed to be dynamically allocated, however, as the extra
      temporary 3K in stack usage for qemu-img does not seem worrisome.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/qapi.c              | 4 ++--
 block/qcow.c              | 2 +-
 block/qcow2.c             | 3 ++-
 block/vvfat.c             | 4 ++--
 include/block/block_int.h | 8 ++++----
 qemu-img.c                | 4 ++--
 6 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index e51bade..a34ac5c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -211,10 +211,10 @@ void bdrv_query_image_info(BlockDriverState *bs,
 
     backing_filename = bs->backing_file;
     if (backing_filename[0] != '\0') {
-        backing_filename2 = g_malloc0(1024);
+        backing_filename2 = g_malloc0(PATH_MAX);
         info->backing_filename = g_strdup(backing_filename);
         info->has_backing_filename = true;
-        bdrv_get_full_backing_filename(bs, backing_filename2, 1024, &err);
+        bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err);
         if (err) {
             error_propagate(errp, err);
             qapi_free_ImageInfo(info);
diff --git a/block/qcow.c b/block/qcow.c
index ece2269..ccbe9e0 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -215,7 +215,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     /* read the backing file name */
     if (header.backing_file_offset != 0) {
         len = header.backing_file_size;
-        if (len > 1023) {
+        if (len > 1023 || len > sizeof(bs->backing_file)) {
             error_setg(errp, "Backing file name too long");
             ret = -EINVAL;
             goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index e4e690a..dbaf016 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -868,7 +868,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     /* read the backing file name */
     if (header.backing_file_offset != 0) {
         len = header.backing_file_size;
-        if (len > MIN(1023, s->cluster_size - header.backing_file_offset)) {
+        if (len > MIN(1023, s->cluster_size - header.backing_file_offset) ||
+            len > sizeof(bs->backing_file)) {
             error_setg(errp, "Backing file name too long");
             ret = -EINVAL;
             goto fail;
diff --git a/block/vvfat.c b/block/vvfat.c
index e34a789..a1a44f0 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2909,8 +2909,8 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
 
     array_init(&(s->commits), sizeof(commit_t));
 
-    s->qcow_filename = g_malloc(1024);
-    ret = get_tmp_filename(s->qcow_filename, 1024);
+    s->qcow_filename = g_malloc(PATH_MAX);
+    ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "can't create temporary file");
         goto err;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 06a21dd..e264be9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -339,13 +339,13 @@ struct BlockDriverState {
      * regarding this BDS's context */
     QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
 
-    char filename[1024];
-    char backing_file[1024]; /* if non zero, the image is a diff of
-                                this file image */
+    char filename[PATH_MAX];
+    char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
+                                    this file image */
     char backing_format[16]; /* if non-zero and backing_file exists */
 
     QDict *full_open_options;
-    char exact_filename[1024];
+    char exact_filename[PATH_MAX];
 
     BlockDriverState *backing_hd;
     BlockDriverState *file;
diff --git a/qemu-img.c b/qemu-img.c
index 7876258..4e9a7f5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2556,7 +2556,7 @@ static int img_rebase(int argc, char **argv)
 
     /* For safe rebasing we need to compare old and new backing file */
     if (!unsafe) {
-        char backing_name[1024];
+        char backing_name[PATH_MAX];
 
         blk_old_backing = blk_new_with_bs("old_backing", &error_abort);
         bs_old_backing = blk_bs(blk_old_backing);
@@ -2614,7 +2614,7 @@ static int img_rebase(int argc, char **argv)
         }
         old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
         if (old_backing_num_sectors < 0) {
-            char backing_name[1024];
+            char backing_name[PATH_MAX];
 
             bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
             error_report("Could not get size of '%s': %s",
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 1/6] block: vmdk - make ret variable usage clear

[John Snow]

On 01/20/2015 12:31 PM, Jeff Cody wrote:
> Keep the variable 'ret' something that is returned by the function it is
> defined in.  For the return value of 'sscanf', use a more meaningful
> variable name.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/vmdk.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 52cb888..dc6459c 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -785,6 +785,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>                                 const char *desc_file_path, Error **errp)
>   {
>       int ret;
> +    int matches;
>       char access[11];
>       char type[11];
>       char fname[512];
> @@ -796,6 +797,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>       BDRVVmdkState *s = bs->opaque;
>       VmdkExtent *extent;
>
> +

Stray newline.

>       while (*p) {
>           /* parse extent line in one of below formats:
>            *
> @@ -805,23 +807,23 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>            * RW [size in sectors] VMFSSPARSE "file-name.vmdk"
>            */
>           flat_offset = -1;
> -        ret = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
> -                access, &sectors, type, fname, &flat_offset);
> -        if (ret < 4 || strcmp(access, "RW")) {
> +        matches = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
> +                         access, &sectors, type, fname, &flat_offset);
> +        if (matches < 4 || strcmp(access, "RW")) {
>               goto next_line;
>           } else if (!strcmp(type, "FLAT")) {
> -            if (ret != 5 || flat_offset < 0) {
> +            if (matches != 5 || flat_offset < 0) {
>                   error_setg(errp, "Invalid extent lines: \n%s", p);
>                   return -EINVAL;
>               }
>           } else if (!strcmp(type, "VMFS")) {
> -            if (ret == 4) {
> +            if (matches == 4) {
>                   flat_offset = 0;
>               } else {
>                   error_setg(errp, "Invalid extent lines:\n%s", p);
>                   return -EINVAL;
>               }
> -        } else if (ret != 4) {
> +        } else if (matches != 4) {
>               error_setg(errp, "Invalid extent lines:\n%s", p);
>               return -EINVAL;
>           }
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/6] block: vmdk - move string allocations from stack to the heap

[John Snow]

On 01/20/2015 12:31 PM, Jeff Cody wrote:
> Functions 'vmdk_parse_extents' and 'vmdk_create' allocate several
> PATH_MAX sized arrays on the stack.  Make these dynamically allocated.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/vmdk.c | 64 ++++++++++++++++++++++++++++++++++++------------------------
>   1 file changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index dc6459c..043a750 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -784,7 +784,7 @@ static int vmdk_open_sparse(BlockDriverState *bs,
>   static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>                                 const char *desc_file_path, Error **errp)
>   {
> -    int ret;
> +    int ret = 0;
>       int matches;
>       char access[11];
>       char type[11];
> @@ -792,12 +792,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>       const char *p = desc;
>       int64_t sectors = 0;
>       int64_t flat_offset;
> -    char extent_path[PATH_MAX];
> +    char *extent_path = g_malloc0(PATH_MAX);
>       BlockDriverState *extent_file;
>       BDRVVmdkState *s = bs->opaque;
>       VmdkExtent *extent;
>
> -

The stray newline you added is now gone!

>       while (*p) {
>           /* parse extent line in one of below formats:
>            *
> @@ -814,18 +813,21 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>           } else if (!strcmp(type, "FLAT")) {
>               if (matches != 5 || flat_offset < 0) {
>                   error_setg(errp, "Invalid extent lines: \n%s", p);
> -                return -EINVAL;
> +                ret = -EINVAL;
> +                goto exit;
>               }
>           } else if (!strcmp(type, "VMFS")) {
>               if (matches == 4) {
>                   flat_offset = 0;
>               } else {
>                   error_setg(errp, "Invalid extent lines:\n%s", p);
> -                return -EINVAL;
> +                ret = -EINVAL;
> +                goto exit;
>               }
>           } else if (matches != 4) {
>               error_setg(errp, "Invalid extent lines:\n%s", p);
> -            return -EINVAL;
> +            ret = -EINVAL;
> +            goto exit;
>           }
>
>           if (sectors <= 0 ||
> @@ -840,7 +842,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>           {
>               error_setg(errp, "Cannot use relative extent paths with VMDK "
>                          "descriptor file '%s'", bs->file->filename);
> -            return -EINVAL;
> +            ret = -EINVAL;
> +            goto exit;
>           }
>
>           path_combine(extent_path, sizeof(extent_path),
> @@ -849,7 +852,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>           ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
>                           bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
>           if (ret) {
> -            return ret;
> +            goto exit;
>           }
>
>           /* save to extents array */
> @@ -860,7 +863,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>                               0, 0, 0, 0, 0, &extent, errp);
>               if (ret < 0) {
>                   bdrv_unref(extent_file);
> -                return ret;
> +                goto exit;
>               }
>               extent->flat_start_offset = flat_offset << 9;
>           } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
> @@ -874,13 +877,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>               g_free(buf);
>               if (ret) {
>                   bdrv_unref(extent_file);
> -                return ret;
> +                goto exit;
>               }
>               extent = &s->extents[s->num_extents - 1];
>           } else {
>               error_setg(errp, "Unsupported extent type '%s'", type);
>               bdrv_unref(extent_file);
> -            return -ENOTSUP;
> +            ret = -ENOTSUP;
> +            goto exit;
>           }
>           extent->type = g_strdup(type);
>   next_line:
> @@ -893,7 +897,9 @@ next_line:
>               p++;
>           }
>       }
> -    return 0;
> +exit:
> +    g_free(extent_path);
> +    return ret;
>   }
>
>   static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
> @@ -1797,10 +1803,15 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
>       int ret = 0;
>       bool flat, split, compress;
>       GString *ext_desc_lines;
> -    char path[PATH_MAX], prefix[PATH_MAX], postfix[PATH_MAX];
> +    char *path = g_malloc0(PATH_MAX);
> +    char *prefix = g_malloc0(PATH_MAX);
> +    char *postfix = g_malloc0(PATH_MAX);
> +    char *desc_line = g_malloc0(BUF_SIZE);
> +    char *ext_filename = g_malloc0(PATH_MAX);
> +    char *desc_filename = g_malloc0(PATH_MAX);
>       const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
>       const char *desc_extent_line;
> -    char parent_desc_line[BUF_SIZE] = "";
> +    char *parent_desc_line = g_malloc0(BUF_SIZE);
>       uint32_t parent_cid = 0xffffffff;
>       uint32_t number_heads = 16;
>       bool zeroed_grain = false;
> @@ -1916,33 +1927,27 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
>           }
>           parent_cid = vmdk_read_cid(bs, 0);
>           bdrv_unref(bs);
> -        snprintf(parent_desc_line, sizeof(parent_desc_line),
> +        snprintf(parent_desc_line, BUF_SIZE,
>                   "parentFileNameHint=\"%s\"", backing_file);
>       }
>
>       /* Create extents */
>       filesize = total_size;
>       while (filesize > 0) {
> -        char desc_line[BUF_SIZE];
> -        char ext_filename[PATH_MAX];
> -        char desc_filename[PATH_MAX];
>           int64_t size = filesize;
>
>           if (split && size > split_size) {
>               size = split_size;
>           }
>           if (split) {
> -            snprintf(desc_filename, sizeof(desc_filename), "%s-%c%03d%s",
> +            snprintf(desc_filename, PATH_MAX, "%s-%c%03d%s",
>                       prefix, flat ? 'f' : 's', ++idx, postfix);
>           } else if (flat) {
> -            snprintf(desc_filename, sizeof(desc_filename), "%s-flat%s",
> -                    prefix, postfix);
> +            snprintf(desc_filename, PATH_MAX, "%s-flat%s", prefix, postfix);
>           } else {
> -            snprintf(desc_filename, sizeof(desc_filename), "%s%s",
> -                    prefix, postfix);
> +            snprintf(desc_filename, PATH_MAX, "%s%s", prefix, postfix);
>           }
> -        snprintf(ext_filename, sizeof(ext_filename), "%s%s",
> -                path, desc_filename);
> +        snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
>
>           if (vmdk_create_extent(ext_filename, size,
>                                  flat, compress, zeroed_grain, opts, errp)) {
> @@ -1952,7 +1957,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
>           filesize -= size;
>
>           /* Format description line */
> -        snprintf(desc_line, sizeof(desc_line),
> +        snprintf(desc_line, BUF_SIZE,
>                       desc_extent_line, size / BDRV_SECTOR_SIZE, desc_filename);
>           g_string_append(ext_desc_lines, desc_line);
>       }
> @@ -2007,6 +2012,13 @@ exit:
>       g_free(backing_file);
>       g_free(fmt);
>       g_free(desc);
> +    g_free(path);
> +    g_free(prefix);
> +    g_free(postfix);
> +    g_free(desc_line);
> +    g_free(ext_filename);
> +    g_free(desc_filename);
> +    g_free(parent_desc_line);
>       g_string_free(ext_desc_lines, true);
>       return ret;
>   }
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/6] block: qapi - move string allocation from stack to the heap

[John Snow]

On 01/20/2015 12:31 PM, Jeff Cody wrote:
> Rather than declaring 'backing_filename2' on the stack in
> bdrv_quiery_image_info(), dynamically allocate it on the heap.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/qapi.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index a6fd6f7..e51bade 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -175,7 +175,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
>   {
>       int64_t size;
>       const char *backing_filename;
> -    char backing_filename2[1024];
> +    char *backing_filename2 = NULL;
>       BlockDriverInfo bdi;
>       int ret;
>       Error *err = NULL;
> @@ -211,13 +211,14 @@ void bdrv_query_image_info(BlockDriverState *bs,
>
>       backing_filename = bs->backing_file;
>       if (backing_filename[0] != '\0') {
> +        backing_filename2 = g_malloc0(1024);
>           info->backing_filename = g_strdup(backing_filename);
>           info->has_backing_filename = true;
> -        bdrv_get_full_backing_filename(bs, backing_filename2,
> -                                       sizeof(backing_filename2), &err);
> +        bdrv_get_full_backing_filename(bs, backing_filename2, 1024, &err);
>           if (err) {
>               error_propagate(errp, err);
>               qapi_free_ImageInfo(info);
> +            g_free(backing_filename2);
>               return;
>           }
>
> @@ -231,6 +232,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
>               info->backing_filename_format = g_strdup(bs->backing_format);
>               info->has_backing_filename_format = true;
>           }
> +        g_free(backing_filename2);
>       }
>
>       ret = bdrv_query_snapshot_info_list(bs, &info->snapshots, &err);
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/6] block: move string allocation from stack to the heap

[John Snow]

On 01/20/2015 12:31 PM, Jeff Cody wrote:
> Rather than allocate PATH_MAX bytes on the stack, use g_strndup() to
> dynamically allocate the string, and add an exit label for cleanup.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index cbe4a32..39cd7a6 100644
> --- a/block.c
> +++ b/block.c
> @@ -2207,7 +2207,7 @@ int bdrv_commit(BlockDriverState *bs)
>       int n, ro, open_flags;
>       int ret = 0;
>       uint8_t *buf = NULL;
> -    char filename[PATH_MAX];
> +    char *filename = NULL;
>
>       if (!drv)
>           return -ENOMEDIUM;
> @@ -2222,13 +2222,14 @@ int bdrv_commit(BlockDriverState *bs)
>       }
>
>       ro = bs->backing_hd->read_only;
> -    /* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
> -    pstrcpy(filename, sizeof(filename), bs->backing_hd->filename);
> +    /* filename must be NUL-terminated. */
> +    filename = g_strndup(bs->backing_hd->filename, PATH_MAX - 1);
>       open_flags =  bs->backing_hd->open_flags;
>
>       if (ro) {
>           if (bdrv_reopen(bs->backing_hd, open_flags | BDRV_O_RDWR, NULL)) {
> -            return -EACCES;
> +            ret = -EACCES;
> +            goto exit;
>           }
>       }
>
> @@ -2307,6 +2308,8 @@ ro_cleanup:
>           bdrv_reopen(bs->backing_hd, open_flags & ~BDRV_O_RDWR, NULL);
>       }
>
> +exit:
> +    g_free(filename);
>       return ret;
>   }
>
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 5/6] block: mirror - change string allocation to 2-bytes

[John Snow]

On 01/20/2015 12:31 PM, Jeff Cody wrote:
> The backing_filename string in mirror_run() is only used to check
> for a NULL string, so we don't need to allocate 1024 bytes (or, later,
> PATH_MAX bytes), when we only need to copy the first 2 characters.
>
> We technically only need 1 byte, as we are just checking for NULL, but
> since backing_filename[] is populated by bdrv_get_backing_filename(), a
> string size of 1 will always only return '\0';
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/mirror.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 9019d1b..4056164 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -378,7 +378,8 @@ static void coroutine_fn mirror_run(void *opaque)
>       int64_t sector_num, end, sectors_per_chunk, length;
>       uint64_t last_pause_ns;
>       BlockDriverInfo bdi;
> -    char backing_filename[1024];
> +    char backing_filename[2]; /* we only need 2 characters because we are only
> +                                 checking for a NULL string */
>       int ret = 0;
>       int n;
>
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 6/6] block: update string sizes for filename, backing_file, exact_filename

[John Snow]

On 01/20/2015 12:31 PM, Jeff Cody wrote:
> The string field entries 'filename', 'backing_file', and
> 'exact_filename' in the BlockDriverState struct are defined as 1024
> bytes.
>
> However, many places that use these values accept a maximum of PATH_MAX
> bytes, so we have a mixture of 1024 byte and PATH_MAX byte allocations.
> This patch makes the BlockDriverStruct field string sizes match usage.
>
> This patch also does a few fixes related to the size that needs to
> happen now:
>
>      * the block qapi driver is updated to use PATH_MAX bytes
>      * the qcow and qcow2 drivers have an additional safety check
>      * the block vvfat driver is updated to use PATH_MAX bytes
>        for the size of backing_file, for systems where PATH_MAX is < 1024
>        bytes.
>      * qemu-img uses PATH_MAX rather than 1024.  These instances were not
>        changed to be dynamically allocated, however, as the extra
>        temporary 3K in stack usage for qemu-img does not seem worrisome.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/qapi.c              | 4 ++--
>   block/qcow.c              | 2 +-
>   block/qcow2.c             | 3 ++-
>   block/vvfat.c             | 4 ++--
>   include/block/block_int.h | 8 ++++----
>   qemu-img.c                | 4 ++--
>   6 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index e51bade..a34ac5c 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -211,10 +211,10 @@ void bdrv_query_image_info(BlockDriverState *bs,
>
>       backing_filename = bs->backing_file;
>       if (backing_filename[0] != '\0') {
> -        backing_filename2 = g_malloc0(1024);
> +        backing_filename2 = g_malloc0(PATH_MAX);
>           info->backing_filename = g_strdup(backing_filename);
>           info->has_backing_filename = true;
> -        bdrv_get_full_backing_filename(bs, backing_filename2, 1024, &err);
> +        bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err);
>           if (err) {
>               error_propagate(errp, err);
>               qapi_free_ImageInfo(info);
> diff --git a/block/qcow.c b/block/qcow.c
> index ece2269..ccbe9e0 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -215,7 +215,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>       /* read the backing file name */
>       if (header.backing_file_offset != 0) {
>           len = header.backing_file_size;
> -        if (len > 1023) {
> +        if (len > 1023 || len > sizeof(bs->backing_file)) {
>               error_setg(errp, "Backing file name too long");
>               ret = -EINVAL;
>               goto fail;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index e4e690a..dbaf016 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -868,7 +868,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>       /* read the backing file name */
>       if (header.backing_file_offset != 0) {
>           len = header.backing_file_size;
> -        if (len > MIN(1023, s->cluster_size - header.backing_file_offset)) {
> +        if (len > MIN(1023, s->cluster_size - header.backing_file_offset) ||
> +            len > sizeof(bs->backing_file)) {
>               error_setg(errp, "Backing file name too long");
>               ret = -EINVAL;
>               goto fail;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index e34a789..a1a44f0 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2909,8 +2909,8 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
>
>       array_init(&(s->commits), sizeof(commit_t));
>
> -    s->qcow_filename = g_malloc(1024);
> -    ret = get_tmp_filename(s->qcow_filename, 1024);
> +    s->qcow_filename = g_malloc(PATH_MAX);
> +    ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "can't create temporary file");
>           goto err;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 06a21dd..e264be9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -339,13 +339,13 @@ struct BlockDriverState {
>        * regarding this BDS's context */
>       QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
>
> -    char filename[1024];
> -    char backing_file[1024]; /* if non zero, the image is a diff of
> -                                this file image */
> +    char filename[PATH_MAX];
> +    char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
> +                                    this file image */
>       char backing_format[16]; /* if non-zero and backing_file exists */
>
>       QDict *full_open_options;
> -    char exact_filename[1024];
> +    char exact_filename[PATH_MAX];
>
>       BlockDriverState *backing_hd;
>       BlockDriverState *file;
> diff --git a/qemu-img.c b/qemu-img.c
> index 7876258..4e9a7f5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2556,7 +2556,7 @@ static int img_rebase(int argc, char **argv)
>
>       /* For safe rebasing we need to compare old and new backing file */
>       if (!unsafe) {
> -        char backing_name[1024];
> +        char backing_name[PATH_MAX];
>
>           blk_old_backing = blk_new_with_bs("old_backing", &error_abort);
>           bs_old_backing = blk_bs(blk_old_backing);
> @@ -2614,7 +2614,7 @@ static int img_rebase(int argc, char **argv)
>           }
>           old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
>           if (old_backing_num_sectors < 0) {
> -            char backing_name[1024];
> +            char backing_name[PATH_MAX];
>
>               bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
>               error_report("Could not get size of '%s': %s",
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/6] block: vmdk - make ret variable usage clear

[Stefan Hajnoczi]

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

On Tue, Jan 20, 2015 at 12:31:28PM -0500, Jeff Cody wrote:
> Keep the variable 'ret' something that is returned by the function it is
> defined in.  For the return value of 'sscanf', use a more meaningful
> variable name.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/vmdk.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 2/6] block: vmdk - move string allocations from stack to the heap

[Stefan Hajnoczi]

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

On Tue, Jan 20, 2015 at 12:31:29PM -0500, Jeff Cody wrote:
> @@ -792,12 +792,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>      const char *p = desc;
>      int64_t sectors = 0;
>      int64_t flat_offset;
> -    char extent_path[PATH_MAX];
> +    char *extent_path = g_malloc0(PATH_MAX);

Simpler alternative that has no risk of memory leaks:

  extent_path = g_malloc0(PATH_MAX);
  path_combine(extent_path, sizeof(extent_path),
          desc_file_path, fname);
  extent_file = NULL;
  ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
                  bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
  g_free(extent_path);
  if (ret) {
      goto exit;
  }

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 2/6] block: vmdk - move string allocations from stack to the heap

[Daniel P. Berrange]

On Thu, Jan 22, 2015 at 11:17:35AM +0000, Stefan Hajnoczi wrote:
> On Tue, Jan 20, 2015 at 12:31:29PM -0500, Jeff Cody wrote:
> > @@ -792,12 +792,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> >      const char *p = desc;
> >      int64_t sectors = 0;
> >      int64_t flat_offset;
> > -    char extent_path[PATH_MAX];
> > +    char *extent_path = g_malloc0(PATH_MAX);
> 
> Simpler alternative that has no risk of memory leaks:
> 
>   extent_path = g_malloc0(PATH_MAX);
>   path_combine(extent_path, sizeof(extent_path),
>           desc_file_path, fname);
>   extent_file = NULL;
>   ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
>                   bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
>   g_free(extent_path);

Much better would be to change path_combine so it doesn't need to have
the destination alloc done by the caller. Just have it allocate the
right sized string buffer directly & return that and avoid the madness
of PATH_MAX entirely.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 3/6] block: qapi - move string allocation from stack to the heap

[Stefan Hajnoczi]

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

On Tue, Jan 20, 2015 at 12:31:30PM -0500, Jeff Cody wrote:
> Rather than declaring 'backing_filename2' on the stack in
> bdrv_quiery_image_info(), dynamically allocate it on the heap.

s/quiery/query/

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/qapi.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index a6fd6f7..e51bade 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -175,7 +175,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
>  {
>      int64_t size;
>      const char *backing_filename;
> -    char backing_filename2[1024];
> +    char *backing_filename2 = NULL;
>      BlockDriverInfo bdi;
>      int ret;
>      Error *err = NULL;
> @@ -211,13 +211,14 @@ void bdrv_query_image_info(BlockDriverState *bs,
>  
>      backing_filename = bs->backing_file;
>      if (backing_filename[0] != '\0') {
> +        backing_filename2 = g_malloc0(1024);

backing_filename2 is only used inside the body of this if statement.
Please move the declaration in here to avoid initializing with NULL
(that value is never used but I had to check the surrounding code to
figure that out).

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 4/6] block: move string allocation from stack to the heap

[Stefan Hajnoczi]

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

On Tue, Jan 20, 2015 at 12:31:31PM -0500, Jeff Cody wrote:
> Rather than allocate PATH_MAX bytes on the stack, use g_strndup() to
> dynamically allocate the string, and add an exit label for cleanup.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Zombie alert!

This is a funny:

Since September 2012 in commit 0bce597d6ec34b2af802799eb53ebc863c704d05
("block: convert bdrv_commit() to use bdrv_reopen()") the filename
variable has not been used.

We continued to maintain this variable faithfully, for example, to fix a
buffer overflow in commit c2cba3d9314f972dfaf724d0ec2d018eb54c95f1
("block: avoid buffer overrun by using pstrcpy, not strncpy").

Please kill the zombie filename variable instead of switching to heap
allocation :-).

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 5/6] block: mirror - change string allocation to 2-bytes

[Stefan Hajnoczi]

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

On Tue, Jan 20, 2015 at 12:31:32PM -0500, Jeff Cody wrote:
> The backing_filename string in mirror_run() is only used to check
> for a NULL string, so we don't need to allocate 1024 bytes (or, later,
> PATH_MAX bytes), when we only need to copy the first 2 characters.
> 
> We technically only need 1 byte, as we are just checking for NULL, but
> since backing_filename[] is populated by bdrv_get_backing_filename(), a
> string size of 1 will always only return '\0';
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/mirror.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 6/6] block: update string sizes for filename, backing_file, exact_filename

[Stefan Hajnoczi]

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

On Tue, Jan 20, 2015 at 12:31:33PM -0500, Jeff Cody wrote:
> The string field entries 'filename', 'backing_file', and
> 'exact_filename' in the BlockDriverState struct are defined as 1024
> bytes.
> 
> However, many places that use these values accept a maximum of PATH_MAX
> bytes, so we have a mixture of 1024 byte and PATH_MAX byte allocations.
> This patch makes the BlockDriverStruct field string sizes match usage.
> 
> This patch also does a few fixes related to the size that needs to
> happen now:
> 
>     * the block qapi driver is updated to use PATH_MAX bytes
>     * the qcow and qcow2 drivers have an additional safety check
>     * the block vvfat driver is updated to use PATH_MAX bytes
>       for the size of backing_file, for systems where PATH_MAX is < 1024
>       bytes.
>     * qemu-img uses PATH_MAX rather than 1024.  These instances were not
>       changed to be dynamically allocated, however, as the extra
>       temporary 3K in stack usage for qemu-img does not seem worrisome.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/qapi.c              | 4 ++--
>  block/qcow.c              | 2 +-
>  block/qcow2.c             | 3 ++-
>  block/vvfat.c             | 4 ++--
>  include/block/block_int.h | 8 ++++----
>  qemu-img.c                | 4 ++--
>  6 files changed, 13 insertions(+), 12 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 4/6] block: move string allocation from stack to the heap

[Jeff Cody]

On Thu, Jan 22, 2015 at 11:37:54AM +0000, Stefan Hajnoczi wrote:
> On Tue, Jan 20, 2015 at 12:31:31PM -0500, Jeff Cody wrote:
> > Rather than allocate PATH_MAX bytes on the stack, use g_strndup() to
> > dynamically allocate the string, and add an exit label for cleanup.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> Zombie alert!
> 
> This is a funny:
> 
> Since September 2012 in commit 0bce597d6ec34b2af802799eb53ebc863c704d05
> ("block: convert bdrv_commit() to use bdrv_reopen()") the filename
> variable has not been used.
> 
> We continued to maintain this variable faithfully, for example, to fix a
> buffer overflow in commit c2cba3d9314f972dfaf724d0ec2d018eb54c95f1
> ("block: avoid buffer overrun by using pstrcpy, not strncpy").
> 
> Please kill the zombie filename variable instead of switching to heap
> allocation :-).

Ha! OK, will kill the zombie :)