git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 6/6] http-fetch: Use temporary files for pack-*.idx until verified

From
Shawn O. Pearce <spearce@spearce.org>
Date
Apr 15, 2010, 19:09 UTC
Message-ID
<1271358560-8946-7-git-send-email-spearce@spearce.org>
In-Reply-To
<20100415141504.GB17883@spearce.org>

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.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 cache.h               |    2 +-
 http.c                |   62 +++++++++++++++++++++++++++++++-----------------
 pack-check.c          |   15 ++++++++---
 pack.h                |    1 +
 sha1_file.c           |    6 +++-
 t/t5550-http-fetch.sh |   15 ++++++++++++
 6 files changed, 72 insertions(+), 29 deletions(-)
diff --git a/cache.h b/cache.h
index 4150603..0d101e4 100644
--- a/cache.h
+++ b/cache.h
@@ -905,7 +905,7 @@ struct extra_have_objects {
 extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
 
-extern struct packed_git *parse_pack_index(unsigned char *sha1);
+extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
diff --git a/http.c b/http.c
index aa3e380..2d88034 100644
--- a/http.c
+++ b/http.c
@@ -897,47 +897,65 @@ 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 *hex = xstrdup(sha1_to_hex(sha1));
-	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", hex);
+		fprintf(stderr, "Getting index for pack %s\n", sha1_to_hex(sha1));
 
 	end_url_with_slash(&buf, base_url);
-	strbuf_addf(&buf, "objects/pack/pack-%s.idx", hex);
+	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(hex);
 	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;
 
-	if (fetch_pack_index(sha1, base_url))
-		return -1;
+	if (!has_pack_index(sha1)) {
+		tmp_idx = fetch_pack_index(sha1, base_url);
+		if (!tmp_idx)
+			return -1;
+	}
 
-	new_pack = parse_pack_index(sha1);
-	if (!new_pack)
+	new_pack = parse_pack_index(sha1, tmp_idx);
+	if (!new_pack) {
+		if (tmp_idx) {
+			unlink(tmp_idx);
+			free(tmp_idx);
+		}
 		return -1; /* parse_pack_index() already issued error message */
+	}
+
+	if (tmp_idx) {
+		int ret;
+
+		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;
+	}
+
 	new_pack->next = *packs_head;
 	*packs_head = new_pack;
 	return 0;
diff --git a/pack-check.c b/pack-check.c
index 166ca70..9baba12 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,
 	return err;
 }
 
-int verify_pack(struct packed_git *p)
+int verify_pack_index(struct packed_git *p)
 {
 	off_t index_size;
 	const unsigned char *index_base;
 	git_SHA_CTX ctx;
 	unsigned char sha1[20];
 	int err = 0;
-	struct pack_window *w_curs = NULL;
 
 	if (open_pack_index(p))
 		return error("packfile %s index not opened", p->pack_name);
@@ -154,9 +153,17 @@ int verify_pack(struct packed_git *p)
 	if (hashcmp(sha1, index_base + index_size - 20))
 		err = error("Packfile index for %s SHA1 mismatch",
 			    p->pack_name);
+	return err;
+}
+
+int verify_pack(struct packed_git *p)
+{
+	int err = 0;
+	struct pack_window *w_curs = NULL;
 
-	/* Verify pack file */
-	err |= verify_packfile(p, &w_curs);
+	err |= verify_pack_index(p);
+	if (!err)
+		err |= verify_packfile(p, &w_curs);
 	unuse_pack(&w_curs);
 
 	return err;
diff --git a/pack.h b/pack.h
index d268c01..bb27576 100644
--- a/pack.h
+++ b/pack.h
@@ -57,6 +57,7 @@ struct pack_idx_entry {
 
 extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);
 extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
+extern int verify_pack_index(struct packed_git *);
 extern int verify_pack(struct packed_git *);
 extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
 extern char *index_pack_lockfile(int fd);
diff --git a/sha1_file.c b/sha1_file.c
index 820063e..232e14d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -838,12 +838,14 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)
 	return p;
 }
 
