[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Manos Pitsidianakis
Subject: Re: [Qemu-devel] [PATCH v2] block: fix leaks in bdrv_open_driver()
Date: Tue, 11 Jul 2017 21:50:35 +0300
User-agent: NeoMutt/20170609-57-1e93be (1.8.3)

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

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

Signed-off-by: Manos Pitsidianakis <address@hidden>

 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;

-    /* 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

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.

I can send a seperate fix for this. 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.

Attachment: signature.asc
Description: PGP signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]