[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 <jcody@redhat.com>
---
 block/vmdk.c | 64 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

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>

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

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