Re: [Qemu-devel] [PATCH v8 08/12] block: Parse "backing" option to reference existing BDS

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Dec 13, 2013 at 03:35:16PM +0800, Fam Zheng wrote:
> diff --git a/block.c b/block.c
> index b3993d7..fba7148 100644
> --- a/block.c
> +++ b/block.c
> @@ -1191,11 +1191,25 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>      /* If there is a backing file, use it */
>      if ((flags & BDRV_O_NO_BACKING) == 0) {
>          QDict *backing_options;
> -
> -        qdict_extract_subqdict(options, &backing_options, "backing.");
> -        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> -        if (ret < 0) {
> -            goto close_and_fail;
> +        const char *backing_name;
> +        BlockDriverState *backing_hd;
> +
> +        backing_name = qdict_get_try_str(options, "backing");
> +        qdict_del(options, "backing");

This causes a use-after-free since backing_name is a const char pointer
to the qdict element!

> +        if (backing_name) {
> +            backing_hd = bdrv_find(backing_name);
> +            if (!backing_hd) {
> +                error_set(&local_err, QERR_DEVICE_NOT_FOUND, backing_name);
> +                ret = -ENOENT;
> +                goto close_and_fail;
> +            }
> +            bdrv_set_backing_hd(bs, backing_hd);
> +        } else {
> +            qdict_extract_subqdict(options, &backing_options, "backing.");
> +            ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> +            if (ret < 0) {
> +                goto close_and_fail;
> +            }
>          }

Seems like users can specify backing=foo backing.file=/tmp/a and we
ignore backing.file.  Is it useful to silently ignore the backing.
subdict?  The user may have given useless options by mistake.  An error
would help prevent weird options combinations.

>      }
>  
> @@ -1682,7 +1696,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>      assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
>      assert(bs_new->job == NULL);
>      assert(bs_new->dev == NULL);
> -    assert(bdrv_op_blocker_is_empty(bs_new));
>      assert(bs_new->io_limits_enabled == false);
>      assert(!throttle_have_timer(&bs_new->throttle_state));
>  
> @@ -1701,7 +1714,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>      /* Check a few fields that should remain attached to the device */
>      assert(bs_new->dev == NULL);
>      assert(bs_new->job == NULL);
> -    assert(bdrv_op_blocker_is_empty(bs_new));
>      assert(bs_new->io_limits_enabled == false);
>      assert(!throttle_have_timer(&bs_new->throttle_state));

Why are these hunks part of this patch?  I guess it makes sense *not* to
check for blockers in bdrv_swap().  Instead the high-level functions in
blockdev.c and elsewhere should check blockers.