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