From cb9daf16db0e9935179fd995785e94c996267268 Mon Sep 17 00:00:00 2001 From: SZEDER Gábor Date: Fri, 5 Jun 2020 13:00:24 +0000 Subject: commit-graph: fix parsing the Chunk Lookup table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The commit-graph file format specifies that the chunks may be in any order. However, if the OID Lookup chunk happens to be the last one in the file, then any command attempting to access the commit-graph data will fail with: fatal: invalid commit position. commit-graph is likely corrupt In this case the error is wrong, the commit-graph file does conform to the specification, but the parsing of the Chunk Lookup table is a bit buggy, and leaves the field holding the number of commits in the commit-graph zero-initialized. The number of commits in the commit-graph is determined while parsing the Chunk Lookup table, by dividing the size of the OID Lookup chunk with the hash size. However, the Chunk Lookup table doesn't actually store the size of the chunks, but it stores their starting offset. Consequently, the size of a chunk can only be calculated by subtracting the starting offsets of that chunk from the offset of the subsequent chunk, or in case of the last chunk from the offset recorded in the terminating label. This is currenly implemented in a bit complicated way: as we iterate over the entries of the Chunk Lookup table, we check the ID of each chunk and store its starting offset, then we check the ID of the last seen chunk and calculate its size using its previously saved offset if necessary (at the moment it's only necessary for the OID Lookup chunk). Alas, while parsing the Chunk Lookup table we only interate through the "real" chunks, but never look at the terminating label, thus don't even check whether it's necessary to calulate the size of the last chunk. Consequently, if the OID Lookup chunk is the last one, then we don't calculate its size and turn don't run the piece of code determining the number of commits in the commit graph, leaving the field holding that number unchanged (i.e. zero-initialized), eventually triggering the sanity check in load_oid_from_graph(). Fix this by iterating through all entries in the Chunk Lookup table, including the terminating label. Note that this is the minimal fix, suitable for the maintenance track. A better fix would be to simplify how the chunk sizes are calculated, but that is a more invasive change, less suitable for 'maint', so that will be done in later patches. This additional flexibility of scanning more chunks breaks a test for "git commit-graph verify" so alter that test to mutate the commit-graph to have an even lower chunk count. Signed-off-by: SZEDER Gábor Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- t/t5318-commit-graph.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 't/t5318-commit-graph.sh') diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 18304a65e4..79e7fbcd40 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -488,7 +488,7 @@ test_expect_success 'detect bad hash version' ' ' test_expect_success 'detect low chunk count' ' - corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\02" \ + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\01" \ "missing the .* chunk" ' -- cgit v1.3-5-g9baa From 2ad4f1a7c43cac4ba52cff56f9090d3322c67224 Mon Sep 17 00:00:00 2001 From: SZEDER Gábor Date: Fri, 5 Jun 2020 13:00:29 +0000 Subject: commit-graph: simplify parse_commit_graph() #1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While we iterate over all entries of the Chunk Lookup table we make sure that we don't attempt to read past the end of the mmap-ed commit-graph file, and check in each iteration that the chunk ID and offset we are about to read is still within the mmap-ed memory region. However, these checks in each iteration are not really necessary, because the number of chunks in the commit-graph file is already known before this loop from the just parsed commit-graph header. So let's check that the commit-graph file is large enough for all entries in the Chunk Lookup table before we start iterating over those entries, and drop those per-iteration checks. While at it, take into account the size of everything that is necessary to have a valid commit-graph file, i.e. the size of the header, the size of the mandatory OID Fanout chunk, and the size of the signature in the trailer as well. Note that this necessitates the change of the error message as well, and, consequently, have to update the 'detect incorrect chunk count' test in 't5318-commit-graph.sh' as well. Signed-off-by: SZEDER Gábor Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- commit-graph.c | 16 +++++++++------- t/t5318-commit-graph.sh | 3 ++- 2 files changed, 11 insertions(+), 8 deletions(-) (limited to 't/t5318-commit-graph.sh') diff --git a/commit-graph.c b/commit-graph.c index 6ed649388d..9927762f18 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -272,6 +272,15 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, graph->data = graph_map; graph->data_len = graph_size; + if (graph_size < GRAPH_HEADER_SIZE + + (graph->num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH + + GRAPH_FANOUT_SIZE + the_hash_algo->rawsz) { + error(_("commit-graph file is too small to hold %u chunks"), + graph->num_chunks); + free(graph); + return NULL; + } + last_chunk_id = 0; last_chunk_offset = 8; chunk_lookup = data + 8; @@ -280,13 +289,6 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd, uint64_t chunk_offset; int chunk_repeated = 0; - if (data + graph_size - chunk_lookup < - GRAPH_CHUNKLOOKUP_WIDTH) { - error(_("commit-graph chunk lookup table entry missing; file may be incomplete")); - free(graph); - return NULL; - } - chunk_id = get_be32(chunk_lookup + 0); chunk_offset = get_be64(chunk_lookup + 4); diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 79e7fbcd40..1073f9e3cf 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -574,7 +574,8 @@ test_expect_success 'detect invalid checksum hash' ' test_expect_success 'detect incorrect chunk count' ' corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \ - "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET + "commit-graph file is too small to hold [0-9]* chunks" \ + $GRAPH_CHUNK_LOOKUP_OFFSET ' test_expect_success 'git fsck (checks commit-graph)' ' -- cgit v1.3-5-g9baa