On Wed, Oct 11, 2023 at 02:46:28PM -0400, Taylor Blau wrote:
> > +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
