From 03b6aeb27443f117a3d8375f01bc38baeeff65a5 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Sat, 17 Apr 2010 13:07:34 -0700 Subject: [PATCH 01/12] http.c: Remove bad free of static block The filename variable here is pointing to a block of memory that was allocated by sha1_file.c and is also held in a static variable scoped within the sha1_pack_name() function. Doing a free() here is returning that memory to the allocator while we might still try to reuse it on a subsequent sha1_pack_name() invocation. That's not acceptable, so don't free it. Signed-off-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- http.c | 1 - 1 file changed, 1 deletion(-) diff --git a/http.c b/http.c index deab59551d..9526491a45 100644 --- a/http.c +++ b/http.c @@ -1082,7 +1082,6 @@ struct http_pack_request *new_http_pack_request( return preq; abort: - free(filename); free(preq->url); free(preq); return NULL; From d761b2ac0e42416f7fb7752d6ac6cc02c26ea14f Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Sat, 17 Apr 2010 13:07:35 -0700 Subject: [PATCH 02/12] t5550-http-fetch: Use subshell for repository operations Change into the server repository's directory using a subshell, so we can return back to the top of the trash directory before doing anything more in the test script. Signed-off-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- t/t5550-http-fetch.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index 8cfce969bc..78c31c961e 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -55,9 +55,10 @@ test_expect_success 'http remote detects correct HEAD' ' test_expect_success 'fetch packed objects' ' cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git && - cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git && - git --bare repack && - git --bare prune-packed && + (cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git && + git --bare repack && + git --bare prune-packed + ) && git clone $HTTPD_URL/dumb/repo_pack.git ' From 021ab6f00b66d0d3931310e77383239a606c96c2 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Sat, 17 Apr 2010 13:07:36 -0700 Subject: [PATCH 03/12] http.c: Tiny refactoring of finish_http_pack_request Always remove the struct packed_git from the active list, even if the rename of the temporary file fails. While we are here, simplify the code a bit by using a common local variable name ("p") to hold the relevant packed_git. Signed-off-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- http.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/http.c b/http.c index 9526491a45..e9e2269566 100644 --- a/http.c +++ b/http.c @@ -1002,8 +1002,9 @@ int finish_http_pack_request(struct http_pack_request *preq) { int ret; struct packed_git **lst; + struct packed_git *p = preq->target; - preq->target->pack_size = ftell(preq->packfile); + p->pack_size = ftell(preq->packfile); if (preq->packfile != NULL) { fclose(preq->packfile); @@ -1011,18 +1012,17 @@ int finish_http_pack_request(struct http_pack_request *preq) preq->slot->local = NULL; } - ret = move_temp_to_file(preq->tmpfile, preq->filename); - if (ret) - return ret; - lst = preq->lst; - while (*lst != preq->target) + while (*lst != p) lst = &((*lst)->next); *lst = (*lst)->next; - if (verify_pack(preq->target)) + ret = move_temp_to_file(preq->tmpfile, preq->filename); + if (ret) + return ret; + if (verify_pack(p)) return -1; - install_packed_git(preq->target); + install_packed_git(p); return 0; } From 3065274c58a4f4d0c6eef7e29a1484cf2c288131 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Sat, 17 Apr 2010 13:07:37 -0700 Subject: [PATCH 04/12] http.c: Drop useless != NULL test in finish_http_pack_request The test preq->packfile != NULL is always true. If packfile was actually NULL when entering this function the ftell() above would crash out with a SIGSEGV, resulting in never reaching this point. Simplify the code by just removing the conditional. Signed-off-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- http.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/http.c b/http.c index e9e2269566..7942eea5d8 100644 --- a/http.c +++ b/http.c @@ -1005,12 +1005,9 @@ int finish_http_pack_request(struct http_pack_request *preq) struct packed_git *p = preq->target; p->pack_size = ftell(preq->packfile); - - if (preq->packfile != NULL) { - fclose(preq->packfile); - preq->packfile = NULL; - preq->slot->local = NULL; - } + fclose(preq->packfile); + preq->packfile = NULL; + preq->slot->local = NULL; lst = preq->lst; while (*lst != p) From 0da8b2e7c80a6dd9743e5233cdc5acd836c9a8d3 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Sat, 17 Apr 2010 13:07:38 -0700 Subject: [PATCH 05/12] http.c: Don't store destination name in request structures The destination name within the object store is easily computed on demand, reusing a static buffer held by sha1_file.c. We don't need to copy the entire path into the request structure for safe keeping, when it can be easily reformatted after the download has been completed. This reduces the size of the per-request structure, and removes yet another PATH_MAX based limit. Signed-off-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- http-walker.c | 2 +- http.c | 14 ++++++-------- http.h | 2 -- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/http-walker.c b/http-walker.c index 700bc13112..3a726dea08 100644 --- a/http-walker.c +++ b/http-walker.c @@ -510,7 +510,7 @@ static int fetch_object(struct walker *walker, struct alt_base *repo, unsigned c ret = error("File %s has bad hash", hex); } else if (req->rename < 0) { ret = error("unable to write sha1 filename %s", - req->filename); + sha1_file_name(req->sha1)); } release_http_object_request(req); diff --git a/http.c b/http.c index 7942eea5d8..7ee1ba5a00 100644 --- a/http.c +++ b/http.c @@ -1014,7 +1014,7 @@ int finish_http_pack_request(struct http_pack_request *preq) lst = &((*lst)->next); *lst = (*lst)->next; - ret = move_temp_to_file(preq->tmpfile, preq->filename); + ret = move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1)); if (ret) return ret; if (verify_pack(p)) @@ -1043,7 +1043,6 @@ struct http_pack_request *new_http_pack_request( preq->url = strbuf_detach(&buf, NULL); filename = sha1_pack_name(target->sha1); - snprintf(preq->filename, sizeof(preq->filename), "%s", filename); snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", filename); preq->packfile = fopen(preq->tmpfile, "a"); if (!preq->packfile) { @@ -1133,7 +1132,6 @@ struct http_object_request *new_http_object_request(const char *base_url, freq->localfile = -1; filename = sha1_file_name(sha1); - snprintf(freq->filename, sizeof(freq->filename), "%s", filename); snprintf(freq->tmpfile, sizeof(freq->tmpfile), "%s.temp", filename); @@ -1162,8 +1160,8 @@ struct http_object_request *new_http_object_request(const char *base_url, } if (freq->localfile < 0) { - error("Couldn't create temporary file %s for %s: %s", - freq->tmpfile, freq->filename, strerror(errno)); + error("Couldn't create temporary file %s: %s", + freq->tmpfile, strerror(errno)); goto abort; } @@ -1210,8 +1208,8 @@ struct http_object_request *new_http_object_request(const char *base_url, prev_posn = 0; lseek(freq->localfile, 0, SEEK_SET); if (ftruncate(freq->localfile, 0) < 0) { - error("Couldn't truncate temporary file %s for %s: %s", - freq->tmpfile, freq->filename, strerror(errno)); + error("Couldn't truncate temporary file %s: %s", + freq->tmpfile, strerror(errno)); goto abort; } } @@ -1287,7 +1285,7 @@ int finish_http_object_request(struct http_object_request *freq) return -1; } freq->rename = - move_temp_to_file(freq->tmpfile, freq->filename); + move_temp_to_file(freq->tmpfile, sha1_file_name(freq->sha1)); return freq->rename; } diff --git a/http.h b/http.h index 5c9441c10c..84bdbd0f76 100644 --- a/http.h +++ b/http.h @@ -152,7 +152,6 @@ struct http_pack_request struct packed_git *target; struct packed_git **lst; FILE *packfile; - char filename[PATH_MAX]; char tmpfile[PATH_MAX]; struct curl_slist *range_header; struct active_request_slot *slot; @@ -167,7 +166,6 @@ extern void release_http_pack_request(struct http_pack_request *preq); struct http_object_request { char *url; - char filename[PATH_MAX]; char tmpfile[PATH_MAX]; int localfile; CURLcode curl_result; From 162eb5f838630f75f78f26d28b46b02781724b7d Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 19 Apr 2010 07:23:05 -0700 Subject: [PATCH 06/12] http.c: Remove unnecessary strdup of sha1_to_hex result Most of the time the dumb HTTP transport is run without the verbose flag set, so we only need the result of sha1_to_hex(sha1) once, to construct the pack URL. Don't bother with an unnecessary malloc, copy, free chain of this buffer. If verbose is set, we'll format the SHA-1 twice now. But this tiny extra CPU time spent is nothing compared to the slowdown that is usually imposed by the verbose messages being sent to the tty, and is entirely trivial compared to the latency involved with the remote HTTP server sending something as big as a pack file. Signed-off-by: Shawn O. Pearce Acked-by: Tay Ray Chuan Signed-off-by: Junio C Hamano --- http.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/http.c b/http.c index 7ee1ba5a00..95e3b8bec8 100644 --- a/http.c +++ b/http.c @@ -899,7 +899,6 @@ int http_fetch_ref(const char *base, struct ref *ref) static int 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; struct strbuf buf = STRBUF_INIT; @@ -910,10 +909,10 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url) } 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); @@ -921,7 +920,6 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url) ret = error("Unable to get pack index %s\n", url); cleanup: - free(hex); free(url); return ret; } From fa5fc15d6ecfb9452c578bb4c80e98ccca12750c Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 19 Apr 2010 07:23:06 -0700 Subject: [PATCH 07/12] Introduce close_pack_index to permit replacement By closing the pack index, a caller can later overwrite the index with an updated index file, possibly after converting from v1 to the v2 format. Because p->index_data is NULL after close, on the next access the index will be opened again and the other members will be updated with new data. Signed-off-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- cache.h | 1 + sha1_file.c | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/cache.h b/cache.h index 6e54993256..0eba039dbf 100644 --- a/cache.h +++ b/cache.h @@ -911,6 +911,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1, extern void pack_report(void); extern int open_pack_index(struct packed_git *); +extern void close_pack_index(struct packed_git *); extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *); extern void close_pack_windows(struct packed_git *); extern void unuse_pack(struct pack_window **); diff --git a/sha1_file.c b/sha1_file.c index c23cc5e6e1..4e82654d7c 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -606,6 +606,14 @@ void unuse_pack(struct pack_window **w_cursor) } } +void close_pack_index(struct packed_git *p) +{ + if (p->index_data) { + munmap((void *)p->index_data, p->index_size); + p->index_data = NULL; + } +} + /* * This is used by git-repack in case a newly created pack happens to * contain the same set of objects as an existing one. In that case @@ -627,8 +635,7 @@ void free_pack_by_name(const char *pack_name) close_pack_windows(p); if (p->pack_fd != -1) close(p->pack_fd); - if (p->index_data) - munmap((void *)p->index_data, p->index_size); + close_pack_index(p); free(p->bad_object_sha1); *pp = p->next; free(p); From 9b0aa728705439ca4b4e7ec845f79f8487059320 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 19 Apr 2010 07:23:07 -0700 Subject: [PATCH 08/12] Extract verify_pack_index for reuse from verify_pack The dumb HTTP transport should verify an index is completely valid before trying to use it. That requires checking the header/footer but also checking the complete content SHA-1. All of this logic is already in the front half of verify_pack, so pull it out into a new function that can be reused. Signed-off-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- pack-check.c | 15 ++++++++++++--- pack.h | 1 + 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/pack-check.c b/pack-check.c index 166ca703c1..395fb9527a 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,8 +153,18 @@ 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; + + err |= verify_pack_index(p); + if (!p->index_data) + return -1; - /* Verify pack file */ err |= verify_packfile(p, &w_curs); unuse_pack(&w_curs); diff --git a/pack.h b/pack.h index b759a23d4d..880f9c2930 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); From 7b64469a363c10e9e875a73d32a220ba48694a28 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 19 Apr 2010 07:23:08 -0700 Subject: [PATCH 09/12] Allow parse_pack_index on temporary files The easiest way to verify a pack index is to open it through the standard parse_pack_index function, permitting the header check to happen when the file is mapped. However, the dumb HTTP client needs to verify a pack index before its moved into its proper file name within the objects/pack directory, to prevent a corrupt index from being made available. So permit the caller to specify the exact path of the index file. For now we're still using the final destination name within the sole call site in http.c, but eventually we will start to parse the temporary path instead. Signed-off-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- cache.h | 2 +- http.c | 2 +- sha1_file.c | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/cache.h b/cache.h index 0eba039dbf..7db23eff26 100644 --- a/cache.h +++ b/cache.h @@ -900,7 +900,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 95e3b8bec8..9c62632202 100644 --- a/http.c +++ b/http.c @@ -932,7 +932,7 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head, if (fetch_pack_index(sha1, base_url)) return -1; - new_pack = parse_pack_index(sha1); + new_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1)); if (!new_pack) return -1; /* parse_pack_index() already issued error message */ new_pack->next = *packs_head; diff --git a/sha1_file.c b/sha1_file.c index 4e82654d7c..9f3f514e71 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -845,9 +845,8 @@ 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); From fe72d420ab4dda593dddece7b907ee7868ced127 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 19 Apr 2010 07:23:09 -0700 Subject: [PATCH 10/12] http-fetch: Use index-pack rather than verify-pack to check packs To ensure we don't leave a corrupt pack file positioned as though it were a valid pack file, run index-pack on the temporary pack before we rename it to its final name. If index-pack crashes out when it discovers file corruption (e.g. GitHub's error HTML at the end of the file), simply delete the temporary files to cleanup. By waiting until the pack has been validated before we move it to its final name, we eliminate a race condition where another concurrent reader might try to access the pack at the same time that we are still trying to verify its not corrupt. Switching from verify-pack to index-pack is a change in behavior, but it should turn out better for users. The index-pack algorithm tries to minimize disk seeks, as well as the number of times any given object is inflated, by organizing its work along delta chains. The verify-pack logic does not attempt to do this, thrashing the delta base cache and the filesystem cache. By recreating the index file locally, we also can automatically upgrade from a v1 pack table of contents to v2. This makes the CRC32 data available for use during later repacks, even if the server didn't have them on hand. Signed-off-by: Shawn O. Pearce Acked-by: Tay Ray Chuan Signed-off-by: Junio C Hamano --- http.c | 46 +++++++++++++++++++++++++++++++++++-------- t/t5550-http-fetch.sh | 15 ++++++++++++++ 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/http.c b/http.c index 9c62632202..2ebd679393 100644 --- a/http.c +++ b/http.c @@ -1,6 +1,7 @@ #include "http.h" #include "pack.h" #include "sideband.h" +#include "run-command.h" int data_received; int active_requests; @@ -998,11 +999,14 @@ void release_http_pack_request(struct http_pack_request *preq) int finish_http_pack_request(struct http_pack_request *preq) { - int ret; struct packed_git **lst; struct packed_git *p = preq->target; + char *tmp_idx; + struct child_process ip; + const char *ip_argv[8]; + + close_pack_index(p); - p->pack_size = ftell(preq->packfile); fclose(preq->packfile); preq->packfile = NULL; preq->slot->local = NULL; @@ -1012,13 +1016,39 @@ int finish_http_pack_request(struct http_pack_request *preq) lst = &((*lst)->next); *lst = (*lst)->next; - ret = move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1)); - if (ret) - return ret; - if (verify_pack(p)) - return -1; - install_packed_git(p); + tmp_idx = xstrdup(preq->tmpfile); + strcpy(tmp_idx + strlen(tmp_idx) - strlen(".pack.temp"), + ".idx.temp"); + ip_argv[0] = "index-pack"; + ip_argv[1] = "-o"; + ip_argv[2] = tmp_idx; + ip_argv[3] = preq->tmpfile; + ip_argv[4] = NULL; + + memset(&ip, 0, sizeof(ip)); + ip.argv = ip_argv; + ip.git_cmd = 1; + ip.no_stdin = 1; + ip.no_stdout = 1; + + if (run_command(&ip)) { + unlink(preq->tmpfile); + unlink(tmp_idx); + free(tmp_idx); + return -1; + } + + unlink(sha1_pack_index_name(p->sha1)); + + if (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1)) + || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) { + free(tmp_idx); + return -1; + } + + install_packed_git(p); + free(tmp_idx); return 0; } diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index 78c31c961e..1a4dfc9695 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -62,6 +62,21 @@ test_expect_success 'fetch packed objects' ' git clone $HTTPD_URL/dumb/repo_pack.git ' +test_expect_success 'fetch notices corrupt pack' ' + cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git && + (cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git && + p=`ls objects/pack/pack-*.pack` && + chmod u+w $p && + printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc + ) && + mkdir repo_bad1.git && + (cd repo_bad1.git && + git --bare init && + test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad1.git && + test 0 = `ls objects/pack/pack-*.pack | wc -l` + ) +' + test_expect_success 'did not use upload-pack service' ' grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act : >exp From 750ef42516bb343a7755f003720e43cd8dd64c3e Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 19 Apr 2010 07:23:10 -0700 Subject: [PATCH 11/12] 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 Signed-off-by: Junio C Hamano --- http.c | 56 ++++++++++++++++++++++++++++++------------- t/t5550-http-fetch.sh | 15 ++++++++++++ 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/http.c b/http.c index 2ebd679393..0813c9ecd8 100644 --- 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 (fetch_pack_index(sha1, base_url)) + 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; + } + + 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 1a4dfc9695..fc675b50ad 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 && + 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 From 90d05713575ea6ed21d05228bcda8461f7b28ccf Mon Sep 17 00:00:00 2001 From: Tay Ray Chuan Date: Mon, 19 Apr 2010 22:46:43 +0800 Subject: [PATCH 12/12] http.c::new_http_pack_request: do away with the temp variable filename Now that the temporary variable char *filename is only used in one place, do away with it and just call sha1_pack_name() directly. Signed-off-by: Tay Ray Chuan Acked-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- http.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index 0813c9ecd8..e41fcc3800 100644 --- a/http.c +++ b/http.c @@ -1077,7 +1077,6 @@ int finish_http_pack_request(struct http_pack_request *preq) struct http_pack_request *new_http_pack_request( struct packed_git *target, const char *base_url) { - char *filename; long prev_posn = 0; char range[RANGE_HEADER_SIZE]; struct strbuf buf = STRBUF_INIT; @@ -1092,8 +1091,8 @@ struct http_pack_request *new_http_pack_request( sha1_to_hex(target->sha1)); preq->url = strbuf_detach(&buf, NULL); - filename = sha1_pack_name(target->sha1); - snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", filename); + snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", + sha1_pack_name(target->sha1)); preq->packfile = fopen(preq->tmpfile, "a"); if (!preq->packfile) { error("Unable to open local file %s for pack",