Re: [PATCH 12/20] commit-graph: check size of commit data chunk

From: Jeff King <peff@peff.net>
Date: Oct 11, 2023, 23:22 UTC
Message-ID: <20231011232217.GL518221@coredump.intra.peff.net>
In-Reply-To: <ZSbthK5919QdrEPx@nand.local>

On Wed, Oct 11, 2023 at 02:46:28PM -0400, Taylor Blau wrote:

Show 15 quoted lines

> > +static int graph_read_commit_data(const unsigned char *chunk_start,
> > +				  size_t chunk_size, void *data)
> > +{
> > +	struct commit_graph *g = data;
> > +	if (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)
> 
> Should this be guarded with an st_mult? I think that GRAPH_DATA_WIDTH is
> defined as (the_hash_algo->rawsz + 16), so I *think* that the expression
> in the parenthesis would get done as a size_t, and then g->num_commits
> would be widened to a size_t for the purposes of evaluating this
> expression.
> 
> So I think that this is all OK in the sense that we'd never underflow
> the 64-bit space, and having more than 2^64-1/36 (some eighteen
> quintillion) commits is... unlikely ;-).

Hmm, yeah, I think you are right, but I agree it's awfully subtle. There is no reason somebody couldn't later change "rawsz" to a smaller type (after all, we know it's going to be tiny), and it would be quite surprising if that introduces an overflow in far-away code. We should protect ourselves here.

-Peff