Re: [PATCH v3 03/10] bloom: fix logic in get_bloom_filter()
From: SZEDER Gábor <hidden>
Date: 2020-06-27 16:33:42
On Fri, Jun 26, 2020 at 12:30:29PM +0000, Derrick Stolee via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Derrick Stolee <redacted> The get_bloom_filter() method is a bit complicated in some parts where it does not need to be. In particular, it needs to return a NULL filter only when compute_if_not_present is zero AND the filter data cannot be loaded from a commit-graph file. This currently happens by accident because the commit-graph does not load changed-path Bloom filters from an existing commit-graph when writing a new one. This will change in a later patch. Also clean up some style issues while we are here. Helped-by: René Scharfe [off-list ref] Signed-off-by: Derrick Stolee <redacted> --- bloom.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)diff --git a/bloom.c b/bloom.c index c38d1cff0c..2af5389795 100644 --- a/bloom.c +++ b/bloom.c@@ -186,7 +186,7 @@ struct bloom_filter *get_bloom_filter(struct repository *r, struct diff_options diffopt; int max_changes = 512; - if (bloom_filters.slab_size == 0) + if (!bloom_filters.slab_size) return NULL; filter = bloom_filter_slab_at(&bloom_filters, c);@@ -194,16 +194,14 @@ struct bloom_filter *get_bloom_filter(struct repository *r, if (!filter->data) { load_commit_graph_info(r, c); if (c->graph_pos != COMMIT_NOT_FROM_GRAPH && - r->objects->commit_graph->chunk_bloom_indexes) { - if (load_bloom_filter_from_graph(r->objects->commit_graph, filter, c)) - return filter; - else - return NULL; - } + r->objects->commit_graph->chunk_bloom_indexes) + load_bloom_filter_from_graph(r->objects->commit_graph, filter, c); } - if (filter->data || !compute_if_not_present) + if (filter->data) return filter; + if (!compute_if_not_present) + return NULL;
Some callers of get_bloom_filter() invoke it with
compute_if_not_present=0, but are not prepared to handle a NULL return
value and dereference it right away:
write_graph_chunk_bloom_indexes():
struct bloom_filter *filter = get_bloom_filter(ctx->r, *list, 0);
cur_pos += filter->len;
write_graph_chunk_bloom_data():
struct bloom_filter *filter = get_bloom_filter(ctx->r, *list, 0);
display_progress(progress, ++i);
hashwrite(f, filter->data, filter->len * sizeof(unsigned char));
I don't know whether this was an issue before, but I didn't really
tried. Unfortunately, starting with this patch this causes
segmentation faults basically in all real repositories I use for
testing.
expecting success of 9999.1 'test':
for i in $(test_seq 1 513)
do
>file-$i || return 1
done &&
git add file-* &&
git commit -q -m one &&
git commit-graph write --reachable --changed-paths
Segmentation fault
not ok 1 - test
Program received signal SIGSEGV, Segmentation fault.
0x0000000000515848 in write_graph_chunk_bloom_indexes (f=0x9fe650,
ctx=0x9d2000) at commit-graph.c:1101
1101 cur_pos += filter->len;
(gdb) print filter
$1 = (struct bloom_filter *) 0x0
repo_diff_setup(r, &diffopt); diffopt.flags.recursive = 1; -- gitgitgadget