On Mon, Sep 09, 2024 at 07:18:28PM -0400, Jeff King wrote:
> When we parse a placeholder like "%(trailers:key=foo)", our atom parsing
> function is passed just the argument string "key=foo". We duplicate this
> into its own string, but never free it, causing a leak.
>
> We do the duplication for two reasons:
>
> 1. There's a mismatch with the pretty.c trailer-formatting code that
> we rely on. It expects to see a closing paren, like "key=foo)". So
> we duplicate the argument string with that extra character to pass
> along.
>
> This is probably something we could fix in the long run, but it's
> somewhat non-trivial if we want to avoid regressing error cases for
> things like "git log --format='%(trailer:oops'". So let's accept
> it as a necessity for now.
>
> 2. The argument parser expects to store the list of "key" entries
> ("foo" in this case) in a string-list. It also stores the length of
> the string in the string-list "util" field. The original caller in
> pretty.c uses this with a "nodup" string list to avoid making extra
> copies, which creates a subtle dependency on the lifetime of the
> original format string.
>
> We do the same here, which creates that same dependency. So we
> can't simply free it as soon as the parsing is done.
>
> There are two possible solutions here. The first is to hold on to the
> duplicated "argbuf" string in the used_atom struct, so that it lives as
> long as the string_list which references it.
>
> But I think a less-subtle solution, and what this patch does, is to
> switch to a duplicating string_list. That makes it self-contained, and
> lets us free argbuf immediately. It may involve a few extra allocations,
> but this parsing is something that happens once per program, not once
> per output ref.
Sensible. I found that in many cases, the `nodup` variants of string
lists bring more pain than real benefit.
> This clears up one case that LSan finds in t6300, but there are more.
Yeah, there are a bunch of memory leaks around atom parsing in general
exposed by t6300. Thanks for plugging some of them!
Patrick
