fsck: validate .rev file header

While parsing a .rev file, we check the header information to be sure it makes sense. This happens before doing any additional validation such as a checksum or value check. In order to differentiate between a bad header and a non-existent file, we need to update the API for loading a reverse index. Make load_pack_revindex_from_disk() non-static and specify that a positive value means "the file does not exist" while other errors during parsing are negative values. Since an invalid header prevents setting up the structures we would use for further validations, we can stop at that point. The place where we can distinguish between a missing file and a corrupt file is inside load_revindex_from_disk(), which is used both by pack rev-indexes and multi-pack-index rev-indexes. Some tests in t5326 demonstrate that it is critical to take some conditions to allow positive error signals. Add tests that check the three header values. Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Derrick Stolee committed Apr 17, 2023 at 16:21 UTC 5a6072f631dcf4d9f65e83b08d14c82e2af45dd8
5 files changed +36 -6
builtin/fsck.c
+8 -2
@@ -872,8 +872,14 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
872 }
873
874 for (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {
875 - if (!load_pack_revindex(the_repository, p) &&
876 - verify_pack_revindex(p)) {
875 + int load_error = load_pack_revindex_from_disk(p);
876 +
877 + if (load_error < 0) {
878 + error(_("unable to load rev-index for pack '%s'"), p->pack_name);
879 + res = ERROR_PACK_REV_INDEX;
880 + } else if (!load_error &&
881 + !load_pack_revindex(the_repository, p) &&
882 + verify_pack_revindex(p)) {
883 error(_("invalid rev-index for pack '%s'"), p->pack_name);
884 res = ERROR_PACK_REV_INDEX;
885 }
pack-bitmap.c
+2 -2
@@ -379,7 +379,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
379 goto cleanup;
380 }
381
382 - if (load_midx_revindex(bitmap_git->midx) < 0) {
382 + if (load_midx_revindex(bitmap_git->midx)) {
383 warning(_("multi-pack bitmap is missing required reverse index"));
384 goto cleanup;
385 }
@@ -2140,7 +2140,7 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
2140
2141 if (!bitmap_is_midx(bitmap_git))
2142 load_reverse_index(r, bitmap_git);
2143 - else if (load_midx_revindex(bitmap_git->midx) < 0)
2143 + else if (load_midx_revindex(bitmap_git->midx))
2144 BUG("rebuild_existing_bitmaps: missing required rev-cache "
2145 "extension");
2146
pack-revindex.c
+3 -2
@@ -212,7 +212,8 @@ static int load_revindex_from_disk(char *revindex_name,
212 fd = git_open(revindex_name);
213
214 if (fd < 0) {
215 - ret = -1;
215 + /* "No file" means return 1. */
216 + ret = 1;
217 goto cleanup;
218 }
219 if (fstat(fd, &st)) {
@@ -264,7 +265,7 @@ cleanup:
265 return ret;
266 }
267
267 -static int load_pack_revindex_from_disk(struct packed_git *p)
268 +int load_pack_revindex_from_disk(struct packed_git *p)
269 {
270 char *revindex_name;
271 int ret;
pack-revindex.h
+8
@@ -51,6 +51,14 @@ struct repository;
51 */
52 int load_pack_revindex(struct repository *r, struct packed_git *p);
53
54 +/*
55 + * Specifically load a pack revindex from disk.
56 + *
57 + * Returns 0 on success, 1 on "no .rev file", and -1 when there is an
58 + * error parsing the .rev file.
59 + */
60 +int load_pack_revindex_from_disk(struct packed_git *p);
61 +
62 /*
63 * verify_pack_revindex verifies that the on-disk rev-index for the given
64 * pack-file is the same that would be created if written from scratch.
t/t5325-reverse-index.sh
+15
@@ -190,4 +190,19 @@ test_expect_success 'fsck catches invalid row position' '
190 "invalid rev-index position"
191 '
192
193 +test_expect_success 'fsck catches invalid header: magic number' '
194 + corrupt_rev_and_verify 1 "\07" \
195 + "reverse-index file .* has unknown signature"
196 +'
197 +
198 +test_expect_success 'fsck catches invalid header: version' '
199 + corrupt_rev_and_verify 7 "\02" \
200 + "reverse-index file .* has unsupported version"
201 +'
202 +
203 +test_expect_success 'fsck catches invalid header: hash function' '
204 + corrupt_rev_and_verify 11 "\03" \
205 + "reverse-index file .* has unsupported hash id"
206 +'
207 +
208 test_done