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

[Paolo Bonzini <pbonzini@redhat.com>]

On 22/01/2015 14:03, 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 | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index dc6459c..7d079ad 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -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;
>      BlockDriverState *extent_file;
>      BDRVVmdkState *s = bs->opaque;
>      VmdkExtent *extent;
>  
> -
>      while (*p) {
>          /* parse extent line in one of below formats:
>           *
> @@ -843,11 +842,13 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>              return -EINVAL;
>          }
>  
> +        extent_path = g_malloc0(PATH_MAX);
>          path_combine(extent_path, sizeof(extent_path),

Oops, sizeof(extent_path) changed from PATH_MAX to sizeof(char*).
Coverity found this instance, I didn't check for others.

Paolo

>                  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) {
>              return ret;
>          }
> @@ -1797,10 +1798,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 +1922,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 +1952,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 +2007,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;
>  }
>