Update filename string sizes in block layer
v3 cover Update filename string sizes in block layer
The block layer uses a mixture of 'PATH_MAX' and '1024' string sizes for filenames (and backing filenames).
This series consolidates all that usage to 'PATH_MAX'. Since most platforms (especially the most common platforms for QEMU) have a PATH_MAX larger than 1024 bytes, this series also changes stack allocations of PATH_MAX to be dynamically allocated.
Note: checkpatch.pl complains about an extra space in a printf in
patches 1 & 2. The lines complained about are in the diff context and not the actual changes, so I did not fix them up to satisfy checkpatch.
Changes from v3:
- simplified extent_path handling in vmdk_parse_extents() (Thanks Stefan)
- moved declaration of backing_filename2 to inside if statement in bdrv_query_image_info() (Thanks Stefan)
- removed zombie variable in bdrv_commit (Thanks Stefan)
- fixed typo in commit message (Thanks Stefan)
Changes from v2:
- Change stack allocations to dybnamic (Thanks Kevin)
- Update qcow/qcow2 ti perform safety checks for platforms that have a PATH_MAX < 1024 (thanks John, Kevin).
Jeff Cody (6):
block: vmdk - make ret variable usage clear
block: vmdk - move string allocations from stack to the heap
block: qapi - move string allocation from stack to the heap
block: remove unused variable in bdrv_commit
block: mirror - change string allocation to 2-bytes
block: update string sizes for filename,backing_file,exact_filename
block.c | 3 --- block/mirror.c | 3 ++- block/qapi.c | 7 ++++--- block/qcow.c | 2 +- block/qcow2.c | 3 ++- block/vmdk.c | 51 ++++++++++++++++++++++++++++------------------- block/vvfat.c | 4 ++-- include/block/block_int.h | 8 ++++---- qemu-img.c | 4 ++-- 9 files changed, 47 insertions(+), 38 deletions(-)
Signature · 1 line
1.9.3
PATCH v3 1/6 block: vmdk - make ret variable usage clear
Keep the variable 'ret' something that is returned by the function it is defined in. For the return value of 'sscanf', use a more meaningful variable name.
--- block/vmdk.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
block/vmdk.c +8 -6
@@ -785,6 +785,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, const char *desc_file_path, Error **errp) { int ret; +int matches;char access[11 ];char type[11 ];char fname[512 ];
@@ -796,6 +797,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,BDRVVmdkState *s = bs->opaque;VmdkExtent *extent; +while (*p) {/* parse extent line in one of below formats: *
@@ -805,23 +807,23 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,* RW [size in sectors] VMFSSPARSE "file-name.vmdk" */ flat_offset =-1 ; - ret =sscanf (p,"%10s %" SCNd64" %10s \"%511[^\n\r\"]\" %" SCNd64, - access, §ors, type, fname, &flat_offset); -if (ret <4 ||strcmp (access,"RW" )) { + matches =sscanf (p,"%10s %" SCNd64" %10s \"%511[^\n\r\"]\" %" SCNd64, + access, §ors, type, fname, &flat_offset); +if (matches <4 ||strcmp (access,"RW" )) { goto next_line; }else if (!strcmp (type,"FLAT" )) { -if (ret !=5 || flat_offset <0 ) { +if (matches !=5 || flat_offset <0 ) {error_setg (errp,"Invalid extent lines: \n%s" , p);return -EINVAL ; } }else if (!strcmp (type,"VMFS" )) { -if (ret ==4 ) { +if (matches ==4 ) { flat_offset =0 ; }else {error_setg (errp,"Invalid extent lines:\n%s" , p);return -EINVAL ; } - }else if (ret !=4 ) { + }else if (matches !=4 ) {error_setg (errp,"Invalid extent lines:\n%s" , p);return -EINVAL ; }
Signature · 1 line
1.9.3
PATCH v3 2/6 block: vmdk - move string allocations from stack to the heap
In reply to Jeff CodyFunctions 'vmdk_parse_extents' and 'vmdk_create' allocate several PATH_MAX sized arrays on the stack. Make these dynamically allocated.
--- block/vmdk.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-)
block/vmdk.c +23 -16
@@ -792,12 +792,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,const char *p = desc;int64_t sectors =0 ;int64_t flat_offset; -char extent_path[PATH_MAX ]; +char *extent_path;BlockDriverState *extent_file;BDRVVmdkState *s = bs->opaque;VmdkExtent *extent; -while (*p) {/* parse extent line in one of below formats: *
@@ -843,11 +842,13 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,return -EINVAL ; } + extent_path =g_malloc0 (PATH_MAX );path_combine (extent_path,sizeof (extent_path), desc_file_path, fname); extent_file =NULL ; ret =bdrv_open (&extent_file, extent_path,NULL ,NULL , bs->open_flags |BDRV_O_PROTOCOL ,NULL , errp); +g_free (extent_path);if (ret) {return ret; }
@@ -1797,10 +1798,15 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)int ret =0 ;bool flat, split, compress;GString *ext_desc_lines; -char path[PATH_MAX ], prefix[PATH_MAX ], postfix[PATH_MAX ]; +char *path =g_malloc0 (PATH_MAX ); +char *prefix =g_malloc0 (PATH_MAX ); +char *postfix =g_malloc0 (PATH_MAX ); +char *desc_line =g_malloc0 (BUF_SIZE ); +char *ext_filename =g_malloc0 (PATH_MAX ); +char *desc_filename =g_malloc0 (PATH_MAX );const int64_t split_size =0x80000000 ;/* VMDK has constant split size */ const char *desc_extent_line; -char parent_desc_line[BUF_SIZE ] ="" ; +char *parent_desc_line =g_malloc0 (BUF_SIZE );uint32_t parent_cid =0xffffffff ;uint32_t number_heads =16 ;bool zeroed_grain = false;
@@ -1916,33 +1922,27 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) } parent_cid =vmdk_read_cid (bs,0 );bdrv_unref (bs); -snprintf (parent_desc_line,sizeof (parent_desc_line), +snprintf (parent_desc_line,BUF_SIZE ,"parentFileNameHint=\"%s\"" , backing_file); }/* Create extents */ filesize = total_size;while (filesize >0 ) { -char desc_line[BUF_SIZE ]; -char ext_filename[PATH_MAX ]; -char desc_filename[PATH_MAX ];int64_t size = filesize;if (split && size > split_size) { size = split_size; }if (split) { -snprintf (desc_filename,sizeof (desc_filename),"%s-%c%03d%s" , +snprintf (desc_filename,PATH_MAX ,"%s-%c%03d%s" , prefix, flat ?'f' :'s' , ++idx, postfix); }else if (flat) { -snprintf (desc_filename,sizeof (desc_filename),"%s-flat%s" , - prefix, postfix); +snprintf (desc_filename,PATH_MAX ,"%s-flat%s" , prefix, postfix); }else { -snprintf (desc_filename,sizeof (desc_filename),"%s%s" , - prefix, postfix); +snprintf (desc_filename,PATH_MAX ,"%s%s" , prefix, postfix); } -snprintf (ext_filename,sizeof (ext_filename),"%s%s" , - path, desc_filename); +snprintf (ext_filename,PATH_MAX ,"%s%s" , path, desc_filename);if (vmdk_create_extent (ext_filename, size, flat, compress, zeroed_grain, opts, errp)) {
@@ -1952,7 +1952,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) filesize -= size;/* Format description line */ -snprintf (desc_line,sizeof (desc_line), +snprintf (desc_line,BUF_SIZE , desc_extent_line, size /BDRV_SECTOR_SIZE , desc_filename);g_string_append (ext_desc_lines, desc_line); }
@@ -2007,6 +2007,13 @@ exit:g_free (backing_file);g_free (fmt);g_free (desc); +g_free (path); +g_free (prefix); +g_free (postfix); +g_free (desc_line); +g_free (ext_filename); +g_free (desc_filename); +g_free (parent_desc_line);g_string_free (ext_desc_lines, true);return ret; }
Signature · 1 line
1.9.3
PATCH v3 3/6 block: qapi - move string allocation from stack to the heap
In reply to Jeff CodyRather than declaring 'backing_filename2' on the stack in bdrv_query_image_info(), dynamically allocate it on the heap.
--- block/qapi.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
block/qapi.c +4 -3
@@ -175,7 +175,6 @@ void bdrv_query_image_info(BlockDriverState *bs, {int64_t size;const char *backing_filename; -char backing_filename2[1024 ];BlockDriverInfo bdi;int ret;Error *err =NULL ;
@@ -211,13 +210,14 @@ void bdrv_query_image_info(BlockDriverState *bs, backing_filename = bs->backing_file;if (backing_filename[0 ] !='\0' ) { +char *backing_filename2 =g_malloc0 (1024 ); info->backing_filename =g_strdup (backing_filename); info->has_backing_filename = true; -bdrv_get_full_backing_filename (bs, backing_filename2, -sizeof (backing_filename2), &err); +bdrv_get_full_backing_filename (bs, backing_filename2,1024 , &err);if (err) {error_propagate (errp, err);qapi_free_ImageInfo (info); +g_free (backing_filename2);return ; }
@@ -231,6 +231,7 @@ void bdrv_query_image_info(BlockDriverState *bs, info->backing_filename_format =g_strdup (bs->backing_format); info->has_backing_filename_format = true; } +g_free (backing_filename2); } ret =bdrv_query_snapshot_info_list (bs, &info->snapshots, &err);
Signature · 1 line
1.9.3
PATCH v3 5/6 block: mirror - change string allocation to 2-bytes
In reply to Jeff CodyThe backing_filename string in mirror_run() is only used to check for a NULL string, so we don't need to allocate 1024 bytes (or, later, PATH_MAX bytes), when we only need to copy the first 2 characters.
We technically only need 1 byte, as we are just checking for NULL, but since backing_filename[] is populated by bdrv_get_backing_filename(), a string size of 1 will always only return '\0';
--- block/mirror.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
block/mirror.c +2 -1
@@ -378,7 +378,8 @@ 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[2 ];/* we only need 2 characters because we are only +checking for a NULL string */ int ret =0 ;int n;
Signature · 1 line
1.9.3
PATCH v3 4/6 block: remove unused variable in bdrv_commit
In reply to Jeff CodyAs Stefan pointed out, the variable 'filename' in bdrv_commit is unused, despite being maintained in previous patches.
With this patch, get rid of the variable for good.
--- block.c | 3 --- 1 file changed, 3 deletions(-)
block.c +0 -3
@@ -2207,7 +2207,6 @@ int bdrv_commit(BlockDriverState *bs)int n, ro, open_flags;int ret =0 ;uint8_t *buf =NULL ; -char filename[PATH_MAX ];if (!drv)return -ENOMEDIUM ;
@@ -2222,8 +2221,6 @@ int bdrv_commit(BlockDriverState *bs) } ro = bs->backing_hd->read_only; -/* Use pstrcpy (not strncpy): filename must be NUL-terminated. */ -pstrcpy (filename,sizeof (filename), bs->backing_hd->filename); open_flags = bs->backing_hd->open_flags;if (ro) {
Signature · 1 line
1.9.3
PATCH v3 6/6 block: update string sizes for filename, backing_file, exact_filename
In reply to Jeff CodyThe 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.
--- 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(-)
block/qapi.c +2 -2
@@ -210,10 +210,10 @@ void bdrv_query_image_info(BlockDriverState *bs, backing_filename = bs->backing_file;if (backing_filename[0 ] !='\0' ) { -char *backing_filename2 =g_malloc0 (1024 ); +char *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);
block/qcow.c +1 -1
@@ -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;
block/qcow2.c +2 -1
@@ -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;
block/vvfat.c +2 -2
@@ -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;
include/block/block_int.h +4 -4
@@ -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;
qemu-img.c +2 -2
@@ -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",
Signature · 1 line
1.9.3
Kevin Wolf
AppliedAm 22.01.2015 um 14:03 hat Jeff Cody geschrieben:
Quoted 19 lines
The block layer uses a mixture of '
PATH_MAX' and '1024' string sizes for filenames (and backing filenames).This series consolidates all that usage to '
PATH_MAX'. Since most platforms (especially the most common platforms for QEMU) have aPATH_MAXlarger than 1024 bytes, this series also changes stack allocations ofPATH_MAXto be dynamically allocated.Note: checkpatch.pl complains about an extra space in a printf in
patches 1 & 2. The lines complained about are in the diff context and not the actual changes, so I did not fix them up to satisfy checkpatch.Changes from v3:
- simplifiedextent_pathhandling invmdk_parse_extents()(Thanks Stefan)
- moved declaration ofbacking_filename2to inside if statement inbdrv_query_image_info()(Thanks Stefan)
- removed zombie variable inbdrv_commit(Thanks Stefan)
- fixed typo in commit message (Thanks Stefan)
Quoted 5 lines
Changes from v2:
- Change stack allocations to dybnamic (Thanks Kevin)
- Update qcow/qcow2 ti perform safety checks for platforms that have aPATH_MAX< 1024 (thanks John, Kevin).
Thanks, applied to the block branch.
Kevin
Re: [Qemu-devel] [PATCH v3 2/6] block: vmdk - move string allocations from stack to the heap
In reply to Jeff CodyQuoted 27 lines · Jeff Cody
Functions '
vmdk_parse_extents' and 'vmdk_create' allocate severalPATH_MAXsized arrays on the stack. Make these dynamically allocated.Signed-off-by Jeff Cody <[email protected]>--- block/vmdk.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-)block/vmdk.c +1
-2@@ -792,12 +792,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,const char *p = desc;int64_t sectors =0 ;int64_t flat_offset; -char extent_path[PATH_MAX ]; +char *extent_path;BlockDriverState *extent_file;BDRVVmdkState *s = bs->opaque;VmdkExtent *extent; -while (*p) {/* parse extent line in one of below formats: *
block/vmdk.c static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, · line 846
Quoted 5 lines
block/vmdk.c +1
-0@@ -843,11 +842,13 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,return -EINVAL ; } + extent_path =g_malloc0 (PATH_MAX ); path_combine(extent_path, sizeof(extent_path),
Oops, sizeof(extent_path) changed from PATH_MAX to sizeof(char*). Coverity found this instance, I didn't check for others.
Paolo
Quoted 89 lines
block/vmdk.c +21
-14desc_file_path, fname); extent_file =NULL ; ret =bdrv_open (&extent_file, extent_path,NULL ,NULL , bs->open_flags |BDRV_O_PROTOCOL ,NULL , errp); +g_free (extent_path);if (ret) {return ret; }@@ -1797,10 +1798,15 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)int ret =0 ;bool flat, split, compress;GString *ext_desc_lines; -char path[PATH_MAX ], prefix[PATH_MAX ], postfix[PATH_MAX ]; +char *path =g_malloc0 (PATH_MAX ); +char *prefix =g_malloc0 (PATH_MAX ); +char *postfix =g_malloc0 (PATH_MAX ); +char *desc_line =g_malloc0 (BUF_SIZE ); +char *ext_filename =g_malloc0 (PATH_MAX ); +char *desc_filename =g_malloc0 (PATH_MAX );const int64_t split_size =0x80000000 ;/* VMDK has constant split size */ const char *desc_extent_line; -char parent_desc_line[BUF_SIZE ] ="" ; +char *parent_desc_line =g_malloc0 (BUF_SIZE );uint32_t parent_cid =0xffffffff ;uint32_t number_heads =16 ;bool zeroed_grain = false;@@ -1916,33 +1922,27 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) } parent_cid =vmdk_read_cid (bs,0 );bdrv_unref (bs); -snprintf (parent_desc_line,sizeof (parent_desc_line), +snprintf (parent_desc_line,BUF_SIZE ,"parentFileNameHint=\"%s\"" , backing_file); }/* Create extents */ filesize = total_size;while (filesize >0 ) { -char desc_line[BUF_SIZE ]; -char ext_filename[PATH_MAX ]; -char desc_filename[PATH_MAX ];int64_t size = filesize;if (split && size > split_size) { size = split_size; }if (split) { -snprintf (desc_filename,sizeof (desc_filename),"%s-%c%03d%s" , +snprintf (desc_filename,PATH_MAX ,"%s-%c%03d%s" , prefix, flat ?'f' :'s' , ++idx, postfix); }else if (flat) { -snprintf (desc_filename,sizeof (desc_filename),"%s-flat%s" , - prefix, postfix); +snprintf (desc_filename,PATH_MAX ,"%s-flat%s" , prefix, postfix); }else { -snprintf (desc_filename,sizeof (desc_filename),"%s%s" , - prefix, postfix); +snprintf (desc_filename,PATH_MAX ,"%s%s" , prefix, postfix); } -snprintf (ext_filename,sizeof (ext_filename),"%s%s" , - path, desc_filename); +snprintf (ext_filename,PATH_MAX ,"%s%s" , path, desc_filename);if (vmdk_create_extent (ext_filename, size, flat, compress, zeroed_grain, opts, errp)) {@@ -1952,7 +1952,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp) filesize -= size;/* Format description line */ -snprintf (desc_line,sizeof (desc_line), +snprintf (desc_line,BUF_SIZE , desc_extent_line, size /BDRV_SECTOR_SIZE , desc_filename);g_string_append (ext_desc_lines, desc_line); }@@ -2007,6 +2007,13 @@ exit:g_free (backing_file);g_free (fmt);g_free (desc); +g_free (path); +g_free (prefix); +g_free (postfix); +g_free (desc_line); +g_free (ext_filename); +g_free (desc_filename); +g_free (parent_desc_line);g_string_free (ext_desc_lines, true);return ret; }