On Tue, Jul 11, 2017 at 05:16:17PM +0200, Kevin Wolf wrote:
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;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 <el13...@mail.ntua.gr> --- 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.
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.
signature.asc
Description: PGP signature