Re: [Qemu-devel] [PATCH v2 6/6] block: update string sizes for filename, backing_file, exact_filename

[John Snow <jsnow@redhat.com>]

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>