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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 09, 2023 at 05:05:50PM -0400, Jeff King wrote:
> ---
>  bloom.c              | 24 ++++++++++++++++++++++++
>  commit-graph.c       | 10 ++++++++++
>  commit-graph.h       |  1 +
>  t/t4216-log-bloom.sh | 28 ++++++++++++++++++++++++++++
>  4 files changed, 63 insertions(+)
>
> diff --git a/bloom.c b/bloom.c
> index aef6b5fea2..61abad7f8c 100644
> --- a/bloom.c
> +++ b/bloom.c
> @@ -29,6 +29,26 @@ static inline unsigned char get_bitmask(uint32_t pos)
>  	return ((unsigned char)1) << (pos & (BITS_PER_WORD - 1));
>  }
>
> +static int check_bloom_offset(struct commit_graph *g, uint32_t pos,
> +			      uint32_t offset)
> +{
> +	/*
> +	 * Note that we allow offsets equal to the data size, which would set
> +	 * our pointers at one past the end of the chunk memory. This is
> +	 * necessary because the on-disk index points to the end of the
> +	 * entries (so we can compute size by comparing adjacent ones). And
> +	 * naturally the final entry's end is one-past-the-end of the chunk.
> +	 */
> +	if (offset <= g->chunk_bloom_data_size - BLOOMDATA_CHUNK_HEADER_SIZE)
> +		return 0;
> +
> +	warning("ignoring out-of-range offset (%"PRIuMAX") for changed-path"
> +		" filter at pos %"PRIuMAX" of %s (chunk size: %"PRIuMAX")",
> +		(uintmax_t)offset, (uintmax_t)pos,
> +		g->filename, (uintmax_t)g->chunk_bloom_data_size);

Should this be marked for translation?

> +	return -1;
> +}
> +
>  static int load_bloom_filter_from_graph(struct commit_graph *g,
>  					struct bloom_filter *filter,
>  					uint32_t graph_pos)
> @@ -51,6 +71,10 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,
>  	else
>  		start_index = 0;
>
> +	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.

But avoiding this check when lex_pos == 0 is straightforward, so is
probably worth doing.

(As an aside, we should be able to simplify that if statement to just
"(if lex_pos)", since lex_pos will never be negative).

> +		return 0;
> +
>  	filter->len = end_index - start_index;
>  	filter->data = (unsigned char *)(g->chunk_bloom_data +
>  					sizeof(unsigned char) * start_index +
> diff --git a/commit-graph.c b/commit-graph.c
> index f446e76c28..f7a42be6d0 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -365,7 +365,17 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
>  {
>  	struct commit_graph *g = data;
>  	uint32_t hash_version;
> +
> +	if (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) {
> +		warning("ignoring too-small changed-path chunk"
> +			" (%"PRIuMAX" < %"PRIuMAX") in commit-graph file",
> +			(uintmax_t)chunk_size,
> +			(uintmax_t)BLOOMDATA_CHUNK_HEADER_SIZE);

Ditto on the translation suggestion from above.

Thanks,
Taylor