From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59082) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBLX3-0005oM-Qz for qemu-devel@nongnu.org; Wed, 14 Jan 2015 05:50:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YBLWy-0000jC-GP for qemu-devel@nongnu.org; Wed, 14 Jan 2015 05:50:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57607) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBLWy-0000j4-90 for qemu-devel@nongnu.org; Wed, 14 Jan 2015 05:50:32 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0EAoVlJ017284 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 14 Jan 2015 05:50:31 -0500 Date: Wed, 14 Jan 2015 11:50:28 +0100 From: Kevin Wolf Message-ID: <20150114105028.GF5136@noname.redhat.com> References: <54B584E7.2090500@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54B584E7.2090500@redhat.com> Subject: Re: [Qemu-devel] [PATCH] block: update string sizes for filename, backing_file, exact_filename List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: Jeff Cody , famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com Am 13.01.2015 um 21:49 hat John Snow geschrieben: > > > On 01/13/2015 12:03 PM, Jeff Cody wrote: > >The string field entries 'filename', 'backing_file', and > >'exact_filename' in the BlockDriverState struct are defined as 1024 > >bytes. > > > >However, most places that use these values accept a maximum of PATH_MAX > >bytes. This patch makes the BlockDriverStruct field string sizes match > >the most common usage. > > > >This patch also updates two block drivers that still use 1024-byte sized > >arrays for 'backing_file'. > > > >Signed-off-by: Jeff Cody > >--- > > block/mirror.c | 2 +- > > block/qapi.c | 2 +- > > include/block/block_int.h | 8 ++++---- > > 3 files changed, 6 insertions(+), 6 deletions(-) > > > >diff --git a/block/mirror.c b/block/mirror.c > >index 9019d1b..57154eb 100644 > >--- a/block/mirror.c > >+++ b/block/mirror.c > >@@ -378,7 +378,7 @@ static void coroutine_fn mirror_run(void *opaque) > > int64_t sector_num, end, sectors_per_chunk, length; > > uint64_t last_pause_ns; > > BlockDriverInfo bdi; > >- char backing_filename[1024]; > >+ char backing_filename[PATH_MAX]; > > int ret = 0; > > int n; > > > >diff --git a/block/qapi.c b/block/qapi.c > >index a6fd6f7..c097238 100644 > >--- a/block/qapi.c > >+++ b/block/qapi.c > >@@ -175,7 +175,7 @@ void bdrv_query_image_info(BlockDriverState *bs, > > { > > int64_t size; > > const char *backing_filename; > >- char backing_filename2[1024]; > >+ char backing_filename2[PATH_MAX]; > > BlockDriverInfo bdi; > > int ret; > > Error *err = NULL; We shouldn't have had paths on the stack before this patch, and after increasing the array size, we should have them even less. Actually, mirror_run() could probably do with a char[2] or even access bs->backing_file directly without copying things around. It has that array only so it can check that backing_filename isn't empty. > >diff --git a/include/block/block_int.h b/include/block/block_int.h > >index 06a21dd..e264be9 100644 > >--- a/include/block/block_int.h > >+++ b/include/block/block_int.h > >@@ -339,13 +339,13 @@ struct BlockDriverState { > > * regarding this BDS's context */ > > QLIST_HEAD(, BdrvAioNotifier) aio_notifiers; > > > >- char filename[1024]; > >- char backing_file[1024]; /* if non zero, the image is a diff of > >- this file image */ > >+ char filename[PATH_MAX]; > >+ char backing_file[PATH_MAX]; /* if non zero, the image is a diff of > >+ this file image */ > > char backing_format[16]; /* if non-zero and backing_file exists */ > > > > QDict *full_open_options; > >- char exact_filename[1024]; > >+ char exact_filename[PATH_MAX]; > > > > BlockDriverState *backing_hd; > > BlockDriverState *file; > > > > Is it important that qcow2_open seems to enforce a 1023-char length > backing_file name? > > From qcow2.c, qcow2_open (currently line ~871): > if (len > MIN(1023, s->cluster_size - > header.backing_file_offset)) { It is relevant for PATH_MAX < 1023. In such cases we have a buffer overflow now. For cases where PATH_MAX > 1023: The limitation has become part of the file format definition, so we can't remove it (otherwise older qemu versions wouldn't be able to read the image). Kevin