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(+)