Re: [PATCH 17/20] commit-graph: check bounds when accessing BDAT chunk

[Jeff King <peff@peff.net>]

On Wed, Oct 11, 2023 at 03:11:59PM -0400, Taylor Blau wrote:

> > +	if (check_bloom_offset(g, lex_pos, end_index) < 0 ||
> > +	    check_bloom_offset(g, lex_pos - 1, start_index) < 0)
> 
> Can lex_pos ever be zero? I can't think of any reason that it couldn't,
> and indeed the (elided) diff context shows that immediately preceding
> this if-statement is another that checks "if (lex_pos > 0)".
> 
> So I think we'd want to avoid checking lex_pos - 1 if we know that
> lex_pos is zero. Not that any of this really matters, since the only
> thing we use the computed value for is showing the graph position in the
> warning() message. So seeing a bogus value there isn't the end of the
> world.

If lex_pos is 0, then we set start_index to 0 manually, which we know to
be valid. So we can't trigger a bogus warning(). My thinking was that it
was OK to just let this fall out naturally as it does here, since it
makes the code shorter. But if we want to cover that case separately,
I think you'd want to split the checks like:

diff --git a/bloom.c b/bloom.c
index 1474aa19fa..d9ad69ad82 100644
--- a/bloom.c
+++ b/bloom.c
@@ -65,16 +65,16 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,
 	lex_pos = graph_pos - g->num_commits_in_base;
 
 	end_index = get_be32(g->chunk_bloom_indexes + 4 * lex_pos);
+	if (check_bloom_offset(g, lex_pos, end_index) < 0)
+		return 0;
 
-	if (lex_pos > 0)
+	if (lex_pos > 0) {
 		start_index = get_be32(g->chunk_bloom_indexes + 4 * (lex_pos - 1));
-	else
+		if (check_bloom_offset(g, lex_pos - 1, start_index) < 0)
+			return 0;
+	} else
 		start_index = 0;
 
-	if (check_bloom_offset(g, lex_pos, end_index) < 0 ||
-	    check_bloom_offset(g, lex_pos - 1, start_index) < 0)
-		return 0;
-
 	if (end_index < start_index) {
 		warning("ignoring decreasing changed-path index offsets"
 			" (%"PRIuMAX" > %"PRIuMAX") for positions"

-Peff