RESEND - Update filename string sizes in block layer

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

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

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

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

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

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

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

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

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

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

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

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