RESEND - Update filename string sizes in block layer

PATCH Superseded Jeff Cody 4 participants 21 messages Jan 20, 2015 – Jan 22, 2015 Block layer

lore

Share & tools

lore

Jeff Cody

author

2015-01-20 17:31 UTC #0 lore raw

v2 cover RESEND - Update filename string sizes in block layer

[ 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(-)

Signature · 1 line

1.9.3

Jeff Cody

author

2015-01-20 17:31 UTC #4 lore raw

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

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 <[email protected]>

---
 block/vmdk.c | 64 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

block/vmdk.c +38 ~~-26~~

index dc6459c..043a750 100644
@@ -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;
 }

Signature · 1 line

1.9.3

Jeff Cody

author

2015-01-20 17:31 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 block/qapi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

block/qapi.c +5 -3

index a6fd6f7..e51bade 100644
@@ -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);

Signature · 1 line

1.9.3

Jeff Cody

author

2015-01-20 17:31 UTC #11 lore raw

PATCH v2 4/6 block: move string allocation from stack to the heap

In reply to 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 <[email protected]>

---
 block.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

block.c +7 -4

index cbe4a32..39cd7a6 100644
@@ -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;
 }

Signature · 1 line

1.9.3

Jeff Cody

author

2015-01-20 17:31 UTC #15 lore raw

PATCH v2 5/6 block: mirror - change string allocation to 2-bytes

In reply to 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 <[email protected]>

---
 block/mirror.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

block/mirror.c +2 -1

index 9019d1b..4056164 100644
@@ -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;

Signature · 1 line

1.9.3

Jeff Cody

author

2015-01-20 17:31 UTC #1 lore raw

PATCH v2 1/6 block: vmdk - make ret variable usage clear

In reply to 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 <[email protected]>

---
 block/vmdk.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

block/vmdk.c +8 -6

index 52cb888..dc6459c 100644
@@ -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;
         }

Signature · 1 line

1.9.3

Jeff Cody

author

2015-01-20 17:31 UTC #18 lore raw

PATCH v2 6/6 block: update string sizes for filename, backing_file, exact_filename

In reply to 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 <[email protected]>

---
 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(-)

block/qapi.c +2 -2

index e51bade..a34ac5c 100644
@@ -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);

block/qcow.c +1 -1

index ece2269..ccbe9e0 100644
@@ -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;

block/qcow2.c +2 -1

index e4e690a..dbaf016 100644
@@ -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;

block/vvfat.c +2 -2

index e34a789..a1a44f0 100644
@@ -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;

include/block/block_int.h +4 -4

index 06a21dd..e264be9 100644
@@ -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;

qemu-img.c +2 -2

index 7876258..4e9a7f5 100644
@@ -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",

Signature · 1 line

1.9.3

John Snow

Reviewed-by on v2 1/6

2015-01-20 18:37 UTC #2 lore raw

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

In reply to Jeff Cody

Quoted 22 lines · 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 <[email protected]>

---
  block/vmdk.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

block/vmdk.c +1 -0

index 52cb888..dc6459c 100644
@@ -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];

block/vmdk.c static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, · line 800

Quoted 4 lines

block/vmdk.c +1 -0

@@ -796,6 +797,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
      BDRVVmdkState *s = bs->opaque;
      VmdkExtent *extent;
 
+

Stray newline.

Quoted 24 lines

block/vmdk.c +5 -5

      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) {

block/vmdk.c static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, · line 829

Quoted 10 lines

block/vmdk.c +1 -1

                  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 <[email protected]>

John Snow

Reviewed-by on v2 2/6

2015-01-20 18:37 UTC #5 lore raw

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

In reply to Jeff Cody

Quoted 22 lines · 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 <[email protected]>

---
  block/vmdk.c | 64 ++++++++++++++++++++++++++++++++++++------------------------
  1 file changed, 38 insertions(+), 26 deletions(-)

block/vmdk.c +1 -1

index dc6459c..043a750 100644
@@ -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];

block/vmdk.c static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, · line 800

Quoted 10 lines

block/vmdk.c +1 -2

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

Quoted 155 lines

block/vmdk.c +29 ~~-23~~

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

block/vmdk.c exit: · line 2024

Quoted 10 lines

block/vmdk.c +7 -0

+    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 <[email protected]>

John Snow

Reviewed-by on v2 3/6

2015-01-20 18:37 UTC #9 lore raw

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

In reply to Jeff Cody

Quoted 40 lines · 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 <[email protected]>

---
  block/qapi.c | 8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

block/qapi.c +4 -3

index a6fd6f7..e51bade 100644
@@ -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;
          }

