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

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

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

On Fri, Jul 07, 2017 at 11:28:15AM +0200, Kevin Wolf wrote:
>Am 29.06.2017 um 22:06 hat Manos Pitsidianakis geschrieben:
>> On Thu, Jun 29, 2017 at 03:57:49PM +0200, Kevin Wolf wrote:
>> >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.
>>
>> Perhaps it would be better to destroy bs at failure in
>> bdrv_open_driver and not leave it to the caller which takes care of
>> bdrv_close and unrefing bs->file anyway (Also bs->children). Setting
>> bs->drv to NULL at failure in bdrv_open_driver means some things
>> won't be executed in bdrv_close when the bs is destroyed eventually
>> as well, so that fixes another mistake.
>
>Oh, didn't I reply here yet? Your suggestion sounds good to me.

I ended up sending a v2 some days ago, and instead just not setting 
bs->drv to NULL unless open failed which I think is cleaner.

https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg00019.html

The open's ret value is stored in a boolean, but probably would be 
better to goto a specific open_fail label. If you think the change is ok 
I will resend it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]