[PATCH 0/10] some commit-graph leak fixes

[PATCH 0/10] some commit-graph leak fixes

[Jeff King]

I noticed while working on the jk/commit-graph-verify-fix topic that
free_commit_graph() leaks any slices of a commit-graph-chain except for
the first. I naively hoped that fixing that would make t5324 leak-free,
but it turns out there were a number of other leaks, so I fixed those,
too. A couple of them were in the merge code, which in turn means a
bunch of new test scripts are now leak-free.

Even though I saw the problem on that other topic, there's no dependency
here; this series can be applied directly to master (or possibly even
maint, though I didn't try).

  [01/10]: t6700: mark test as leak-free
  [02/10]: commit-reach: free temporary list in get_octopus_merge_bases()
  [03/10]: merge: free result of repo_get_merge_bases()
  [04/10]: commit-graph: move slab-clearing to close_commit_graph()
  [05/10]: commit-graph: free all elements of graph chain
  [06/10]: commit-graph: delay base_graph assignment in add_graph_to_chain()
  [07/10]: commit-graph: free graph struct that was not added to chain
  [08/10]: commit-graph: free write-context entries before overwriting
  [09/10]: commit-graph: free write-context base_graph_name during cleanup
  [10/10]: commit-graph: clear oidset after finishing write

 builtin/commit-graph.c             |  1 +
 builtin/merge.c                    |  5 +++-
 commit-graph.c                     | 40 ++++++++++++++----------------
 commit-reach.c                     |  1 +
 t/t4214-log-graph-octopus.sh       |  1 +
 t/t4215-log-skewed-merges.sh       |  1 +
 t/t5324-split-commit-graph.sh      |  2 ++
 t/t5328-commit-graph-64bit-time.sh |  2 ++
 t/t5521-pull-options.sh            |  1 +
 t/t6009-rev-list-parent.sh         |  1 +
 t/t6416-recursive-corner-cases.sh  |  1 +
 t/t6433-merge-toplevel.sh          |  1 +
 t/t6437-submodule-merge.sh         |  1 +
 t/t6700-tree-depth.sh              |  2 ++
 t/t7602-merge-octopus-many.sh      |  1 +
 t/t7603-merge-reduce-heads.sh      |  1 +
 t/t7607-merge-state.sh             |  1 +
 t/t7608-merge-messages.sh          |  1 +
 18 files changed, 42 insertions(+), 22 deletions(-)

-Peff

[PATCH 01/10] t6700: mark test as leak-free

[Jeff King]

This test has never leaked since it was added. Let's annotate it to make
sure it stays that way (and to reduce noise when looking for other
leak-free scripts after we fix some leaks).

Signed-off-by: Jeff King <peff@peff.net>
---
Obviously not directly related to the rest; this could be spun off to
its own series, or put atop jk/tree-name-and-depth-limit and merged from
there.

 t/t6700-tree-depth.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh
index e410c41234..9e70a7c763 100755
--- a/t/t6700-tree-depth.sh
+++ b/t/t6700-tree-depth.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='handling of deep trees in various commands'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # We'll test against two depths here: a small one that will let us check the
-- 
2.42.0.810.gbc538a0ee6

[PATCH 02/10] commit-reach: free temporary list in get_octopus_merge_bases()

[Jeff King]

We loop over the set of commits to merge, and for each one compute the
merge base against the existing set of merge base candidates we've
found. Then we replace the candidate set with a simple assignment of the
list head, leaking the old list. We should free it first before
assignment.

This makes t5521 leak-free, so mark it as such.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-reach.c          | 1 +
 t/t5521-pull-options.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/commit-reach.c b/commit-reach.c
index 4b7c233fd4..a868a575ea 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -173,6 +173,7 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)
 			for (k = bases; k; k = k->next)
 				end = k;
 		}
+		free_commit_list(ret);
 		ret = new_commits;
 	}
 	return ret;
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 264de29c35..079b2f2536 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -5,6 +5,7 @@ test_description='pull options'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.42.0.810.gbc538a0ee6

[PATCH 03/10] merge: free result of repo_get_merge_bases()

