From: Jeff King <hidden> Date: 2026-07-01 06:35:40
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
From: Jeff King <hidden> Date: 2026-07-01 06:39:44
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 <redacted>
---
bloom.c | 5 +++++
1 file changed, 5 insertions(+)
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.
From: Junio C Hamano <hidden> Date: 2026-07-01 15:50:51
Jeff King [off-list ref] 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 <redacted>
---
bloom.c | 5 +++++
1 file changed, 5 insertions(+)
From: Jeff King <hidden> Date: 2026-07-01 06:40:53
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 <redacted>
---
revision.c | 2 ++
1 file changed, 2 insertions(+)
From: Patrick Steinhardt <hidden> Date: 2026-07-01 08:26:54
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
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().
From: Junio C Hamano <hidden> Date: 2026-07-01 15:52:51
Jeff King [off-list ref] 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.
quoted hunk
Signed-off-by: Jeff King <redacted>
---
revision.c | 2 ++
1 file changed, 2 insertions(+)
From: Jeff King <hidden> Date: 2026-07-01 06:42:04
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 <redacted>
---
line-log.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)