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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

03.05.2021 15:14, Vladimir Sementsov-Ogievskiy wrote:
> 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.
> 
> 
> 

documentation done:

   [PATCH 7/6] block: document child argument of bdrv_attach_child_common()

-- 
Best regards,
Vladimir