From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:54065)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1Umlpk-0004U8-J4
	for qemu-devel@nongnu.org; Wed, 12 Jun 2013 10:15:35 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1Umlph-0006vg-PO
	for qemu-devel@nongnu.org; Wed, 12 Jun 2013 10:15:32 -0400
Received: from mx1.redhat.com ([209.132.183.28]:30607)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1Umlph-0006vU-I4
	for qemu-devel@nongnu.org; Wed, 12 Jun 2013 10:15:29 -0400
Date: Wed, 12 Jun 2013 16:15:27 +0200
From: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20130612141527.GA17741@stefanha-thinkpad.muc.redhat.com>
References: <1371024284-3572-1-git-send-email-evgeny.budilovsky@ravellosystems.com>
	<1371035190-13328-1-git-send-email-evgeny.budilovsky@ravellosystems.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1371035190-13328-1-git-send-email-evgeny.budilovsky@ravellosystems.com>
Subject: Re: [Qemu-devel] [PATCH v2] allow reading variable size vmdk
 descriptor files
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Evgeny Budilovsky <evgeny.budilovsky@ravellosystems.com>
Cc: Kevin Wolf <kwolf@redhat.com>, hvx@ravellosystems.com, qemu-devel@nongnu.org

On Wed, Jun 12, 2013 at 02:06:30PM +0300, Evgeny Budilovsky wrote:
> @@ -719,27 +719,40 @@ 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_getlength(bs->file);
> +    if (size < 0) {
> +        return -EINVAL;
> +    }
> +
> +    size = MIN(size, 1 << 20);  /* avoid unbounded allocation */

I think this is okay, initially I was worried that this function might
be called to probe files which we have not identified as descriptor
files yet (they could be big!).

But looking at the callers, it should be reasonable to read up to 1 MB.
(Would have been bad to read 1 MB just to check if this parses as a
descriptor file.)

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

Thanks for pointing out the g_malloc0(), I missed it.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>