From ec1e28ef9c30468d2e76e41c88a1611e63047f61 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 23 Jun 2021 14:39:12 -0400 Subject: midx: don't reuse corrupt MIDXs when writing When writing a new multi-pack index, Git tries to reuse as much of the data from an existing MIDX as possible, like object offsets. This is done to avoid re-opening a bunch of *.idx files unnecessarily, but can lead to problems if the data we are reusing is corrupt. That's because we'll blindly reuse data from an existing MIDX without checking its trailing checksum for validity. So if there is memory corruption while writing a MIDX, or disk corruption in the intervening period between writing and reuse, we'll blindly propagate those bad values forward. Suppose we experience a memory corruption while writing a MIDX such that we write an incorrect object offset (or alternatively, the disk corrupts the data after being written, but before being reused). Then when we go to write a new MIDX, we'll reuse the bad object offset without checking its validity. This means that the MIDX we just wrote is broken, but its trailing checksum is in-tact, since we never bothered to look at the values before writing. In the above, a "git multi-pack-index verify" would have caught the problem before writing, but writing a new MIDX wouldn't have noticed anything wrong, blindly carrying forward the corrupt offset. Individual pack indexes check their validity by verifying the crc32 attached to each entry when carrying data forward during a repack. We could solve this problem for MIDXs in the same way, but individual crc32's don't make much sense, since their entries are so small. Likewise, checking the whole file on every read may be prohibitively expensive if a repository has a lot of objects, packs, or both. But we can check the trailing checksum when reusing an existing MIDX when writing a new one. And a corrupt MIDX need not stop us from writing a new one, since we can just avoid reusing the existing one at all and pretend as if we are writing a new MIDX from scratch. Suggested-by: Derrick Stolee Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- t/t5319-multi-pack-index.sh | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 't/t5319-multi-pack-index.sh') diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 5641d158df..d582f370c4 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -410,6 +410,14 @@ test_expect_success 'git-fsck incorrect offset' ' "git -c core.multipackindex=true fsck" ' +test_expect_success 'corrupt MIDX is not reused' ' + corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \ + "incorrect object offset" && + git multi-pack-index write 2>err && + test_i18ngrep checksum.mismatch err && + git multi-pack-index verify +' + test_expect_success 'repack progress off for redirected stderr' ' GIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack 2>err && test_line_count = 0 err -- cgit v1.2.3