git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Sep 10, 2024, 06:09 UTC
Message-ID
<Zt_ifxQq-6wNaQ3J@pks.im>
In-Reply-To
<20240909231828.GF921834@coredump.intra.peff.net>
On Mon, Sep 09, 2024 at 07:18:28PM -0400, Jeff King wrote:
Show 35 quoted lines
> 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
Previous: Jeff KingNext: Jeff King
Message 13 of 27 in “ref-filter %(trailer) fixes”
  1. 0/9 ref-filter %(trailer) fixesJeff King, Sep 9, 2024
  2. 1/9 t6300: drop newline from wrapped test titleJeff King, Sep 9, 2024
  3. 2/9 ref-filter: avoid extra copies of payload/signatureJeff King, Sep 9, 2024
  4. Patrick SteinhardtSep 10, 2024
  5. Jeff KingSep 10, 2024
  6. 3/9 ref-filter: strip signature when parsing tag trailersJeff King, Sep 9, 2024
  7. Patrick SteinhardtSep 10, 2024
  8. Jeff KingSep 10, 2024
  9. 4/9 ref-filter: drop useless cast in trailers_atom_parser()Jeff King, Sep 9, 2024
  10. 5/9 ref-filter: store ref_trailer_buf data per-atomJeff King, Sep 9, 2024
  11. Patrick SteinhardtSep 10, 2024
  12. 6/9 ref-filter: fix leak of %(trailers) "argbuf"Jeff King, Sep 9, 2024
  13. Patrick SteinhardtSep 10, 2024
  14. Jeff KingSep 10, 2024
  15. 7/9 ref-filter: fix leak with %(describe) argumentsJeff King, Sep 9, 2024
  16. 8/9 ref-filter: fix leak when formatting %(push:remoteref)Jeff King, Sep 9, 2024
  17. Patrick SteinhardtSep 10, 2024
  18. 9/9 ref-filter: add ref_format_clear() functionJeff King, Sep 9, 2024
  19. Patrick SteinhardtSep 10, 2024
  20. Jeff KingSep 10, 2024
  21. 10/9 ref-filter: fix leak with unterminated %(if) atomsPatrick Steinhardt, Sep 10, 2024
  22. Jeff KingSep 10, 2024
  23. Junio C HamanoSep 10, 2024
  24. Patrick SteinhardtSep 12, 2024
  25. Jeff KingSep 12, 2024
  26. Patrick SteinhardtSep 12, 2024
  27. Junio C HamanoSep 12, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.