Re: [PATCH v2 3/3] Revert "pack-objects: lazily set up "struct rev_info", don't leak"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 28, 2022, 17:57 UTC
Message-ID: <221128.86sfi3j5uy.gmgdl@evledraar.gmail.com>
In-Reply-To: <59431916-9f55-d0f4-da54-e7369803eb4c@web.de>

On Mon, Nov 28 2022, René Scharfe wrote:

Show 13 quoted lines

> Am 28.11.2022 um 15:34 schrieb Ævar Arnfjörð Bjarmason:
> [...]
>> I mean skip it when it's not needed, it's needed when we call
>> get_object_list().
>>
>> But what "problem" is being caused by get_object_list()? That there's
>> some other case(s) where it'll leak still? I haven't checked, I think we
>> should leave that for some other time if there's such leaks, and just
>> not introduce any new leaks in this topic.
>
> The problem is "How to use struct rev_info without leaks?".  No matter
> where you move it, the leak will be present until the TODO in
> release_revisions() is done.

No, the problem at hand is that "--filter" doesn't accept N arguments, which is easily fixed.

Junio then tested the patch, and noted CI failing. Yes we have various outstanding issues with the revisions API etc. leaking, but the scope-creep of conflating those questions with a regression fix doesn't seem necessary.

Show 13 quoted lines

>>> [...]
>>> Well, that TODO fix should remove this new diff_free() call, but I
>>> agree that this is fragile.
>>>
>>> Removing the "TEST_PASSES_SANITIZE_LEAK=true" line from affected tests
>>> is probably better.
>>
>> Or just not introduce new leaks, per my suggested fix-up at
>> https://lore.kernel.org/git/221128.86zgcbl0pe.gmgdl@evledraar.gmail.com/
>> (which it looks like you haven't seen when this E-Mail is composed...).
>
> Not adding leaks is a good idea.  AFAICS none of my patches so far add
> any.

I don't see where debating what constitutes a "new" or "added" memory leak is going to get us.

But to clarify I'm not saying you're responsible for the revisions/filter APIs you're running into, and some of those are existing leaks.

I'm also saying that if you run:

	make SANITIZE=leak test T=...

For some values of T=... (and the whole test suite with "GIT_TEST_PASSING_SANITIZE_LEAK=true") your 2/3 passes certain tests, and your 3/3 doesn't.

> Patch 3 of v2 exposes an existing one that was only triggered by
> the --filter option before.  Which is also not ideal, of course, but
> giving it more visibility hopefully will motivate a proper fix.

We try not to break CI. Only finding out that existing CI breaks once a patch is queued & Junio & others start testing it to hunt down issues isn't a good use of anyone's time.

So, maybe you think this area of CI is useless etc., but again, if you're submitting a change to advocate for that can we peel that away from a regression we can fix relatively easily?

Show 13 quoted lines

>>>> As you'd see if you made release_revisions() simply call
>>>> diff_free(&revs.diffopt) doing so would reveal some really gnarly edge
>>>> cases.
>>>
>>> That was my first attempt; it breaks lots of tests due to double frees.
>>
>> Right, to be clear I'm saying that none of this is needed right now,
>> i.e. I don't get why we'd want the scope-creep past the hunk I noted in
>> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/
>> for the --filter bug fix (plus the tests you're adding).
>
> Well, you asked to squash the minimal fix into the laziness removal in
> https://lore.kernel.org/git/221112.86bkpcmm6i.gmgdl@evledraar.gmail.com/

The second part of the relevant request being so that we could regression test the memory leak(s).

So, thanks for trying to address that in some way, but proceeding to break linux-leaks, and then arguing that that's OK seems like an odd way to go about that...

Show 5 quoted lines

> Reverting 5cb28270a1 (pack-objects: lazily set up "struct rev_info",
> don't leak, 2022-03-28) wholesale is the simplest way to reach the goals
> of regression fix, simplification and standard compliance.  Except that
> the leak check tests have come to depend on the leak being hidden in the
> --filter corner.  So going back to v1 sure seems attractive.

Yeah.

Honestly I'd mostly forgotten about the v1 review, and only skimmed things then. So, I've just noticed that the "squash" I'm suggesting is basically equivalent to your v1 now.

But yeah I think it's fine to just skip testing the regression fix with SANITIZE=leak for now, we can do it some other time.

I just brought it up in the v1 feedback because that patch prominently notes the leak fix, so if we can get regression test it relatively easily that seemed like a good change to make.

Show 21 quoted lines

>>> [...]
>>> I saw it as the way towards a release_revisions() that calls diff_free()
>>> itself: Add such calls to each of them, fix the "gnarlyness"
>>> individually, finally move them all into release_revisions().  The only
>>> problem is that there are 60+ callsites.
>>
>> I think this is a really bad approach in general.
>>
>> Yes, it may happen to work to free() some data from under an API, but
>> it's just as likely that we'll miss that this one caller is screwing
>> with its internal state, and e.g. when some new revision.c code is used
>> it'll go boom.
>>
>> If we wanted to phase in such a free() of "foo" I think the right way
>> would be to add some "revs.free_foo = 1" flag, giving the API a chance
>> to treat that sanely, not to start poking at members of the struct, and
>> assuming that its release() won't be free()-ing them.
>
> And that's why you added no_free to struct diff_options.  We could use
> it here by setting it in repo_init_revisions() and unsetting in
> cmd_pack_objects() and elsewhere, until it is set everywhere.

FWIW I think the "no_free" isn't all that good of an approach in retrospect either, but at least it's (mostly) self-contained in the struct that owns it.

But it's rather messy, because some of the time it's embedded in a "struct rev_info" that should really own it, and sometimes now. At the time release_revisions() didn't exist yet, so making some forward progress with the diff leaks was easiest with the "no_free".

Show 8 quoted lines

>> But as noted above & in the linked I think we can defer all of that. The
>> only reason we're discussing this is because you're changing the
>> lazy-init to be not-lazy, and introducing new leaks as a result.>
>> I've shown a couple of approaches in this thread of fixing the issue(s)
>> at hand without introducing such leaks, so ...
>
> As noted above: These leaks are not new, they are just moved into
> test coverage.

Right, and in some cases we should definitely just un-mark a test as being tested under linux-leaks to make forward progress.

But in this case I just don't see how that's a good trade-off. We can fix the --filter bug without that, and yes we'll have some new/existing leaks, but those are all contained in tests that are not tested by linux-leaks now.

Whereas the larger rewrite to make the init non-lazy would lose us test coverage.