| Message ID | e30df0f1b5fdb73afaa38de419a8980d0a0d1696.1421168564.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jcody@redhat.com> > --- > 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; > 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)) {
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 <jcody@redhat.com> > >--- > > 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
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; 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;
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 <jcody@redhat.com> --- block/mirror.c | 2 +- block/qapi.c | 2 +- include/block/block_int.h | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-)