From b3449b151767e34a82bf996c4d63feed9ef854bd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 6 Jan 2026 13:58:49 +0100 Subject: [PATCH 1/2] builtin/gc: fix condition for whether to write commit graphs When performing auto-maintenance we check whether commit graphs need to be generated by counting the number of commits that are reachable by any reference, but not covered by a commit graph. This search is performed by iterating through all references and then doing a depth-first search until we have found enough commits that are not present in the commit graph. This logic has a memory leak though: Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x55555562e433 in malloc (git+0xda433) #1 0x555555964322 in do_xmalloc ../wrapper.c:55:8 #2 0x5555559642e6 in xmalloc ../wrapper.c:76:9 #3 0x55555579bf29 in commit_list_append ../commit.c:1872:35 #4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4 #5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12 #6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9 #7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9 #8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11 #9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8 #10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9 #11 0x55555575166a in run_builtin ../git.c:506:11 #12 0x5555557502f0 in handle_builtin ../git.c:779:9 #13 0x555555751127 in run_argv ../git.c:862:4 #14 0x55555575007b in cmd_main ../git.c:984:19 #15 0x5555557523aa in main ../common-main.c:9:11 #16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab) #17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab) #18 0x5555555f0934 in _start (git+0x9c934) The root cause of this memory leak is our use of `commit_list_append()`. This function expects as parameters the item to append and the _tail_ of the list to append. This tail will then be overwritten with the new tail of the list so that it can be used in subsequent calls. But we call it with `commit_list_append(parent->item, &stack)`, so we end up losing everything but the new item. This issue only surfaces when counting merge commits. Next to being a memory leak, it also shows that we're in fact miscounting as we only respect children of the last parent. All previous parents are discarded, so their children will be disregarded unless they are hit via another reference. While crafting a test case for the issue I was puzzled that I couldn't establish the proper border at which the auto-condition would be fulfilled. As it turns out, there's another bug: if an object is at the tip of any reference we don't mark it as seen. Consequently, if it is the tip of or reachable via another ref, we'd count that object multiple times. Fix both of these bugs so that we properly count objects without leaking any memory. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/gc.c | 8 +++++--- t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 92c6e7b954..17ff68cbd9 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1130,8 +1130,10 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data) return 0; commit = lookup_commit(the_repository, maybe_peeled); - if (!commit) + if (!commit || commit->object.flags & SEEN) return 0; + commit->object.flags |= SEEN; + if (repo_parse_commit(the_repository, commit) || commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH) return 0; @@ -1141,7 +1143,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data) if (data->num_not_in_graph >= data->limit) return 1; - commit_list_append(commit, &stack); + commit_list_insert(commit, &stack); while (!result && stack) { struct commit_list *parent; @@ -1162,7 +1164,7 @@ static int dfs_on_ref(const struct reference *ref, void *cb_data) break; } - commit_list_append(parent->item, &stack); + commit_list_insert(parent->item, &stack); } } diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 6b36f52df7..7cc0ce57f8 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -206,6 +206,31 @@ test_expect_success 'commit-graph auto condition' ' test_subcommand $COMMIT_GRAPH_WRITE err && test_grep "is not a valid task" err From 3d099686560b848fefe71b7e8edf70d1674b9c73 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 6 Jan 2026 13:58:50 +0100 Subject: [PATCH 2/2] odb: properly close sources before freeing them It is possible to hit a memory leak when reading data from a submodule via git-grep(1): Direct leak of 192 byte(s) in 1 object(s) allocated from: #0 0x55555562e726 in calloc (git+0xda726) #1 0x555555964734 in xcalloc ../wrapper.c:154:8 #2 0x555555835136 in load_multi_pack_index_one ../midx.c:135:2 #3 0x555555834fd6 in load_multi_pack_index ../midx.c:382:6 #4 0x5555558365b6 in prepare_multi_pack_index_one ../midx.c:716:17 #5 0x55555586c605 in packfile_store_prepare ../packfile.c:1103:3 #6 0x55555586c90c in packfile_store_reprepare ../packfile.c:1118:2 #7 0x5555558546b3 in odb_reprepare ../odb.c:1106:2 #8 0x5555558539e4 in do_oid_object_info_extended ../odb.c:715:4 #9 0x5555558533d1 in odb_read_object_info_extended ../odb.c:862:8 #10 0x5555558540bd in odb_read_object ../odb.c:920:6 #11 0x55555580a330 in grep_source_load_oid ../grep.c:1934:12 #12 0x55555580a13a in grep_source_load ../grep.c:1986:10 #13 0x555555809103 in grep_source_is_binary ../grep.c:2014:7 #14 0x555555807574 in grep_source_1 ../grep.c:1625:8 #15 0x555555807322 in grep_source ../grep.c:1837:10 #16 0x5555556a5c58 in run ../builtin/grep.c:208:10 #17 0x55555562bb42 in void* ThreadStartFunc(void*) lsan_interceptors.cpp.o #18 0x7ffff7a9a979 in start_thread (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x9a979) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab) #19 0x7ffff7b22d2b in __GI___clone3 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x122d2b) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab) The root caues of this leak is the way we set up and release the submodule: 1. We use `repo_submodule_init()` to initialize a new repository. This repository is stored in `repos_to_free`. 2. We now read data from the submodule repository. 3. We then call `repo_clear()` on the submodule repositories. 4. `repo_clear()` calls `odb_free()`. 5. `odb_free()` calls `odb_free_sources()` followed by `odb_close()`. The issue here is the 5th step: we call `odb_free_sources()` _before_ we call `odb_close()`. But `odb_free_sources()` already frees all sources, so the logic that closes them in `odb_close()` now becomes a no-op. As a consequence, we never explicitly close sources at all. Fix the leak by closing the store before we free the sources. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/odb.c b/odb.c index dc8f292f3d..8e67afe185 100644 --- a/odb.c +++ b/odb.c @@ -1132,13 +1132,13 @@ void odb_free(struct object_database *o) oidmap_clear(&o->replace_map, 1); pthread_mutex_destroy(&o->replace_mutex); + odb_close(o); odb_free_sources(o); for (size_t i = 0; i < o->cached_object_nr; i++) free((char *) o->cached_objects[i].value.buf); free(o->cached_objects); - odb_close(o); packfile_store_free(o->packfiles); string_list_clear(&o->submodule_source_paths, 0);