[PATCH 0/3] bloom-related leak fixes

[PATCH 0/3] bloom-related leak fixes

[Jeff King]

Here are a few small leak fixes that only show up when you run the test
suite with GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1.

Combined with the commit-graph leak-fix here:

  https://lore.kernel.org/git/20260630064301.GB3733961@coredump.intra.peff.net/

and Kaartic's pending fix from this thread:

  https://lore.kernel.org/git/20260614141600.620272-1-kaartic.sivaraam@gmail.com/

This will fix most of the leaks we'd see if we ran linux-TEST-vars jobs
with leak-checking. There are a few more related to building with
openssl for sha1, but I'll tackle those separately.

  [1/3]: bloom: make bloom-filter slab initialization idempotent
  [2/3]: revision: avoid leaking bloom keyvecs with multiple traversals
  [3/3]: line-log: drop extra copy of range with bloom filters

 bloom.c    | 5 +++++
 line-log.c | 3 +--
 revision.c | 2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

-Peff

[PATCH 1/3] bloom: make bloom-filter slab initialization idempotent

[Jeff King]

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 <peff@peff.net>
---
 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,
-- 
2.55.0.394.gcf1c5597d2

Re: [PATCH 1/3] bloom: make bloom-filter slab initialization idempotent

[Derrick Stolee]

On 7/1/2026 2:39 AM, Jeff King wrote:
> 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.
...

> This patch takes a smaller and more direct route to just dealing with
> the potential leak issue.

> +static int bloom_filter_slab_initialized;

>  void init_bloom_filters(void)
>  {
> +	if (bloom_filter_slab_initialized)
> +		return;
>  	init_bloom_filter_slab(&bloom_filters);
> +	bloom_filter_slab_initialized = 1;
>  }

>  {
>  	deep_clear_bloom_filter_slab(&bloom_filters, free_one_bloom_filter);
> +	bloom_filter_slab_initialized = 0;
>  }
This patch looks like the right fix.

Thanks,
-Stolee

Re: [PATCH 1/3] bloom: make bloom-filter slab initialization idempotent

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Yeah, that sounds like an accident waiting to happen.

>
>   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.

Besides, if the test-tool initializes explicitly and the production
code does not and relies on lazy initialization, we are not testing
the production setting, which may hide bugs in lazy initialization.

>   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.

"bool bloom_filter_slab_initialied()" that is generated by including
commit-slab-impl.h can be as intimate with the implementation as we
want, though ;-)

> 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.

Good.

> This patch takes a smaller and more direct route to just dealing with
> the potential leak issue.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  bloom.c | 5 +++++
>  1 file changed, 5 insertions(+)

Looks trivially correct.

> 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,

[PATCH 2/3] revision: avoid leaking bloom keyvecs with multiple traversals

[Jeff King]

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 <peff@peff.net>
---
 revision.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/revision.c b/revision.c
index e91d7e1f11..0ef9d895f0 100644
--- a/revision.c
+++ b/revision.c
@@ -707,6 +707,8 @@ static int convert_pathspec_to_bloom_keyvec(struct bloom_keyvec **out,
 
 static void prepare_to_use_bloom_filter(struct rev_info *revs)
 {
+	release_revisions_bloom_keyvecs(revs);
+
 	if (!revs->commits)
 		return;
 
-- 
2.55.0.394.gcf1c5597d2

Re: [PATCH 2/3] revision: avoid leaking bloom keyvecs with multiple traversals

[Patrick Steinhardt]

On Wed, Jul 01, 2026 at 02:40:52AM -0400, Jeff King wrote:
[snip]
> 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.

Right. We could of course start to record the pruning pathspec so that
we're able to tell these cases apart, and if so we could reuse the bloom
keyvec entries safely. But as you mention...

> 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!).

... we haven't been doing that beforehand, either, so it's fine to not
care about that for now and just plug the memory leak.

Patrick

Re: [PATCH 2/3] revision: avoid leaking bloom keyvecs with multiple traversals

[Derrick Stolee]

On 7/1/2026 2:40 AM, Jeff King wrote:
> 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().

>  static void prepare_to_use_bloom_filter(struct rev_info *revs)
>  {
> +	release_revisions_bloom_keyvecs(revs);
> +
I continue to support the obviously-correct and simple solution to
these leaks.

Thanks,
-Stolee

Re: [PATCH 2/3] revision: avoid leaking bloom keyvecs with multiple traversals

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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!).

Good to see the thinking behind the design recorded so clearly in the log
message.  That thinking being on the more conservative side is a big plus.



> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  revision.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index e91d7e1f11..0ef9d895f0 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -707,6 +707,8 @@ static int convert_pathspec_to_bloom_keyvec(struct bloom_keyvec **out,
>  
>  static void prepare_to_use_bloom_filter(struct rev_info *revs)
>  {
> +	release_revisions_bloom_keyvecs(revs);
> +
>  	if (!revs->commits)
>  		return;

[PATCH 3/3] line-log: drop extra copy of range with bloom filters

[Jeff King]

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 <peff@peff.net>
---
 line-log.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/line-log.c b/line-log.c
index 5fc75ae275..0179f138f7 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1141,8 +1141,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);
-- 
2.55.0.394.gcf1c5597d2

Re: [PATCH 0/3] bloom-related leak fixes

[Derrick Stolee]

On 7/1/2026 2:35 AM, Jeff King wrote:
> Here are a few small leak fixes that only show up when you run the test
> suite with GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1.
> 
> Combined with the commit-graph leak-fix here:
> 
>   https://lore.kernel.org/git/20260630064301.GB3733961@coredump.intra.peff.net/
> 
> and Kaartic's pending fix from this thread:
> 
>   https://lore.kernel.org/git/20260614141600.620272-1-kaartic.sivaraam@gmail.com/
> 
> This will fix most of the leaks we'd see if we ran linux-TEST-vars jobs
> with leak-checking. There are a few more related to building with
> openssl for sha1, but I'll tackle those separately.
Thanks for fixing these leaks in the simplest way possible in
each scenario.

Thanks,
-Stolee

Re: [PATCH 0/3] bloom-related leak fixes

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> On 7/1/2026 2:35 AM, Jeff King wrote:
>> Here are a few small leak fixes that only show up when you run the test
>> suite with GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1.
>> 
>> Combined with the commit-graph leak-fix here:
>> 
>>   https://lore.kernel.org/git/20260630064301.GB3733961@coredump.intra.peff.net/
>> 
>> and Kaartic's pending fix from this thread:
>> 
>>   https://lore.kernel.org/git/20260614141600.620272-1-kaartic.sivaraam@gmail.com/
>> 
>> This will fix most of the leaks we'd see if we ran linux-TEST-vars jobs
>> with leak-checking. There are a few more related to building with
>> openssl for sha1, but I'll tackle those separately.
> Thanks for fixing these leaks in the simplest way possible in
> each scenario.

Yup, these were delight to read.