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

>
> On Mon, Nov 28 2022, René Scharfe wrote:
>
>> Am 28.11.2022 um 12:31 schrieb Ævar Arnfjörð Bjarmason:
>>>
>>> On Mon, Nov 28 2022, René Scharfe wrote:
>>>
>>>> Am 28.11.2022 um 11:03 schrieb Junio C Hamano:
>>>>> René Scharfe <l.s.r@web.de> writes:
>>>>>
>>>>>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.
>>>>>>
>>>>>> 5cb28270a1 (pack-objects: lazily set up "struct rev_info", don't leak,
>>>>>> 2022-03-28) avoided leaking rev_info allocations in many cases by
>>>>>> calling repo_init_revisions() only when the .filter member was actually
>>>>>> needed, but then still leaking it.  That was fixed later by 2108fe4a19
>>>>>> (revisions API users: add straightforward release_revisions(),
>>>>>> 2022-04-13), making the reverted commit unnecessary.
>>>>>
>>>>> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange
>>>>> way.
>>>>>
>>>>> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917
>>>>>
>>>>> Does anybody want to help looking into it?
>>>
>>> [I see we crossed E-Mails]:
>>> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/
>>>
>>>> The patch exposes that release_revisions() leaks the diffopt allocations
>>>> as we're yet to address the TODO added by 54c8a7c379 (revisions API: add
>>>> a TODO for diff_free(&revs->diffopt), 2022-04-14).
>>>
>>> That's correct, and we have that leak in various places in our codebase,
>>> but per the above side-thread I think this is primarily exposing that
>>> we're setting up the "struct rev_info" with your change when we don't
>>> need to. Why can't we just skip it?
>>
>> I have no idea how to stop get_object_list() from using struct rev_info.
>> We could let it take a struct list_objects_filter_options pointer
>> instead and have it build a struct rev_info internally, but that would
>> just move the problem, not solve it.
>
> 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.

>
>>> Yeah, if we do set it up we'll run into an outstanding leak, and that
>>> should also be fixed (I have some local patches...), but the other cases
>>> I know of where we'll leak that data is where we're actually using the
>>> "struct rev_info".
>>>
>>> I haven't tried tearing your change apart to poke at it myself, and
>>> maybe there's some really good reason for why you can't separate getting
>>> rid of the J.5.7 dependency and removing the lazy-init.
>>>
>>>> The patch below plugs it locally.
>>>>
>>>> --- >8 ---
>>>> Subject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()
>>>>
>>>> Signed-off-by: René Scharfe <l.s.r@web.de>
>>>> ---
>>>>  builtin/pack-objects.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>>>> index 3e74fbb0cd..a47a3f0fba 100644
>>>> --- a/builtin/pack-objects.c
>>>> +++ b/builtin/pack-objects.c
>>>> @@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>>>>  	} else {
>>>>  		get_object_list(&revs, rp.nr, rp.v);
>>>>  	}
>>>> +	diff_free(&revs.diffopt);
>>>>  	release_revisions(&revs);
>>>>  	cleanup_preferred_base();
>>>>  	if (include_tag && nr_result)
>>>
>>> So, the main motivation for the change was paranoia that a compiler or
>>> platform might show up without J.5.7 support and that would bite us, but
>>> we're now adding a double-free-in-waiting?
>>>
>>> I think we're both a bit paranoid, but clearly have different
>>> paranoia-priorities :)
>>>
>>> If we do end up with some hack like this instead of fixing the
>>> underlying problem I'd much prefer that such a hack just be an UNLEAK()
>>> here.
>>>
>>> I.e. we have a destructor for "revs.*" already, let's not bypass it and
>>> start freeing things from under it, which will result in a double-free
>>> if we forget this callsite once the TODO in 54c8a7c379 is addressed.
>>
>> 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...).

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

>
>>> I haven't dug into this one, but offhand I'm not confident in saying
>>> that this isn't exposing us to some aspect of that gnarlyness (maybe
>>> not, it's been a while since I looked).
>>
>> 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.

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