diff mbox

allow reading variable size vmdk descriptor files

Message ID 1371024284-3572-1-git-send-email-evgeny.budilovsky@ravellosystems.com
State New
Headers show

Commit Message

Evgeny Budilovsky June 12, 2013, 8:04 a.m. UTC 
The hard-coded 2k buffer on the stack won't allow reading big descriptor
files which can be generated when storing big images (For example 500G
vmdk splitted to 2G chunks).

Signed-off-by: Evgeny Budilovsky <evgeny.budilovsky@ravellosystems.com>
---
 block/vmdk.c |   28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

--
1.7.9.5

Comments

Stefan Hajnoczi June 12, 2013, 10:17 a.m. UTC | #1 
On Wed, Jun 12, 2013 at 11:04:44AM +0300, Evgeny Budilovsky wrote:
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 608daaf..1bc944b 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -719,27 +719,41 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
>                                 int64_t desc_offset)
>  {
>      int ret;
> -    char buf[2048];
> +    char *buf = NULL;
>      char ct[128];
>      BDRVVmdkState *s = bs->opaque;
> +    int64_t size;
> 
> -    ret = bdrv_pread(bs->file, desc_offset, buf, sizeof(buf));
> +    size = bdrv_get_allocated_file_size(bs);

Please use bdrv_getlength() instead of bdrv_get_allocated_file_size(),
which checks stat.st_blocks.  From the stat(2) man page:

  The st_blocks field indicates the number of blocks  allocated  to
  the  file,  512-byte  units.  (This may be smaller than st_size/512
  when the file has holes.).

> +    if (size < 0) {
> +        return -EINVAL;
> +    }
> +
> +    buf = g_malloc0(size+1);

Spaces please:
g_malloc0(size + 1)

> +
> +    ret = bdrv_pread(bs->file, desc_offset, buf, size);
>      if (ret < 0) {
> -        return ret;
> +        goto exit;
>      }
> -    buf[2047] = '\0';

The buffer must be NUL-terminated.

>      if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
> -        return -EMEDIUMTYPE;
> +        ret = -EMEDIUMTYPE;
> +        goto exit;
>      }
>      if (strcmp(ct, "monolithicFlat") &&
>          strcmp(ct, "twoGbMaxExtentSparse") &&
>          strcmp(ct, "twoGbMaxExtentFlat")) {
>          fprintf(stderr,
>                  "VMDK: Not supported image type \"%s\""".\n", ct);
> -        return -ENOTSUP;
> +        ret = -ENOTSUP;
> +        goto exit;
>      }
>      s->desc_offset = 0;
> -    return vmdk_parse_extents(buf, bs, bs->file->filename);
> +    ret = vmdk_parse_extents(buf, bs, bs->file->filename);
> +exit:
> +    if (buf) {
> +        g_free(buf);
> +    }

