diff --git a/commit-graph.c b/commit-graph.c index ee66098e07..acac9bf6e1 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -275,68 +275,37 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r, return ret; } -static int verify_commit_graph_lite(struct commit_graph *g) +static int graph_read_oid_fanout(const unsigned char *chunk_start, + size_t chunk_size, void *data) { + struct commit_graph *g = data; int i; - /* - * Basic validation shared between parse_commit_graph() - * which'll be called every time the graph is used, and the - * much more expensive verify_commit_graph() used by - * "commit-graph verify". - * - * There should only be very basic checks here to ensure that - * we don't e.g. segfault in fill_commit_in_graph(), but - * because this is a very hot codepath nothing that e.g. loops - * over g->num_commits, or runs a checksum on the commit-graph - * itself. - */ - if (!g->chunk_oid_fanout) { - error("commit-graph is missing the OID Fanout chunk"); - return 1; - } - if (!g->chunk_oid_lookup) { - error("commit-graph is missing the OID Lookup chunk"); - return 1; - } - if (!g->chunk_commit_data) { - error("commit-graph is missing the Commit Data chunk"); - return 1; - } + if (chunk_size != 256 * sizeof(uint32_t)) + return error(_("commit-graph oid fanout chunk is wrong size")); + g->chunk_oid_fanout = (const uint32_t *)chunk_start; + g->num_commits = ntohl(g->chunk_oid_fanout[255]); for (i = 0; i < 255; i++) { uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]); uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]); if (oid_fanout1 > oid_fanout2) { - error("commit-graph fanout values out of order"); + error(_("commit-graph fanout values out of order")); return 1; } } - if (ntohl(g->chunk_oid_fanout[255]) != g->num_commits) { - error("commit-graph oid table and fanout disagree on size"); - return 1; - } return 0; } -static int graph_read_oid_fanout(const unsigned char *chunk_start, - size_t chunk_size, void *data) -{ - struct commit_graph *g = data; - if (chunk_size != 256 * sizeof(uint32_t)) - return error("commit-graph oid fanout chunk is wrong size"); - g->chunk_oid_fanout = (const uint32_t *)chunk_start; - return 0; -} - static int graph_read_oid_lookup(const unsigned char *chunk_start, size_t chunk_size, void *data) { struct commit_graph *g = data; g->chunk_oid_lookup = chunk_start; - g->num_commits = chunk_size / g->hash_len; + if (chunk_size / g->hash_len != g->num_commits) + return error(_("commit-graph OID lookup chunk is the wrong size")); return 0; } @@ -344,8 +313,8 @@ 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) - return error("commit-graph commit data chunk is wrong size"); + if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits) + return error(_("commit-graph commit data chunk is wrong size")); g->chunk_commit_data = chunk_start; return 0; } @@ -354,8 +323,8 @@ static int graph_read_generation_data(const unsigned char *chunk_start, size_t chunk_size, void *data) { struct commit_graph *g = data; - if (chunk_size != g->num_commits * sizeof(uint32_t)) - return error("commit-graph generations chunk is wrong size"); + if (chunk_size / sizeof(uint32_t) != g->num_commits) + return error(_("commit-graph generations chunk is wrong size")); g->chunk_generation_data = chunk_start; return 0; } @@ -364,8 +333,8 @@ static int graph_read_bloom_index(const unsigned char *chunk_start, size_t chunk_size, void *data) { struct commit_graph *g = data; - if (chunk_size != g->num_commits * 4) { - warning("commit-graph changed-path index chunk is too small"); + if (chunk_size / 4 != g->num_commits) { + warning(_("commit-graph changed-path index chunk is too small")); return -1; } g->chunk_bloom_indexes = chunk_start; @@ -379,8 +348,8 @@ static int graph_read_bloom_data(const unsigned char *chunk_start, uint32_t hash_version; if (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) { - warning("ignoring too-small changed-path chunk" - " (%"PRIuMAX" < %"PRIuMAX") in commit-graph file", + warning(_("ignoring too-small changed-path chunk" + " (%"PRIuMAX" < %"PRIuMAX") in commit-graph file"), (uintmax_t)chunk_size, (uintmax_t)BLOOMDATA_CHUNK_HEADER_SIZE); return -1; @@ -462,9 +431,19 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s, GRAPH_HEADER_SIZE, graph->num_chunks, 1)) goto free_and_return; - read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph); - read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph); - read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph); + if (read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph)) { + error(_("commit-graph required OID fanout chunk missing or corrupted")); + goto free_and_return; + } + if (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) { + error(_("commit-graph required OID lookup chunk missing or corrupted")); + goto free_and_return; + } + if (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) { + error(_("commit-graph required commit data chunk missing or corrupted")); + goto free_and_return; + } + pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges, &graph->chunk_extra_edges_size); pair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs, @@ -499,9 +478,6 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s, oidread(&graph->oid, graph->data + graph->data_len - graph->hash_len); - if (verify_commit_graph_lite(graph)) - goto free_and_return; - free_chunkfile(cf); return graph; @@ -629,7 +605,7 @@ int open_commit_graph_chain(const char *chain_file, /* treat empty files the same as missing */ errno = ENOENT; } else { - warning("commit-graph chain file too small"); + warning(_("commit-graph chain file too small")); errno = EINVAL; } return 0; @@ -970,7 +946,7 @@ static int fill_commit_in_graph(struct repository *r, parent_data_pos = edge_value & GRAPH_EDGE_LAST_MASK; do { if (g->chunk_extra_edges_size / sizeof(uint32_t) <= parent_data_pos) { - error("commit-graph extra-edges pointer out of bounds"); + error(_("commit-graph extra-edges pointer out of bounds")); free_commit_list(item->parents); item->parents = NULL; item->object.parsed = 0; @@ -2690,10 +2666,6 @@ static int verify_one_commit_graph(struct repository *r, struct commit *seen_gen_zero = NULL; struct commit *seen_gen_non_zero = NULL; - verify_commit_graph_error = verify_commit_graph_lite(g); - if (verify_commit_graph_error) - return verify_commit_graph_error; - if (!commit_graph_checksum_valid(g)) { graph_report(_("the commit-graph file has incorrect checksum and is likely corrupt")); verify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH; diff --git a/midx.c b/midx.c index 2f3863c936..1d14661dad 100644 --- a/midx.c +++ b/midx.c @@ -64,6 +64,7 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m) static int midx_read_oid_fanout(const unsigned char *chunk_start, size_t chunk_size, void *data) { + int i; struct multi_pack_index *m = data; m->chunk_oid_fanout = (uint32_t *)chunk_start; @@ -71,6 +72,16 @@ static int midx_read_oid_fanout(const unsigned char *chunk_start, error(_("multi-pack-index OID fanout is of the wrong size")); return 1; } + for (i = 0; i < 255; i++) { + uint32_t oid_fanout1 = ntohl(m->chunk_oid_fanout[i]); + uint32_t oid_fanout2 = ntohl(m->chunk_oid_fanout[i+1]); + + if (oid_fanout1 > oid_fanout2) { + error(_("oid fanout out of order: fanout[%d] = %"PRIx32" > %"PRIx32" = fanout[%d]"), + i, oid_fanout1, oid_fanout2, i + 1); + return 1; + } + } m->num_objects = ntohl(m->chunk_oid_fanout[255]); return 0; } @@ -1782,15 +1793,6 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag } stop_progress(&progress); - for (i = 0; i < 255; i++) { - uint32_t oid_fanout1 = ntohl(m->chunk_oid_fanout[i]); - uint32_t oid_fanout2 = ntohl(m->chunk_oid_fanout[i + 1]); - - if (oid_fanout1 > oid_fanout2) - midx_report(_("oid fanout out of order: fanout[%d] = %"PRIx32" > %"PRIx32" = fanout[%d]"), - i, oid_fanout1, oid_fanout2, i + 1); - } - if (m->num_objects == 0) { midx_report(_("the midx contains no oid")); /* diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index d4fc65e078..7fe7c72a87 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -540,17 +540,17 @@ test_expect_success 'detect low chunk count' ' test_expect_success 'detect missing OID fanout chunk' ' corrupt_graph_and_verify $GRAPH_BYTE_OID_FANOUT_ID "\0" \ - "missing the OID Fanout chunk" + "commit-graph required OID fanout chunk missing or corrupted" ' test_expect_success 'detect missing OID lookup chunk' ' corrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_ID "\0" \ - "missing the OID Lookup chunk" + "commit-graph required OID lookup chunk missing or corrupted" ' test_expect_success 'detect missing commit data chunk' ' corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATA_ID "\0" \ - "missing the Commit Data chunk" + "commit-graph required commit data chunk missing or corrupted" ' test_expect_success 'detect incorrect fanout' ' @@ -560,7 +560,7 @@ test_expect_success 'detect incorrect fanout' ' test_expect_success 'detect incorrect fanout final value' ' corrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 "\01" \ - "oid table and fanout disagree on size" + "OID lookup chunk is the wrong size" ' test_expect_success 'detect incorrect OID order' ' @@ -842,7 +842,7 @@ test_expect_success 'reader notices too-small oid fanout chunk' ' check_corrupt_chunk OIDF clear $(printf "000000%02x" $(test_seq 250)) && cat >expect.err <<-\EOF && error: commit-graph oid fanout chunk is wrong size - error: commit-graph is missing the OID Fanout chunk + error: commit-graph required OID fanout chunk missing or corrupted EOF test_cmp expect.err err ' @@ -850,7 +850,8 @@ test_expect_success 'reader notices too-small oid fanout chunk' ' test_expect_success 'reader notices fanout/lookup table mismatch' ' check_corrupt_chunk OIDF 1020 "FFFFFFFF" && cat >expect.err <<-\EOF && - error: commit-graph oid table and fanout disagree on size + error: commit-graph OID lookup chunk is the wrong size + error: commit-graph required OID lookup chunk missing or corrupted EOF test_cmp expect.err err ' @@ -866,6 +867,7 @@ test_expect_success 'reader notices out-of-bounds fanout' ' check_corrupt_chunk OIDF 0 $(printf "%02x000000" $(test_seq 0 254)) && cat >expect.err <<-\EOF && error: commit-graph fanout values out of order + error: commit-graph required OID fanout chunk missing or corrupted EOF test_cmp expect.err err ' @@ -874,7 +876,7 @@ test_expect_success 'reader notices too-small commit data chunk' ' check_corrupt_chunk CDAT clear 00000000 && cat >expect.err <<-\EOF && error: commit-graph commit data chunk is wrong size - error: commit-graph is missing the Commit Data chunk + error: commit-graph required commit data chunk missing or corrupted EOF test_cmp expect.err err ' diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index c4c6060cee..c20aafe99a 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -1157,4 +1157,18 @@ test_expect_success 'reader notices too-small revindex chunk' ' test_cmp expect.err err ' +test_expect_success 'reader notices out-of-bounds fanout' ' + # This is similar to the out-of-bounds fanout test in t5318. The values + # in adjacent entries should be large but not identical (they + # are used as hi/lo starts for a binary search, which would then abort + # immediately). + corrupt_chunk OIDF 0 $(printf "%02x000000" $(test_seq 0 254)) && + test_must_fail git log 2>err && + cat >expect <<-\EOF && + error: oid fanout out of order: fanout[254] = fe000000 > 5c = fanout[255] + fatal: multi-pack-index required OID fanout chunk missing or corrupted + EOF + test_cmp expect err +' + test_done