From 7270956809b8380cce9c0e511795fb192918dd02 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 1 Jul 2026 02:39:42 -0400 Subject: [PATCH 1/3] bloom: make bloom-filter slab initialization idempotent Before using any of the commit-graph bloom-filter code, somebody needs to call init_bloom_filters(). This initializes the commit-slab we use for storing filter information. But we don't want to call it twice (without a matching deinit call in the middle), since it overwrites the existing slab pointers, leaking the old values. Usually this init call is done lazily by parse_commit_graph() when we read a graph file that contains bloom data. But this can lead to some oddities: 1. We may call parse_commit_graph() multiple times when we have a split commit graph. I think this doesn't produce any user-visible bug, because we parse all of the files back-to-back. So even though we call init_bloom_filters() multiple times, we never look up any commits in between, so the slab is always empty and initializing it again happens to do nothing. This is a little sketchy to rely on, though. 2. We call init_bloom_filters() directly in the "test-tool bloom" helper so we can call get_or_compute_bloom_filter(). Normally this is OK, as there is no bloom data in the on-disk graph file. But if you build with SANITIZE=leak and run: GIT_TEST_COMMIT_GRAPH=1 \ GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1 \ ./t0095-bloom.sh there's a leak that happens like this: a. Our direct init_bloom_filters() sets up the slab. b. In get_or_compute_bloom_filter() we look in the slab for a cached entry. We won't find anything yet, but since we don't use the read-only "peek" accessor (since we'll fill in the entry if not present), this actually populates the slab with an allocated chunk. c. Now we look for an entry in the graph files. So we have to load them and end up in parse_commit_graph(), which calls init_bloom_filters() again. That trashes our existing slab allocation, which is now leaked. 3. There's a similar case in write_commit_graph(), which calls init_bloom_filters() before get_or_compute_bloom_filter(). I think this code path is lucky to avoid the leak because it reads the graph files first, then calls its init_bloom_filters(), and then starts filling in entries. So even though it has the same overwrite problem, we'd never actually allocate any slab entries between overwrites. The easiest solution here is just to make initialization of the slab idempotent using an extra flag. We could actually get away without using the extra flag, for example by checking whether bloom_filters.stride has been set. But it's probably better to avoid being too intimate with the commit-slab details. Likewise we don't actually need to re-initialize after a deinit call; the slab-clearing function leaves things in a usable state. But it seemed less surprising to pair the init/deinit calls explicitly. I suspect this could all be cleaned up a bit more, but it's tricky. The only function which uses the slab is get_or_compute_bloom_filter(), so it would be much simpler if it just lazy-initialized the slab itself. But I think there is a subtle dependency here: we usually only initialize the slab when we find a graph file that has bloom entries. So if we were to lose that signal, then even repos without on-disk bloom data would start trying to populate the slab, wasting memory that will never get entries filled in from the disk. So we'd need some other way of signaling "it is worth considering bloom entries at all". This patch takes a smaller and more direct route to just dealing with the potential leak issue. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- bloom.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bloom.c b/bloom.c index a805ac0c29..c98d1672ad 100644 --- a/bloom.c +++ b/bloom.c @@ -16,6 +16,7 @@ define_commit_slab(bloom_filter_slab, struct bloom_filter); static struct bloom_filter_slab bloom_filters; +static int bloom_filter_slab_initialized; struct pathmap_hash_entry { struct hashmap_entry entry; @@ -263,7 +264,10 @@ void add_key_to_filter(const struct bloom_key *key, void init_bloom_filters(void) { + if (bloom_filter_slab_initialized) + return; init_bloom_filter_slab(&bloom_filters); + bloom_filter_slab_initialized = 1; } static void free_one_bloom_filter(struct bloom_filter *filter) @@ -276,6 +280,7 @@ static void free_one_bloom_filter(struct bloom_filter *filter) void deinit_bloom_filters(void) { deep_clear_bloom_filter_slab(&bloom_filters, free_one_bloom_filter); + bloom_filter_slab_initialized = 0; } struct bloom_keyvec *bloom_keyvec_new(const char *path, size_t len, From c34573e638262aeb2749493d3d79200f4fa419e1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 1 Jul 2026 02:40:52 -0400 Subject: [PATCH 2/3] revision: avoid leaking bloom keyvecs with multiple traversals In prepare_revision_walk(), we convert the pruning pathspecs into bloom-filter "keyvecs" via prepare_to_use_bloom_filter(). This allocates memory which is then freed eventually by release_revisions(), via release_revisions_bloom_keyvecs(). But there's one case where we leak. If a caller uses the same rev_info for multiple walks, calling prepare_revision_walk() multiple times, then subsequent calls will overwrite the earlier keyvecs, leaking them. This can happen with "git show foo bar", which does a separate no-walk traversal for "foo" and "bar". Building with SANITIZE=leak and running the test suite like: GIT_TEST_COMMIT_GRAPH=1 \ GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1 \ ./t4013-diff-various.sh will trigger a complaint from LSan. It does not happen without those extra flags because we don't store on-disk bloom filters by default, and thus we optimize out the keyvec computation. We can fix the leak by discarding the old entries before generating new ones. There's an alternative fix, which is that prepare_to_use_bloom_filter() could notice that we already have keyvec entries and just reuse them. But this is less safe; the keyvec depends on the pruning pathspec, and we don't know if that has changed. I think it would _probably_ work in practice, since any caller using a rev_info for multiple traversals is probably doing so with the same pathspec. But it would also create a very subtle bug if that assumption is violated. So we'll do the safer thing here, and generate fresh keyvec entries for each traversal. The efficiency difference is probably not noticeable, and this is what was happening already (we just weren't bothering to free the old ones!). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- revision.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/revision.c b/revision.c index 599b3a66c3..c2f3276e48 100644 --- a/revision.c +++ b/revision.c @@ -708,6 +708,8 @@ cleanup: static void prepare_to_use_bloom_filter(struct rev_info *revs) { + release_revisions_bloom_keyvecs(revs); + if (!revs->commits) return; From 459088ec2e3f9c4fea4040d39502d0e011e0ac42 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 1 Jul 2026 02:42:03 -0400 Subject: [PATCH 3/3] line-log: drop extra copy of range with bloom filters When line_log_process_ranges_arbitrary_commit() finds out from a Bloom filter that a commit didn't touch the path in question, it can quickly pass its range on to the parent commit. It does so by making a copy of the range, and passing that copy to add_line_range(). But add_line_range() already makes its own copy (either directly, or by merging with an existing range for that parent). So the copy we make is leaked. We can plug the leak by just passing our range directly, without the extra copy. The bug goes back to f32dde8c12 (line-log: integrate with changed-path Bloom filters, 2020-05-11). We didn't notice because the test suite never explicitly combines these features! You can observe it by building with SANITIZE=leak and running t4211 with some extra flags: GIT_TEST_COMMIT_GRAPH=1 \ GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1 \ ./t4211-line-log.sh It would probably be useful to have some more targeted test coverage of these features together. But I don't think there's much point in just blindly copying the existing tests and adding bloom-filter support. We already do that via the linux-TEST-vars CI job. We just don't run the leak-checking build with those flags (so if there were a correctness problem, we'd have noticed, just not a leak). So I think we'd benefit from somebody clueful thinking about the interaction of these features and testing the corner cases. But for the purposes of this leak fix, I think we can just rely on the recipe above (and consider running an extra leak-test job with more TEST-vars set). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- line-log.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/line-log.c b/line-log.c index 858a899cd2..0a03f20118 100644 --- a/line-log.c +++ b/line-log.c @@ -1153,8 +1153,7 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit if (range) { if (commit->parents && !bloom_filter_check(rev, commit, range)) { - struct line_log_data *prange = line_log_data_copy(range); - add_line_range(rev, commit->parents->item, prange); + add_line_range(rev, commit->parents->item, range); clear_commit_line_range(rev, commit); } else if (commit->parents && commit->parents->next) changed = process_ranges_merge_commit(rev, commit, range);