commit-graph.c: prevent overflow in add_graph_to_chain()

The commit-graph uses a fanout table with 4-byte entries to store the number of commits at each shard of the commit-graph. So it is OK to have a commit graph with as many as 2^32-1 stored commits. But we risk overflowing any computation which may exceed the 32-bit (unsigned) maximum when those computations are (incorrectly) performed using 32-bit operands. There are a couple of spots in `add_graph_to_chain()` where we could potentially overflow the result: - First, when comparing the list of existing entries in the commit-graph chain. It is unlikely that this should ever overflow, since it would require having roughly 2^32-1/g->hash_len commit-graphs in the chain. But let's guard that computation with a `st_mult()` just to be safe. - Second, when computing the number of commits in the graph added to the front of the chain. This value is also a 32-bit unsigned, but we should make sure that it does not grow beyond the maximum value. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Jul 12, 2023 at 19:37 UTC 209250ef38f353f174ee9e90e55f5a4605449f70
1 file changed +9 -2
commit-graph.c
+9 -2
@@ -482,7 +482,7 @@ static int add_graph_to_chain(struct commit_graph *g,
482
483 if (!cur_g ||
484 !oideq(&oids[n], &cur_g->oid) ||
485 - !hasheq(oids[n].hash, g->chunk_base_graphs + g->hash_len * n)) {
485 + !hasheq(oids[n].hash, g->chunk_base_graphs + st_mult(g->hash_len, n))) {
486 warning(_("commit-graph chain does not match"));
487 return 0;
488 }
@@ -492,8 +492,15 @@ static int add_graph_to_chain(struct commit_graph *g,
492
493 g->base_graph = chain;
494
495 - if (chain)
495 + if (chain) {
496 + if (unsigned_add_overflows(chain->num_commits,
497 + chain->num_commits_in_base)) {
498 + warning(_("commit count in base graph too high: %"PRIuMAX),
499 + (uintmax_t)chain->num_commits_in_base);
500 + return 0;
501 + }
502 g->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;
503 + }
504
505 return 1;
506 }