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>

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 2ab78129bde8fbae5bce2df0bac49f23d339a18d..2414190c049b42efb7676eb50eefb52712d8ebb7 100644 (file)
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -872,8 +872,14 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
        }
 
        for (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {
-               if (!load_pack_revindex(the_repository, p) &&
-                   verify_pack_revindex(p)) {
+               int load_error = load_pack_revindex_from_disk(p);
+
+               if (load_error < 0) {
+                       error(_("unable to load rev-index for pack '%s'"), p->pack_name);
+                       res = ERROR_PACK_REV_INDEX;
+               } else if (!load_error &&
+                          !load_pack_revindex(the_repository, p) &&
+                          verify_pack_revindex(p)) {
                        error(_("invalid rev-index for pack '%s'"), p->pack_name);
                        res = ERROR_PACK_REV_INDEX;
                }

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 38b35c48237c9976bac816d49eb290741d2dcdb8..3828aab612a00bc45639ff3e841a8edda2b9a156 100644 (file)
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -379,7 +379,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
                goto cleanup;
        }
 
-       if (load_midx_revindex(bitmap_git->midx) < 0) {
+       if (load_midx_revindex(bitmap_git->midx)) {
                warning(_("multi-pack bitmap is missing required reverse index"));
                goto cleanup;
        }
@@ -2140,7 +2140,7 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,
 
        if (!bitmap_is_midx(bitmap_git))
                load_reverse_index(r, bitmap_git);
-       else if (load_midx_revindex(bitmap_git->midx) < 0)
+       else if (load_midx_revindex(bitmap_git->midx))
                BUG("rebuild_existing_bitmaps: missing required rev-cache "
                    "extension");
 

diff --git a/pack-revindex.c b/pack-revindex.c
index 62a9846470c792aa6bdbfcbfd0d3beefa1e7d40b..146334e2c96df2e60d164b1f126295ca18d34e93 100644 (file)
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -212,7 +212,8 @@ static int load_revindex_from_disk(char *revindex_name,
        fd = git_open(revindex_name);
 
        if (fd < 0) {
-               ret = -1;
+               /* "No file" means return 1. */
+               ret = 1;
                goto cleanup;
        }
        if (fstat(fd, &st)) {
@@ -264,7 +265,7 @@ cleanup:
        return ret;
 }
 
-static int load_pack_revindex_from_disk(struct packed_git *p)
+int load_pack_revindex_from_disk(struct packed_git *p)
 {
        char *revindex_name;
        int ret;

diff --git a/pack-revindex.h b/pack-revindex.h
index c8861873b02c1d2d2144977e8f9ff69dda6477a3..6dd47efea10ec69a3438f0a804a71bf13cd91c37 100644 (file)
--- a/pack-revindex.h
+++ b/pack-revindex.h
@@ -51,6 +51,14 @@ struct repository;
  */
 int load_pack_revindex(struct repository *r, struct packed_git *p);
 
+/*
+ * Specifically load a pack revindex from disk.
+ *
+ * Returns 0 on success, 1 on "no .rev file", and -1 when there is an
+ * error parsing the .rev file.
+ */
+int load_pack_revindex_from_disk(struct packed_git *p);
+
 /*
  * verify_pack_revindex verifies that the on-disk rev-index for the given
  * pack-file is the same that would be created if written from scratch.

diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh
index 5c3c80f88f07d979750fac69f51ae4fe83180560..431a603ca0e61d7085f28333c6a28c120ea45828 100755 (executable)
--- a/t/t5325-reverse-index.sh
+++ b/t/t5325-reverse-index.sh
@@ -190,4 +190,19 @@ test_expect_success 'fsck catches invalid row position' '
                "invalid rev-index position"
 '
 
+test_expect_success 'fsck catches invalid header: magic number' '
+       corrupt_rev_and_verify 1 "\07" \
+               "reverse-index file .* has unknown signature"
+'
+
+test_expect_success 'fsck catches invalid header: version' '
+       corrupt_rev_and_verify 7 "\02" \
+               "reverse-index file .* has unsupported version"
+'
+
+test_expect_success 'fsck catches invalid header: hash function' '
+       corrupt_rev_and_verify 11 "\03" \
+               "reverse-index file .* has unsupported hash id"
+'
+
 test_done