[Jeff King]

We call repo_get_merge_bases(), which allocates a commit_list, but never
free the result, causing a leak.

The obvious solution is to free it, but we need to look at the contents
of the first item to decide whether to leave the loop. One option is to
free it in both code paths. But since the commit that the list points to
is longer-lived than the list itself, we can just dereference it
immediately, free the list, and then continue with the existing logic.
This is about the same amount of code, but keeps the list management all
in one place.

This lets us mark a number of merge-related test scripts as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/merge.c                   | 5 ++++-
 t/t4214-log-graph-octopus.sh      | 1 +
 t/t4215-log-skewed-merges.sh      | 1 +
 t/t6009-rev-list-parent.sh        | 1 +
 t/t6416-recursive-corner-cases.sh | 1 +
 t/t6433-merge-toplevel.sh         | 1 +
 t/t6437-submodule-merge.sh        | 1 +
 t/t7602-merge-octopus-many.sh     | 1 +
 t/t7603-merge-reduce-heads.sh     | 1 +
 t/t7607-merge-state.sh            | 1 +
 t/t7608-merge-messages.sh         | 1 +
 11 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index fd21c0d4f4..ac4816c14e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1634,6 +1634,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 		for (j = remoteheads; j; j = j->next) {
 			struct commit_list *common_one;
+			struct commit *common_item;
 
 			/*
 			 * Here we *have* to calculate the individual
@@ -1643,7 +1644,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			common_one = repo_get_merge_bases(the_repository,
 							  head_commit,
 							  j->item);
-			if (!oideq(&common_one->item->object.oid, &j->item->object.oid)) {
+			common_item = common_one->item;
+			free_commit_list(common_one);
+			if (!oideq(&common_item->object.oid, &j->item->object.oid)) {
 				up_to_date = 0;
 				break;
 			}
diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh
index f70c46bbbf..7905597869 100755
--- a/t/t4214-log-graph-octopus.sh
+++ b/t/t4214-log-graph-octopus.sh
@@ -5,6 +5,7 @@ test_description='git log --graph of skewed left octopus merge.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-log-graph.sh
 
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index 28d0779a8c..b877ac7235 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -2,6 +2,7 @@
 
 test_description='git log --graph of skewed merges'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-log-graph.sh
 
diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 5a67bbc760..ced40157ed 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -5,6 +5,7 @@ test_description='ancestor culling and limiting by parent number'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_revlist () {
diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh
index 17b54d625d..5f414abc89 100755
--- a/t/t6416-recursive-corner-cases.sh
+++ b/t/t6416-recursive-corner-cases.sh
@@ -5,6 +5,7 @@ test_description='recursive merge corner cases involving criss-cross merges'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-merge.sh
 
diff --git a/t/t6433-merge-toplevel.sh b/t/t6433-merge-toplevel.sh
index b16031465f..2b42f095dc 100755
--- a/t/t6433-merge-toplevel.sh
+++ b/t/t6433-merge-toplevel.sh
@@ -5,6 +5,7 @@ test_description='"git merge" top-level frontend'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 t3033_reset () {
diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh
index c9a86f2e94..daa507862c 100755
--- a/t/t6437-submodule-merge.sh
+++ b/t/t6437-submodule-merge.sh
@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-merge.sh
 
diff --git a/t/t7602-merge-octopus-many.sh b/t/t7602-merge-octopus-many.sh
index ff085b086c..3669d33bd5 100755
--- a/t/t7602-merge-octopus-many.sh
+++ b/t/t7602-merge-octopus-many.sh
@@ -4,6 +4,7 @@ test_description='git merge
 
 Testing octopus merge with more than 25 refs.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index 4887ca705b..0e85b21ec8 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh
@@ -4,6 +4,7 @@ test_description='git merge
 
 Testing octopus merge when reducing parents to independent branches.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # 0 - 1
diff --git a/t/t7607-merge-state.sh b/t/t7607-merge-state.sh
index 89a62ac53b..9001674f2e 100755
--- a/t/t7607-merge-state.sh
+++ b/t/t7607-merge-state.sh
@@ -4,6 +4,7 @@ test_description="Test that merge state is as expected after failed merge"
 
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'Ensure we restore original state if no merge strategy handles it' '
diff --git a/t/t7608-merge-messages.sh b/t/t7608-merge-messages.sh
index 0b908ab2e7..2179938c43 100755
--- a/t/t7608-merge-messages.sh
+++ b/t/t7608-merge-messages.sh
@@ -4,6 +4,7 @@ test_description='test auto-generated merge messages'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_oneline() {
-- 
2.42.0.810.gbc538a0ee6

[PATCH 04/10] commit-graph: move slab-clearing to close_commit_graph()

[Jeff King]

When closing and freeing a commit-graph, the main entry point is
close_commit_graph(), which then uses close_commit_graph_one() to
recurse through the base_graph links and free each one.

Commit 957ba814bf (commit-graph: when closing the graph, also release
the slab, 2021-09-08) put the call to clear the slab into the recursive
function, but this is pointless: there's only a single global slab
variable. It works OK in practice because clearing the slab is
idempotent, but it makes the code harder to reason about and refactor.

Move it into the parent function so it's only called once (and there are
no other direct callers of the recursive close_commit_graph_one(), so we
are not hurting them).

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 5e8a3a5085..dc54ef4776 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -728,13 +728,13 @@ static void close_commit_graph_one(struct commit_graph *g)
 	if (!g)
 		return;
 
-	clear_commit_graph_data_slab(&commit_graph_data_slab);
 	close_commit_graph_one(g->base_graph);
 	free_commit_graph(g);
 }
 
 void close_commit_graph(struct raw_object_store *o)
 {
+	clear_commit_graph_data_slab(&commit_graph_data_slab);
 	close_commit_graph_one(o->commit_graph);
 	o->commit_graph = NULL;
 }
-- 
2.42.0.810.gbc538a0ee6

[PATCH 05/10] commit-graph: free all elements of graph chain

[Jeff King]

When running "commit-graph verify", we call free_commit_graph(). That's
sufficient for the case of a single graph file, but if we loaded a chain
of split graph files, they form a linked list via the base_graph
pointers. We need to free all of them, or we leak all but the first
struct.

We can make this work by teaching free_commit_graph() to walk the
base_graph pointers and free each element. This in turn lets us simplify
close_commit_graph(), which does the same thing by recursion (we cannot
just use close_commit_graph() in "commit-graph verify", as the function
takes a pointer to an object store, and the verify command creates a
single one-off graph struct).

While indenting the code in free_commit_graph() for the loop, I noticed
that setting g->data to NULL is rather pointless, as we free the struct
a few lines later. So I cleaned that up while we're here.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index dc54ef4776..2f75ecd9ae 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -723,19 +723,10 @@ struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r)
 	return NULL;
 }
 
-static void close_commit_graph_one(struct commit_graph *g)
-{
-	if (!g)
-		return;
-
-	close_commit_graph_one(g->base_graph);
-	free_commit_graph(g);
-}
-
 void close_commit_graph(struct raw_object_store *o)
 {
 	clear_commit_graph_data_slab(&commit_graph_data_slab);
-	close_commit_graph_one(o->commit_graph);
+	free_commit_graph(o->commit_graph);
 	o->commit_graph = NULL;
 }
 
@@ -2753,15 +2744,17 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
 
 void free_commit_graph(struct commit_graph *g)
 {
-	if (!g)
-		return;
-	if (g->data) {
-		munmap((void *)g->data, g->data_len);
-		g->data = NULL;
+	while (g) {
+		struct commit_graph *next = g->base_graph;
+
+		if (g->data)
+			munmap((void *)g->data, g->data_len);
+		free(g->filename);
+		free(g->bloom_filter_settings);
+		free(g);
+
+		g = next;
 	}
-	free(g->filename);
-	free(g->bloom_filter_settings);
-	free(g);
 }
 
 void disable_commit_graph(struct repository *r)
-- 
2.42.0.810.gbc538a0ee6

[PATCH 06/10] commit-graph: delay base_graph assignment in add_graph_to_chain()

[Jeff King]

When adding a graph to a chain, we do some consistency checks and then
if everything looks good, set g->base_graph to add a link to the chain.
But when we added a new consistency check in 209250ef38 (commit-graph.c:
prevent overflow in add_graph_to_chain(), 2023-07-12), it comes _after_
we've already set g->base_graph. So we might return failure, even though
we actually added to the chain.

This hasn't caused a bug yet, because after failing to add to the chain,
we discard the failed graph struct completely, leaking it. But in order
to fix that, it's important that the struct be in a consistent and
predictable state after the failure.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 2f75ecd9ae..2c72a554c2 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -498,8 +498,6 @@ static int add_graph_to_chain(struct commit_graph *g,
 		cur_g = cur_g->base_graph;
 	}
 
-	g->base_graph = chain;
-
 	if (chain) {
 		if (unsigned_add_overflows(chain->num_commits,
 					   chain->num_commits_in_base)) {
@@ -510,6 +508,8 @@ static int add_graph_to_chain(struct commit_graph *g,
 		g->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;
 	}
 
+	g->base_graph = chain;
+
 	return 1;
 }
 
-- 
2.42.0.810.gbc538a0ee6

[PATCH 07/10] commit-graph: free graph struct that was not added to chain

[Jeff King]

When reading the graph chain file, we open (and allocate) each
individual slice it mentions and then add them to a linked-list chain.
But if adding to the chain fails (e.g., because the base-graph chunk it
contains didn't match what we expected), we leave the function without
freeing the graph struct that caused the failure, leaking it.

We can fix it by calling free_graph_commit().

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 2c72a554c2..4aa2f294f1 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -566,6 +566,8 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
 				if (add_graph_to_chain(g, graph_chain, oids, i)) {
 					graph_chain = g;
 					valid = 1;
+				} else {
+					free_commit_graph(g);
 				}
 
 				break;
-- 
2.42.0.810.gbc538a0ee6

[PATCH 08/10] commit-graph: free write-context entries before overwriting

[Jeff King]

When writing a split graph file, we replace the final element of the
commit_graph_hash_after and commit_graph_filenames_after arrays. But
since these are allocated strings, we need to free them before
overwriting to avoid leaking the old string.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 4aa2f294f1..744b7eb1a3 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2065,9 +2065,11 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 			free(graph_name);
 		}
 
+		free(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
 		ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash));
 		final_graph_name = get_split_graph_filename(ctx->odb,
 					ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
+		free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);
 		ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
 
 		result = rename(ctx->graph_name, final_graph_name);
-- 
2.42.0.810.gbc538a0ee6

[PATCH 09/10] commit-graph: free write-context base_graph_name during cleanup

[Jeff King]

Commit 6c622f9f0b (commit-graph: write commit-graph chains, 2019-06-18)
added a base_graph_name string to the write_commit_graph_context struct.
But the end-of-function cleanup forgot to free it, causing a leak.

This (presumably in combination with the preceding leak-fixes) lets us
mark t5328 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c                     | 1 +
 t/t5328-commit-graph-64bit-time.sh | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 744b7eb1a3..e4d09da090 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2518,6 +2518,7 @@ int write_commit_graph(struct object_directory *odb,
 
 cleanup:
 	free(ctx->graph_name);
+	free(ctx->base_graph_name);
 	free(ctx->commits.list);
 	oid_array_clear(&ctx->oids);
 	clear_topo_level_slab(&topo_levels);
diff --git a/t/t5328-commit-graph-64bit-time.sh b/t/t5328-commit-graph-64bit-time.sh
index e9c521c061..ca476e80a0 100755
--- a/t/t5328-commit-graph-64bit-time.sh
+++ b/t/t5328-commit-graph-64bit-time.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='commit graph with 64-bit timestamps'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 if ! test_have_prereq TIME_IS_64BIT || ! test_have_prereq TIME_T_IS_64BIT
-- 
2.42.0.810.gbc538a0ee6

[PATCH 10/10] commit-graph: clear oidset after finishing write

[Jeff King]

In graph_write() we store commits in an oidset, but never clean it up,
leaking the contents. We should clear it in the cleanup section.

The oidset comes from 6830c36077 (commit-graph.h: replace 'commit_hex'
with 'commits', 2020-04-13), but it was just replacing a string_list
that was also leaked. Curiously, we fixed the leak of some adjacent
variables in commit fa8953cb40 (builtin/commit-graph.c: extract
'read_one_commit()', 2020-05-18), but the oidset wasn't included for
some reason.

In combination with the preceding commits, this lets us mark t5324 as
leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit-graph.c        | 1 +
 t/t5324-split-commit-graph.sh | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c88389df24..c527a8369e 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix)
 	FREE_AND_NULL(options);
 	string_list_clear(&pack_indexes, 0);
 	strbuf_release(&buf);
+	oidset_clear(&commits);
 	return result;
 }
 
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 36c4141e67..52e8a3e619 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='split commit graph'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 GIT_TEST_COMMIT_GRAPH=0
-- 
2.42.0.810.gbc538a0ee6

Is SANITIZE=leak make test unreliable for anyone else?

[Eric W. Biederman]

Jeff,

Please accept my apologies for slightly hijacking your posting, but I
see you have been fixing some leaks, and so presumably you are familiar
with building git with "SANITIZE=leak".

I have fixed some leaks in my SHA1+SHA256 patchset recently and while
tracking them down I found that simply enabling SANITIZE=leak caused
"make test" on git v2.42 without patches to give different failures
from test run to test run.

Well actually I wound up with the following command line:
GIT_TEST_PASSING_SANITIZE_LEAK=true GIT_TEST_SANITIZE_LEAK_LOG=true SANITIZE=leak DEVELOPER=1 make test

I had removed "-j32" to make things more reproducible.

I observed this unreliability with SANITIZE=leak when building git on
an fully updated version of debian 12.

My big question is:

    Do other people see random test failures when SANITIZE=leak is enabled?

Is it just me?

Thanks,
Eric

Re: Is SANITIZE=leak make test unreliable for anyone else?

[Jeff King]

On Tue, Oct 03, 2023 at 08:33:26PM -0500, Eric W. Biederman wrote:

> My big question is:
> 
>     Do other people see random test failures when SANITIZE=leak is enabled?
> 
> Is it just me?

Yes, I've seen this. You mentioned that you were testing with v2.42,
which lacks 370ef7e40d (test-lib: ignore uninteresting LSan output,
2023-08-28). Try using the current version of 'master', or just
cherry-picking that commit onto v2.42.

A few other tips to avoid confusing results (though they at least do not
vary from run to run):

  - use the LEAK_LOG option, since you otherwise miss some cases (it
    looks like you already are from what you posted above)

  - gcc and clang sometimes produce different results. Right now I get
    no leak from gcc on t9004, but clang reports one (I think clang is
    right here)

  - turn off compiler optimizations; we've had cases where code
    reordering/removal creates false positives. Oh, hmm, I forgot we do
    this by default since d3775de074 (Makefile: force -O0 when compiling
    with SANITIZE=leak, 2022-10-18), so your v2.42 should be covered.

-Peff

Re: Is SANITIZE=leak make test unreliable for anyone else?

[Eric W. Biederman]

Jeff King <peff@peff.net> writes:

> On Tue, Oct 03, 2023 at 08:33:26PM -0500, Eric W. Biederman wrote:
>
>> My big question is:
>> 
>>     Do other people see random test failures when SANITIZE=leak is enabled?
>> 
>> Is it just me?
>
> Yes, I've seen this. You mentioned that you were testing with v2.42,
> which lacks 370ef7e40d (test-lib: ignore uninteresting LSan output,
> 2023-08-28). Try using the current version of 'master', or just
> cherry-picking that commit onto v2.42.
>
> A few other tips to avoid confusing results (though they at least do not
> vary from run to run):
>
>   - use the LEAK_LOG option, since you otherwise miss some cases (it
>     looks like you already are from what you posted above)
>
>   - gcc and clang sometimes produce different results. Right now I get
>     no leak from gcc on t9004, but clang reports one (I think clang is
>     right here)
>
>   - turn off compiler optimizations; we've had cases where code
>     reordering/removal creates false positives. Oh, hmm, I forgot we do
>     this by default since d3775de074 (Makefile: force -O0 when compiling
>     with SANITIZE=leak, 2022-10-18), so your v2.42 should be covered.

I just tried master, aka commit d0e8084c65cb ("The fourteenth batch").

What I see on a random failure looks like:

> make -C t/ all
> make[1]: Entering directory '/home/user/projects/git/git/t'
> rm -f -r 'test-results'
> GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && make aggregate-results-and-cleanup
> make[2]: Entering directory '/home/user/projects/git/git/t'
> *** t0000-basic.sh ***
> Segmentation fault
> error: test_bool_env requires bool values both for $GIT_TEST_PASSING_SANITIZE_LEAK and for the default fallback

Which doesn't sound like anything you have described so I am guessing it
is something with my environment I need to track down.

Eric

Re: Is SANITIZE=leak make test unreliable for anyone else?

[Jeff King]

On Wed, Oct 04, 2023 at 09:19:40AM -0500, Eric W. Biederman wrote:

> What I see on a random failure looks like:
> 
> > make -C t/ all
> > make[1]: Entering directory '/home/user/projects/git/git/t'
> > rm -f -r 'test-results'
> > GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && make aggregate-results-and-cleanup
> > make[2]: Entering directory '/home/user/projects/git/git/t'
> > *** t0000-basic.sh ***
> > Segmentation fault
> > error: test_bool_env requires bool values both for $GIT_TEST_PASSING_SANITIZE_LEAK and for the default fallback
> 
> Which doesn't sound like anything you have described so I am guessing it
> is something with my environment I need to track down.

No, that seems different entirely. You'll have to figure out which
program is segfaulting and why (if you can see it in a script besides
t0000 you're probably better off, as that one is a maze of
tests-within-tests, since it is testing the test-harness itself).

Although the "error" you see maybe implies that it is failing early on
in test-lib.sh, when we are calling "test-tool env-helper". If that is
segfaulting there is probably something very wrong with your build.

-Peff

Re: Is SANITIZE=leak make test unreliable for anyone else?

[Eric W. Biederman]

Jeff King <peff@peff.net> writes:

> On Wed, Oct 04, 2023 at 09:19:40AM -0500, Eric W. Biederman wrote:
>
>> What I see on a random failure looks like:
>> 
>> > make -C t/ all
>> > make[1]: Entering directory '/home/user/projects/git/git/t'
>> > rm -f -r 'test-results'
>> > GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && make aggregate-results-and-cleanup
>> > make[2]: Entering directory '/home/user/projects/git/git/t'
>> > *** t0000-basic.sh ***
>> > Segmentation fault
>> > error: test_bool_env requires bool values both for $GIT_TEST_PASSING_SANITIZE_LEAK and for the default fallback
>> 
>> Which doesn't sound like anything you have described so I am guessing it
>> is something with my environment I need to track down.
>
> No, that seems different entirely. You'll have to figure out which
> program is segfaulting and why (if you can see it in a script besides
> t0000 you're probably better off, as that one is a maze of
> tests-within-tests, since it is testing the test-harness itself).
>
> Although the "error" you see maybe implies that it is failing early on
> in test-lib.sh, when we are calling "test-tool env-helper". If that is
> segfaulting there is probably something very wrong with your build.

Just to document what I am seeing it appears to be some odd interaction
with address space randomization.

If I run my make as: "setarch --addr-no-randomize make test"

I don't see coredumps any more.

Now to dig a deeper and see if I can figure out what about address space
randomization is making things break.


Eric

Re: [PATCH 01/10] t6700: mark test as leak-free

[Taylor Blau]

On Tue, Oct 03, 2023 at 04:26:09PM -0400, Jeff King wrote:
> This test has never leaked since it was added. Let's annotate it to make
> sure it stays that way (and to reduce noise when looking for other
> leak-free scripts after we fix some leaks).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Obviously not directly related to the rest; this could be spun off to
> its own series, or put atop jk/tree-name-and-depth-limit and merged from
> there.

I wondered how I missed this in tb/mark-more-tests-as-leak-free, but the
answer is trivial: that topic preceded your tree depth stuff ;-).

This change makes good sense, and I don't think it's worth spinning off
into its own series.

Thanks,
Taylor

Re: [PATCH 03/10] merge: free result of repo_get_merge_bases()

[Taylor Blau]

On Tue, Oct 03, 2023 at 04:27:24PM -0400, Jeff King wrote:
> We call repo_get_merge_bases(), which allocates a commit_list, but never
> free the result, causing a leak.
>
> The obvious solution is to free it, but we need to look at the contents
> of the first item to decide whether to leave the loop. One option is to
> free it in both code paths. But since the commit that the list points to
> is longer-lived than the list itself, we can just dereference it
> immediately, free the list, and then continue with the existing logic.
> This is about the same amount of code, but keeps the list management all
> in one place.
>
> This lets us mark a number of merge-related test scripts as leak-free.

Wow, getting 10 newly leak-free tests for half as many lines of code is
terrific. Woohoo!

Thanks,
Taylor

Re: [PATCH 04/10] commit-graph: move slab-clearing to close_commit_graph()

[Taylor Blau]

On Tue, Oct 03, 2023 at 04:27:52PM -0400, Jeff King wrote:
> When closing and freeing a commit-graph, the main entry point is
> close_commit_graph(), which then uses close_commit_graph_one() to
> recurse through the base_graph links and free each one.
>
> Commit 957ba814bf (commit-graph: when closing the graph, also release
> the slab, 2021-09-08) put the call to clear the slab into the recursive
> function, but this is pointless: there's only a single global slab
> variable. It works OK in practice because clearing the slab is
> idempotent, but it makes the code harder to reason about and refactor.

Well reasoned and explained, this change makes perfect sense to me.

Thanks,
Taylor

Re: [PATCH 06/10] commit-graph: delay base_graph assignment in add_graph_to_chain()

[Taylor Blau]

On Tue, Oct 03, 2023 at 04:30:04PM -0400, Jeff King wrote:
> When adding a graph to a chain, we do some consistency checks and then
> if everything looks good, set g->base_graph to add a link to the chain.
> But when we added a new consistency check in 209250ef38 (commit-graph.c:
> prevent overflow in add_graph_to_chain(), 2023-07-12), it comes _after_
> we've already set g->base_graph. So we might return failure, even though
> we actually added to the chain.
>
> This hasn't caused a bug yet, because after failing to add to the chain,
> we discard the failed graph struct completely, leaking it. But in order
> to fix that, it's important that the struct be in a consistent and
> predictable state after the failure.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  commit-graph.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 2f75ecd9ae..2c72a554c2 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -498,8 +498,6 @@ static int add_graph_to_chain(struct commit_graph *g,
>  		cur_g = cur_g->base_graph;
>  	}
>
> -	g->base_graph = chain;
> -
>  	if (chain) {
>  		if (unsigned_add_overflows(chain->num_commits,
>  					   chain->num_commits_in_base)) {
> @@ -510,6 +508,8 @@ static int add_graph_to_chain(struct commit_graph *g,
>  		g->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;
>  	}
>
> +	g->base_graph = chain;
> +

Oops. That looks like my fault. Thanks for catching, this switch makes
sense to me.

Thanks,
Taylor

Re: [PATCH 08/10] commit-graph: free write-context entries before overwriting

[Taylor Blau]

On Tue, Oct 03, 2023 at 04:30:55PM -0400, Jeff King wrote:
> diff --git a/commit-graph.c b/commit-graph.c
> index 4aa2f294f1..744b7eb1a3 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2065,9 +2065,11 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>  			free(graph_name);
>  		}
>
> +		free(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
>  		ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash));
>  		final_graph_name = get_split_graph_filename(ctx->odb,
>  					ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
> +		free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);
>  		ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
>
>  		result = rename(ctx->graph_name, final_graph_name);

This hunk makes sense. It might be nice in the future to do something
like:

--- 8< ---
diff --git a/commit-graph.c b/commit-graph.c
index 5e8a3a5085..cadccbe276 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -59,7 +59,7 @@ void git_test_write_commit_graph_or_die(void)

 #define GRAPH_EXTRA_EDGES_NEEDED 0x80000000
 #define GRAPH_EDGE_LAST_MASK 0x7fffffff
-#define GRAPH_PARENT_NONE 0x70000000
+tdefine GRAPH_PARENT_NONE 0x70000000

 #define GRAPH_LAST_EDGE 0x80000000

@@ -1033,11 +1033,11 @@ struct write_commit_graph_context {
 	uint64_t progress_cnt;

 	char *base_graph_name;
-	int num_commit_graphs_before;
-	int num_commit_graphs_after;
-	char **commit_graph_filenames_before;
-	char **commit_graph_filenames_after;
-	char **commit_graph_hash_after;
+	struct {
+		size_t nr;
+		char **fname;
+		char **hash;
+	} graphs_before, graphs_after;
 	uint32_t new_num_commits_in_base;
 	struct commit_graph *new_base_graph;
--- >8 ---

...making the corresponding changes throughout the rest of the file. But
that is definitely out of scope here, and could easily be left for
another day.

#leftoverbits

Thanks,
Taylor

Re: [PATCH 0/10] some commit-graph leak fixes

[Taylor Blau]

On Tue, Oct 03, 2023 at 04:25:04PM -0400, Jeff King wrote:
> I noticed while working on the jk/commit-graph-verify-fix topic that
> free_commit_graph() leaks any slices of a commit-graph-chain except for
> the first. I naively hoped that fixing that would make t5324 leak-free,
> but it turns out there were a number of other leaks, so I fixed those,
> too. A couple of them were in the merge code, which in turn means a
> bunch of new test scripts are now leak-free.
>
> Even though I saw the problem on that other topic, there's no dependency
> here; this series can be applied directly to master (or possibly even
> maint, though I didn't try).

Thanks for carefully finding and explaining these various leaks. The
series is a definite improvement, and after reviewing closely I couldn't
find anything worth changing. LGTM!

Thanks,
Taylor

Re: [PATCH 08/10] commit-graph: free write-context entries before overwriting

[Jeff King]

On Thu, Oct 05, 2023 at 01:51:42PM -0400, Taylor Blau wrote:

> @@ -1033,11 +1033,11 @@ struct write_commit_graph_context {
>  	uint64_t progress_cnt;
> 
>  	char *base_graph_name;
> -	int num_commit_graphs_before;
> -	int num_commit_graphs_after;
> -	char **commit_graph_filenames_before;
> -	char **commit_graph_filenames_after;
> -	char **commit_graph_hash_after;
> +	struct {
> +		size_t nr;
> +		char **fname;
> +		char **hash;
> +	} graphs_before, graphs_after;
>  	uint32_t new_num_commits_in_base;
>  	struct commit_graph *new_base_graph;
> --- >8 ---
> 
> ...making the corresponding changes throughout the rest of the file. But
> that is definitely out of scope here, and could easily be left for
> another day.

I agree that it would make things a bit more readable, but there
currently is no "hash_before". So they're not quite symmetric.

-Peff

Re: [PATCH 0/10] some commit-graph leak fixes

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Tue, Oct 03, 2023 at 04:25:04PM -0400, Jeff King wrote:
>> I noticed while working on the jk/commit-graph-verify-fix topic that
>> free_commit_graph() leaks any slices of a commit-graph-chain except for
>> the first. I naively hoped that fixing that would make t5324 leak-free,
>> but it turns out there were a number of other leaks, so I fixed those,
>> too. A couple of them were in the merge code, which in turn means a
>> bunch of new test scripts are now leak-free.
>>
>> Even though I saw the problem on that other topic, there's no dependency
>> here; this series can be applied directly to master (or possibly even
>> maint, though I didn't try).
>
> Thanks for carefully finding and explaining these various leaks. The
> series is a definite improvement, and after reviewing closely I couldn't
> find anything worth changing. LGTM!

Thanks, both.  Let's merge it down to 'next'.