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

[Junio C Hamano <gitster@pobox.com>]

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,