[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: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2] block: fix leaks in bdrv_open_driver() |
Date: |
Tue, 11 Jul 2017 17:16:17 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
Kevin