-struct packed_git *parse_pack_index(unsigned char *sha1)
+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)
 {
-	const char *idx_path = sha1_pack_index_name(sha1);
 	const char *path = sha1_pack_name(sha1);
 	struct packed_git *p = alloc_packed_git(strlen(path) + 1);
 
+	if (!idx_path)
+		idx_path = sha1_pack_index_name(sha1);
+
 	strcpy(p->pack_name, path);
 	hashcpy(p->sha1, sha1);
 	if (check_packed_git_idx(idx_path, p)) {
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index bdac8d7..ee170d3 100755
--- 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 &&
+	 dd if=/dev/zero of=$p bs=256 count=1 seek=1
+	) &&
+	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

-- 
1.7.1.rc1.269.ga27c7
Previous: Shawn O. PearceNext: Ilari Liusvaara
Message 45 of 46 in “git fetch over http:// left my repo broken”
  1. Christian HalstrickApr 15, 2010
  2. Michael J GruberApr 15, 2010
  3. Ilari LiusvaaraApr 15, 2010
  4. Shawn O. PearceApr 15, 2010
  5. 0/6 detect dumb HTTP pack file corruptionShawn O. Pearce, Apr 15, 2010
  6. Junio C HamanoApr 17, 2010
  7. Shawn O. PearceApr 17, 2010
  8. 1/6 http.c: Remove bad free of static blockShawn O. Pearce, Apr 15, 2010
  9. 2/6 t5550-http-fetch: Use subshell for repository operationsShawn O. Pearce, Apr 15, 2010
  10. 3/6 http.c: Tiny refactoring of finish_http_pack_requestShawn O. Pearce, Apr 15, 2010
  11. 4/6 http.c: Drop useless != NULL test in finish_http_pack_requestShawn O. Pearce, Apr 15, 2010
  12. 5/6 http-fetch: Use index-pack rather than verify-pack to check packsShawn O. Pearce, Apr 15, 2010
  13. Johannes SixtApr 15, 2010
  14. 5/6 http-fetch: Use index-pack rather than verify-pack to check packsShawn O. Pearce, Apr 15, 2010
  15. Tay Ray ChuanApr 16, 2010
  16. Shawn O. PearceApr 17, 2010
  17. 6/6 http-fetch: Use temporary files for pack-*.idx until verifiedShawn O. Pearce, Apr 15, 2010
  18. Tay Ray ChuanApr 16, 2010
  19. 01/11 http.c: Remove bad free of static blockShawn O. Pearce, Apr 17, 2010
  20. 02/11 t5550-http-fetch: Use subshell for repository operationsShawn O. Pearce, Apr 17, 2010
  21. 03/11 http.c: Tiny refactoring of finish_http_pack_requestShawn O. Pearce, Apr 17, 2010
  22. 04/11 http.c: Drop useless != NULL test in finish_http_pack_requestShawn O. Pearce, Apr 17, 2010
  23. 05/11 http.c: Don't store destination name in request structuresShawn O. Pearce, Apr 17, 2010
  24. Tay Ray ChuanApr 18, 2010
  25. 06/11 http.c: Remove unnecessary strdup of sha1_to_hex resultShawn O. Pearce, Apr 17, 2010
  26. Tay Ray ChuanApr 18, 2010
  27. 07/11 Introduce close_pack_index to permit replacementShawn O. Pearce, Apr 17, 2010
  28. 08/11 Extract verify_pack_index for reuse from verify_packShawn O. Pearce, Apr 17, 2010
  29. 09/11 Allow parse_pack_index on temporary filesShawn O. Pearce, Apr 17, 2010
  30. 10/11 http-fetch: Use index-pack rather than verify-pack to check packsShawn O. Pearce, Apr 17, 2010
  31. Tay Ray ChuanApr 18, 2010
  32. 11/11 http-fetch: Use temporary files for pack-*.idx until verifiedShawn O. Pearce, Apr 17, 2010
  33. Tay Ray ChuanApr 18, 2010
  34. 00/11 Resend sp/maint-dumb-http-pack-reidxShawn O. Pearce, Apr 19, 2010
  35. Tay Ray ChuanApr 19, 2010
  36. Shawn O. PearceApr 19, 2010
  37. Tay Ray ChuanApr 20, 2010
  38. 06/11 http.c: Remove unnecessary strdup of sha1_to_hex resultShawn O. Pearce, Apr 19, 2010
  39. 07/11 Introduce close_pack_index to permit replacementShawn O. Pearce, Apr 19, 2010
  40. 08/11 Extract verify_pack_index for reuse from verify_packShawn O. Pearce, Apr 19, 2010
  41. 09/11 Allow parse_pack_index on temporary filesShawn O. Pearce, Apr 19, 2010
  42. 10/11 http-fetch: Use index-pack rather than verify-pack to check packsShawn O. Pearce, Apr 19, 2010
  43. Tay Ray ChuanApr 19, 2010
  44. 11/11 http-fetch: Use temporary files for pack-*.idx until verifiedShawn O. Pearce, Apr 19, 2010
  45. 6/6 http-fetch: Use temporary files for pack-*.idx until verifiedShawn O. Pearce, Apr 15, 2010
  46. Ilari LiusvaaraApr 15, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.