block/qapi.c void bdrv_query_image_info(BlockDriverState *bs, · line 238

Quoted 7 lines

block/qapi.c +1 -0

@@ -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 <[email protected]>

John Snow

Reviewed-by on v2 4/6

2015-01-20 18:37 UTC #12 lore raw

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

In reply to Jeff Cody

Quoted 41 lines · 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 <[email protected]>

---
  block.c | 11 +++++++----
  1 file changed, 7 insertions(+), 4 deletions(-)

block.c +5 -4

index cbe4a32..39cd7a6 100644
@@ -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;
          }
      }

block.c ro_cleanup: · line 2315

Quoted 8 lines

block.c +2 -0

@@ -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 <[email protected]>

John Snow

Reviewed-by on v2 5/6

2015-01-20 18:37 UTC #16 lore raw

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

In reply to Jeff Cody

Quoted 19 lines · 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 <[email protected]>
---
  block/mirror.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
block/mirror.c +0 -0
index 9019d1b..4056164 100644

block/mirror.c static void coroutine_fn mirror_run(void *opaque) · line 385

Quoted 9 lines

block/mirror.c +2 -1

@@ -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 <[email protected]>

John Snow

Reviewed-by on v2 6/6

2015-01-20 18:37 UTC #19 lore raw

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

In reply to Jeff Cody

Quoted 126 lines · Jeff Cody

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

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

Signed-off-by Jeff Cody <[email protected]>

---
  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(-)

block/qapi.c +2 -2

index e51bade..a34ac5c 100644
@@ -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);

block/qcow.c +1 -1

index ece2269..ccbe9e0 100644
@@ -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;

block/qcow2.c +2 -1

index e4e690a..dbaf016 100644
@@ -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;

block/vvfat.c +2 -2

index e34a789..a1a44f0 100644
@@ -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;

include/block/block_int.h +4 -4

index 06a21dd..e264be9 100644
@@ -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;

qemu-img.c +1 -1

index 7876258..4e9a7f5 100644
@@ -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);

qemu-img.c static int img_rebase(int argc, char **argv) · line 2620

Quoted 8 lines

qemu-img.c +1 -1

@@ -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 <[email protected]>

Stefan Hajnoczi

Reviewed-by on v2 1/6

2015-01-22 10:56 UTC #3 lore raw

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

In reply to Jeff Cody

Quoted 8 lines · 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 <[email protected]>
---
 block/vmdk.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Reviewed-by Stefan Hajnoczi <[email protected]>

Stefan Hajnoczi

2015-01-22 11:17 UTC #6 lore raw

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

In reply to Jeff Cody

block/vmdk.c static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, · line 795

Quoted 6 lines · Jeff Cody

block/vmdk.c +1 -1

@@ -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;
  }

Daniel P. Berrange

2015-01-22 11:23 UTC #7 lore raw

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

Quoted 10 lines · Stefan Hajnoczi

block/vmdk.c static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, · line 795
Quoted 6 lines · Jeff Cody
block/vmdk.c +1 -1
@@ -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:

Quoted 7 lines

  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

Signature · 4 lines

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

Stefan Hajnoczi

2015-01-22 11:24 UTC #10 lore raw

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

In reply to Jeff Cody

Quoted 2 lines · Jeff Cody

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

s/quiery/query/

Quoted 20 lines

Signed-off-by Jeff Cody <[email protected]>

---
 block/qapi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

block/qapi.c +1 -1

index a6fd6f7..e51bade 100644
@@ -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;

block/qapi.c void bdrv_query_image_info(BlockDriverState *bs, · line 214

Quoted 4 lines

block/qapi.c +1 -0

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

Stefan Hajnoczi

2015-01-22 11:38 UTC #13 lore raw

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

In reply to Jeff Cody

Quoted 7 lines · 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 <[email protected]>
---
 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 :-).

Stefan Hajnoczi

Reviewed-by on v2 5/6

2015-01-22 11:41 UTC #17 lore raw

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

In reply to Jeff Cody

Quoted 4 lines · 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.

Quoted 8 lines

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 <[email protected]>
---
 block/mirror.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by Stefan Hajnoczi <[email protected]>

Stefan Hajnoczi

Reviewed-by on v2 6/6

2015-01-22 11:46 UTC #20 lore raw

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

In reply to Jeff Cody

Quoted 21 lines · 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 <[email protected]>

Quoted 8 lines

---
 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 <[email protected]>

Jeff Cody

author

2015-01-22 12:15 UTC #14 lore raw

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

In reply to Stefan Hajnoczi

Quoted 17 lines · Stefan Hajnoczi

Quoted 7 lines · 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 <[email protected]>
---
 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.

Quoted 6 lines

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 :)