The if is not necessary since g_free(NULL) is a nop.
Kevin Wolf June 12, 2013, 10:30 a.m. UTC | #2 
Am 12.06.2013 um 10:04 hat Evgeny Budilovsky geschrieben:
> The hard-coded 2k buffer on the stack won't allow reading big descriptor
> files which can be generated when storing big images (For example 500G
> vmdk splitted to 2G chunks).
> 
> Signed-off-by: Evgeny Budilovsky <evgeny.budilovsky@ravellosystems.com>
> ---
>  block/vmdk.c |   28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 608daaf..1bc944b 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -719,27 +719,41 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
>                                 int64_t desc_offset)
>  {
>      int ret;
> -    char buf[2048];
> +    char *buf = NULL;
>      char ct[128];
>      BDRVVmdkState *s = bs->opaque;
> +    int64_t size;
> 
> -    ret = bdrv_pread(bs->file, desc_offset, buf, sizeof(buf));
> +    size = bdrv_get_allocated_file_size(bs);
> +    if (size < 0) {
> +        return -EINVAL;
> +    }
> +
> +    buf = g_malloc0(size+1);

This is an unbounded allocation. Not sure if this is a good idea. Can we
restrict the maximum size to something reasonably small, like a megabyte?

Kevin
Evgeny Budilovsky June 12, 2013, 10:38 a.m. UTC | #3 
On Wed, Jun 12, 2013 at 1:17 PM, Stefan Hajnoczi <stefanha@redhat.com>wrote:

> On Wed, Jun 12, 2013 at 11:04:44AM +0300, Evgeny Budilovsky wrote:
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index 608daaf..1bc944b 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -719,27 +719,41 @@ static int vmdk_open_desc_file(BlockDriverState
> *bs, int flags,
> >                                 int64_t desc_offset)
> >  {
> >      int ret;
> > -    char buf[2048];
> > +    char *buf = NULL;
> >      char ct[128];
> >      BDRVVmdkState *s = bs->opaque;
> > +    int64_t size;
> >
> > -    ret = bdrv_pread(bs->file, desc_offset, buf, sizeof(buf));
> > +    size = bdrv_get_allocated_file_size(bs);
>
> Please use bdrv_getlength() instead of bdrv_get_allocated_file_size(),
> which checks stat.st_blocks.  From the stat(2) man page:
>
>   The st_blocks field indicates the number of blocks  allocated  to
>   the  file,  512-byte  units.  (This may be smaller than st_size/512
>   when the file has holes.).
>
> applied

>  > +    if (size < 0) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    buf = g_malloc0(size+1);
>
> Spaces please:
> g_malloc0(size + 1)
>
> applied

>  > +
> > +    ret = bdrv_pread(bs->file, desc_offset, buf, size);
> >      if (ret < 0) {
> > -        return ret;
> > +        goto exit;
> >      }
> > -    buf[2047] = '\0';
>
> The buffer must be NUL-terminated.
>
> g_malloc0 allocates buffer which is zero initialized so I can skeep the
null termination


>  >      if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
> > -        return -EMEDIUMTYPE;
> > +        ret = -EMEDIUMTYPE;
> > +        goto exit;
> >      }
> >      if (strcmp(ct, "monolithicFlat") &&
> >          strcmp(ct, "twoGbMaxExtentSparse") &&
> >          strcmp(ct, "twoGbMaxExtentFlat")) {
> >          fprintf(stderr,
> >                  "VMDK: Not supported image type \"%s\""".\n", ct);
> > -        return -ENOTSUP;
> > +        ret = -ENOTSUP;
> > +        goto exit;
> >      }
> >      s->desc_offset = 0;
> > -    return vmdk_parse_extents(buf, bs, bs->file->filename);
> > +    ret = vmdk_parse_extents(buf, bs, bs->file->filename);
> > +exit:
> > +    if (buf) {
> > +        g_free(buf);
> > +    }
>
> The if is not necessary since g_free(NULL) is a nop.
>

applied
Evgeny Budilovsky June 12, 2013, 10:41 a.m. UTC | #4 
On Wed, Jun 12, 2013 at 1:30 PM, Kevin Wolf <kwolf@redhat.com> wrote:

> Am 12.06.2013 um 10:04 hat Evgeny Budilovsky geschrieben:
> > The hard-coded 2k buffer on the stack won't allow reading big descriptor
> > files which can be generated when storing big images (For example 500G
> > vmdk splitted to 2G chunks).
> >
> > Signed-off-by: Evgeny Budilovsky <evgeny.budilovsky@ravellosystems.com>
> > ---
> >  block/vmdk.c |   28 +++++++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index 608daaf..1bc944b 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -719,27 +719,41 @@ static int vmdk_open_desc_file(BlockDriverState
> *bs, int flags,
> >                                 int64_t desc_offset)
> >  {
> >      int ret;
> > -    char buf[2048];
> > +    char *buf = NULL;
> >      char ct[128];
> >      BDRVVmdkState *s = bs->opaque;
> > +    int64_t size;
> >
> > -    ret = bdrv_pread(bs->file, desc_offset, buf, sizeof(buf));
> > +    size = bdrv_get_allocated_file_size(bs);
> > +    if (size < 0) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    buf = g_malloc0(size+1);
>
> This is an unbounded allocation. Not sure if this is a good idea. Can we
> restrict the maximum size to something reasonably small, like a megabyte?
>
> Kevin
>

yes good idea !
diff mbox

Patch

diff --git a/block/vmdk.c b/block/vmdk.c
index 608daaf..1bc944b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -719,27 +719,41 @@  static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
                                int64_t desc_offset)
 {
     int ret;
-    char buf[2048];
+    char *buf = NULL;
     char ct[128];
     BDRVVmdkState *s = bs->opaque;
+    int64_t size;

-    ret = bdrv_pread(bs->file, desc_offset, buf, sizeof(buf));
+    size = bdrv_get_allocated_file_size(bs);
+    if (size < 0) {
+        return -EINVAL;
+    }
+
+    buf = g_malloc0(size+1);
+
+    ret = bdrv_pread(bs->file, desc_offset, buf, size);
     if (ret < 0) {
-        return ret;
+        goto exit;
     }
-    buf[2047] = '\0';
     if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
-        return -EMEDIUMTYPE;
+        ret = -EMEDIUMTYPE;
+        goto exit;
     }
     if (strcmp(ct, "monolithicFlat") &&
         strcmp(ct, "twoGbMaxExtentSparse") &&
         strcmp(ct, "twoGbMaxExtentFlat")) {
         fprintf(stderr,
                 "VMDK: Not supported image type \"%s\""".\n", ct);
-        return -ENOTSUP;
+        ret = -ENOTSUP;
+        goto exit;
     }
     s->desc_offset = 0;
-    return vmdk_parse_extents(buf, bs, bs->file->filename);
+    ret = vmdk_parse_extents(buf, bs, bs->file->filename);
+exit:
+    if (buf) {
+        g_free(buf);
+    }
+    return ret;
 }

 static int vmdk_open(BlockDriverState *bs, QDict *options, int flags)