http-fetch: Use temporary files for pack-*.idx until verified

Verify that a downloaded pack-*.idx file is consistent and valid
as an index file before we rename it into its final destination.
This prevents a corrupt index file from later being treated as a
usable file, confusing readers.

Check that we do not have the pack index file before invoking
fetch_pack_index(); that way, we can do without the has_pack_index()
check in fetch_pack_index().

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/http.c b/http.c
index 2ebd679393223aa186b2b6cc5a0bfe8274d496cf..0813c9ecd8553e5bbae8ebc504401afc11e2bc27 100644 (file)
--- a/http.c
+++ b/http.c
@@ -897,18 +897,11 @@ int http_fetch_ref(const char *base, struct ref *ref)
 }
 
 /* Helpers for fetching packs */
-static int fetch_pack_index(unsigned char *sha1, const char *base_url)
+static char *fetch_pack_index(unsigned char *sha1, const char *base_url)
 {
-       int ret = 0;
-       char *filename;
-       char *url = NULL;
+       char *url, *tmp;
        struct strbuf buf = STRBUF_INIT;
 
-       if (has_pack_index(sha1)) {
-               ret = 0;
-               goto cleanup;
-       }
-
        if (http_is_verbose)
                fprintf(stderr, "Getting index for pack %s\n", sha1_to_hex(sha1));
 
@@ -916,26 +909,55 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
        strbuf_addf(&buf, "objects/pack/pack-%s.idx", sha1_to_hex(sha1));
        url = strbuf_detach(&buf, NULL);
 
-       filename = sha1_pack_index_name(sha1);
-       if (http_get_file(url, filename, 0) != HTTP_OK)
-               ret = error("Unable to get pack index %s\n", url);
+       strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(sha1));
+       tmp = strbuf_detach(&buf, NULL);
+
+       if (http_get_file(url, tmp, 0) != HTTP_OK) {
+               error("Unable to get pack index %s\n", url);
+               free(tmp);
+               tmp = NULL;
+       }
 
-cleanup:
        free(url);
-       return ret;
+       return tmp;
 }
 
 static int fetch_and_setup_pack_index(struct packed_git **packs_head,
        unsigned char *sha1, const char *base_url)
 {
        struct packed_git *new_pack;
+       char *tmp_idx = NULL;
+       int ret;
+
+       if (has_pack_index(sha1)) {
+               new_pack = parse_pack_index(sha1, NULL);
+               if (!new_pack)
+                       return -1; /* parse_pack_index() already issued error message */
+               goto add_pack;
+       }
 
-       if (fetch_pack_index(sha1, base_url))
+       tmp_idx = fetch_pack_index(sha1, base_url);
+       if (!tmp_idx)
                return -1;
 
-       new_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));
-       if (!new_pack)
+       new_pack = parse_pack_index(sha1, tmp_idx);
+       if (!new_pack) {
+               unlink(tmp_idx);
+               free(tmp_idx);
+
                return -1; /* parse_pack_index() already issued error message */
+       }
+
+       ret = verify_pack_index(new_pack);
+       if (!ret) {
+               close_pack_index(new_pack);
+               ret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));
+       }
+       free(tmp_idx);
+       if (ret)
+               return -1;
+
+add_pack:
        new_pack->next = *packs_head;
        *packs_head = new_pack;
        return 0;

diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 1a4dfc9695af92cd5774471977b25afdba8627ec..fc675b50adfa945724cd7d17dbb6034df53426d3 100755 (executable)
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -77,6 +77,21 @@ test_expect_success 'fetch notices corrupt pack' '
        )
 '
 
+test_expect_success 'fetch notices corrupt idx' '
+       cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+       (cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+        p=`ls objects/pack/pack-*.idx` &&
+        chmod u+w $p &&
+        printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+       ) &&
+       mkdir repo_bad2.git &&
+       (cd repo_bad2.git &&
+        git --bare init &&
+        test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&
+        test 0 = `ls objects/pack | wc -l`
+       )
+'
+
 test_expect_success 'did not use upload-pack service' '
        grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act
        : >exp