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

[PATCH 08/13] sparse-checkout: always free "line" strbuf after reading input

From
Jeff King <peff@peff.net>
Date
May 31, 2024, 11:34 UTC
Message-ID
<20240531113407.GH428814@coredump.intra.peff.net>
In-Reply-To
<20240531112433.GA428583@coredump.intra.peff.net>

In add_patterns_from_input(), we may read lines from a file with a loop like this:

  while (!strbuf_getline(&line, file)) {
	...
	strbuf_to_cone_pattern(&line, pl);
  }
  /* we don't strbuf_release(&line) here! */

This generally is OK because strbuf_to_cone_pattern() consumes the buffer via strbuf_detach(). But we can leak in a few cases:

  1. We don't always consume the buffer! If the line ends up empty after
     trimming, we leave strbuf_to_cone_pattern() without detaching. In
     most cases this is OK, because a subsequent getline() call will use
     the same buffer. But if you had an empty line at the end of file,
     for example, it would leak.
  2. Even if strbuf_to_cone_pattern() always consumed the buffer,
     there's a subtle issue with strbuf_getline(). As we saw in
     94e2aa555e (strbuf: fix leak when `appendwholeline()` fails with
     EOF, 2024-05-27), it's possible for it to return EOF with an
     allocated buffer (e.g., if the underlying getdelim() call saw an
     error). So we should always strbuf_release() after finishing a read
     loop like this.

Note that even the code to read patterns from argv has the same problem. Because that also uses strbuf_to_cone_pattern(), we stuff each argv entry into a strbuf. It uses the same "line" strbuf as the getline code, but we should position the strbuf_release() to cover both code paths.

This fixes at least 9 leaks found in t1091.
Signed-off-by: Jeff King <peff@peff.net>
---
This touches on the strbuf_appendwholeline() thing we were talking about
in the earlier thread. But even if we taught strbuf_getline() to never
return an allocated buf on EOF, we'd still need this because of point
(1) above. I do suspect this anti-pattern may exist in more places,
though (it was also present in the preimage of patch 7).
 builtin/sparse-checkout.c | 1 +
 1 file changed, 1 insertion(+)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 75c07d5bb4..8f8f5c359f 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -581,6 +581,7 @@ static void add_patterns_from_input(struct pattern_list *pl,
 				strbuf_to_cone_pattern(&line, pl);
 			}
 		}
+		strbuf_release(&line);
 	} else {
 		if (file) {
 			struct strbuf line = STRBUF_INIT;
-- 
2.45.1.727.ge984192922
Previous: Jeff KingNext: Jeff King
Message 12 of 43 in “leak fixes for sparse-checkout code”
  1. 0/13 leak fixes for sparse-checkout codeJeff King, May 31, 2024
  2. 01/13 sparse-checkout: free string list in write_cone_to_file()Jeff King, May 31, 2024
  3. 02/13 sparse-checkout: pass string literals directly to add_pattern()Jeff King, May 31, 2024
  4. Junio C HamanoMay 31, 2024
  5. Jeff KingJun 4, 2024
  6. 03/13 dir.c: free strings in sparse cone pattern hashmapsJeff King, May 31, 2024
  7. 04/13 sparse-checkout: clear patterns when init() sees existing sparse fileJeff King, May 31, 2024
  8. 05/13 dir.c: free removed sparse-pattern hashmap entriesJeff King, May 31, 2024
  9. 06/13 dir.c: always copy input to add_pattern()Jeff King, May 31, 2024
  10. Junio C HamanoMay 31, 2024
  11. 07/13 sparse-checkout: reuse --stdin buffer when reading patternsJeff King, May 31, 2024
  12. 08/13 sparse-checkout: always free "line" strbuf after reading inputJeff King, May 31, 2024
  13. 09/13 sparse-checkout: refactor temporary sparse_checkout_patternsJeff King, May 31, 2024
  14. Patrick SteinhardtJun 4, 2024
  15. Jeff KingJun 4, 2024
  16. 10/13 sparse-checkout: free sparse_filename after useJeff King, May 31, 2024
  17. Patrick SteinhardtJun 4, 2024
  18. Jeff KingJun 4, 2024
  19. Patrick SteinhardtJun 4, 2024
  20. 11/13 sparse-checkout: free pattern list in sparse_checkout_list()Jeff King, May 31, 2024
  21. 12/13 sparse-checkout: free string list after displayingJeff King, May 31, 2024
  22. 13/13 sparse-checkout: free duplicate hashmap entriesJeff King, May 31, 2024
  23. Patrick SteinhardtJun 4, 2024
  24. Jeff KingMay 31, 2024
  25. Junio C HamanoMay 31, 2024
  26. 0/13 leak fixes for sparse-checkout codeJeff King, Jun 4, 2024
  27. 01/13 sparse-checkout: free string list in write_cone_to_file()Jeff King, Jun 4, 2024
  28. 02/13 sparse-checkout: pass string literals directly to add_pattern()Jeff King, Jun 4, 2024
  29. 03/13 dir.c: free strings in sparse cone pattern hashmapsJeff King, Jun 4, 2024
  30. 04/13 sparse-checkout: clear patterns when init() sees existing sparse fileJeff King, Jun 4, 2024
  31. 05/13 dir.c: free removed sparse-pattern hashmap entriesJeff King, Jun 4, 2024
  32. 06/13 dir.c: always copy input to add_pattern()Jeff King, Jun 4, 2024
  33. René ScharfeJun 5, 2024
  34. Jeff KingJun 5, 2024
  35. Junio C HamanoJun 5, 2024
  36. 07/13 sparse-checkout: reuse --stdin buffer when reading patternsJeff King, Jun 4, 2024
  37. 08/13 sparse-checkout: always free "line" strbuf after reading inputJeff King, Jun 4, 2024
  38. 09/13 sparse-checkout: refactor temporary sparse_checkout_patternsJeff King, Jun 4, 2024
  39. 10/13 sparse-checkout: free sparse_filename after useJeff King, Jun 4, 2024
  40. 11/13 sparse-checkout: free pattern list in sparse_checkout_list()Jeff King, Jun 4, 2024
  41. 12/13 sparse-checkout: free string list after displayingJeff King, Jun 4, 2024
  42. 13/13 sparse-checkout: free duplicate hashmap entriesJeff King, Jun 4, 2024
  43. Patrick SteinhardtJun 4, 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.