Re: [PATCH 6/9] ref-filter: fix leak of %(trailers) "argbuf"

[Patrick Steinhardt <ps@pks.im>]

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