From 180ca19ae02be70f9b158bfd7dec1ff123b9cf8c Mon Sep 17 00:00:00 2001 From: Manos Pitsidianakis Date: Fri, 14 Jul 2017 17:35:48 +0300 Subject: block: fix leaks in bdrv_open_driver() 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, it calls bdrv_close() which checks bs->drv to perform the needed cleanups and also call the driver's close function. Since it cleans up options and opaque we must take care not leave dangling pointers. The error paths in bdrv_open_driver() are now two: If open fails, drv->bdrv_close() should not be called. Unref the child if it exists, free what we allocated and set bs->drv to NULL. Return the error and let callers free their stuff. If open succeeds but we fail after, return the error and let callers unref and delete their bs, while cleaning up their allocations. Signed-off-by: Manos Pitsidianakis Signed-off-by: Kevin Wolf --- block.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) (limited to 'block.c') diff --git a/block.c b/block.c index 7a78bc6..ce9cce7 100644 --- a/block.c +++ b/block.c @@ -1119,20 +1119,19 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, } else { error_setg_errno(errp, -ret, "Could not open image"); } - goto free_and_fail; + goto open_failed; } ret = refresh_total_sectors(bs, bs->total_sectors); if (ret < 0) { error_setg_errno(errp, -ret, "Could not refresh total sector count"); - goto free_and_fail; + return ret; } bdrv_refresh_limits(bs, &local_err); if (local_err) { error_propagate(errp, local_err); - ret = -EINVAL; - goto free_and_fail; + return -EINVAL; } assert(bdrv_opt_mem_align(bs) != 0); @@ -1140,12 +1139,14 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, assert(is_power_of_2(bs->bl.request_alignment)); return 0; - -free_and_fail: - /* FIXME Close bs first if already opened*/ +open_failed: + bs->drv = NULL; + if (bs->file != NULL) { + bdrv_unref_child(bs, bs->file); + bs->file = NULL; + } g_free(bs->opaque); bs->opaque = NULL; - bs->drv = NULL; return ret; } @@ -1166,7 +1167,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) { QDECREF(bs->explicit_options); + bs->explicit_options = NULL; QDECREF(bs->options); + bs->options = NULL; bdrv_unref(bs); return NULL; } @@ -2600,9 +2603,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, fail: blk_unref(file); - if (bs->file != NULL) { - bdrv_unref_child(bs, bs->file); - } QDECREF(snapshot_options); QDECREF(bs->explicit_options); QDECREF(bs->options); -- cgit v1.1