Re: [PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()

> On 04/11/2022 14:50, Phillip Wood wrote:
>> On 03/11/2022 17:06, Ævar Arnfjörð Bjarmason wrote:
>>> When "read_strategy_opts()" is called we may have populated the
>>> "opts->strategy" before, so we'll need to free() it to avoid leaking
>>> memory. 
>> Where is the previous value coming from? I guess it may be the
>> config but otherwise I'm confused.
>
> Having looked a bit more at this I think this is the wrong fix. The
> reason we're overwriting the existing value is that we're reading some 
> of the state files twice. I think the only way to get to this point is
> to go through a code path that calls rebase.c:read_basic_state() which 
> already populates these options via a later call to
> rebase.c:get_replay_opts(). I think the correct fixes looks something 
> like the diff below.
>
> I have also looked at the cherry-pick/revert case and I think that
> we're leaking opts->strategy (and probably some others) when running
>
> 	git cherry-pick --continue
>
> after
>
> 	git -c pull.twohead=recursive cherry-pick -s ort <some commits>

> I'm not sure what your strategy has been with the fixes in this series
> but we're never going to have 100% coverage of all the option 
> combinations for rebase & cherry-pick so I think it is helpful to
> treat these LSAN reports as a starting point for looking into why the
> leak is occurring and also look for similar leaks.