Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()

[Max Reitz <mreitz@redhat.com>]

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.

Max