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

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

03.05.2021 17:33, Kevin Wolf wrote:
> Am 03.05.2021 um 15:09 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 03.05.2021 15:41, Kevin Wolf wrote:
>>> Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 03.05.2021 14:05, Kevin Wolf wrote:
>>>>> Like other error paths, this one needs to call tran_finalize() and clean
>>>>> up the BlockReopenQueue, too.
>>>>
>>>> We don't need the "abort" loop on that path. And clean-up of
>>>> BlockReopenQueue is at "cleanup:" label.
>>>
>>> The cleanup of the BlockReopenQueue itself is there, but not of all
>>> fields in it. Specifically, these lines are needed:
>>>
>>>       qobject_unref(bs_entry->state.explicit_options);
>>>       qobject_unref(bs_entry->state.options);
>>>
>>> The references are taken in bdrv_reopen_queue_child() and either used in
>>> commit or released on abort. Doing nothing with them leaks them.
>>
>> Oops. Somehow I decided they are set in _prepare.
>>
>>>
>>>> So I'd prefer Peter's suggestion (my "[PATCH 2/6] block:
>>>> bdrv_reopen_multiple(): fix leak of tran object")
>>>
>>> I don't like it because I think every call that doesn't end in a commit,
>>> should jump to the abort label to make sure that everything that remains
>>> unused because of this is correctly cleaned up.
>>>
>>
>>
>> agree now..
>>
>> Still, don't we miss these two qobject_unref() calls on success path?
> 
> No, they are put to use in bdrv_reopen_commit():
> 
>      qobject_unref(bs->explicit_options);
>      qobject_unref(bs->options);
> 
>      bs->explicit_options   = reopen_state->explicit_options;
>      bs->options            = reopen_state->options;
> 
> Kevin
> 

OK then. I should have checked myself :\

-- 
Best regards,
Vladimir