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

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

From
Jeff King <peff@peff.net>
Date
Sep 9, 2024, 23:18 UTC
Message-ID
<20240909231828.GF921834@coredump.intra.peff.net>
In-Reply-To
<20240909230758.GA921697@coredump.intra.peff.net>

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.

This clears up one case that LSan finds in t6300, but there are more.
Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index ebddc041c7..54c5079dde 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -567,7 +567,8 @@ static int trailers_atom_parser(struct ref_format *format UNUSED,
 	atom->u.contents.trailer_opts.no_divider = 1;
 
 	if (arg) {
-		const char *argbuf = xstrfmt("%s)", arg);
+		char *argbuf = xstrfmt("%s)", arg);
+		const char *arg = argbuf;
 		char *invalid_arg = NULL;
 		struct ref_trailer_buf *tb;
 
@@ -579,21 +580,23 @@ static int trailers_atom_parser(struct ref_format *format UNUSED,
 		 * They must be allocated in a separate, stable struct.
 		 */
 		atom->u.contents.trailer_buf = tb = xmalloc(sizeof(*tb));
-		string_list_init_nodup(&tb->filter_list);
+		string_list_init_dup(&tb->filter_list);
 		strbuf_init(&tb->sepbuf, 0);
 		strbuf_init(&tb->kvsepbuf, 0);
 
 		if (format_set_trailers_options(&atom->u.contents.trailer_opts,
 						&tb->filter_list,
 						&tb->sepbuf, &tb->kvsepbuf,
-						&argbuf, &invalid_arg)) {
+						&arg, &invalid_arg)) {
 			if (!invalid_arg)
 				strbuf_addf(err, _("expected %%(trailers:key=<value>)"));
 			else
 				strbuf_addf(err, _("unknown %%(trailers) argument: %s"), invalid_arg);
 			free(invalid_arg);
+			free(argbuf);
 			return -1;
 		}
+		free(argbuf);
 	}
 	atom->u.contents.option = C_TRAILERS;
 	return 0;
-- 
2.46.0.867.gf99b2b8e0f
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 12 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.