{"thread":{"id":"60302","subject":"[PATCH 0/10] some commit-graph leak fixes","startedAt":"2023-10-03T20:25:08Z","lastAt":"2023-10-06T00:40:07Z","messageCount":24,"participants":["Jeff King","Eric W. Biederman","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"482602","messageId":"20231003202504.GA7697@coredump.intra.peff.net","threadId":"60302","inReplyTo":null,"subject":"[PATCH 0/10] some commit-graph leak fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:25:04Z","receivedAt":"2023-10-03T20:25:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed while working on the jk/commit-graph-verify-fix topic that\nfree_commit_graph() leaks any slices of a commit-graph-chain except for\nthe first. I naively hoped that fixing that would make t5324 leak-free,\nbut it turns out there were a number of other leaks, so I fixed those,\ntoo. A couple of them were in the merge code, which in turn means a\nbunch of new test scripts are now leak-free.\n\nEven though I saw the problem on that other topic, there's no dependency\nhere; this series can be applied directly to master (or possibly even\nmaint, though I didn't try).\n\n  [01/10]: t6700: mark test as leak-free\n  [02/10]: commit-reach: free temporary list in get_octopus_merge_bases()\n  [03/10]: merge: free result of repo_get_merge_bases()\n  [04/10]: commit-graph: move slab-clearing to close_commit_graph()\n  [05/10]: commit-graph: free all elements of graph chain\n  [06/10]: commit-graph: delay base_graph assignment in add_graph_to_chain()\n  [07/10]: commit-graph: free graph struct that was not added to chain\n  [08/10]: commit-graph: free write-context entries before overwriting\n  [09/10]: commit-graph: free write-context base_graph_name during cleanup\n  [10/10]: commit-graph: clear oidset after finishing write\n\n builtin/commit-graph.c             |  1 +\n builtin/merge.c                    |  5 +++-\n commit-graph.c                     | 40 ++++++++++++++----------------\n commit-reach.c                     |  1 +\n t/t4214-log-graph-octopus.sh       |  1 +\n t/t4215-log-skewed-merges.sh       |  1 +\n t/t5324-split-commit-graph.sh      |  2 ++\n t/t5328-commit-graph-64bit-time.sh |  2 ++\n t/t5521-pull-options.sh            |  1 +\n t/t6009-rev-list-parent.sh         |  1 +\n t/t6416-recursive-corner-cases.sh  |  1 +\n t/t6433-merge-toplevel.sh          |  1 +\n t/t6437-submodule-merge.sh         |  1 +\n t/t6700-tree-depth.sh              |  2 ++\n t/t7602-merge-octopus-many.sh      |  1 +\n t/t7603-merge-reduce-heads.sh      |  1 +\n t/t7607-merge-state.sh             |  1 +\n t/t7608-merge-messages.sh          |  1 +\n 18 files changed, 42 insertions(+), 22 deletions(-)\n\n-Peff\n"},{"id":"482604","messageId":"20231003202609.GA7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 01/10] t6700: mark test as leak-free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:26:09Z","receivedAt":"2023-10-03T20:26:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This test has never leaked since it was added. Let's annotate it to make\nsure it stays that way (and to reduce noise when looking for other\nleak-free scripts after we fix some leaks).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously not directly related to the rest; this could be spun off to\nits own series, or put atop jk/tree-name-and-depth-limit and merged from\nthere.\n\n t/t6700-tree-depth.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh\nindex e410c41234..9e70a7c763 100755\n--- a/t/t6700-tree-depth.sh\n+++ b/t/t6700-tree-depth.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='handling of deep trees in various commands'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # We'll test against two depths here: a small one that will let us check the\n-- \n2.42.0.810.gbc538a0ee6\n\n"},{"id":"482605","messageId":"20231003202630.GB7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 02/10] commit-reach: free temporary list in get_octopus_merge_bases()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:26:30Z","receivedAt":"2023-10-03T20:26:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We loop over the set of commits to merge, and for each one compute the\nmerge base against the existing set of merge base candidates we've\nfound. Then we replace the candidate set with a simple assignment of the\nlist head, leaking the old list. We should free it first before\nassignment.\n\nThis makes t5521 leak-free, so mark it as such.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-reach.c          | 1 +\n t/t5521-pull-options.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 4b7c233fd4..a868a575ea 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -173,6 +173,7 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)\n \t\t\tfor (k = bases; k; k = k->next)\n \t\t\t\tend = k;\n \t\t}\n+\t\tfree_commit_list(ret);\n \t\tret = new_commits;\n \t}\n \treturn ret;\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 264de29c35..079b2f2536 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -5,6 +5,7 @@ test_description='pull options'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.42.0.810.gbc538a0ee6\n\n"},{"id":"482606","messageId":"20231003202724.GC7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 03/10] merge: free result of repo_get_merge_bases()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:27:24Z","receivedAt":"2023-10-03T20:27:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We call repo_get_merge_bases(), which allocates a commit_list, but never\nfree the result, causing a leak.\n\nThe obvious solution is to free it, but we need to look at the contents\nof the first item to decide whether to leave the loop. One option is to\nfree it in both code paths. But since the commit that the list points to\nis longer-lived than the list itself, we can just dereference it\nimmediately, free the list, and then continue with the existing logic.\nThis is about the same amount of code, but keeps the list management all\nin one place.\n\nThis lets us mark a number of merge-related test scripts as leak-free.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/merge.c                   | 5 ++++-\n t/t4214-log-graph-octopus.sh      | 1 +\n t/t4215-log-skewed-merges.sh      | 1 +\n t/t6009-rev-list-parent.sh        | 1 +\n t/t6416-recursive-corner-cases.sh | 1 +\n t/t6433-merge-toplevel.sh         | 1 +\n t/t6437-submodule-merge.sh        | 1 +\n t/t7602-merge-octopus-many.sh     | 1 +\n t/t7603-merge-reduce-heads.sh     | 1 +\n t/t7607-merge-state.sh            | 1 +\n t/t7608-merge-messages.sh         | 1 +\n 11 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex fd21c0d4f4..ac4816c14e 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1634,6 +1634,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \n \t\tfor (j = remoteheads; j; j = j->next) {\n \t\t\tstruct commit_list *common_one;\n+\t\t\tstruct commit *common_item;\n \n \t\t\t/*\n \t\t\t * Here we *have* to calculate the individual\n@@ -1643,7 +1644,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tcommon_one = repo_get_merge_bases(the_repository,\n \t\t\t\t\t\t\t  head_commit,\n \t\t\t\t\t\t\t  j->item);\n-\t\t\tif (!oideq(&common_one->item->object.oid, &j->item->object.oid)) {\n+\t\t\tcommon_item = common_one->item;\n+\t\t\tfree_commit_list(common_one);\n+\t\t\tif (!oideq(&common_item->object.oid, &j->item->object.oid)) {\n \t\t\t\tup_to_date = 0;\n \t\t\t\tbreak;\n \t\t\t}\ndiff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh\nindex f70c46bbbf..7905597869 100755\n--- a/t/t4214-log-graph-octopus.sh\n+++ b/t/t4214-log-graph-octopus.sh\n@@ -5,6 +5,7 @@ test_description='git log --graph of skewed left octopus merge.'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-log-graph.sh\n \ndiff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh\nindex 28d0779a8c..b877ac7235 100755\n--- a/t/t4215-log-skewed-merges.sh\n+++ b/t/t4215-log-skewed-merges.sh\n@@ -2,6 +2,7 @@\n \n test_description='git log --graph of skewed merges'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-log-graph.sh\n \ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 5a67bbc760..ced40157ed 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -5,6 +5,7 @@ test_description='ancestor culling and limiting by parent number'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check_revlist () {\ndiff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh\nindex 17b54d625d..5f414abc89 100755\n--- a/t/t6416-recursive-corner-cases.sh\n+++ b/t/t6416-recursive-corner-cases.sh\n@@ -5,6 +5,7 @@ test_description='recursive merge corner cases involving criss-cross merges'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-merge.sh\n \ndiff --git a/t/t6433-merge-toplevel.sh b/t/t6433-merge-toplevel.sh\nindex b16031465f..2b42f095dc 100755\n--- a/t/t6433-merge-toplevel.sh\n+++ b/t/t6433-merge-toplevel.sh\n@@ -5,6 +5,7 @@ test_description='\"git merge\" top-level frontend'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n t3033_reset () {\ndiff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh\nindex c9a86f2e94..daa507862c 100755\n--- a/t/t6437-submodule-merge.sh\n+++ b/t/t6437-submodule-merge.sh\n@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1\n export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-merge.sh\n \ndiff --git a/t/t7602-merge-octopus-many.sh b/t/t7602-merge-octopus-many.sh\nindex ff085b086c..3669d33bd5 100755\n--- a/t/t7602-merge-octopus-many.sh\n+++ b/t/t7602-merge-octopus-many.sh\n@@ -4,6 +4,7 @@ test_description='git merge\n \n Testing octopus merge with more than 25 refs.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh\nindex 4887ca705b..0e85b21ec8 100755\n--- a/t/t7603-merge-reduce-heads.sh\n+++ b/t/t7603-merge-reduce-heads.sh\n@@ -4,6 +4,7 @@ test_description='git merge\n \n Testing octopus merge when reducing parents to independent branches.'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # 0 - 1\ndiff --git a/t/t7607-merge-state.sh b/t/t7607-merge-state.sh\nindex 89a62ac53b..9001674f2e 100755\n--- a/t/t7607-merge-state.sh\n+++ b/t/t7607-merge-state.sh\n@@ -4,6 +4,7 @@ test_description=\"Test that merge state is as expected after failed merge\"\n \n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'Ensure we restore original state if no merge strategy handles it' '\ndiff --git a/t/t7608-merge-messages.sh b/t/t7608-merge-messages.sh\nindex 0b908ab2e7..2179938c43 100755\n--- a/t/t7608-merge-messages.sh\n+++ b/t/t7608-merge-messages.sh\n@@ -4,6 +4,7 @@ test_description='test auto-generated merge messages'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check_oneline() {\n-- \n2.42.0.810.gbc538a0ee6\n\n"},{"id":"482607","messageId":"20231003202752.GD7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 04/10] commit-graph: move slab-clearing to close_commit_graph()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:27:52Z","receivedAt":"2023-10-03T20:27:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When closing and freeing a commit-graph, the main entry point is\nclose_commit_graph(), which then uses close_commit_graph_one() to\nrecurse through the base_graph links and free each one.\n\nCommit 957ba814bf (commit-graph: when closing the graph, also release\nthe slab, 2021-09-08) put the call to clear the slab into the recursive\nfunction, but this is pointless: there's only a single global slab\nvariable. It works OK in practice because clearing the slab is\nidempotent, but it makes the code harder to reason about and refactor.\n\nMove it into the parent function so it's only called once (and there are\nno other direct callers of the recursive close_commit_graph_one(), so we\nare not hurting them).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5e8a3a5085..dc54ef4776 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -728,13 +728,13 @@ static void close_commit_graph_one(struct commit_graph *g)\n \tif (!g)\n \t\treturn;\n \n-\tclear_commit_graph_data_slab(&commit_graph_data_slab);\n \tclose_commit_graph_one(g->base_graph);\n \tfree_commit_graph(g);\n }\n \n void close_commit_graph(struct raw_object_store *o)\n {\n+\tclear_commit_graph_data_slab(&commit_graph_data_slab);\n \tclose_commit_graph_one(o->commit_graph);\n \to->commit_graph = NULL;\n }\n-- \n2.42.0.810.gbc538a0ee6\n\n"},{"id":"482608","messageId":"20231003202930.GE7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 05/10] commit-graph: free all elements of graph chain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:29:30Z","receivedAt":"2023-10-03T20:29:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When running \"commit-graph verify\", we call free_commit_graph(). That's\nsufficient for the case of a single graph file, but if we loaded a chain\nof split graph files, they form a linked list via the base_graph\npointers. We need to free all of them, or we leak all but the first\nstruct.\n\nWe can make this work by teaching free_commit_graph() to walk the\nbase_graph pointers and free each element. This in turn lets us simplify\nclose_commit_graph(), which does the same thing by recursion (we cannot\njust use close_commit_graph() in \"commit-graph verify\", as the function\ntakes a pointer to an object store, and the verify command creates a\nsingle one-off graph struct).\n\nWhile indenting the code in free_commit_graph() for the loop, I noticed\nthat setting g->data to NULL is rather pointless, as we free the struct\na few lines later. So I cleaned that up while we're here.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c | 29 +++++++++++------------------\n 1 file changed, 11 insertions(+), 18 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex dc54ef4776..2f75ecd9ae 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -723,19 +723,10 @@ struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r)\n \treturn NULL;\n }\n \n-static void close_commit_graph_one(struct commit_graph *g)\n-{\n-\tif (!g)\n-\t\treturn;\n-\n-\tclose_commit_graph_one(g->base_graph);\n-\tfree_commit_graph(g);\n-}\n-\n void close_commit_graph(struct raw_object_store *o)\n {\n \tclear_commit_graph_data_slab(&commit_graph_data_slab);\n-\tclose_commit_graph_one(o->commit_graph);\n+\tfree_commit_graph(o->commit_graph);\n \to->commit_graph = NULL;\n }\n \n@@ -2753,15 +2744,17 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n \n void free_commit_graph(struct commit_graph *g)\n {\n-\tif (!g)\n-\t\treturn;\n-\tif (g->data) {\n-\t\tmunmap((void *)g->data, g->data_len);\n-\t\tg->data = NULL;\n+\twhile (g) {\n+\t\tstruct commit_graph *next = g->base_graph;\n+\n+\t\tif (g->data)\n+\t\t\tmunmap((void *)g->data, g->data_len);\n+\t\tfree(g->filename);\n+\t\tfree(g->bloom_filter_settings);\n+\t\tfree(g);\n+\n+\t\tg = next;\n \t}\n-\tfree(g->filename);\n-\tfree(g->bloom_filter_settings);\n-\tfree(g);\n }\n \n void disable_commit_graph(struct repository *r)\n-- \n2.42.0.810.gbc538a0ee6\n\n"},{"id":"482609","messageId":"20231003203004.GF7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 06/10] commit-graph: delay base_graph assignment in add_graph_to_chain()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:30:04Z","receivedAt":"2023-10-03T20:30:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When adding a graph to a chain, we do some consistency checks and then\nif everything looks good, set g->base_graph to add a link to the chain.\nBut when we added a new consistency check in 209250ef38 (commit-graph.c:\nprevent overflow in add_graph_to_chain(), 2023-07-12), it comes _after_\nwe've already set g->base_graph. So we might return failure, even though\nwe actually added to the chain.\n\nThis hasn't caused a bug yet, because after failing to add to the chain,\nwe discard the failed graph struct completely, leaking it. But in order\nto fix that, it's important that the struct be in a consistent and\npredictable state after the failure.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2f75ecd9ae..2c72a554c2 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -498,8 +498,6 @@ static int add_graph_to_chain(struct commit_graph *g,\n \t\tcur_g = cur_g->base_graph;\n \t}\n \n-\tg->base_graph = chain;\n-\n \tif (chain) {\n \t\tif (unsigned_add_overflows(chain->num_commits,\n \t\t\t\t\t   chain->num_commits_in_base)) {\n@@ -510,6 +508,8 @@ static int add_graph_to_chain(struct commit_graph *g,\n \t\tg->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;\n \t}\n \n+\tg->base_graph = chain;\n+\n \treturn 1;\n }\n \n-- \n2.42.0.810.gbc538a0ee6\n\n"},{"id":"482610","messageId":"20231003203044.GG7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 07/10] commit-graph: free graph struct that was not added to chain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:30:44Z","receivedAt":"2023-10-03T20:30:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When reading the graph chain file, we open (and allocate) each\nindividual slice it mentions and then add them to a linked-list chain.\nBut if adding to the chain fails (e.g., because the base-graph chunk it\ncontains didn't match what we expected), we leave the function without\nfreeing the graph struct that caused the failure, leaking it.\n\nWe can fix it by calling free_graph_commit().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2c72a554c2..4aa2f294f1 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -566,6 +566,8 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \t\t\t\tif (add_graph_to_chain(g, graph_chain, oids, i)) {\n \t\t\t\t\tgraph_chain = g;\n \t\t\t\t\tvalid = 1;\n+\t\t\t\t} else {\n+\t\t\t\t\tfree_commit_graph(g);\n \t\t\t\t}\n \n \t\t\t\tbreak;\n-- \n2.42.0.810.gbc538a0ee6\n\n"},{"id":"482611","messageId":"20231003203055.GH7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 08/10] commit-graph: free write-context entries before overwriting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:30:55Z","receivedAt":"2023-10-03T20:30:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When writing a split graph file, we replace the final element of the\ncommit_graph_hash_after and commit_graph_filenames_after arrays. But\nsince these are allocated strings, we need to free them before\noverwriting to avoid leaking the old string.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 4aa2f294f1..744b7eb1a3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2065,9 +2065,11 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\tfree(graph_name);\n \t\t}\n \n+\t\tfree(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);\n \t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash));\n \t\tfinal_graph_name = get_split_graph_filename(ctx->odb,\n \t\t\t\t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);\n+\t\tfree(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);\n \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n \n \t\tresult = rename(ctx->graph_name, final_graph_name);\n-- \n2.42.0.810.gbc538a0ee6\n\n"},{"id":"482612","messageId":"20231003203111.GI7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 09/10] commit-graph: free write-context base_graph_name during cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:31:11Z","receivedAt":"2023-10-03T20:31:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 6c622f9f0b (commit-graph: write commit-graph chains, 2019-06-18)\nadded a base_graph_name string to the write_commit_graph_context struct.\nBut the end-of-function cleanup forgot to free it, causing a leak.\n\nThis (presumably in combination with the preceding leak-fixes) lets us\nmark t5328 as leak-free.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c                     | 1 +\n t/t5328-commit-graph-64bit-time.sh | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 744b7eb1a3..e4d09da090 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2518,6 +2518,7 @@ int write_commit_graph(struct object_directory *odb,\n \n cleanup:\n \tfree(ctx->graph_name);\n+\tfree(ctx->base_graph_name);\n \tfree(ctx->commits.list);\n \toid_array_clear(&ctx->oids);\n \tclear_topo_level_slab(&topo_levels);\ndiff --git a/t/t5328-commit-graph-64bit-time.sh b/t/t5328-commit-graph-64bit-time.sh\nindex e9c521c061..ca476e80a0 100755\n--- a/t/t5328-commit-graph-64bit-time.sh\n+++ b/t/t5328-commit-graph-64bit-time.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='commit graph with 64-bit timestamps'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n if ! test_have_prereq TIME_IS_64BIT || ! test_have_prereq TIME_T_IS_64BIT\n-- \n2.42.0.810.gbc538a0ee6\n\n"},{"id":"482614","messageId":"20231003203130.GJ7812@coredump.intra.peff.net","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"[PATCH 10/10] commit-graph: clear oidset after finishing write","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-03T20:31:30Z","receivedAt":"2023-10-03T20:31:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In graph_write() we store commits in an oidset, but never clean it up,\nleaking the contents. We should clear it in the cleanup section.\n\nThe oidset comes from 6830c36077 (commit-graph.h: replace 'commit_hex'\nwith 'commits', 2020-04-13), but it was just replacing a string_list\nthat was also leaked. Curiously, we fixed the leak of some adjacent\nvariables in commit fa8953cb40 (builtin/commit-graph.c: extract\n'read_one_commit()', 2020-05-18), but the oidset wasn't included for\nsome reason.\n\nIn combination with the preceding commits, this lets us mark t5324 as\nleak-free.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit-graph.c        | 1 +\n t/t5324-split-commit-graph.sh | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex c88389df24..c527a8369e 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix)\n \tFREE_AND_NULL(options);\n \tstring_list_clear(&pack_indexes, 0);\n \tstrbuf_release(&buf);\n+\toidset_clear(&commits);\n \treturn result;\n }\n \ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 36c4141e67..52e8a3e619 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='split commit graph'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n GIT_TEST_COMMIT_GRAPH=0\n-- \n2.42.0.810.gbc538a0ee6\n"},{"id":"482633","messageId":"878r8j2mu1.fsf@email.froward.int.ebiederm.org","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"Is SANITIZE=leak make test unreliable for anyone else?","fromName":"Eric W. Biederman","fromEmail":"ebiederm@xmission.com","sentAt":"2023-10-04T01:33:26Z","receivedAt":"2023-10-04T01:33:37Z","isPatch":false,"sender":{"key":"ebiederm@xmission.com","avatar":"https://avatars.githubusercontent.com/u/7477136?v=4"},"body":"\nJeff,\n\nPlease accept my apologies for slightly hijacking your posting, but I\nsee you have been fixing some leaks, and so presumably you are familiar\nwith building git with \"SANITIZE=leak\".\n\nI have fixed some leaks in my SHA1+SHA256 patchset recently and while\ntracking them down I found that simply enabling SANITIZE=leak caused\n\"make test\" on git v2.42 without patches to give different failures\nfrom test run to test run.\n\nWell actually I wound up with the following command line:\nGIT_TEST_PASSING_SANITIZE_LEAK=true GIT_TEST_SANITIZE_LEAK_LOG=true SANITIZE=leak DEVELOPER=1 make test\n\nI had removed \"-j32\" to make things more reproducible.\n\nI observed this unreliability with SANITIZE=leak when building git on\nan fully updated version of debian 12.\n\nMy big question is:\n\n    Do other people see random test failures when SANITIZE=leak is enabled?\n\nIs it just me?\n\nThanks,\nEric\n"},{"id":"482647","messageId":"20231004132132.GC607079@coredump.intra.peff.net","threadId":"60302","inReplyTo":"878r8j2mu1.fsf@email.froward.int.ebiederm.org","subject":"Re: Is SANITIZE=leak make test unreliable for anyone else?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-04T13:21:32Z","receivedAt":"2023-10-04T13:21:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2023 at 08:33:26PM -0500, Eric W. Biederman wrote:\n\n> My big question is:\n> \n>     Do other people see random test failures when SANITIZE=leak is enabled?\n> \n> Is it just me?\n\nYes, I've seen this. You mentioned that you were testing with v2.42,\nwhich lacks 370ef7e40d (test-lib: ignore uninteresting LSan output,\n2023-08-28). Try using the current version of 'master', or just\ncherry-picking that commit onto v2.42.\n\nA few other tips to avoid confusing results (though they at least do not\nvary from run to run):\n\n  - use the LEAK_LOG option, since you otherwise miss some cases (it\n    looks like you already are from what you posted above)\n\n  - gcc and clang sometimes produce different results. Right now I get\n    no leak from gcc on t9004, but clang reports one (I think clang is\n    right here)\n\n  - turn off compiler optimizations; we've had cases where code\n    reordering/removal creates false positives. Oh, hmm, I forgot we do\n    this by default since d3775de074 (Makefile: force -O0 when compiling\n    with SANITIZE=leak, 2022-10-18), so your v2.42 should be covered.\n\n-Peff\n"},{"id":"482649","messageId":"871qea31xf.fsf@email.froward.int.ebiederm.org","threadId":"60302","inReplyTo":"20231004132132.GC607079@coredump.intra.peff.net","subject":"Re: Is SANITIZE=leak make test unreliable for anyone else?","fromName":"Eric W. Biederman","fromEmail":"ebiederm@xmission.com","sentAt":"2023-10-04T14:19:40Z","receivedAt":"2023-10-04T14:20:08Z","isPatch":false,"sender":{"key":"ebiederm@xmission.com","avatar":"https://avatars.githubusercontent.com/u/7477136?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Oct 03, 2023 at 08:33:26PM -0500, Eric W. Biederman wrote:\n>\n>> My big question is:\n>> \n>>     Do other people see random test failures when SANITIZE=leak is enabled?\n>> \n>> Is it just me?\n>\n> Yes, I've seen this. You mentioned that you were testing with v2.42,\n> which lacks 370ef7e40d (test-lib: ignore uninteresting LSan output,\n> 2023-08-28). Try using the current version of 'master', or just\n> cherry-picking that commit onto v2.42.\n>\n> A few other tips to avoid confusing results (though they at least do not\n> vary from run to run):\n>\n>   - use the LEAK_LOG option, since you otherwise miss some cases (it\n>     looks like you already are from what you posted above)\n>\n>   - gcc and clang sometimes produce different results. Right now I get\n>     no leak from gcc on t9004, but clang reports one (I think clang is\n>     right here)\n>\n>   - turn off compiler optimizations; we've had cases where code\n>     reordering/removal creates false positives. Oh, hmm, I forgot we do\n>     this by default since d3775de074 (Makefile: force -O0 when compiling\n>     with SANITIZE=leak, 2022-10-18), so your v2.42 should be covered.\n\nI just tried master, aka commit d0e8084c65cb (\"The fourteenth batch\").\n\nWhat I see on a random failure looks like:\n\n> make -C t/ all\n> make[1]: Entering directory '/home/user/projects/git/git/t'\n> rm -f -r 'test-results'\n> GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && make aggregate-results-and-cleanup\n> make[2]: Entering directory '/home/user/projects/git/git/t'\n> *** t0000-basic.sh ***\n> Segmentation fault\n> error: test_bool_env requires bool values both for $GIT_TEST_PASSING_SANITIZE_LEAK and for the default fallback\n\nWhich doesn't sound like anything you have described so I am guessing it\nis something with my environment I need to track down.\n\nEric\n"},{"id":"482650","messageId":"20231004144734.GA1143669@coredump.intra.peff.net","threadId":"60302","inReplyTo":"871qea31xf.fsf@email.froward.int.ebiederm.org","subject":"Re: Is SANITIZE=leak make test unreliable for anyone else?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-04T14:47:34Z","receivedAt":"2023-10-04T14:47:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 04, 2023 at 09:19:40AM -0500, Eric W. Biederman wrote:\n\n> What I see on a random failure looks like:\n> \n> > make -C t/ all\n> > make[1]: Entering directory '/home/user/projects/git/git/t'\n> > rm -f -r 'test-results'\n> > GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && make aggregate-results-and-cleanup\n> > make[2]: Entering directory '/home/user/projects/git/git/t'\n> > *** t0000-basic.sh ***\n> > Segmentation fault\n> > error: test_bool_env requires bool values both for $GIT_TEST_PASSING_SANITIZE_LEAK and for the default fallback\n> \n> Which doesn't sound like anything you have described so I am guessing it\n> is something with my environment I need to track down.\n\nNo, that seems different entirely. You'll have to figure out which\nprogram is segfaulting and why (if you can see it in a script besides\nt0000 you're probably better off, as that one is a maze of\ntests-within-tests, since it is testing the test-harness itself).\n\nAlthough the \"error\" you see maybe implies that it is failing early on\nin test-lib.sh, when we are calling \"test-tool env-helper\". If that is\nsegfaulting there is probably something very wrong with your build.\n\n-Peff\n"},{"id":"482656","messageId":"87o7he1jp4.fsf@email.froward.int.ebiederm.org","threadId":"60302","inReplyTo":"20231004144734.GA1143669@coredump.intra.peff.net","subject":"Re: Is SANITIZE=leak make test unreliable for anyone else?","fromName":"Eric W. Biederman","fromEmail":"ebiederm@xmission.com","sentAt":"2023-10-04T15:38:47Z","receivedAt":"2023-10-04T15:39:00Z","isPatch":false,"sender":{"key":"ebiederm@xmission.com","avatar":"https://avatars.githubusercontent.com/u/7477136?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Oct 04, 2023 at 09:19:40AM -0500, Eric W. Biederman wrote:\n>\n>> What I see on a random failure looks like:\n>> \n>> > make -C t/ all\n>> > make[1]: Entering directory '/home/user/projects/git/git/t'\n>> > rm -f -r 'test-results'\n>> > GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && make aggregate-results-and-cleanup\n>> > make[2]: Entering directory '/home/user/projects/git/git/t'\n>> > *** t0000-basic.sh ***\n>> > Segmentation fault\n>> > error: test_bool_env requires bool values both for $GIT_TEST_PASSING_SANITIZE_LEAK and for the default fallback\n>> \n>> Which doesn't sound like anything you have described so I am guessing it\n>> is something with my environment I need to track down.\n>\n> No, that seems different entirely. You'll have to figure out which\n> program is segfaulting and why (if you can see it in a script besides\n> t0000 you're probably better off, as that one is a maze of\n> tests-within-tests, since it is testing the test-harness itself).\n>\n> Although the \"error\" you see maybe implies that it is failing early on\n> in test-lib.sh, when we are calling \"test-tool env-helper\". If that is\n> segfaulting there is probably something very wrong with your build.\n\nJust to document what I am seeing it appears to be some odd interaction\nwith address space randomization.\n\nIf I run my make as: \"setarch --addr-no-randomize make test\"\n\nI don't see coredumps any more.\n\nNow to dig a deeper and see if I can figure out what about address space\nrandomization is making things break.\n\n\nEric\n"},{"id":"482697","messageId":"ZR71EdeO1Ige1+Xe@nand.local","threadId":"60302","inReplyTo":"20231003202609.GA7812@coredump.intra.peff.net","subject":"Re: [PATCH 01/10] t6700: mark test as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-05T17:40:33Z","receivedAt":"2023-10-05T17:40:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2023 at 04:26:09PM -0400, Jeff King wrote:\n> This test has never leaked since it was added. Let's annotate it to make\n> sure it stays that way (and to reduce noise when looking for other\n> leak-free scripts after we fix some leaks).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Obviously not directly related to the rest; this could be spun off to\n> its own series, or put atop jk/tree-name-and-depth-limit and merged from\n> there.\n\nI wondered how I missed this in tb/mark-more-tests-as-leak-free, but the\nanswer is trivial: that topic preceded your tree depth stuff ;-).\n\nThis change makes good sense, and I don't think it's worth spinning off\ninto its own series.\n\nThanks,\nTaylor\n"},{"id":"482698","messageId":"ZR71aA4RMoi2TcOJ@nand.local","threadId":"60302","inReplyTo":"20231003202724.GC7812@coredump.intra.peff.net","subject":"Re: [PATCH 03/10] merge: free result of repo_get_merge_bases()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-05T17:42:00Z","receivedAt":"2023-10-05T17:42:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2023 at 04:27:24PM -0400, Jeff King wrote:\n> We call repo_get_merge_bases(), which allocates a commit_list, but never\n> free the result, causing a leak.\n>\n> The obvious solution is to free it, but we need to look at the contents\n> of the first item to decide whether to leave the loop. One option is to\n> free it in both code paths. But since the commit that the list points to\n> is longer-lived than the list itself, we can just dereference it\n> immediately, free the list, and then continue with the existing logic.\n> This is about the same amount of code, but keeps the list management all\n> in one place.\n>\n> This lets us mark a number of merge-related test scripts as leak-free.\n\nWow, getting 10 newly leak-free tests for half as many lines of code is\nterrific. Woohoo!\n\nThanks,\nTaylor\n"},{"id":"482700","messageId":"ZR71mMhMMvEpVidN@nand.local","threadId":"60302","inReplyTo":"20231003202752.GD7812@coredump.intra.peff.net","subject":"Re: [PATCH 04/10] commit-graph: move slab-clearing to close_commit_graph()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-05T17:42:48Z","receivedAt":"2023-10-05T17:44:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2023 at 04:27:52PM -0400, Jeff King wrote:\n> When closing and freeing a commit-graph, the main entry point is\n> close_commit_graph(), which then uses close_commit_graph_one() to\n> recurse through the base_graph links and free each one.\n>\n> Commit 957ba814bf (commit-graph: when closing the graph, also release\n> the slab, 2021-09-08) put the call to clear the slab into the recursive\n> function, but this is pointless: there's only a single global slab\n> variable. It works OK in practice because clearing the slab is\n> idempotent, but it makes the code harder to reason about and refactor.\n\nWell reasoned and explained, this change makes perfect sense to me.\n\nThanks,\nTaylor\n"},{"id":"482701","messageId":"ZR71+Ht3G/xLqJ++@nand.local","threadId":"60302","inReplyTo":"20231003203004.GF7812@coredump.intra.peff.net","subject":"Re: [PATCH 06/10] commit-graph: delay base_graph assignment in add_graph_to_chain()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-05T17:44:24Z","receivedAt":"2023-10-05T17:45:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2023 at 04:30:04PM -0400, Jeff King wrote:\n> When adding a graph to a chain, we do some consistency checks and then\n> if everything looks good, set g->base_graph to add a link to the chain.\n> But when we added a new consistency check in 209250ef38 (commit-graph.c:\n> prevent overflow in add_graph_to_chain(), 2023-07-12), it comes _after_\n> we've already set g->base_graph. So we might return failure, even though\n> we actually added to the chain.\n>\n> This hasn't caused a bug yet, because after failing to add to the chain,\n> we discard the failed graph struct completely, leaking it. But in order\n> to fix that, it's important that the struct be in a consistent and\n> predictable state after the failure.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  commit-graph.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 2f75ecd9ae..2c72a554c2 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -498,8 +498,6 @@ static int add_graph_to_chain(struct commit_graph *g,\n>  \t\tcur_g = cur_g->base_graph;\n>  \t}\n>\n> -\tg->base_graph = chain;\n> -\n>  \tif (chain) {\n>  \t\tif (unsigned_add_overflows(chain->num_commits,\n>  \t\t\t\t\t   chain->num_commits_in_base)) {\n> @@ -510,6 +508,8 @@ static int add_graph_to_chain(struct commit_graph *g,\n>  \t\tg->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;\n>  \t}\n>\n> +\tg->base_graph = chain;\n> +\n\nOops. That looks like my fault. Thanks for catching, this switch makes\nsense to me.\n\nThanks,\nTaylor\n"},{"id":"482703","messageId":"ZR73rpsKiTBF/rYj@nand.local","threadId":"60302","inReplyTo":"20231003203055.GH7812@coredump.intra.peff.net","subject":"Re: [PATCH 08/10] commit-graph: free write-context entries before overwriting","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-05T17:51:42Z","receivedAt":"2023-10-05T17:51:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2023 at 04:30:55PM -0400, Jeff King wrote:\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 4aa2f294f1..744b7eb1a3 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -2065,9 +2065,11 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n>  \t\t\tfree(graph_name);\n>  \t\t}\n>\n> +\t\tfree(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);\n>  \t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash));\n>  \t\tfinal_graph_name = get_split_graph_filename(ctx->odb,\n>  \t\t\t\t\tctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);\n> +\t\tfree(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);\n>  \t\tctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;\n>\n>  \t\tresult = rename(ctx->graph_name, final_graph_name);\n\nThis hunk makes sense. It might be nice in the future to do something\nlike:\n\n--- 8< ---\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5e8a3a5085..cadccbe276 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -59,7 +59,7 @@ void git_test_write_commit_graph_or_die(void)\n\n #define GRAPH_EXTRA_EDGES_NEEDED 0x80000000\n #define GRAPH_EDGE_LAST_MASK 0x7fffffff\n-#define GRAPH_PARENT_NONE 0x70000000\n+tdefine GRAPH_PARENT_NONE 0x70000000\n\n #define GRAPH_LAST_EDGE 0x80000000\n\n@@ -1033,11 +1033,11 @@ struct write_commit_graph_context {\n \tuint64_t progress_cnt;\n\n \tchar *base_graph_name;\n-\tint num_commit_graphs_before;\n-\tint num_commit_graphs_after;\n-\tchar **commit_graph_filenames_before;\n-\tchar **commit_graph_filenames_after;\n-\tchar **commit_graph_hash_after;\n+\tstruct {\n+\t\tsize_t nr;\n+\t\tchar **fname;\n+\t\tchar **hash;\n+\t} graphs_before, graphs_after;\n \tuint32_t new_num_commits_in_base;\n \tstruct commit_graph *new_base_graph;\n--- >8 ---\n\n...making the corresponding changes throughout the rest of the file. But\nthat is definitely out of scope here, and could easily be left for\nanother day.\n\n#leftoverbits\n\nThanks,\nTaylor\n"},{"id":"482704","messageId":"ZR732biF718ju9QU@nand.local","threadId":"60302","inReplyTo":"20231003202504.GA7697@coredump.intra.peff.net","subject":"Re: [PATCH 0/10] some commit-graph leak fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-05T17:52:25Z","receivedAt":"2023-10-05T17:52:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 03, 2023 at 04:25:04PM -0400, Jeff King wrote:\n> I noticed while working on the jk/commit-graph-verify-fix topic that\n> free_commit_graph() leaks any slices of a commit-graph-chain except for\n> the first. I naively hoped that fixing that would make t5324 leak-free,\n> but it turns out there were a number of other leaks, so I fixed those,\n> too. A couple of them were in the merge code, which in turn means a\n> bunch of new test scripts are now leak-free.\n>\n> Even though I saw the problem on that other topic, there's no dependency\n> here; this series can be applied directly to master (or possibly even\n> maint, though I didn't try).\n\nThanks for carefully finding and explaining these various leaks. The\nseries is a definite improvement, and after reviewing closely I couldn't\nfind anything worth changing. LGTM!\n\nThanks,\nTaylor\n"},{"id":"482717","messageId":"20231005210312.GB981206@coredump.intra.peff.net","threadId":"60302","inReplyTo":"ZR73rpsKiTBF/rYj@nand.local","subject":"Re: [PATCH 08/10] commit-graph: free write-context entries before overwriting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-05T21:03:12Z","receivedAt":"2023-10-05T21:03:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 05, 2023 at 01:51:42PM -0400, Taylor Blau wrote:\n\n> @@ -1033,11 +1033,11 @@ struct write_commit_graph_context {\n>  \tuint64_t progress_cnt;\n> \n>  \tchar *base_graph_name;\n> -\tint num_commit_graphs_before;\n> -\tint num_commit_graphs_after;\n> -\tchar **commit_graph_filenames_before;\n> -\tchar **commit_graph_filenames_after;\n> -\tchar **commit_graph_hash_after;\n> +\tstruct {\n> +\t\tsize_t nr;\n> +\t\tchar **fname;\n> +\t\tchar **hash;\n> +\t} graphs_before, graphs_after;\n>  \tuint32_t new_num_commits_in_base;\n>  \tstruct commit_graph *new_base_graph;\n> --- >8 ---\n> \n> ...making the corresponding changes throughout the rest of the file. But\n> that is definitely out of scope here, and could easily be left for\n> another day.\n\nI agree that it would make things a bit more readable, but there\ncurrently is no \"hash_before\". So they're not quite symmetric.\n\n-Peff\n"},{"id":"482732","messageId":"xmqqjzs0bn38.fsf@gitster.g","threadId":"60302","inReplyTo":"ZR732biF718ju9QU@nand.local","subject":"Re: [PATCH 0/10] some commit-graph leak fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-06T00:39:55Z","receivedAt":"2023-10-06T00:40:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Tue, Oct 03, 2023 at 04:25:04PM -0400, Jeff King wrote:\n>> I noticed while working on the jk/commit-graph-verify-fix topic that\n>> free_commit_graph() leaks any slices of a commit-graph-chain except for\n>> the first. I naively hoped that fixing that would make t5324 leak-free,\n>> but it turns out there were a number of other leaks, so I fixed those,\n>> too. A couple of them were in the merge code, which in turn means a\n>> bunch of new test scripts are now leak-free.\n>>\n>> Even though I saw the problem on that other topic, there's no dependency\n>> here; this series can be applied directly to master (or possibly even\n>> maint, though I didn't try).\n>\n> Thanks for carefully finding and explaining these various leaks. The\n> series is a definite improvement, and after reviewing closely I couldn't\n> find anything worth changing. LGTM!\n\nThanks, both.  Let's merge it down to 'next'.\n"}]}