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

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

Looks trivially correct.