|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() |
Date: | Mon, 3 May 2021 15:14:47 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 |
03.05.2021 14:53, Max Reitz wrote:
On 03.05.21 13:51, Vladimir Sementsov-Ogievskiy wrote:03.05.2021 14:49, Max Reitz wrote:On 03.05.21 13:05, Kevin Wolf wrote:The error path needs to call tran_finalize(), too. Fixes: CID 1452773 Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1 Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 874c22c43e..5c0ced6238 100644 --- a/block.c +++ b/block.c @@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, child_role, perm, shared_perm, opaque, &child, tran, errp); if (ret < 0) { - bdrv_unref(child_bs); - return NULL; + assert(child == NULL); + goto out; } ret = bdrv_refresh_perms(child_bs, errp); - tran_finalize(tran, ret); +out: + tran_finalize(tran, ret); bdrv_unref(child_bs); return child;Looks OK, so: Reviewed-by: Max Reitz <mreitz@redhat.com> However, the function’s description says that it will return NULL on error. But if bdrv_refresh_perms() fails, it will still return a non-NULL child. Is that right?No, it's reset to NULL on transaction abort, so code is correct. It's not obvious, and I've added a comment and assertion in my version of this fix "[PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child"The fact that the transaction keeps the pointer to this local variable around is a bit horrifying, but well.
Yes this looks overcomplicated here. But it's useful for bdrv_set_backing_noperm where we would have to add a separate entry to @tran to rollback bs->backing change. Probably, it's better to refactor this thing. Or at least document that out-pointer argument goes into transaction, and should be valid up to transaction finalization. -- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |