{"thread":{"id":"61569","subject":"[PATCH 0/13] leak fixes for sparse-checkout code","startedAt":"2024-05-31T11:24:35Z","lastAt":"2024-06-05T16:52:35Z","messageCount":43,"participants":["Jeff King","Junio C Hamano","Patrick Steinhardt","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":13},"messages":[{"id":"495986","messageId":"20240531112433.GA428583@coredump.intra.peff.net","threadId":"61569","inReplyTo":null,"subject":"[PATCH 0/13] leak fixes for sparse-checkout code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:24:33Z","receivedAt":"2024-05-31T11:24:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"So Patrick nerd-sniped me by asking if my earlier leakfix for git-mv was\ntriggered by the test suite. It was, in t7002, but that wasn't enough to\nmake the script leak-free. So I figured, how hard could it be to go all\nthe way?\n\nWell. It only took a few patches (1-5), but in the process I stumbled on\na rather tricky interface oddity of add_pattern(), which caused some\nother leaks. The interface is fixed in patch 6, and the matching leak\ngoes away in patch 7. Of course, I wanted to make sure it was tested, so\nafter poking around I found that t1091 triggered it.\n\nBut as you might guess, that didn't make t1091 leak-free. And I couldn't\nbear leaving it on a cliffhanger like that, so patches 8-13 fix the rest\nof the issues triggered by that script.\n\nAnd along the way we managed to make t1090 and t3602 leak-free, too\n(actually in patch 2, but I didn't notice until the whole thing was\ndone).\n\nThese should apply on top of jk/leakfixes, since the leak-freeness of\nt7002 depends on the fix there.\n\n  [01/13]: sparse-checkout: free string list in write_cone_to_file()\n  [02/13]: sparse-checkout: pass string literals directly to add_pattern()\n  [03/13]: dir.c: free strings in sparse cone pattern hashmaps\n  [04/13]: sparse-checkout: clear patterns when init() sees existing sparse file\n  [05/13]: dir.c: free removed sparse-pattern hashmap entries\n  [06/13]: dir.c: always copy input to add_pattern()\n  [07/13]: sparse-checkout: reuse --stdin buffer when reading patterns\n  [08/13]: sparse-checkout: always free \"line\" strbuf after reading input\n  [09/13]: sparse-checkout: refactor temporary sparse_checkout_patterns\n  [10/13]: sparse-checkout: free sparse_filename after use\n  [11/13]: sparse-checkout: free pattern list in sparse_checkout_list()\n  [12/13]: sparse-checkout: free string list after displaying\n  [13/13]: sparse-checkout: free duplicate hashmap entries\n\n builtin/sparse-checkout.c          | 49 +++++++++++++++++++-----------\n dir.c                              | 42 ++++++++++++++++---------\n dir.h                              |  3 +-\n t/t1090-sparse-checkout-scope.sh   |  1 +\n t/t1091-sparse-checkout-builtin.sh |  1 +\n t/t3602-rm-sparse-checkout.sh      |  1 +\n t/t7002-mv-sparse-checkout.sh      |  1 +\n 7 files changed, 65 insertions(+), 33 deletions(-)\n\n-Peff\n"},{"id":"495987","messageId":"20240531112502.GA428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 01/13] sparse-checkout: free string list in write_cone_to_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:25:02Z","receivedAt":"2024-05-31T11:25:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use a string list to hold sorted and de-duped patterns, but don't\nfree it before leaving the function, causing a leak.\n\nThis drops the number of leaks found in t7002 from 27 to 25.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 0f52e25249..8747191484 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -311,6 +311,8 @@ static void write_cone_to_file(FILE *fp, struct pattern_list *pl)\n \t\tfprintf(fp, \"%s/\\n\", pattern);\n \t\tfree(pattern);\n \t}\n+\n+\tstring_list_clear(&sl, 0);\n }\n \n static int write_patterns_and_update(struct pattern_list *pl)\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495988","messageId":"20240531112613.GB428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 02/13] sparse-checkout: pass string literals directly to add_pattern()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:26:13Z","receivedAt":"2024-05-31T11:26:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The add_pattern() function takes a pattern string, but neither makes a\ncopy of it nor takes ownership of the memory. So it is the caller's\nresponsibility to make sure the string hangs around as long as the\npattern_list which references it.\n\nThere are a few cases in sparse-checkout where we use string literal\npatterns by stuffing them into a strbuf, detaching the buffer, and then\npassing the result into add_pattern(). This creates a leak when the\npattern_list is eventually cleared, since we don't retain a copy of the\ndetached buffer to free.\n\nBut we can observe that the whole strbuf dance is unnecessary. The point\nwas presumably[1] to satisfy the lifetime requirement of the string. But\nstring literals have static duration; we can count on them lasting for\nthe whole program.\n\nSo we can fix the leak by just passing them directly. And as a bonus,\nthat simplifies the code. The leaks can be seen in t7002, which drops\nfrom 25 leaks to 22 with this patch. It also makes t3602 and t1090\nleak-free.\n\nIn the long run, we will also want to clean up this (undocumented!)\nmemory lifetime requirement of add_pattern(). But that can come in a\nlater patch; passing the string literals directly will be the right\nthing either way.\n\n[1] The code in question comes from 416adc8711 (sparse-checkout: update\n    working directory in-process for 'init', 2019-11-21) and 99dfa6f970\n    (sparse-checkout: use in-process update for disable subcommand,\n    2019-11-21), but I didn't see anything in their commit messages or\n    on the list explaining the strbufs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c        | 11 +++--------\n t/t1090-sparse-checkout-scope.sh |  1 +\n t/t3602-rm-sparse-checkout.sh    |  1 +\n 3 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 8747191484..7c17ed238c 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -442,7 +442,6 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n \tchar *sparse_filename;\n \tint res;\n \tstruct object_id oid;\n-\tstruct strbuf pattern = STRBUF_INIT;\n \n \tstatic struct option builtin_sparse_checkout_init_options[] = {\n \t\tOPT_BOOL(0, \"cone\", &init_opts.cone_mode,\n@@ -493,10 +492,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n-\tstrbuf_addstr(&pattern, \"/*\");\n-\tadd_pattern(strbuf_detach(&pattern, NULL), empty_base, 0, &pl, 0);\n-\tstrbuf_addstr(&pattern, \"!/*/\");\n-\tadd_pattern(strbuf_detach(&pattern, NULL), empty_base, 0, &pl, 0);\n+\tadd_pattern(\"/*\", empty_base, 0, &pl, 0);\n+\tadd_pattern(\"!/*/\", empty_base, 0, &pl, 0);\n \tpl.use_cone_patterns = init_opts.cone_mode;\n \n \treturn write_patterns_and_update(&pl);\n@@ -893,7 +890,6 @@ static int sparse_checkout_disable(int argc, const char **argv,\n \t\tOPT_END(),\n \t};\n \tstruct pattern_list pl;\n-\tstruct strbuf match_all = STRBUF_INIT;\n \n \t/*\n \t * We do not exit early if !core_apply_sparse_checkout; due to the\n@@ -919,8 +915,7 @@ static int sparse_checkout_disable(int argc, const char **argv,\n \tpl.use_cone_patterns = 0;\n \tcore_apply_sparse_checkout = 1;\n \n-\tstrbuf_addstr(&match_all, \"/*\");\n-\tadd_pattern(strbuf_detach(&match_all, NULL), empty_base, 0, &pl, 0);\n+\tadd_pattern(\"/*\", empty_base, 0, &pl, 0);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.sparse_index = 0;\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 3a14218b24..da0e7714d5 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n TEST_CREATE_REPO_NO_TEMPLATE=1\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh\nindex 08580fd3dc..fcdefba48c 100755\n--- a/t/t3602-rm-sparse-checkout.sh\n+++ b/t/t3602-rm-sparse-checkout.sh\n@@ -2,6 +2,7 @@\n \n test_description='git rm in sparse checked out working trees'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' \"\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495990","messageId":"20240531112657.GC428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 03/13] dir.c: free strings in sparse cone pattern hashmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:26:57Z","receivedAt":"2024-05-31T11:26:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The pattern_list structs used for cone-mode sparse lookups use a few\nextra hashmaps. These store pattern_entry structs, each of which has its\nown heap-allocated pattern string. When we clean up the hashmaps, we\nfree the individual pattern_entry structs, but forget to clean up the\nembedded strings, causing memory leaks.\n\nWe can fix this by iterating over the hashmaps to free the extra\nstrings. This reduces the numbers of leaks in t7002 from 22 to 9.\n\nOne alternative here would be to make the string a FLEX_ARRAY member of\nthe pattern_entry. Then there's no extra free() required, and as a bonus\nit would be a little more efficient. However, some of the refactoring\ngets awkward, as we are often assigning strings allocated by helper\nfunctions. So let's just fix the leak for now, and we can explore bigger\nrefactoring separately.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 2d83f3311a..9cc4a68fbc 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -726,6 +726,17 @@ static char *dup_and_filter_pattern(const char *pattern)\n \treturn result;\n }\n \n+static void clear_pattern_entry_hashmap(struct hashmap *map)\n+{\n+\tstruct hashmap_iter iter;\n+\tstruct pattern_entry *entry;\n+\n+\thashmap_for_each_entry(map, &iter, entry, ent) {\n+\t\tfree(entry->pattern);\n+\t}\n+\thashmap_clear_and_free(map, struct pattern_entry, ent);\n+}\n+\n static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern *given)\n {\n \tstruct pattern_entry *translated;\n@@ -855,8 +866,8 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \n clear_hashmaps:\n \twarning(_(\"disabling cone pattern matching\"));\n-\thashmap_clear_and_free(&pl->parent_hashmap, struct pattern_entry, ent);\n-\thashmap_clear_and_free(&pl->recursive_hashmap, struct pattern_entry, ent);\n+\tclear_pattern_entry_hashmap(&pl->recursive_hashmap);\n+\tclear_pattern_entry_hashmap(&pl->parent_hashmap);\n \tpl->use_cone_patterns = 0;\n }\n \n@@ -956,8 +967,8 @@ void clear_pattern_list(struct pattern_list *pl)\n \t\tfree(pl->patterns[i]);\n \tfree(pl->patterns);\n \tfree(pl->filebuf);\n-\thashmap_clear_and_free(&pl->recursive_hashmap, struct pattern_entry, ent);\n-\thashmap_clear_and_free(&pl->parent_hashmap, struct pattern_entry, ent);\n+\tclear_pattern_entry_hashmap(&pl->recursive_hashmap);\n+\tclear_pattern_entry_hashmap(&pl->parent_hashmap);\n \n \tmemset(pl, 0, sizeof(*pl));\n }\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495991","messageId":"20240531112753.GD428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 04/13] sparse-checkout: clear patterns when init() sees existing sparse file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:27:53Z","receivedAt":"2024-05-31T11:27:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In sparse_checkout_init(), we first try to load patterns from an\nexisting file. If we found any, we return immediately, but end up\nleaking the patterns we parsed. Fixing this reduces the number of leaks\nin t7002 from 9 down to 5.\n\nNote that there are two other exits from the function, but they don't\nneed the same treatment:\n\n  - if we can't resolve HEAD, we write out a hard-coded sparse file and\n    return. But we know the pattern list is empty there, since we didn't\n    find any in the on-disk file and we haven't yet added any of our\n    own.\n\n  - otherwise, we do populate the list and then tail-call into\n    write_patterns_and_update(). But that function frees the\n    pattern_list itself, so we don't need to.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 7c17ed238c..923e6ecc0a 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -472,6 +472,7 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n \t/* If we already have a sparse-checkout file, use it. */\n \tif (res >= 0) {\n \t\tfree(sparse_filename);\n+\t\tclear_pattern_list(&pl);\n \t\treturn update_working_directory(NULL);\n \t}\n \n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495992","messageId":"20240531112818.GE428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 05/13] dir.c: free removed sparse-pattern hashmap entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:28:18Z","receivedAt":"2024-05-31T11:28:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In add_pattern_to_hashsets(), we remove entries from the\nrecursive_hashmap when adding similar ones to the parent_hashmap. I\nwon't pretend to understand all of what's going on here, but there's an\nobvious leak: whatever we removed from recursive_hashmap is not\nreferenced anywhere else, and is never free()d.\n\nWe can easily fix this by asking the hashmap to return a pointer to the\nold entry. This makes t7002 now completely leak-free.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c                         | 8 +++++++-\n t/t7002-mv-sparse-checkout.sh | 1 +\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 9cc4a68fbc..d812d521b0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -810,6 +810,8 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \n \tif (given->patternlen > 2 &&\n \t    !strcmp(given->pattern + given->patternlen - 2, \"/*\")) {\n+\t\tstruct pattern_entry *old;\n+\n \t\tif (!(given->flags & PATTERN_FLAG_NEGATIVE)) {\n \t\t\t/* Not a cone pattern. */\n \t\t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n@@ -835,7 +837,11 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \t\t}\n \n \t\thashmap_add(&pl->parent_hashmap, &translated->ent);\n-\t\thashmap_remove(&pl->recursive_hashmap, &translated->ent, &data);\n+\t\told = hashmap_remove_entry(&pl->recursive_hashmap, translated, ent, &data);\n+\t\tif (old) {\n+\t\t\tfree(old->pattern);\n+\t\t\tfree(old);\n+\t\t}\n \t\tfree(data);\n \t\treturn;\n \t}\ndiff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh\nindex 26582ae4e5..57969ce805 100755\n--- a/t/t7002-mv-sparse-checkout.sh\n+++ b/t/t7002-mv-sparse-checkout.sh\n@@ -2,6 +2,7 @@\n \n test_description='git mv in sparse working trees'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n setup_sparse_checkout () {\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495993","messageId":"20240531113127.GF428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 06/13] dir.c: always copy input to add_pattern()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:31:27Z","receivedAt":"2024-05-31T11:31:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The add_pattern() function has a subtle and undocumented gotcha: the\npattern string you pass in must remain valid as long as the pattern_list\nis in use (and nor do we take ownership of it). This is easy to get\nwrong, causing either subtle bugs (because you free or reuse the string\nbuffer) or leaks (because you copy the string, but don't track ownership\nseparately).\n\nAll of this \"pattern\" code was originally the \"exclude\" mechanism. So\nthis _usually_ works OK because you add entries in one of two ways:\n\n  1. From the command-line (e.g., \"--exclude\"), in which case we're\n     pointing to an argv entry which remains valid for the lifetime of\n     the program.\n\n  2. From a file (e.g., \".gitignore\"), in which case we read the whole\n     file into a buffer, attach it to the pattern_list's \"filebuf\"\n     entry, then parse the buffer in-place (adding NULs). The strings\n     point into the filebuf, which is cleaned up when the whole\n     pattern_list goes away.\n\nBut other code, like sparse-checkout, reads individual lines from stdin\nand passes them one by one to add_pattern(), leaking each. We could fix\nthis by refactoring it to take in the whole buffer at once, like (2)\nabove, and stuff it in \"filebuf\". But given how subtle the interface is,\nlet's just fix it to always copy the string.\n\nThat seems at first like we'd be wasting extra memory, but we can\nmitigate that:\n\n  a. The path_pattern struct already uses a FLEXPTR, since we sometimes\n     make a copy (when we see \"foo/\", we strip off the trailing slash,\n     requiring a modifiable copy of the string).\n\n     Since we'll now always embed the string inside the struct, we can\n     switch to the regular FLEX_ARRAY pattern, saving us 8 bytes of\n     pointer. So patterns with a trailing slash and ones under 8 bytes\n     actually get smaller.\n\n  b. Now that we don't need the original string to hang around, we can\n     get rid of the \"filebuf\" mechanism entirely, and just free the file\n     contents after parsing. Since files are the sources we'd expect to\n     have the largest pattern sets, we should mostly break even on\n     stuffing the same data into the individual structs.\n\nThis patch just adjusts the add_pattern() interface; it doesn't fix any\nleaky callers yet.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c | 15 +++++----------\n dir.h |  3 ++-\n 2 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d812d521b0..8308d167c8 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -925,12 +925,7 @@ void add_pattern(const char *string, const char *base,\n \tint nowildcardlen;\n \n \tparse_path_pattern(&string, &patternlen, &flags, &nowildcardlen);\n-\tif (flags & PATTERN_FLAG_MUSTBEDIR) {\n-\t\tFLEXPTR_ALLOC_MEM(pattern, pattern, string, patternlen);\n-\t} else {\n-\t\tpattern = xmalloc(sizeof(*pattern));\n-\t\tpattern->pattern = string;\n-\t}\n+\tFLEX_ALLOC_MEM(pattern, pattern, string, patternlen);\n \tpattern->patternlen = patternlen;\n \tpattern->nowildcardlen = nowildcardlen;\n \tpattern->base = base;\n@@ -972,7 +967,6 @@ void clear_pattern_list(struct pattern_list *pl)\n \tfor (i = 0; i < pl->nr; i++)\n \t\tfree(pl->patterns[i]);\n \tfree(pl->patterns);\n-\tfree(pl->filebuf);\n \tclear_pattern_entry_hashmap(&pl->recursive_hashmap);\n \tclear_pattern_entry_hashmap(&pl->parent_hashmap);\n \n@@ -1166,23 +1160,23 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n \t}\n \n \tadd_patterns_from_buffer(buf, size, base, baselen, pl);\n+\tfree(buf);\n \treturn 0;\n }\n \n static int add_patterns_from_buffer(char *buf, size_t size,\n \t\t\t\t    const char *base, int baselen,\n \t\t\t\t    struct pattern_list *pl)\n {\n+\tchar *orig = buf;\n \tint i, lineno = 1;\n \tchar *entry;\n \n \thashmap_init(&pl->recursive_hashmap, pl_hashmap_cmp, NULL, 0);\n \thashmap_init(&pl->parent_hashmap, pl_hashmap_cmp, NULL, 0);\n \n-\tpl->filebuf = buf;\n-\n \tif (skip_utf8_bom(&buf, size))\n-\t\tsize -= buf - pl->filebuf;\n+\t\tsize -= buf - orig;\n \n \tentry = buf;\n \n@@ -1222,6 +1216,7 @@ int add_patterns_from_blob_to_list(\n \t\treturn r;\n \n \tadd_patterns_from_buffer(buf, size, base, baselen, pl);\n+\tfree(buf);\n \treturn 0;\n }\n \ndiff --git a/dir.h b/dir.h\nindex b9e8e96128..c8ff308fae 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -62,7 +62,6 @@ struct path_pattern {\n \t */\n \tstruct pattern_list *pl;\n \n-\tconst char *pattern;\n \tint patternlen;\n \tint nowildcardlen;\n \tconst char *base;\n@@ -74,6 +73,8 @@ struct path_pattern {\n \t * and from -1 decrementing for patterns from CLI args.\n \t */\n \tint srcpos;\n+\n+\tchar pattern[FLEX_ARRAY];\n };\n \n /* used for hashmaps for cone patterns */\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495994","messageId":"20240531113151.GG428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 07/13] sparse-checkout: reuse --stdin buffer when reading patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:31:51Z","receivedAt":"2024-05-31T11:31:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we read patterns from --stdin, we loop on strbuf_getline(), and\ndetach each line we read to pass into add_pattern(). This used to be\nnecessary because add_pattern() required that the pattern strings remain\nvalid while the pattern_list was in use. But it also created a leak,\nsince we didn't record the detached buffers anywhere else.\n\nNow that add_pattern() has been modified to make its own copy of the\nstrings, we can stop detaching and fix the leak. This fixes 4 leaks\ndetected in t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 923e6ecc0a..75c07d5bb4 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -585,11 +585,10 @@ static void add_patterns_from_input(struct pattern_list *pl,\n \t\tif (file) {\n \t\t\tstruct strbuf line = STRBUF_INIT;\n \n-\t\t\twhile (!strbuf_getline(&line, file)) {\n-\t\t\t\tsize_t len;\n-\t\t\t\tchar *buf = strbuf_detach(&line, &len);\n-\t\t\t\tadd_pattern(buf, empty_base, 0, pl, 0);\n-\t\t\t}\n+\t\t\twhile (!strbuf_getline(&line, file))\n+\t\t\t\tadd_pattern(line.buf, empty_base, 0, pl, 0);\n+\n+\t\t\tstrbuf_release(&line);\n \t\t} else {\n \t\t\tfor (i = 0; i < argc; i++)\n \t\t\t\tadd_pattern(argv[i], empty_base, 0, pl, 0);\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495995","messageId":"20240531113407.GH428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 08/13] sparse-checkout: always free \"line\" strbuf after reading input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:34:07Z","receivedAt":"2024-05-31T11:34:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In add_patterns_from_input(), we may read lines from a file with a loop\nlike this:\n\n  while (!strbuf_getline(&line, file)) {\n\t...\n\tstrbuf_to_cone_pattern(&line, pl);\n  }\n  /* we don't strbuf_release(&line) here! */\n\nThis generally is OK because strbuf_to_cone_pattern() consumes the\nbuffer via strbuf_detach(). But we can leak in a few cases:\n\n  1. We don't always consume the buffer! If the line ends up empty after\n     trimming, we leave strbuf_to_cone_pattern() without detaching. In\n     most cases this is OK, because a subsequent getline() call will use\n     the same buffer. But if you had an empty line at the end of file,\n     for example, it would leak.\n\n  2. Even if strbuf_to_cone_pattern() always consumed the buffer,\n     there's a subtle issue with strbuf_getline(). As we saw in\n     94e2aa555e (strbuf: fix leak when `appendwholeline()` fails with\n     EOF, 2024-05-27), it's possible for it to return EOF with an\n     allocated buffer (e.g., if the underlying getdelim() call saw an\n     error). So we should always strbuf_release() after finishing a read\n     loop like this.\n\nNote that even the code to read patterns from argv has the same problem.\nBecause that also uses strbuf_to_cone_pattern(), we stuff each argv\nentry into a strbuf. It uses the same \"line\" strbuf as the getline code,\nbut we should position the strbuf_release() to cover both code paths.\n\nThis fixes at least 9 leaks found in t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis touches on the strbuf_appendwholeline() thing we were talking about\nin the earlier thread. But even if we taught strbuf_getline() to never\nreturn an allocated buf on EOF, we'd still need this because of point\n(1) above. I do suspect this anti-pattern may exist in more places,\nthough (it was also present in the preimage of patch 7).\n\n builtin/sparse-checkout.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 75c07d5bb4..8f8f5c359f 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -581,6 +581,7 @@ static void add_patterns_from_input(struct pattern_list *pl,\n \t\t\t\tstrbuf_to_cone_pattern(&line, pl);\n \t\t\t}\n \t\t}\n+\t\tstrbuf_release(&line);\n \t} else {\n \t\tif (file) {\n \t\t\tstruct strbuf line = STRBUF_INIT;\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495996","messageId":"20240531113530.GI428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 09/13] sparse-checkout: refactor temporary sparse_checkout_patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:35:30Z","receivedAt":"2024-05-31T11:35:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In update_working_directory(), we take in a pattern_list, attach it to\nthe repository index by assigning it to index->sparse_checkout_patterns,\nand then call unpack_trees. Afterwards, we remove it by setting\nindex->sparse_checkout_patterns back to NULL.\n\nBut there are two possible leaks here:\n\n  1. If the index already had a populated sparse_checkout_patterns,\n     we've obliterated it. We can fix this by saving and restoring it,\n     rather than always setting it back to NULL.\n\n  2. We may call the function with a NULL pattern_list, expecting it to\n     use the on-disk sparse file. In that case, the index routines will\n     lazy-load the sparse patterns automatically. But now at the end of\n     the function when we restore the patterns, we'll leak those\n     lazy-loaded ones!\n\n     We can fix this by freeing the pattern list before overwriting its\n     pointer whenever it does not match what was passed in (in practice\n     this should only happen when the passed-in list is NULL, but this\n     is erring on the defensive side).\n\nTogether these remove 48 indirect leaks found in t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 8f8f5c359f..356b7349f9 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -205,11 +205,13 @@ static int update_working_directory(struct pattern_list *pl)\n \tstruct unpack_trees_options o;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct repository *r = the_repository;\n+\tstruct pattern_list *old_pl;\n \n \t/* If no branch has been checked out, there are no updates to make. */\n \tif (is_index_unborn(r->index))\n \t\treturn UPDATE_SPARSITY_SUCCESS;\n \n+\told_pl = r->index->sparse_checkout_patterns;\n \tr->index->sparse_checkout_patterns = pl;\n \n \tmemset(&o, 0, sizeof(o));\n@@ -241,7 +243,12 @@ static int update_working_directory(struct pattern_list *pl)\n \n \tclean_tracked_sparse_directories(r);\n \n-\tr->index->sparse_checkout_patterns = NULL;\n+\tif (r->index->sparse_checkout_patterns != pl) {\n+\t\tclear_pattern_list(r->index->sparse_checkout_patterns);\n+\t\tFREE_AND_NULL(r->index->sparse_checkout_patterns);\n+\t} else {\n+\t\tr->index->sparse_checkout_patterns = old_pl;\n+\t}\n \treturn result;\n }\n \n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495997","messageId":"20240531113545.GJ428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 10/13] sparse-checkout: free sparse_filename after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:35:45Z","receivedAt":"2024-05-31T11:35:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We allocate a heap buffer via get_sparse_checkout_filename(). Most calls\nremember to free it, but sparse_checkout_init() forgets to, causing a\nleak. Ironically, it remembers to do so in the error return paths, but\nnot in the path that makes it all the way to the function end!\n\nFixing this clears up 6 leaks from t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 356b7349f9..3af9fec1fb 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n+\tfree(sparse_filename);\n+\n \tadd_pattern(\"/*\", empty_base, 0, &pl, 0);\n \tadd_pattern(\"!/*/\", empty_base, 0, &pl, 0);\n \tpl.use_cone_patterns = init_opts.cone_mode;\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495998","messageId":"20240531113621.GK428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 11/13] sparse-checkout: free pattern list in sparse_checkout_list()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:36:21Z","receivedAt":"2024-05-31T11:36:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In sparse_checkout_list(), we create a pattern_list that needs to\neventually be cleared. We remember to do so in the regular code path,\nbut the cone-mode path does an early return, and forgets to clean up.\n\nWe could fix the leak by adding a new call to clear_pattern_list(). But\nwe can simplify even further by just skipping the early return, pushing\nthe other code path (which consists now of only one line!) into an else\nblock. That also matches the same cone/non-cone if/else used in some\nother functions.\n\nThis fixes 15 leaks found in t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 3af9fec1fb..bc07df925f 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -95,11 +95,10 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix)\n \t\t\tquote_c_style(sl.items[i].string, NULL, stdout, 0);\n \t\t\tprintf(\"\\n\");\n \t\t}\n-\n-\t\treturn 0;\n+\t} else {\n+\t\twrite_patterns_to_file(stdout, &pl);\n \t}\n \n-\twrite_patterns_to_file(stdout, &pl);\n \tclear_pattern_list(&pl);\n \n \treturn 0;\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"495999","messageId":"20240531113643.GL428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 12/13] sparse-checkout: free string list after displaying","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:36:43Z","receivedAt":"2024-05-31T11:36:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In sparse_checkout_list(), we put the hashmap entries into a string_list\nso we can sort them. But after printing, we forget to free the list.\n\nThis patch drops 5 leaks from t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex bc07df925f..0b979a17d6 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -95,6 +95,8 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix)\n \t\t\tquote_c_style(sl.items[i].string, NULL, stdout, 0);\n \t\t\tprintf(\"\\n\");\n \t\t}\n+\n+\t\tstring_list_clear(&sl, 0);\n \t} else {\n \t\twrite_patterns_to_file(stdout, &pl);\n \t}\n-- \n2.45.1.727.ge984192922\n\n"},{"id":"496000","messageId":"20240531113830.GM428814@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH 13/13] sparse-checkout: free duplicate hashmap entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:38:30Z","receivedAt":"2024-05-31T11:38:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In insert_recursive_pattern(), we create a new pattern_entry to insert\ninto the parent_hashmap. If we find that the same entry already exists\nin the hashmap, we skip adding the new one. But we forget to free the new\none, creating a leak.\n\nWe can fix it by cleaning up the discarded entry. It would probably be\npossible to avoid creating it in the first place, but it's non-trivial.\nWe'd have to define a \"keydata\" struct that lets us compare the existing\nentries to the broken-out fields. It's probably not worth the\ncomplexity, so we'll punt on that for now.\n\nThere is one subtlety here: our insertion is happening in a loop, with\neach iteration looking at the pattern we just inserted (hence the\n\"recursive\" in the name). So if we skip insertion, what do we look at?\n\nThe obvious answer is that we should remember the existing duplicate we\nfound and use that. But I _think_ in that case, we probably already have\nall of the recursive bits already (from when the original entry was\nadded). And so just breaking out of the loop would be correct. But I'm\nnot 100% sure on that; after all, the original leaky code could have\ndone the same break, but it didn't.\n\nSo I went with the \"obvious answer\" above, which has no chance of\nchanging the behavior aside from fixing the leak.\n\nWith this patch, t1091 can now be marked leak-free.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c          | 9 ++++++++-\n t/t1091-sparse-checkout-builtin.sh | 1 +\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 0b979a17d6..c8179d59d9 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -523,6 +523,7 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat\n \t\tchar *slash = strrchr(e->pattern, '/');\n \t\tchar *oldpattern = e->pattern;\n \t\tsize_t newlen;\n+\t\tstruct pattern_entry *dup;\n \n \t\tif (!slash || slash == e->pattern)\n \t\t\tbreak;\n@@ -533,8 +534,14 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat\n \t\te->pattern = xstrndup(oldpattern, newlen);\n \t\thashmap_entry_init(&e->ent, fspathhash(e->pattern));\n \n-\t\tif (!hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL))\n+\t\tdup = hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL);\n+\t\tif (!dup)\n \t\t\thashmap_add(&pl->parent_hashmap, &e->ent);\n+\t\telse {\n+\t\t\tfree(e->pattern);\n+\t\t\tfree(e);\n+\t\t\te = dup;\n+\t\t}\n \t}\n }\n \ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex ab3a105fff..8c5cd651b4 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n GIT_TEST_SPLIT_INDEX=false\n export GIT_TEST_SPLIT_INDEX\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n list_files() {\n-- \n2.45.1.727.ge984192922\n"},{"id":"496002","messageId":"20240531114941.GA429026@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"Re: [PATCH 0/13] leak fixes for sparse-checkout code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-31T11:49:41Z","receivedAt":"2024-05-31T11:49:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 31, 2024 at 07:24:33AM -0400, Jeff King wrote:\n\n> But as you might guess, that didn't make t1091 leak-free. And I couldn't\n> bear leaving it on a cliffhanger like that, so patches 8-13 fix the rest\n> of the issues triggered by that script.\n> \n> And along the way we managed to make t1090 and t3602 leak-free, too\n> (actually in patch 2, but I didn't notice until the whole thing was\n> done).\n\nOh, btw, there's one interesting workflow I found. It's nice to see if\nyour incremental work is making things better (and to make sure that the\nfixes are being exercised somewhere in the test suite).  But the\ngranularity of \"is this script leak-free\" is too coarse to see the\nincremental steps.  Likewise even for individual test failures, as you\ncan have many leaks in a single program.\n\nSo I ended up doing this a lot:\n\n  script=t1091-sparse-checkout-builtin.sh\n  make SANITIZE=leak &&\n  (\n\tcd t &&\n\trm -rf test-results &&\n\tLSAN_OPTIONS=abort_on_error=0:exitcode=0 \\\n\tGIT_TEST_SANITIZE_LEAK_LOG=true \\\n\t./$script\n  )\n  for i in Indirect Direct; do\n\techo \"$i: $(grep \"^$i leak\" t/test-results/${script%.sh}.leak/* | wc -l)\"\n  done\n\nIt keeps running instead of aborting on leaks (otherwise your counts\nmay go up as \"failing\" programs are fixed and we run more code). And\ninstead just logs it all and counts up the log entries.\n\nI wonder if it would be useful to have something like that baked into\ntest-lib.\n\n-Peff\n"},{"id":"496013","messageId":"xmqqle3qhvpu.fsf@gitster.g","threadId":"61569","inReplyTo":"20240531114941.GA429026@coredump.intra.peff.net","subject":"Re: [PATCH 0/13] leak fixes for sparse-checkout code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-31T15:01:01Z","receivedAt":"2024-05-31T15:01:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> \tLSAN_OPTIONS=abort_on_error=0:exitcode=0 \\\n> \tGIT_TEST_SANITIZE_LEAK_LOG=true \\\n> \t./$script\n>   )\n>   for i in Indirect Direct; do\n> \techo \"$i: $(grep \"^$i leak\" t/test-results/${script%.sh}.leak/* | wc -l)\"\n>   done\n\nNeat trick.\n\n> It keeps running instead of aborting on leaks (otherwise your counts\n> may go up as \"failing\" programs are fixed and we run more code). And\n> instead just logs it all and counts up the log entries.\n\nIndeed.  It is a very nice idea.\n\n> I wonder if it would be useful to have something like that baked into\n> test-lib.\n>\n> -Peff\n"},{"id":"496020","messageId":"xmqqr0digdqe.fsf@gitster.g","threadId":"61569","inReplyTo":"20240531112613.GB428814@coredump.intra.peff.net","subject":"Re: [PATCH 02/13] sparse-checkout: pass string literals directly to add_pattern()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-31T16:14:49Z","receivedAt":"2024-05-31T16:14:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The add_pattern() function takes a pattern string, but neither makes a\n> copy of it nor takes ownership of the memory. So it is the caller's\n> responsibility to make sure the string hangs around as long as the\n> pattern_list which references it.\n\nWow.  That's an extremely bad API.\n\nI suspect that it long time ago did not aggressively \"pre-parse\" the\ngiven pattern string and kept the original copy in the struct (i.e.,\ntaking ownership), and these copies of the literal strings were made\nat the calling sites with the expectation that the API takes\nownership of them, as most of the strings fed to add_pattern() are\nwhat we read from the environment into heap.\n\n> In the long run, we will also want to clean up this (undocumented!)\n> memory lifetime requirement of add_pattern(). But that can come in a\n> later patch; passing the string literals directly will be the right\n> thing either way.\n\nOK.\n"},{"id":"496031","messageId":"xmqq1q5hehvv.fsf@gitster.g","threadId":"61569","inReplyTo":"20240531113127.GF428814@coredump.intra.peff.net","subject":"Re: [PATCH 06/13] dir.c: always copy input to add_pattern()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-31T22:28:04Z","receivedAt":"2024-05-31T22:28:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   b. Now that we don't need the original string to hang around, we can\n>      get rid of the \"filebuf\" mechanism entirely, and just free the file\n>      contents after parsing. Since files are the sources we'd expect to\n>      have the largest pattern sets, we should mostly break even on\n>      stuffing the same data into the individual structs.\n\n;-).\n\n> diff --git a/dir.c b/dir.c\n> index d812d521b0..8308d167c8 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -925,12 +925,7 @@ void add_pattern(const char *string, const char *base,\n>  \tint nowildcardlen;\n>  \n>  \tparse_path_pattern(&string, &patternlen, &flags, &nowildcardlen);\n> -\tif (flags & PATTERN_FLAG_MUSTBEDIR) {\n> -\t\tFLEXPTR_ALLOC_MEM(pattern, pattern, string, patternlen);\n> -\t} else {\n> -\t\tpattern = xmalloc(sizeof(*pattern));\n> -\t\tpattern->pattern = string;\n> -\t}\n> +\tFLEX_ALLOC_MEM(pattern, pattern, string, patternlen);\n>  \tpattern->patternlen = patternlen;\n>  \tpattern->nowildcardlen = nowildcardlen;\n>  \tpattern->base = base;\n\nNice simplification.\n\n> diff --git a/dir.h b/dir.h\n> index b9e8e96128..c8ff308fae 100644\n> --- a/dir.h\n> +++ b/dir.h\n> @@ -62,7 +62,6 @@ struct path_pattern {\n>  \t */\n>  \tstruct pattern_list *pl;\n>  \n> -\tconst char *pattern;\n>  \tint patternlen;\n>  \tint nowildcardlen;\n>  \tconst char *base;\n> @@ -74,6 +73,8 @@ struct path_pattern {\n>  \t * and from -1 decrementing for patterns from CLI args.\n>  \t */\n>  \tint srcpos;\n> +\n> +\tchar pattern[FLEX_ARRAY];\n>  };\n\nOK.  Looking good.\n\nThanks.\n"},{"id":"496205","messageId":"Zl7Fe6132gy1JclD@framework","threadId":"61569","inReplyTo":"20240531113530.GI428814@coredump.intra.peff.net","subject":"Re: [PATCH 09/13] sparse-checkout: refactor temporary sparse_checkout_patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-04T07:42:51Z","receivedAt":"2024-06-04T07:42:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 31, 2024 at 07:35:30AM -0400, Jeff King wrote:\n> In update_working_directory(), we take in a pattern_list, attach it to\n> the repository index by assigning it to index->sparse_checkout_patterns,\n> and then call unpack_trees. Afterwards, we remove it by setting\n> index->sparse_checkout_patterns back to NULL.\n> \n> But there are two possible leaks here:\n> \n>   1. If the index already had a populated sparse_checkout_patterns,\n>      we've obliterated it. We can fix this by saving and restoring it,\n>      rather than always setting it back to NULL.\n\nSo this isn't only a leak, but also a potential bug because we did not\nrestore the old pattern list?\n\n> @@ -241,7 +243,12 @@ static int update_working_directory(struct pattern_list *pl)\n>  \n>  \tclean_tracked_sparse_directories(r);\n>  \n> -\tr->index->sparse_checkout_patterns = NULL;\n> +\tif (r->index->sparse_checkout_patterns != pl) {\n> +\t\tclear_pattern_list(r->index->sparse_checkout_patterns);\n> +\t\tFREE_AND_NULL(r->index->sparse_checkout_patterns);\n> +\t} else {\n> +\t\tr->index->sparse_checkout_patterns = old_pl;\n> +\t}\n>  \treturn result;\n>  }\n\nWhat I find weird is that we sometimes restore the old value, and\nsometimes we don't. I get that it makes sense to conditionally free only\nthe lazy-loaded list. But shouldn't we then unconditionally assign\n`old_pl`?\n\nMakes me wonder whether I miss some subtlety here.\n\nPatrick\n"},{"id":"496206","messageId":"Zl7FgVPHGiXJYSah@framework","threadId":"61569","inReplyTo":"20240531113545.GJ428814@coredump.intra.peff.net","subject":"Re: [PATCH 10/13] sparse-checkout: free sparse_filename after use","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-04T07:42:57Z","receivedAt":"2024-06-04T07:43:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 31, 2024 at 07:35:45AM -0400, Jeff King wrote:\n> We allocate a heap buffer via get_sparse_checkout_filename(). Most calls\n> remember to free it, but sparse_checkout_init() forgets to, causing a\n> leak. Ironically, it remembers to do so in the error return paths, but\n> not in the path that makes it all the way to the function end!\n> \n> Fixing this clears up 6 leaks from t1091.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/sparse-checkout.c | 2 ++\n>  1 file changed, 2 insertions(+)\n> \n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 356b7349f9..3af9fec1fb 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n>  \t\treturn 0;\n>  \t}\n>  \n> +\tfree(sparse_filename);\n> +\n\nI wonder whether it would make sense to merge this patch and patch 4\nand then refactor the code to have a common exit path.\n\nPatrick\n"},{"id":"496207","messageId":"Zl7FiQ5Kke1ZNq2R@framework","threadId":"61569","inReplyTo":"20240531113830.GM428814@coredump.intra.peff.net","subject":"Re: [PATCH 13/13] sparse-checkout: free duplicate hashmap entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-04T07:43:05Z","receivedAt":"2024-06-04T07:43:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 31, 2024 at 07:38:30AM -0400, Jeff King wrote:\n> @@ -533,8 +534,14 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat\n>  \t\te->pattern = xstrndup(oldpattern, newlen);\n>  \t\thashmap_entry_init(&e->ent, fspathhash(e->pattern));\n>  \n> -\t\tif (!hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL))\n> +\t\tdup = hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL);\n> +\t\tif (!dup)\n>  \t\t\thashmap_add(&pl->parent_hashmap, &e->ent);\n> +\t\telse {\n> +\t\t\tfree(e->pattern);\n> +\t\t\tfree(e);\n> +\t\t\te = dup;\n> +\t\t}\n\nNit: code style. The `if (!dup)` branch should also have curly braces.\n\nPatrick\n"},{"id":"496218","messageId":"20240604094306.GA1298378@coredump.intra.peff.net","threadId":"61569","inReplyTo":"xmqqr0digdqe.fsf@gitster.g","subject":"Re: [PATCH 02/13] sparse-checkout: pass string literals directly to add_pattern()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T09:43:06Z","receivedAt":"2024-06-04T09:43:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 31, 2024 at 09:14:49AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The add_pattern() function takes a pattern string, but neither makes a\n> > copy of it nor takes ownership of the memory. So it is the caller's\n> > responsibility to make sure the string hangs around as long as the\n> > pattern_list which references it.\n> \n> Wow.  That's an extremely bad API.\n> \n> I suspect that it long time ago did not aggressively \"pre-parse\" the\n> given pattern string and kept the original copy in the struct (i.e.,\n> taking ownership), and these copies of the literal strings were made\n> at the calling sites with the expectation that the API takes\n> ownership of them, as most of the strings fed to add_pattern() are\n> what we read from the environment into heap.\n\nYeah, I actually dug into the history a bit but didn't find any real\nsmoking gun of when it crossed the line to \"horrible\". It was gradual. ;)\n\nBut yes, it was originally just an array of the pattern strings (in\n2005!), and then grew more features, and then grew more of an interface\nfor adding patterns from other sources, and so on. I'm mildly surprised\nnobody ran afoul of it before now.\n\n-Peff\n"},{"id":"496219","messageId":"20240604095348.GB1298378@coredump.intra.peff.net","threadId":"61569","inReplyTo":"Zl7Fe6132gy1JclD@framework","subject":"Re: [PATCH 09/13] sparse-checkout: refactor temporary sparse_checkout_patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T09:53:48Z","receivedAt":"2024-06-04T09:53:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 04, 2024 at 09:42:51AM +0200, Patrick Steinhardt wrote:\n\n> > But there are two possible leaks here:\n> > \n> >   1. If the index already had a populated sparse_checkout_patterns,\n> >      we've obliterated it. We can fix this by saving and restoring it,\n> >      rather than always setting it back to NULL.\n> \n> So this isn't only a leak, but also a potential bug because we did not\n> restore the old pattern list?\n\nThat's a good question, but I think in practice, it's not possible to\ntrigger the bug. Within a run that calls this function, we only care\nabout index->sparse_checkout_patterns in the context of this function\nitself (that is, we do not otherwise try to read the sparse index). And\nin this function, we always want to use the pattern_list that the caller\nhands us.\n\nSo nobody actually cares about the \"restored\" pattern list at all\n(except that in the lazy-load case, we leak it). Every call will\noverwrite it anyway.\n\n> > @@ -241,7 +243,12 @@ static int update_working_directory(struct pattern_list *pl)\n> >  \n> >  \tclean_tracked_sparse_directories(r);\n> >  \n> > -\tr->index->sparse_checkout_patterns = NULL;\n> > +\tif (r->index->sparse_checkout_patterns != pl) {\n> > +\t\tclear_pattern_list(r->index->sparse_checkout_patterns);\n> > +\t\tFREE_AND_NULL(r->index->sparse_checkout_patterns);\n> > +\t} else {\n> > +\t\tr->index->sparse_checkout_patterns = old_pl;\n> > +\t}\n> >  \treturn result;\n> >  }\n> \n> What I find weird is that we sometimes restore the old value, and\n> sometimes we don't. I get that it makes sense to conditionally free only\n> the lazy-loaded list. But shouldn't we then unconditionally assign\n> `old_pl`?\n\nYes, I think it would be more correct to do so (otherwise we risk\nleaking old_pl). In practice, as I mentioned above, this is the only\nfunction that actually assigns to the index list. So after this hunk, I\nthink we'd always come in to the function with a NULL value in the\nindex (it starts NULL, we restore NULL after using a passed-in value,\nand we restore free-and-NULL after lazy-loading).\n\nSo we _could_ actually drop the \"save old_pl\" hunk from the beginning of\nthe function entirely. I only hit that case after trying something like:\n\n  /* only throw away patterns if they were the ones we own */\n  if (r->index->sparse_checkout_patterns == pl)\n\tr->index->sparse_checkout_patterns = NULL;\n\nat the end of the function to fix the leak. And with that code, we _do_\nsee the lazy-loaded entries in subsequent calls. If we instead do:\n\n  /* throw away lazy-loaded ones */\n  if (r->index->sparse_checkout_patterns != pl) {\n\tclear_pattern_list(r->index->sparse_checkout_patterns);\n\tFREE_AND_NULL(r->index->sparse_checkout_patterns);\n  }\n\nThen I think you don't really need the else clause at all. But I was\ntrying to be defensive and not make too many assumptions. I think in\nthat spirit, just restoring old_pl always is the right thing, even\nthough I don't think we'd ever trigger that.\n\n-Peff\n"},{"id":"496221","messageId":"20240604100142.GC1298378@coredump.intra.peff.net","threadId":"61569","inReplyTo":"Zl7FgVPHGiXJYSah@framework","subject":"Re: [PATCH 10/13] sparse-checkout: free sparse_filename after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:01:42Z","receivedAt":"2024-06-04T10:01:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 04, 2024 at 09:42:57AM +0200, Patrick Steinhardt wrote:\n\n> > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> > index 356b7349f9..3af9fec1fb 100644\n> > --- a/builtin/sparse-checkout.c\n> > +++ b/builtin/sparse-checkout.c\n> > @@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n> >  \t\treturn 0;\n> >  \t}\n> >  \n> > +\tfree(sparse_filename);\n> > +\n> \n> I wonder whether it would make sense to merge this patch and patch 4\n> and then refactor the code to have a common exit path.\n\nI thought about that, too, but it doesn't quite work. In the non-error\nexit path we _don't_ clean up the pattern_list, because we tail-call\ninto write_patterns_and_update(), which frees it itself.\n\nIf we refactored that function to _not_ free, and then switched here to\na \"ret\" variable, like:\n\n\t...\n\tret = write_patterns_and_update(&pl);\n  out:\n\tclear_pattern_list(&pl);\n\tfree(sparse_filename);\n\treturn ret;\n\nit could work. I mostly tried to err on the side of minimizing\nrefactoring, though.\n\n-Peff\n"},{"id":"496223","messageId":"20240604100814.GA1304520@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240531112433.GA428583@coredump.intra.peff.net","subject":"[PATCH v2 0/13] leak fixes for sparse-checkout code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:08:14Z","receivedAt":"2024-06-04T10:08:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 31, 2024 at 07:24:34AM -0400, Jeff King wrote:\n\n> So Patrick nerd-sniped me by asking if my earlier leakfix for git-mv was\n> triggered by the test suite. It was, in t7002, but that wasn't enough to\n> make the script leak-free. So I figured, how hard could it be to go all\n> the way?\n> \n> Well. It only took a few patches (1-5), but in the process I stumbled on\n> a rather tricky interface oddity of add_pattern(), which caused some\n> other leaks. The interface is fixed in patch 6, and the matching leak\n> goes away in patch 7. Of course, I wanted to make sure it was tested, so\n> after poking around I found that t1091 triggered it.\n> \n> But as you might guess, that didn't make t1091 leak-free. And I couldn't\n> bear leaving it on a cliffhanger like that, so patches 8-13 fix the rest\n> of the issues triggered by that script.\n> \n> And along the way we managed to make t1090 and t3602 leak-free, too\n> (actually in patch 2, but I didn't notice until the whole thing was\n> done).\n> \n> These should apply on top of jk/leakfixes, since the leak-freeness of\n> t7002 depends on the fix there.\n\nHere's a v2 with a few minor updates, based on review from Patrick. The\nrestoration of \"old_pl\" in patch 9 is now done unconditionally (it\ndoesn't matter in practice due to a bunch of subtle things, but it's the\nless surprising and more defensive choice). And a small style update in\npatch 13.\n\nRange-diff:\n\n 1:  1be529905f =  1:  1be529905f sparse-checkout: free string list in write_cone_to_file()\n 2:  8e36bf2213 =  2:  8e36bf2213 sparse-checkout: pass string literals directly to add_pattern()\n 3:  1a3c5819f9 =  3:  1a3c5819f9 dir.c: free strings in sparse cone pattern hashmaps\n 4:  a492842dd3 =  4:  a492842dd3 sparse-checkout: clear patterns when init() sees existing sparse file\n 5:  2d8d402809 =  5:  2d8d402809 dir.c: free removed sparse-pattern hashmap entries\n 6:  addb69e229 =  6:  addb69e229 dir.c: always copy input to add_pattern()\n 7:  f2ee2192f7 =  7:  f2ee2192f7 sparse-checkout: reuse --stdin buffer when reading patterns\n 8:  d3f7ea12ab =  8:  d3f7ea12ab sparse-checkout: always free \"line\" strbuf after reading input\n 9:  b5c07325d2 !  9:  b78d3b51b9 sparse-checkout: refactor temporary sparse_checkout_patterns\n    @@ builtin/sparse-checkout.c: static int update_working_directory(struct pattern_li\n     +\tif (r->index->sparse_checkout_patterns != pl) {\n     +\t\tclear_pattern_list(r->index->sparse_checkout_patterns);\n     +\t\tFREE_AND_NULL(r->index->sparse_checkout_patterns);\n    -+\t} else {\n    -+\t\tr->index->sparse_checkout_patterns = old_pl;\n     +\t}\n    ++\tr->index->sparse_checkout_patterns = old_pl;\n    ++\n      \treturn result;\n      }\n      \n10:  26b1e08e6f = 10:  ef05901aef sparse-checkout: free sparse_filename after use\n11:  d14f0f0546 = 11:  7b423b1691 sparse-checkout: free pattern list in sparse_checkout_list()\n12:  84b1a5eb0d = 12:  034668997b sparse-checkout: free string list after displaying\n13:  460a7f9324 ! 13:  82f14304ae sparse-checkout: free duplicate hashmap entries\n    @@ builtin/sparse-checkout.c: static void insert_recursive_pattern(struct pattern_l\n      \n     -\t\tif (!hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL))\n     +\t\tdup = hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL);\n    -+\t\tif (!dup)\n    ++\t\tif (!dup) {\n      \t\t\thashmap_add(&pl->parent_hashmap, &e->ent);\n    -+\t\telse {\n    ++\t\t} else {\n     +\t\t\tfree(e->pattern);\n     +\t\t\tfree(e);\n     +\t\t\te = dup;\n\n  [01/13]: sparse-checkout: free string list in write_cone_to_file()\n  [02/13]: sparse-checkout: pass string literals directly to add_pattern()\n  [03/13]: dir.c: free strings in sparse cone pattern hashmaps\n  [04/13]: sparse-checkout: clear patterns when init() sees existing sparse file\n  [05/13]: dir.c: free removed sparse-pattern hashmap entries\n  [06/13]: dir.c: always copy input to add_pattern()\n  [07/13]: sparse-checkout: reuse --stdin buffer when reading patterns\n  [08/13]: sparse-checkout: always free \"line\" strbuf after reading input\n  [09/13]: sparse-checkout: refactor temporary sparse_checkout_patterns\n  [10/13]: sparse-checkout: free sparse_filename after use\n  [11/13]: sparse-checkout: free pattern list in sparse_checkout_list()\n  [12/13]: sparse-checkout: free string list after displaying\n  [13/13]: sparse-checkout: free duplicate hashmap entries\n\n builtin/sparse-checkout.c          | 49 +++++++++++++++++++-----------\n dir.c                              | 42 ++++++++++++++++---------\n dir.h                              |  3 +-\n t/t1090-sparse-checkout-scope.sh   |  1 +\n t/t1091-sparse-checkout-builtin.sh |  1 +\n t/t3602-rm-sparse-checkout.sh      |  1 +\n t/t7002-mv-sparse-checkout.sh      |  1 +\n 7 files changed, 65 insertions(+), 33 deletions(-)\n\n-Peff\n"},{"id":"496224","messageId":"20240604101305.GA1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 01/13] sparse-checkout: free string list in write_cone_to_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:05Z","receivedAt":"2024-06-04T10:13:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use a string list to hold sorted and de-duped patterns, but don't\nfree it before leaving the function, causing a leak.\n\nThis drops the number of leaks found in t7002 from 27 to 25.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 0f52e25249..8747191484 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -311,6 +311,8 @@ static void write_cone_to_file(FILE *fp, struct pattern_list *pl)\n \t\tfprintf(fp, \"%s/\\n\", pattern);\n \t\tfree(pattern);\n \t}\n+\n+\tstring_list_clear(&sl, 0);\n }\n \n static int write_patterns_and_update(struct pattern_list *pl)\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496225","messageId":"20240604101310.GB1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 02/13] sparse-checkout: pass string literals directly to add_pattern()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:10Z","receivedAt":"2024-06-04T10:13:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The add_pattern() function takes a pattern string, but neither makes a\ncopy of it nor takes ownership of the memory. So it is the caller's\nresponsibility to make sure the string hangs around as long as the\npattern_list which references it.\n\nThere are a few cases in sparse-checkout where we use string literal\npatterns by stuffing them into a strbuf, detaching the buffer, and then\npassing the result into add_pattern(). This creates a leak when the\npattern_list is eventually cleared, since we don't retain a copy of the\ndetached buffer to free.\n\nBut we can observe that the whole strbuf dance is unnecessary. The point\nwas presumably[1] to satisfy the lifetime requirement of the string. But\nstring literals have static duration; we can count on them lasting for\nthe whole program.\n\nSo we can fix the leak by just passing them directly. And as a bonus,\nthat simplifies the code. The leaks can be seen in t7002, which drops\nfrom 25 leaks to 22 with this patch. It also makes t3602 and t1090\nleak-free.\n\nIn the long run, we will also want to clean up this (undocumented!)\nmemory lifetime requirement of add_pattern(). But that can come in a\nlater patch; passing the string literals directly will be the right\nthing either way.\n\n[1] The code in question comes from 416adc8711 (sparse-checkout: update\n    working directory in-process for 'init', 2019-11-21) and 99dfa6f970\n    (sparse-checkout: use in-process update for disable subcommand,\n    2019-11-21), but I didn't see anything in their commit messages or\n    on the list explaining the strbufs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c        | 11 +++--------\n t/t1090-sparse-checkout-scope.sh |  1 +\n t/t3602-rm-sparse-checkout.sh    |  1 +\n 3 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 8747191484..7c17ed238c 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -442,7 +442,6 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n \tchar *sparse_filename;\n \tint res;\n \tstruct object_id oid;\n-\tstruct strbuf pattern = STRBUF_INIT;\n \n \tstatic struct option builtin_sparse_checkout_init_options[] = {\n \t\tOPT_BOOL(0, \"cone\", &init_opts.cone_mode,\n@@ -493,10 +492,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n-\tstrbuf_addstr(&pattern, \"/*\");\n-\tadd_pattern(strbuf_detach(&pattern, NULL), empty_base, 0, &pl, 0);\n-\tstrbuf_addstr(&pattern, \"!/*/\");\n-\tadd_pattern(strbuf_detach(&pattern, NULL), empty_base, 0, &pl, 0);\n+\tadd_pattern(\"/*\", empty_base, 0, &pl, 0);\n+\tadd_pattern(\"!/*/\", empty_base, 0, &pl, 0);\n \tpl.use_cone_patterns = init_opts.cone_mode;\n \n \treturn write_patterns_and_update(&pl);\n@@ -893,7 +890,6 @@ static int sparse_checkout_disable(int argc, const char **argv,\n \t\tOPT_END(),\n \t};\n \tstruct pattern_list pl;\n-\tstruct strbuf match_all = STRBUF_INIT;\n \n \t/*\n \t * We do not exit early if !core_apply_sparse_checkout; due to the\n@@ -919,8 +915,7 @@ static int sparse_checkout_disable(int argc, const char **argv,\n \tpl.use_cone_patterns = 0;\n \tcore_apply_sparse_checkout = 1;\n \n-\tstrbuf_addstr(&match_all, \"/*\");\n-\tadd_pattern(strbuf_detach(&match_all, NULL), empty_base, 0, &pl, 0);\n+\tadd_pattern(\"/*\", empty_base, 0, &pl, 0);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.sparse_index = 0;\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 3a14218b24..da0e7714d5 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n TEST_CREATE_REPO_NO_TEMPLATE=1\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh\nindex 08580fd3dc..fcdefba48c 100755\n--- a/t/t3602-rm-sparse-checkout.sh\n+++ b/t/t3602-rm-sparse-checkout.sh\n@@ -2,6 +2,7 @@\n \n test_description='git rm in sparse checked out working trees'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' \"\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496226","messageId":"20240604101314.GC1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 03/13] dir.c: free strings in sparse cone pattern hashmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:14Z","receivedAt":"2024-06-04T10:13:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The pattern_list structs used for cone-mode sparse lookups use a few\nextra hashmaps. These store pattern_entry structs, each of which has its\nown heap-allocated pattern string. When we clean up the hashmaps, we\nfree the individual pattern_entry structs, but forget to clean up the\nembedded strings, causing memory leaks.\n\nWe can fix this by iterating over the hashmaps to free the extra\nstrings. This reduces the numbers of leaks in t7002 from 22 to 9.\n\nOne alternative here would be to make the string a FLEX_ARRAY member of\nthe pattern_entry. Then there's no extra free() required, and as a bonus\nit would be a little more efficient. However, some of the refactoring\ngets awkward, as we are often assigning strings allocated by helper\nfunctions. So let's just fix the leak for now, and we can explore bigger\nrefactoring separately.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 2d83f3311a..9cc4a68fbc 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -726,6 +726,17 @@ static char *dup_and_filter_pattern(const char *pattern)\n \treturn result;\n }\n \n+static void clear_pattern_entry_hashmap(struct hashmap *map)\n+{\n+\tstruct hashmap_iter iter;\n+\tstruct pattern_entry *entry;\n+\n+\thashmap_for_each_entry(map, &iter, entry, ent) {\n+\t\tfree(entry->pattern);\n+\t}\n+\thashmap_clear_and_free(map, struct pattern_entry, ent);\n+}\n+\n static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern *given)\n {\n \tstruct pattern_entry *translated;\n@@ -855,8 +866,8 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \n clear_hashmaps:\n \twarning(_(\"disabling cone pattern matching\"));\n-\thashmap_clear_and_free(&pl->parent_hashmap, struct pattern_entry, ent);\n-\thashmap_clear_and_free(&pl->recursive_hashmap, struct pattern_entry, ent);\n+\tclear_pattern_entry_hashmap(&pl->recursive_hashmap);\n+\tclear_pattern_entry_hashmap(&pl->parent_hashmap);\n \tpl->use_cone_patterns = 0;\n }\n \n@@ -956,8 +967,8 @@ void clear_pattern_list(struct pattern_list *pl)\n \t\tfree(pl->patterns[i]);\n \tfree(pl->patterns);\n \tfree(pl->filebuf);\n-\thashmap_clear_and_free(&pl->recursive_hashmap, struct pattern_entry, ent);\n-\thashmap_clear_and_free(&pl->parent_hashmap, struct pattern_entry, ent);\n+\tclear_pattern_entry_hashmap(&pl->recursive_hashmap);\n+\tclear_pattern_entry_hashmap(&pl->parent_hashmap);\n \n \tmemset(pl, 0, sizeof(*pl));\n }\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496227","messageId":"20240604101317.GD1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 04/13] sparse-checkout: clear patterns when init() sees existing sparse file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:17Z","receivedAt":"2024-06-04T10:13:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In sparse_checkout_init(), we first try to load patterns from an\nexisting file. If we found any, we return immediately, but end up\nleaking the patterns we parsed. Fixing this reduces the number of leaks\nin t7002 from 9 down to 5.\n\nNote that there are two other exits from the function, but they don't\nneed the same treatment:\n\n  - if we can't resolve HEAD, we write out a hard-coded sparse file and\n    return. But we know the pattern list is empty there, since we didn't\n    find any in the on-disk file and we haven't yet added any of our\n    own.\n\n  - otherwise, we do populate the list and then tail-call into\n    write_patterns_and_update(). But that function frees the\n    pattern_list itself, so we don't need to.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 7c17ed238c..923e6ecc0a 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -472,6 +472,7 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n \t/* If we already have a sparse-checkout file, use it. */\n \tif (res >= 0) {\n \t\tfree(sparse_filename);\n+\t\tclear_pattern_list(&pl);\n \t\treturn update_working_directory(NULL);\n \t}\n \n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496228","messageId":"20240604101320.GE1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 05/13] dir.c: free removed sparse-pattern hashmap entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:20Z","receivedAt":"2024-06-04T10:13:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In add_pattern_to_hashsets(), we remove entries from the\nrecursive_hashmap when adding similar ones to the parent_hashmap. I\nwon't pretend to understand all of what's going on here, but there's an\nobvious leak: whatever we removed from recursive_hashmap is not\nreferenced anywhere else, and is never free()d.\n\nWe can easily fix this by asking the hashmap to return a pointer to the\nold entry. This makes t7002 now completely leak-free.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c                         | 8 +++++++-\n t/t7002-mv-sparse-checkout.sh | 1 +\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 9cc4a68fbc..d812d521b0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -810,6 +810,8 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \n \tif (given->patternlen > 2 &&\n \t    !strcmp(given->pattern + given->patternlen - 2, \"/*\")) {\n+\t\tstruct pattern_entry *old;\n+\n \t\tif (!(given->flags & PATTERN_FLAG_NEGATIVE)) {\n \t\t\t/* Not a cone pattern. */\n \t\t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n@@ -835,7 +837,11 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \t\t}\n \n \t\thashmap_add(&pl->parent_hashmap, &translated->ent);\n-\t\thashmap_remove(&pl->recursive_hashmap, &translated->ent, &data);\n+\t\told = hashmap_remove_entry(&pl->recursive_hashmap, translated, ent, &data);\n+\t\tif (old) {\n+\t\t\tfree(old->pattern);\n+\t\t\tfree(old);\n+\t\t}\n \t\tfree(data);\n \t\treturn;\n \t}\ndiff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh\nindex 26582ae4e5..57969ce805 100755\n--- a/t/t7002-mv-sparse-checkout.sh\n+++ b/t/t7002-mv-sparse-checkout.sh\n@@ -2,6 +2,7 @@\n \n test_description='git mv in sparse working trees'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n setup_sparse_checkout () {\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496229","messageId":"20240604101322.GF1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 06/13] dir.c: always copy input to add_pattern()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:22Z","receivedAt":"2024-06-04T10:13:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The add_pattern() function has a subtle and undocumented gotcha: the\npattern string you pass in must remain valid as long as the pattern_list\nis in use (and nor do we take ownership of it). This is easy to get\nwrong, causing either subtle bugs (because you free or reuse the string\nbuffer) or leaks (because you copy the string, but don't track ownership\nseparately).\n\nAll of this \"pattern\" code was originally the \"exclude\" mechanism. So\nthis _usually_ works OK because you add entries in one of two ways:\n\n  1. From the command-line (e.g., \"--exclude\"), in which case we're\n     pointing to an argv entry which remains valid for the lifetime of\n     the program.\n\n  2. From a file (e.g., \".gitignore\"), in which case we read the whole\n     file into a buffer, attach it to the pattern_list's \"filebuf\"\n     entry, then parse the buffer in-place (adding NULs). The strings\n     point into the filebuf, which is cleaned up when the whole\n     pattern_list goes away.\n\nBut other code, like sparse-checkout, reads individual lines from stdin\nand passes them one by one to add_pattern(), leaking each. We could fix\nthis by refactoring it to take in the whole buffer at once, like (2)\nabove, and stuff it in \"filebuf\". But given how subtle the interface is,\nlet's just fix it to always copy the string.\n\nThat seems at first like we'd be wasting extra memory, but we can\nmitigate that:\n\n  a. The path_pattern struct already uses a FLEXPTR, since we sometimes\n     make a copy (when we see \"foo/\", we strip off the trailing slash,\n     requiring a modifiable copy of the string).\n\n     Since we'll now always embed the string inside the struct, we can\n     switch to the regular FLEX_ARRAY pattern, saving us 8 bytes of\n     pointer. So patterns with a trailing slash and ones under 8 bytes\n     actually get smaller.\n\n  b. Now that we don't need the original string to hang around, we can\n     get rid of the \"filebuf\" mechanism entirely, and just free the file\n     contents after parsing. Since files are the sources we'd expect to\n     have the largest pattern sets, we should mostly break even on\n     stuffing the same data into the individual structs.\n\nThis patch just adjusts the add_pattern() interface; it doesn't fix any\nleaky callers yet.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c | 15 +++++----------\n dir.h |  3 ++-\n 2 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d812d521b0..8308d167c8 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -925,12 +925,7 @@ void add_pattern(const char *string, const char *base,\n \tint nowildcardlen;\n \n \tparse_path_pattern(&string, &patternlen, &flags, &nowildcardlen);\n-\tif (flags & PATTERN_FLAG_MUSTBEDIR) {\n-\t\tFLEXPTR_ALLOC_MEM(pattern, pattern, string, patternlen);\n-\t} else {\n-\t\tpattern = xmalloc(sizeof(*pattern));\n-\t\tpattern->pattern = string;\n-\t}\n+\tFLEX_ALLOC_MEM(pattern, pattern, string, patternlen);\n \tpattern->patternlen = patternlen;\n \tpattern->nowildcardlen = nowildcardlen;\n \tpattern->base = base;\n@@ -972,7 +967,6 @@ void clear_pattern_list(struct pattern_list *pl)\n \tfor (i = 0; i < pl->nr; i++)\n \t\tfree(pl->patterns[i]);\n \tfree(pl->patterns);\n-\tfree(pl->filebuf);\n \tclear_pattern_entry_hashmap(&pl->recursive_hashmap);\n \tclear_pattern_entry_hashmap(&pl->parent_hashmap);\n \n@@ -1166,23 +1160,23 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n \t}\n \n \tadd_patterns_from_buffer(buf, size, base, baselen, pl);\n+\tfree(buf);\n \treturn 0;\n }\n \n static int add_patterns_from_buffer(char *buf, size_t size,\n \t\t\t\t    const char *base, int baselen,\n \t\t\t\t    struct pattern_list *pl)\n {\n+\tchar *orig = buf;\n \tint i, lineno = 1;\n \tchar *entry;\n \n \thashmap_init(&pl->recursive_hashmap, pl_hashmap_cmp, NULL, 0);\n \thashmap_init(&pl->parent_hashmap, pl_hashmap_cmp, NULL, 0);\n \n-\tpl->filebuf = buf;\n-\n \tif (skip_utf8_bom(&buf, size))\n-\t\tsize -= buf - pl->filebuf;\n+\t\tsize -= buf - orig;\n \n \tentry = buf;\n \n@@ -1222,6 +1216,7 @@ int add_patterns_from_blob_to_list(\n \t\treturn r;\n \n \tadd_patterns_from_buffer(buf, size, base, baselen, pl);\n+\tfree(buf);\n \treturn 0;\n }\n \ndiff --git a/dir.h b/dir.h\nindex b9e8e96128..c8ff308fae 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -62,7 +62,6 @@ struct path_pattern {\n \t */\n \tstruct pattern_list *pl;\n \n-\tconst char *pattern;\n \tint patternlen;\n \tint nowildcardlen;\n \tconst char *base;\n@@ -74,6 +73,8 @@ struct path_pattern {\n \t * and from -1 decrementing for patterns from CLI args.\n \t */\n \tint srcpos;\n+\n+\tchar pattern[FLEX_ARRAY];\n };\n \n /* used for hashmaps for cone patterns */\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496230","messageId":"20240604101325.GG1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 07/13] sparse-checkout: reuse --stdin buffer when reading patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:25Z","receivedAt":"2024-06-04T10:13:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we read patterns from --stdin, we loop on strbuf_getline(), and\ndetach each line we read to pass into add_pattern(). This used to be\nnecessary because add_pattern() required that the pattern strings remain\nvalid while the pattern_list was in use. But it also created a leak,\nsince we didn't record the detached buffers anywhere else.\n\nNow that add_pattern() has been modified to make its own copy of the\nstrings, we can stop detaching and fix the leak. This fixes 4 leaks\ndetected in t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 923e6ecc0a..75c07d5bb4 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -585,11 +585,10 @@ static void add_patterns_from_input(struct pattern_list *pl,\n \t\tif (file) {\n \t\t\tstruct strbuf line = STRBUF_INIT;\n \n-\t\t\twhile (!strbuf_getline(&line, file)) {\n-\t\t\t\tsize_t len;\n-\t\t\t\tchar *buf = strbuf_detach(&line, &len);\n-\t\t\t\tadd_pattern(buf, empty_base, 0, pl, 0);\n-\t\t\t}\n+\t\t\twhile (!strbuf_getline(&line, file))\n+\t\t\t\tadd_pattern(line.buf, empty_base, 0, pl, 0);\n+\n+\t\t\tstrbuf_release(&line);\n \t\t} else {\n \t\t\tfor (i = 0; i < argc; i++)\n \t\t\t\tadd_pattern(argv[i], empty_base, 0, pl, 0);\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496231","messageId":"20240604101327.GH1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 08/13] sparse-checkout: always free \"line\" strbuf after reading input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:27Z","receivedAt":"2024-06-04T10:13:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In add_patterns_from_input(), we may read lines from a file with a loop\nlike this:\n\n  while (!strbuf_getline(&line, file)) {\n\t...\n\tstrbuf_to_cone_pattern(&line, pl);\n  }\n  /* we don't strbuf_release(&line) here! */\n\nThis generally is OK because strbuf_to_cone_pattern() consumes the\nbuffer via strbuf_detach(). But we can leak in a few cases:\n\n  1. We don't always consume the buffer! If the line ends up empty after\n     trimming, we leave strbuf_to_cone_pattern() without detaching. In\n     most cases this is OK, because a subsequent getline() call will use\n     the same buffer. But if you had an empty line at the end of file,\n     for example, it would leak.\n\n  2. Even if strbuf_to_cone_pattern() always consumed the buffer,\n     there's a subtle issue with strbuf_getline(). As we saw in\n     94e2aa555e (strbuf: fix leak when `appendwholeline()` fails with\n     EOF, 2024-05-27), it's possible for it to return EOF with an\n     allocated buffer (e.g., if the underlying getdelim() call saw an\n     error). So we should always strbuf_release() after finishing a read\n     loop like this.\n\nNote that even the code to read patterns from argv has the same problem.\nBecause that also uses strbuf_to_cone_pattern(), we stuff each argv\nentry into a strbuf. It uses the same \"line\" strbuf as the getline code,\nbut we should position the strbuf_release() to cover both code paths.\n\nThis fixes at least 9 leaks found in t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 75c07d5bb4..8f8f5c359f 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -581,6 +581,7 @@ static void add_patterns_from_input(struct pattern_list *pl,\n \t\t\t\tstrbuf_to_cone_pattern(&line, pl);\n \t\t\t}\n \t\t}\n+\t\tstrbuf_release(&line);\n \t} else {\n \t\tif (file) {\n \t\t\tstruct strbuf line = STRBUF_INIT;\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496232","messageId":"20240604101330.GI1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 09/13] sparse-checkout: refactor temporary sparse_checkout_patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:30Z","receivedAt":"2024-06-04T10:13:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In update_working_directory(), we take in a pattern_list, attach it to\nthe repository index by assigning it to index->sparse_checkout_patterns,\nand then call unpack_trees. Afterwards, we remove it by setting\nindex->sparse_checkout_patterns back to NULL.\n\nBut there are two possible leaks here:\n\n  1. If the index already had a populated sparse_checkout_patterns,\n     we've obliterated it. We can fix this by saving and restoring it,\n     rather than always setting it back to NULL.\n\n  2. We may call the function with a NULL pattern_list, expecting it to\n     use the on-disk sparse file. In that case, the index routines will\n     lazy-load the sparse patterns automatically. But now at the end of\n     the function when we restore the patterns, we'll leak those\n     lazy-loaded ones!\n\n     We can fix this by freeing the pattern list before overwriting its\n     pointer whenever it does not match what was passed in (in practice\n     this should only happen when the passed-in list is NULL, but this\n     is erring on the defensive side).\n\nTogether these remove 48 indirect leaks found in t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 8f8f5c359f..b84d2e1c80 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -205,11 +205,13 @@ static int update_working_directory(struct pattern_list *pl)\n \tstruct unpack_trees_options o;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tstruct repository *r = the_repository;\n+\tstruct pattern_list *old_pl;\n \n \t/* If no branch has been checked out, there are no updates to make. */\n \tif (is_index_unborn(r->index))\n \t\treturn UPDATE_SPARSITY_SUCCESS;\n \n+\told_pl = r->index->sparse_checkout_patterns;\n \tr->index->sparse_checkout_patterns = pl;\n \n \tmemset(&o, 0, sizeof(o));\n@@ -241,7 +243,12 @@ static int update_working_directory(struct pattern_list *pl)\n \n \tclean_tracked_sparse_directories(r);\n \n-\tr->index->sparse_checkout_patterns = NULL;\n+\tif (r->index->sparse_checkout_patterns != pl) {\n+\t\tclear_pattern_list(r->index->sparse_checkout_patterns);\n+\t\tFREE_AND_NULL(r->index->sparse_checkout_patterns);\n+\t}\n+\tr->index->sparse_checkout_patterns = old_pl;\n+\n \treturn result;\n }\n \n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496233","messageId":"20240604101332.GJ1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 10/13] sparse-checkout: free sparse_filename after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:32Z","receivedAt":"2024-06-04T10:13:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We allocate a heap buffer via get_sparse_checkout_filename(). Most calls\nremember to free it, but sparse_checkout_init() forgets to, causing a\nleak. Ironically, it remembers to do so in the error return paths, but\nnot in the path that makes it all the way to the function end!\n\nFixing this clears up 6 leaks from t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex b84d2e1c80..79342480eb 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n+\tfree(sparse_filename);\n+\n \tadd_pattern(\"/*\", empty_base, 0, &pl, 0);\n \tadd_pattern(\"!/*/\", empty_base, 0, &pl, 0);\n \tpl.use_cone_patterns = init_opts.cone_mode;\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496234","messageId":"20240604101335.GK1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 11/13] sparse-checkout: free pattern list in sparse_checkout_list()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:35Z","receivedAt":"2024-06-04T10:13:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In sparse_checkout_list(), we create a pattern_list that needs to\neventually be cleared. We remember to do so in the regular code path,\nbut the cone-mode path does an early return, and forgets to clean up.\n\nWe could fix the leak by adding a new call to clear_pattern_list(). But\nwe can simplify even further by just skipping the early return, pushing\nthe other code path (which consists now of only one line!) into an else\nblock. That also matches the same cone/non-cone if/else used in some\nother functions.\n\nThis fixes 15 leaks found in t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 79342480eb..fb43bb7577 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -95,11 +95,10 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix)\n \t\t\tquote_c_style(sl.items[i].string, NULL, stdout, 0);\n \t\t\tprintf(\"\\n\");\n \t\t}\n-\n-\t\treturn 0;\n+\t} else {\n+\t\twrite_patterns_to_file(stdout, &pl);\n \t}\n \n-\twrite_patterns_to_file(stdout, &pl);\n \tclear_pattern_list(&pl);\n \n \treturn 0;\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496235","messageId":"20240604101337.GL1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 12/13] sparse-checkout: free string list after displaying","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:37Z","receivedAt":"2024-06-04T10:13:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In sparse_checkout_list(), we put the hashmap entries into a string_list\nso we can sort them. But after printing, we forget to free the list.\n\nThis patch drops 5 leaks from t1091.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex fb43bb7577..e648e035ab 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -95,6 +95,8 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix)\n \t\t\tquote_c_style(sl.items[i].string, NULL, stdout, 0);\n \t\t\tprintf(\"\\n\");\n \t\t}\n+\n+\t\tstring_list_clear(&sl, 0);\n \t} else {\n \t\twrite_patterns_to_file(stdout, &pl);\n \t}\n-- \n2.45.2.807.g3b5fadc4da\n\n"},{"id":"496236","messageId":"20240604101340.GM1304593@coredump.intra.peff.net","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"[PATCH v2 13/13] sparse-checkout: free duplicate hashmap entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-04T10:13:40Z","receivedAt":"2024-06-04T10:13:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In insert_recursive_pattern(), we create a new pattern_entry to insert\ninto the parent_hashmap. If we find that the same entry already exists\nin the hashmap, we skip adding the new one. But we forget to free the new\none, creating a leak.\n\nWe can fix it by cleaning up the discarded entry. It would probably be\npossible to avoid creating it in the first place, but it's non-trivial.\nWe'd have to define a \"keydata\" struct that lets us compare the existing\nentries to the broken-out fields. It's probably not worth the\ncomplexity, so we'll punt on that for now.\n\nThere is one subtlety here: our insertion is happening in a loop, with\neach iteration looking at the pattern we just inserted (hence the\n\"recursive\" in the name). So if we skip insertion, what do we look at?\n\nThe obvious answer is that we should remember the existing duplicate we\nfound and use that. But I _think_ in that case, we probably already have\nall of the recursive bits already (from when the original entry was\nadded). And so just breaking out of the loop would be correct. But I'm\nnot 100% sure on that; after all, the original leaky code could have\ndone the same break, but it didn't.\n\nSo I went with the \"obvious answer\" above, which has no chance of\nchanging the behavior aside from fixing the leak.\n\nWith this patch, t1091 can now be marked leak-free.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c          | 9 ++++++++-\n t/t1091-sparse-checkout-builtin.sh | 1 +\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex e648e035ab..3f2bfce8fa 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -523,6 +523,7 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat\n \t\tchar *slash = strrchr(e->pattern, '/');\n \t\tchar *oldpattern = e->pattern;\n \t\tsize_t newlen;\n+\t\tstruct pattern_entry *dup;\n \n \t\tif (!slash || slash == e->pattern)\n \t\t\tbreak;\n@@ -533,8 +534,14 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat\n \t\te->pattern = xstrndup(oldpattern, newlen);\n \t\thashmap_entry_init(&e->ent, fspathhash(e->pattern));\n \n-\t\tif (!hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL))\n+\t\tdup = hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL);\n+\t\tif (!dup) {\n \t\t\thashmap_add(&pl->parent_hashmap, &e->ent);\n+\t\t} else {\n+\t\t\tfree(e->pattern);\n+\t\t\tfree(e);\n+\t\t\te = dup;\n+\t\t}\n \t}\n }\n \ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex ab3a105fff..8c5cd651b4 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n GIT_TEST_SPLIT_INDEX=false\n export GIT_TEST_SPLIT_INDEX\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n list_files() {\n-- \n2.45.2.807.g3b5fadc4da\n"},{"id":"496243","messageId":"Zl8E0qyamP4WWhkl@ncase","threadId":"61569","inReplyTo":"20240604100142.GC1298378@coredump.intra.peff.net","subject":"Re: [PATCH 10/13] sparse-checkout: free sparse_filename after use","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-04T12:13:06Z","receivedAt":"2024-06-04T12:13:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 04, 2024 at 06:01:42AM -0400, Jeff King wrote:\n> On Tue, Jun 04, 2024 at 09:42:57AM +0200, Patrick Steinhardt wrote:\n> \n> > > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> > > index 356b7349f9..3af9fec1fb 100644\n> > > --- a/builtin/sparse-checkout.c\n> > > +++ b/builtin/sparse-checkout.c\n> > > @@ -500,6 +500,8 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)\n> > >  \t\treturn 0;\n> > >  \t}\n> > >  \n> > > +\tfree(sparse_filename);\n> > > +\n> > \n> > I wonder whether it would make sense to merge this patch and patch 4\n> > and then refactor the code to have a common exit path.\n> \n> I thought about that, too, but it doesn't quite work. In the non-error\n> exit path we _don't_ clean up the pattern_list, because we tail-call\n> into write_patterns_and_update(), which frees it itself.\n> \n> If we refactored that function to _not_ free, and then switched here to\n> a \"ret\" variable, like:\n> \n> \t...\n> \tret = write_patterns_and_update(&pl);\n>   out:\n> \tclear_pattern_list(&pl);\n> \tfree(sparse_filename);\n> \treturn ret;\n> \n> it could work. I mostly tried to err on the side of minimizing\n> refactoring, though.\n\nFair enough, thanks for explaining.\n\nPatrick\n"},{"id":"496244","messageId":"Zl8FVhWLuhruIovE@ncase","threadId":"61569","inReplyTo":"20240604100814.GA1304520@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/13] leak fixes for sparse-checkout code","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-04T12:15:18Z","receivedAt":"2024-06-04T12:15:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 04, 2024 at 06:08:14AM -0400, Jeff King wrote:\n> On Fri, May 31, 2024 at 07:24:34AM -0400, Jeff King wrote:\n> \n> > So Patrick nerd-sniped me by asking if my earlier leakfix for git-mv was\n> > triggered by the test suite. It was, in t7002, but that wasn't enough to\n> > make the script leak-free. So I figured, how hard could it be to go all\n> > the way?\n> > \n> > Well. It only took a few patches (1-5), but in the process I stumbled on\n> > a rather tricky interface oddity of add_pattern(), which caused some\n> > other leaks. The interface is fixed in patch 6, and the matching leak\n> > goes away in patch 7. Of course, I wanted to make sure it was tested, so\n> > after poking around I found that t1091 triggered it.\n> > \n> > But as you might guess, that didn't make t1091 leak-free. And I couldn't\n> > bear leaving it on a cliffhanger like that, so patches 8-13 fix the rest\n> > of the issues triggered by that script.\n> > \n> > And along the way we managed to make t1090 and t3602 leak-free, too\n> > (actually in patch 2, but I didn't notice until the whole thing was\n> > done).\n> > \n> > These should apply on top of jk/leakfixes, since the leak-freeness of\n> > t7002 depends on the fix there.\n> \n> Here's a v2 with a few minor updates, based on review from Patrick. The\n> restoration of \"old_pl\" in patch 9 is now done unconditionally (it\n> doesn't matter in practice due to a bunch of subtle things, but it's the\n> less surprising and more defensive choice). And a small style update in\n> patch 13.\n\nThanks, the range-diff looks as expected.\n\nPatrick\n"},{"id":"496349","messageId":"f6951a8e-ab88-42f1-8c23-acc65d2d5646@web.de","threadId":"61569","inReplyTo":"20240604101322.GF1304593@coredump.intra.peff.net","subject":"Re: [PATCH v2 06/13] dir.c: always copy input to add_pattern()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-05T08:53:53Z","receivedAt":"2024-06-05T08:54:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.06.24 um 12:13 schrieb Jeff King:\n>   b. Now that we don't need the original string to hang around, we can\n>      get rid of the \"filebuf\" mechanism entirely\n\nRight.  This patch does remove its use from dir.c, but leaves it in dir.h:\n\n> diff --git a/dir.h b/dir.h\n> index b9e8e96128..c8ff308fae 100644\n> --- a/dir.h\n> +++ b/dir.h\n> @@ -62,7 +62,6 @@ struct path_pattern {\n>  \t */\n>  \tstruct pattern_list *pl;\n>\n> -\tconst char *pattern;\n>  \tint patternlen;\n>  \tint nowildcardlen;\n>  \tconst char *base;\n> @@ -74,6 +73,8 @@ struct path_pattern {\n>  \t * and from -1 decrementing for patterns from CLI args.\n>  \t */\n>  \tint srcpos;\n> +\n> +\tchar pattern[FLEX_ARRAY];\n>  };\n>\n>  /* used for hashmaps for cone patterns */\n\nIt can be dropped from struct pattern_list now, for completeness' sake.\n\nRené\n\n"},{"id":"496352","messageId":"20240605091850.GG2345232@coredump.intra.peff.net","threadId":"61569","inReplyTo":"f6951a8e-ab88-42f1-8c23-acc65d2d5646@web.de","subject":"Re: [PATCH v2 06/13] dir.c: always copy input to add_pattern()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-05T09:18:50Z","receivedAt":"2024-06-05T09:18:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 05, 2024 at 10:53:53AM +0200, René Scharfe wrote:\n\n> Am 04.06.24 um 12:13 schrieb Jeff King:\n> >   b. Now that we don't need the original string to hang around, we can\n> >      get rid of the \"filebuf\" mechanism entirely\n> \n> Right.  This patch does remove its use from dir.c, but leaves it in dir.h:\n\nHmph. I'm not sure how I managed that. I definitely removed it from the\nstruct definition in order to find all of the sites that mention it, but\nsomehow that didn't end up in the final patch. Thanks for noticing.\n\nIt looks like the topic hasn't hit next yet. Rather than send a v3 with\nthis tiny change, Junio, do you mind squashing this into patch 6 (it's\nd465adca6d in your tree)?\n\n---\ndiff --git a/dir.h b/dir.h\nindex c8ff308fae..1398a53fb4 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -95,9 +95,6 @@ struct pattern_list {\n \tint nr;\n \tint alloc;\n \n-\t/* remember pointer to exclude file contents so we can free() */\n-\tchar *filebuf;\n-\n \t/* origin of list, e.g. path to filename, or descriptive string */\n \tconst char *src;\n \n\n-Peff\n"},{"id":"496380","messageId":"xmqqa5jzuyb3.fsf@gitster.g","threadId":"61569","inReplyTo":"20240605091850.GG2345232@coredump.intra.peff.net","subject":"Re: [PATCH v2 06/13] dir.c: always copy input to add_pattern()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T16:52:32Z","receivedAt":"2024-06-05T16:52:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It looks like the topic hasn't hit next yet. Rather than send a v3 with\n> this tiny change, Junio, do you mind squashing this into patch 6 (it's\n> d465adca6d in your tree)?\n\nDone.  Thanks, both, for being careful.\n"}]}