Re: [Qemu-devel] [PATCH] block: fix bs->file leak in bdrv_new_open_driver()

[Kevin Wolf]

Am 29.06.2017 um 08:03 hat Manos Pitsidianakis geschrieben:
> bdrv_open_driver() is called in two places, bdrv_new_open_driver() and
> bdrv_open_common(). In the latter, failure cleanup in is in its caller,
> bdrv_open_inherit(), which unrefs the bs->file of the failed driver open
> if it exists. Let's check for this in bdrv_new_open_driver() as well.
> 
> Signed-off-by: Manos Pitsidianakis <address@hidden>
> ---
>  block.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 694396281b..aeacd520e0 100644
> --- a/block.c
> +++ b/block.c
> @@ -1165,6 +1165,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver 
> *drv, const char *node_name,
>  
>      ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
>      if (ret < 0) {
> +        if (bs->file != NULL) {
> +            bdrv_unref_child(bs, bs->file);
> +        }
>          QDECREF(bs->explicit_options);
>          QDECREF(bs->options);
>          bdrv_unref(bs);

I think we should set bs->file = NULL here to remove the dangling
pointer. I think it is never accessed anyway because of the
bs->drv = NULL in the error path of bdrv_open_driver(), but better safe
than sorry.

But what would you think about avoiding the code duplication and just
moving the bdrv_unref_child() call from bdrv_open_inherit() down to
bdrv_open_driver(), so that bdrv_new_open_driver() is automatically
covered?

And later we can maybe move it into the individual .bdrv_open
implementations where it really belongs (whoever creates something is
responsible for cleaning it up in error cases).

Kevin