From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51368) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrQbX-0006Cb-BS for qemu-devel@nongnu.org; Tue, 25 Jun 2013 06:36:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UrQWW-0001Vt-01 for qemu-devel@nongnu.org; Tue, 25 Jun 2013 06:31:02 -0400 Received: from omzsmtpe01.verizonbusiness.com ([199.249.25.210]:10766) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrQIZ-0005QI-Te for qemu-devel@nongnu.org; Tue, 25 Jun 2013 06:16:32 -0400 From: Don Slutz Message-ID: <51C96DFA.8050908@terremark.com> Date: Tue, 25 Jun 2013 06:16:26 -0400 MIME-Version: 1.0 References: <1371020901-12782-1-git-send-email-evgeny.budilovsky@ravellosystems.com> <51BA3681.3020003@terremark.com> In-Reply-To: Content-Type: multipart/alternative; boundary="------------040706050907000600030607" Subject: Re: [Qemu-devel] [PATCH] allow reading variable size vmdk descriptor files List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Evgeny Budilovsky Cc: qemu-devel@nongnu.org, Don Slutz --------------040706050907000600030607 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit On 06/14/13 02:41, Evgeny Budilovsky wrote: > > > > On Fri, Jun 14, 2013 at 12:15 AM, Don Slutz > wrote: > > On 06/12/13 03:08, Evgeny Budilovsky wrote: > > 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 > > > --- > 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; > + } > + > > While this is right for vmdk splitted to 2G chunks, I think this > will fail for a big enough "monolithicFlat" vmdk where there is > only the 1 file (g_malloc() will most likely fail for a 500GB file). > > With the "monolithicFlat" vmdk the descriptor file is a small textual > file. So this code should work. In the second version of this patch > I've added some constraint to the allocation size just in case the > file is corrupted or we have misinterpreted the format. > Opps, I did the wrong one. Both createType="streamOptimized" and createType="monolithicSparse" are only 1 file. > size = MIN(size, 1 << 20); /* avoid unbounded allocation */ This will "fix" the issue. -Don Slutz > buf = g_malloc0(size + 1); > > + 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) > -- > 1.7.9.5 > > -Don Slutz > > > > > -- > Best Regards, > Evgeny --------------040706050907000600030607 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 06/14/13 02:41, Evgeny Budilovsky wrote:



On Fri, Jun 14, 2013 at 12:15 AM, Don Slutz <dslutz@verizon.com> wrote:
On 06/12/13 03:08, Evgeny Budilovsky wrote:
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;
+    }
+
While this is right for vmdk splitted to 2G chunks, I think this will fail for a big enough "monolithicFlat" vmdk where there is only the 1 file (g_malloc() will most likely fail for a 500GB file).

With the "monolithicFlat" vmdk the descriptor file is a small textual file. So this code should work. In the second version of this patch I've added some constraint to the allocation size just in case the file is corrupted or we have misinterpreted the format.

Opps, I did the wrong one.  Both createType="streamOptimized" and createType="monolithicSparse" are only 1 file.
size = MIN(size, 1 << 20);  /* avoid unbounded allocation */
This will "fix" the issue.
   -Don Slutz
buf = g_malloc0(size + 1);

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

   -Don Slutz



--
Best Regards,
Evgeny

--------------040706050907000600030607--