midx: stop ignoring malformed oid fanout chunk

When we load the oid-fanout chunk, our callback checks that its size is reasonable and returns an error if not. However, the caller only checks our return value against CHUNK_NOT_FOUND, so we end up ignoring the error completely! Using a too-small fanout table means we end up accessing random memory for the fanout and segfault. We can fix this by checking for any non-zero return value, rather than just CHUNK_NOT_FOUND, and adjusting our error message to cover both cases. We could handle each error code individually, but there's not much point for such a rare case. The extra message produced in the callback makes it clear what is going on. The same pattern is used in the adjacent code. Those cases are actually OK for now because they do not use a custom callback, so the only error they can get is CHUNK_NOT_FOUND. But let's convert them, as this is an accident waiting to happen (especially as we convert some of them away from pair_chunk). The error messages are more verbose, but it should be rare for a user to see these anyway. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 9, 2023 at 16:59 UTC e3c9600397bde3dcd2f628ff1eec098f79f79b67
2 files changed +27 -9
midx.c
+8 -8
@@ -143,14 +143,14 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
143 MIDX_HEADER_SIZE, m->num_chunks))
144 goto cleanup_fail;
145
146 - if (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names) == CHUNK_NOT_FOUND)
147 - die(_("multi-pack-index missing required pack-name chunk"));
148 - if (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m) == CHUNK_NOT_FOUND)
149 - die(_("multi-pack-index missing required OID fanout chunk"));
150 - if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup) == CHUNK_NOT_FOUND)
151 - die(_("multi-pack-index missing required OID lookup chunk"));
152 - if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets) == CHUNK_NOT_FOUND)
153 - die(_("multi-pack-index missing required object offsets chunk"));
146 + if (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names))
147 + die(_("multi-pack-index required pack-name chunk missing or corrupted"));
148 + if (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))
149 + die(_("multi-pack-index required OID fanout chunk missing or corrupted"));
150 + if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup))
151 + die(_("multi-pack-index required OID lookup chunk missing or corrupted"));
152 + if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets))
153 + die(_("multi-pack-index required object offsets chunk missing or corrupted"));
154
155 pair_chunk_unsafe(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);
156
t/t5319-multi-pack-index.sh
+19 -1
@@ -2,6 +2,7 @@
2
3 test_description='multi-pack-indexes'
4 . ./test-lib.sh
5 +. "$TEST_DIRECTORY"/lib-chunk.sh
6
7 GIT_TEST_MULTI_PACK_INDEX=0
8 objdir=.git/objects
@@ -438,7 +439,7 @@ test_expect_success 'verify extended chunk count' '
439
440 test_expect_success 'verify missing required chunk' '
441 corrupt_midx_and_verify $MIDX_BYTE_CHUNK_ID "\01" $objdir \
441 - "missing required"
442 + "required pack-name chunk missing"
443 '
444
445 test_expect_success 'verify invalid chunk offset' '
@@ -1055,4 +1056,21 @@ test_expect_success 'repack with delta islands' '
1056 )
1057 '
1058
1059 +corrupt_chunk () {
1060 + midx=.git/objects/pack/multi-pack-index &&
1061 + test_when_finished "rm -rf $midx" &&
1062 + git repack -ad --write-midx &&
1063 + corrupt_chunk_file $midx "$@"
1064 +}
1065 +
1066 +test_expect_success 'reader notices too-small oid fanout chunk' '
1067 + corrupt_chunk OIDF clear 00000000 &&
1068 + test_must_fail git log 2>err &&
1069 + cat >expect <<-\EOF &&
1070 + error: multi-pack-index OID fanout is of the wrong size
1071 + fatal: multi-pack-index required OID fanout chunk missing or corrupted
1072 + EOF
1073 + test_cmp expect err
1074 +'
1075 +
1076 test_done