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.
