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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2996 bytes --]

Am 29.06.2017 um 14:07 hat Manos Pitsidianakis geschrieben:
> 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 <el13635@mail.ntua.gr>
> >>---
> >> 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.

Yes, but bdrv_unref() doesn't have to expect inconsistent BDSes. It
doesn't access bs->file currently when bs->drv == NULL, but that's more
by luck than by design.

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

I would prefer this, yes.

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

That commit just changed an inconsistent situation where some drivers
did free it and other didn't. But we don't have to move it into the
drivers, it's just an option. As long as bs->file is special cased for
many other things, too, keeping it centrally might make sense.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]