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

[Manos Pitsidianakis]

On Thu, Jun 29, 2017 at 01:18:24PM +0200, Kevin Wolf wrote:
> 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.

You can't see it in the diff but after bdrv_unref(bs), 
bdrv_new_open_driver returns NULL so there won't be any access to bs 
anyway. And since bs is destroyed by bdrv_unref (its refcount is 1), 
there's not really a point in setting bs->file = NULL.

> 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?

The result would be the same, but this will cover future callers of 
bdrv_open_driver. Should I submit a v2?
> 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).

freeing bs->file was recently removed from individual '.bdrv_open's 
since bdrv_open_inherit takes care of it 
(de234897b60e034ba94b307fc289e2dc692c9251). I think this is simpler 
since a driver could neglect to free their bs->file whereas this is a 
catchall solution.

> Kevin

signature.asc
Description: PGP signature