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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: phillip.wood@dunelm.org.uk
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Jeff King <peff@peff.net>,
	Derrick Stolee <derrickstolee@github.com>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()
Date: Tue, 08 Nov 2022 16:26:37 +0100	[thread overview]
Message-ID: <221108.86zgd1qxac.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <66835bfb-2815-4321-9d06-52f85a0c72f9@dunelm.org.uk>

On Tue, Nov 08 2022, Phillip Wood wrote:

> 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>

Related: My just-sent [1].

For this one though, I have a v2 re-roll prepared, in which I explained
why the leak is happening (I'll send this soon to the list, but for
now): https://github.com/avar/git/commit/243ab74120b

> 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.

For this series I'm aiming to solve some common leaks, and increase our
test coverage.

Per [1] the destruction in this area is quite messy, and we should do it
better. But that's a much larger fix. I think in the meantime adding
this free() is the bets way to make incremental progress here, and (an
the commit linked above notes) it's consistent with existing patterns.

1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/