Re: [Qemu-block] [PATCH v2] block: fix leaks in bdrv_open_driver()

[Manos Pitsidianakis]

On Wed, Jul 12, 2017 at 10:33:37AM +0200, Kevin Wolf wrote:
> Am 11.07.2017 um 20:50 hat Manos Pitsidianakis geschrieben:
> > On Tue, Jul 11, 2017 at 05:16:17PM +0200, Kevin Wolf wrote:
> > >Am 01.07.2017 um 17:39 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 move the bs->file cleanup to bdrv_open_driver() to take care of all
> > >>callers and do not set bs->drv to NULL unless the driver's open function
> > >>failed. When bs is destroyed by removing its last reference, bdrv_close()
> > >>checks bs->drv to perform the needed cleanups and also call the driver's close
> > >>function.
> > >>
> > >>Signed-off-by: Manos Pitsidianakis <address@hidden>
> > >>---
> > >>
> > >>v2:
> > >> move bdrv_unref_child(bs, bs->file) to bdrv_open_driver
> > >> do not set bs->drv to NULL if open succeeds
> > >>
> > >> block.c | 21 +++++++++++++--------
> > >> 1 file changed, 13 insertions(+), 8 deletions(-)
> > >>
> > >>diff --git a/block.c b/block.c
> > >>index 694396281b..df2a46990c 100644
> > >>--- a/block.c
> > >>+++ b/block.c
> > >>@@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> > BlockDriver *drv,
> > >> {
> > >>     Error *local_err = NULL;
> > >>     int ret;
> > >>+    bool open_failed;
> > >>
> > >>     bdrv_assign_node_name(bs, node_name, &local_err);
> > >>     if (local_err) {
> > >>@@ -1111,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> > BlockDriver *drv,
> > >>         ret = 0;
> > >>     }
> > >>
> > >>-    if (ret < 0) {
> > >>+    open_failed = ret < 0;
> > >>+
> > >>+    if (open_failed) {
> > >>         if (local_err) {
> > >>             error_propagate(errp, local_err);
> > >>         } else if (bs->filename[0]) {
> > >>@@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, 
> > BlockDriver *drv,
> > >>     return 0;
> > >>
> > >> free_and_fail:
> > >>-    /* FIXME Close bs first if already opened*/
> > >>-    g_free(bs->opaque);
> > >>-    bs->opaque = NULL;
> > >>-    bs->drv = NULL;
> > >>+    if (open_failed) {
> > >>+        g_free(bs->opaque);
> > >>+        bs->opaque = NULL;
> > >>+        bs->drv = NULL;
> > >>+    }
> > >>+    if (bs->file != NULL) {
> > >>+        bdrv_unref_child(bs, bs->file);
> > >>+        bs->file = NULL;
> > >>+    }
> > >
> > >Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers
> > >expect that if an image is opened, it also has a valid bs->file.
> > >
> > >For example, if I add ret = -1 after refresh_total_sectors() (because I
> > >couldn't find an easier way to make it fail intentionally), I get an
> > >ugly heap corruption crash instead of a nice error message with this
> > >patch.
> > >
> > This is triggered by bdrv_open_inherit doing
> > QDECREF(bs->explicit_options) and leaving the dangling pointer. Not
> > setting bs->drv means bdrv_close was called and tried to decref it
> > again, causing the heap error. Setting bs->explicit_options = NULL;
> > right below that fixes the heap corruption for me.
>
> Wouldn't it be better to call drv->bdrv_close() instead and then set
> bs->drv/opaque = NULL like for the other error path?

That was my first approach but I thought it wouldn't look nice since 
bdrv_close is called anyway on delete. I will do it in the next version.
> > I can send a seperate fix for this.
>
> No, this doesn't fail before this patch, so it's a regression and we
> can't merge the patch without a fix. You need to respin this one.
Yes, I meant to first fix this and then apply this patch. It's a 
dangling pointer anyway.
> > I also saw that there's no reason to use a boolean, a label would do
> > just fine so I can change that and finalize the patch in the next
> > version if everything is okay with it.
>
> Yes, that sounds better.
>
> Kevin

signature.asc
Description: PGP signature