| Message ID | 1673c994d5fc21a711576a5fbce49c68ec71ba2a.1421768887.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/20/2015 12:31 PM, Jeff Cody wrote: > The string field entries 'filename', 'backing_file', and > 'exact_filename' in the BlockDriverState struct are defined as 1024 > bytes. > > However, many places that use these values accept a maximum of PATH_MAX > bytes, so we have a mixture of 1024 byte and PATH_MAX byte allocations. > This patch makes the BlockDriverStruct field string sizes match usage. > > This patch also does a few fixes related to the size that needs to > happen now: > > * the block qapi driver is updated to use PATH_MAX bytes > * the qcow and qcow2 drivers have an additional safety check > * the block vvfat driver is updated to use PATH_MAX bytes > for the size of backing_file, for systems where PATH_MAX is < 1024 > bytes. > * qemu-img uses PATH_MAX rather than 1024. These instances were not > changed to be dynamically allocated, however, as the extra > temporary 3K in stack usage for qemu-img does not seem worrisome. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/qapi.c | 4 ++-- > block/qcow.c | 2 +- > block/qcow2.c | 3 ++- > block/vvfat.c | 4 ++-- > include/block/block_int.h | 8 ++++---- > qemu-img.c | 4 ++-- > 6 files changed, 13 insertions(+), 12 deletions(-) > > diff --git a/block/qapi.c b/block/qapi.c > index e51bade..a34ac5c 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -211,10 +211,10 @@ void bdrv_query_image_info(BlockDriverState *bs, > > backing_filename = bs->backing_file; > if (backing_filename[0] != '\0') { > - backing_filename2 = g_malloc0(1024); > + backing_filename2 = g_malloc0(PATH_MAX); > info->backing_filename = g_strdup(backing_filename); > info->has_backing_filename = true; > - bdrv_get_full_backing_filename(bs, backing_filename2, 1024, &err); > + bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err); > if (err) { > error_propagate(errp, err); > qapi_free_ImageInfo(info); > diff --git a/block/qcow.c b/block/qcow.c > index ece2269..ccbe9e0 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -215,7 +215,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, > /* read the backing file name */ > if (header.backing_file_offset != 0) { > len = header.backing_file_size; > - if (len > 1023) { > + if (len > 1023 || len > sizeof(bs->backing_file)) { > error_setg(errp, "Backing file name too long"); > ret = -EINVAL; > goto fail; > diff --git a/block/qcow2.c b/block/qcow2.c > index e4e690a..dbaf016 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -868,7 +868,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > /* read the backing file name */ > if (header.backing_file_offset != 0) { > len = header.backing_file_size; > - if (len > MIN(1023, s->cluster_size - header.backing_file_offset)) { > + if (len > MIN(1023, s->cluster_size - header.backing_file_offset) || > + len > sizeof(bs->backing_file)) { > error_setg(errp, "Backing file name too long"); > ret = -EINVAL; > goto fail; > diff --git a/block/vvfat.c b/block/vvfat.c > index e34a789..a1a44f0 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -2909,8 +2909,8 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp) > > array_init(&(s->commits), sizeof(commit_t)); > > - s->qcow_filename = g_malloc(1024); > - ret = get_tmp_filename(s->qcow_filename, 1024); > + s->qcow_filename = g_malloc(PATH_MAX); > + ret = get_tmp_filename(s->qcow_filename, PATH_MAX); > if (ret < 0) { > error_setg_errno(errp, -ret, "can't create temporary file"); > goto err; > 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; > diff --git a/qemu-img.c b/qemu-img.c > index 7876258..4e9a7f5 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -2556,7 +2556,7 @@ static int img_rebase(int argc, char **argv) > > /* For safe rebasing we need to compare old and new backing file */ > if (!unsafe) { > - char backing_name[1024]; > + char backing_name[PATH_MAX]; > > blk_old_backing = blk_new_with_bs("old_backing", &error_abort); > bs_old_backing = blk_bs(blk_old_backing); > @@ -2614,7 +2614,7 @@ static int img_rebase(int argc, char **argv) > } > old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing); > if (old_backing_num_sectors < 0) { > - char backing_name[1024]; > + char backing_name[PATH_MAX]; > > bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name)); > error_report("Could not get size of '%s': %s", > Reviewed-by: John Snow <jsnow@redhat.com>
On Tue, Jan 20, 2015 at 12:31:33PM -0500, Jeff Cody wrote: > The string field entries 'filename', 'backing_file', and > 'exact_filename' in the BlockDriverState struct are defined as 1024 > bytes. > > However, many places that use these values accept a maximum of PATH_MAX > bytes, so we have a mixture of 1024 byte and PATH_MAX byte allocations. > This patch makes the BlockDriverStruct field string sizes match usage. > > This patch also does a few fixes related to the size that needs to > happen now: > > * the block qapi driver is updated to use PATH_MAX bytes > * the qcow and qcow2 drivers have an additional safety check > * the block vvfat driver is updated to use PATH_MAX bytes > for the size of backing_file, for systems where PATH_MAX is < 1024 > bytes. > * qemu-img uses PATH_MAX rather than 1024. These instances were not > changed to be dynamically allocated, however, as the extra > temporary 3K in stack usage for qemu-img does not seem worrisome. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/qapi.c | 4 ++-- > block/qcow.c | 2 +- > block/qcow2.c | 3 ++- > block/vvfat.c | 4 ++-- > include/block/block_int.h | 8 ++++---- > qemu-img.c | 4 ++-- > 6 files changed, 13 insertions(+), 12 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/block/qapi.c b/block/qapi.c index e51bade..a34ac5c 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -211,10 +211,10 @@ void bdrv_query_image_info(BlockDriverState *bs, backing_filename = bs->backing_file; if (backing_filename[0] != '\0') { - backing_filename2 = g_malloc0(1024); + backing_filename2 = g_malloc0(PATH_MAX); info->backing_filename = g_strdup(backing_filename); info->has_backing_filename = true; - bdrv_get_full_backing_filename(bs, backing_filename2, 1024, &err); + bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err); if (err) { error_propagate(errp, err); qapi_free_ImageInfo(info); diff --git a/block/qcow.c b/block/qcow.c index ece2269..ccbe9e0 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -215,7 +215,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, /* read the backing file name */ if (header.backing_file_offset != 0) { len = header.backing_file_size; - if (len > 1023) { + if (len > 1023 || len > sizeof(bs->backing_file)) { error_setg(errp, "Backing file name too long"); ret = -EINVAL; goto fail; diff --git a/block/qcow2.c b/block/qcow2.c index e4e690a..dbaf016 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -868,7 +868,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, /* read the backing file name */ if (header.backing_file_offset != 0) { len = header.backing_file_size; - if (len > MIN(1023, s->cluster_size - header.backing_file_offset)) { + if (len > MIN(1023, s->cluster_size - header.backing_file_offset) || + len > sizeof(bs->backing_file)) { error_setg(errp, "Backing file name too long"); ret = -EINVAL; goto fail; diff --git a/block/vvfat.c b/block/vvfat.c index e34a789..a1a44f0 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2909,8 +2909,8 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp) array_init(&(s->commits), sizeof(commit_t)); - s->qcow_filename = g_malloc(1024); - ret = get_tmp_filename(s->qcow_filename, 1024); + s->qcow_filename = g_malloc(PATH_MAX); + ret = get_tmp_filename(s->qcow_filename, PATH_MAX); if (ret < 0) { error_setg_errno(errp, -ret, "can't create temporary file"); goto err; 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; diff --git a/qemu-img.c b/qemu-img.c index 7876258..4e9a7f5 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2556,7 +2556,7 @@ static int img_rebase(int argc, char **argv) /* For safe rebasing we need to compare old and new backing file */ if (!unsafe) { - char backing_name[1024]; + char backing_name[PATH_MAX]; blk_old_backing = blk_new_with_bs("old_backing", &error_abort); bs_old_backing = blk_bs(blk_old_backing); @@ -2614,7 +2614,7 @@ static int img_rebase(int argc, char **argv) } old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing); if (old_backing_num_sectors < 0) { - char backing_name[1024]; + char backing_name[PATH_MAX]; bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name)); error_report("Could not get size of '%s': %s",
The string field entries 'filename', 'backing_file', and 'exact_filename' in the BlockDriverState struct are defined as 1024 bytes. However, many places that use these values accept a maximum of PATH_MAX bytes, so we have a mixture of 1024 byte and PATH_MAX byte allocations. This patch makes the BlockDriverStruct field string sizes match usage. This patch also does a few fixes related to the size that needs to happen now: * the block qapi driver is updated to use PATH_MAX bytes * the qcow and qcow2 drivers have an additional safety check * the block vvfat driver is updated to use PATH_MAX bytes for the size of backing_file, for systems where PATH_MAX is < 1024 bytes. * qemu-img uses PATH_MAX rather than 1024. These instances were not changed to be dynamically allocated, however, as the extra temporary 3K in stack usage for qemu-img does not seem worrisome. Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/qapi.c | 4 ++-- block/qcow.c | 2 +- block/qcow2.c | 3 ++- block/vvfat.c | 4 ++-- include/block/block_int.h | 8 ++++---- qemu-img.c | 4 ++-- 6 files changed, 13 insertions(+), 12 deletions(-)