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, 11:31 UTC
- Message-ID
- <221128.864jujmhgp.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <d10de9b5-e5ff-18d6-d950-1d090d87b113@web.de>
On Mon, Nov 28 2022, René Scharfe wrote:
Show 18 quoted lines
> 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?
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.
Show 22 quoted lines
> 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.
As you'd see if you made release_revisions() simply call diff_free(&revs.diffopt) doing so would reveal some really gnarly edge cases.
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).
(IIRC some of the most gnarly edge cases will only show up as CI failures on Windows, to do with the ordering of when we'll fclose() files hanging off that "diffopt").