http: avoid concurrent appends to partial packs

index 09b5d675ee..60ca91cf3a 100644 --- a/Documentation/git-http-fetch.adoc +++ b/Documentation/git-http-fetch.adoc @@ -48,8 +48,9 @@ commit-id:: line (which is not expected in this case), 'git http-fetch' fetches the packfile directly at the given URL and uses index-pack to generate corresponding .idx and .keep files. - The hash is used to determine the name of the temporary file and is - arbitrary. The output of index-pack is printed to stdout. Requires + The hash is used to determine the name of the temporary file. It need + not be the pack hash, but it must uniquely identify the pack contents + for resumption. The output of index-pack is printed to stdout. Requires one or more --index-pack-arg options. --index-pack-arg=<arg>::

index 601a77c3c1..05f68f306a 100644 --- a/http-fetch.c +++ b/http-fetch.c @@ -70,7 +70,8 @@ static void fetch_single_packfile(struct object_id *packfile_hash, if (start_active_slot(preq->slot)) { run_active_slot(preq->slot); - if (results.curl_result != CURLE_OK) { + if (results.curl_result != CURLE_OK && + results.http_code != 416) { struct url_info url; char *nurl = url_normalize(preq->url, &url); if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {

index 3c23cbba27..03dc8102a1 100644 --- a/http-push.c +++ b/http-push.c @@ -595,7 +595,8 @@ static void finish_request(struct transfer_request *request) } else if (request->state == RUN_FETCH_PACKED) { int fail = 1; - if (request->curl_result != CURLE_OK) { + if (request->curl_result != CURLE_OK && + request->http_code != 416) { fprintf(stderr, "Unable to get pack file %s\n%s", request->url, curl_errorstr); } else {

index b58a3b2a92..abafca84d6 100644 --- a/http-walker.c +++ b/http-walker.c @@ -451,7 +451,8 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, if (start_active_slot(preq->slot)) { run_active_slot(preq->slot); - if (results.curl_result != CURLE_OK) { + if (results.curl_result != CURLE_OK && + results.http_code != 416) { error("Unable to get pack file %s\n%s", preq->url, curl_errorstr); goto abort;

index b4e7b8d00b..a530995363 100644 --- a/http.c +++ b/http.c @@ -2688,10 +2688,13 @@ int finish_http_pack_request(struct http_pack_request *preq) int tmpfile_fd; int ret = 0; + /* Another downloader may unlink the staging path while we index it. */ + tmpfile_fd = xdup(fileno(preq->packfile)); fclose(preq->packfile); preq->packfile = NULL; - - tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY); + if (lseek(tmpfile_fd, 0, SEEK_SET) < 0) + die_errno("unable to seek local file %s for pack", + preq->tmpfile.buf); ip.git_cmd = 1; ip.in = tmpfile_fd; @@ -2704,13 +2707,8 @@ int finish_http_pack_request(struct http_pack_request *preq) else ip.no_stdout = 1; - if (run_command(&ip)) { + if (run_command(&ip)) ret = -1; - goto cleanup; - } - -cleanup: - close(tmpfile_fd); unlink(preq->tmpfile.buf); return ret; } @@ -2738,22 +2736,45 @@ struct http_pack_request *new_http_pack_request( struct http_pack_request *new_direct_http_pack_request( const unsigned char *packed_git_hash, char *url) { - off_t prev_posn = 0; + off_t prev_posn; struct http_pack_request *preq; + int fd; CALLOC_ARRAY(preq, 1); strbuf_init(&preq->tmpfile, 0); - preq->url = url; odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack"); strbuf_addstr(&preq->tmpfile, ".temp"); - preq->packfile = fopen(preq->tmpfile.buf, "a"); - if (!preq->packfile) { - error("Unable to open local file %s for pack", - preq->tmpfile.buf); + /* + * MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an + * existing file; reopen a newly created file so others may unlink it. + */ + for (;;) { + fd = open(preq->tmpfile.buf, O_RDWR); + if (fd >= 0 || errno != ENOENT) + break; + fd = open(preq->tmpfile.buf, O_RDWR | O_CREAT | O_EXCL, 0666); + if (fd >= 0) { + close(fd); + continue; + } + if (errno != EEXIST) + break; + } + if (fd < 0) { + error_errno("unable to open local file %s for pack", + preq->tmpfile.buf); goto abort; } + prev_posn = lseek(fd, 0, SEEK_END); + if (prev_posn < 0) { + error_errno("unable to seek local file %s for pack", + preq->tmpfile.buf); + close(fd); + goto abort; + } + preq->packfile = xfdopen(fd, "w"); preq->slot = get_active_slot(); preq->headers = object_request_headers(); @@ -2762,12 +2783,7 @@ struct http_pack_request *new_direct_http_pack_request( curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url); curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER, preq->headers); - /* - * If there is data present from a previous transfer attempt, - * resume where it left off - */ - prev_posn = ftello(preq->packfile); - if (prev_posn>0) { + if (prev_posn > 0) { if (http_is_verbose) fprintf(stderr, "Resuming fetch of pack %s at byte %"PRIuMAX"\n",

index b0080bf204..24d31ae922 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -293,6 +293,250 @@ test_expect_success 'http-fetch --packfile' ' git -C packfileclient cat-file -e "$HASH" ' +test_expect_success 'http-fetch --packfile resumes a partial download' ' + git init packfileclient-resume && + p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git && + ls objects/pack/pack-*.pack) && + tmpfile="packfileclient-resume/.git/objects/pack/pack-$ARBITRARY.pack.temp" && + test_copy_bytes 64 <"$HTTPD_DOCUMENT_ROOT_PATH/repo_pack.git/$p" >"$tmpfile" && + GIT_TRACE_CURL="$TRASH_DIRECTORY/resume.trace" \ + git -C packfileclient-resume http-fetch --packfile="$ARBITRARY" \ + --index-pack-arg=index-pack --index-pack-arg=--stdin \ + --index-pack-arg=--keep \ + "$HTTPD_URL/dumb/repo_pack.git/$p" >out && + test_grep "Range: bytes=64-" resume.trace && + test_path_is_missing "$tmpfile" && + git -C packfileclient-resume cat-file -e "$HASH" +' + +test_expect_success 'http-fetch --packfile permits unlink while indexing' ' + git init packfileclient-unlink && + p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git && + ls objects/pack/pack-*.pack) && + tmpfile="packfileclient-unlink/.git/objects/pack/pack-$ARBITRARY.pack.temp" && + write_script git-unlink-index-pack <<-\EOF && + test -f "$GIT_TEST_PACK_TEMP" || exit 1 + rm "$GIT_TEST_PACK_TEMP" || exit 1 + exec git index-pack "$@" + EOF + test_when_finished "rm -f git-unlink-index-pack" && + PATH="$TRASH_DIRECTORY:$PATH" \ + GIT_TEST_PACK_TEMP="$TRASH_DIRECTORY/$tmpfile" \ + git -C packfileclient-unlink http-fetch --packfile="$ARBITRARY" \ + --index-pack-arg=unlink-index-pack \ + --index-pack-arg=--stdin --index-pack-arg=--keep \ + "$HTTPD_URL/dumb/repo_pack.git/$p" >out && + test_path_is_missing "$tmpfile" && + git -C packfileclient-unlink cat-file -e "$HASH" +' + +test_expect_success PIPE 'concurrent http-fetch --packfile accepts a complete partial' ' + git init packfileclient-concurrent && + p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git && + ls objects/pack/pack-*.pack) && + packhash=$(basename "$p" .pack) && + packhash=${packhash#pack-} && + tmpfile="packfileclient-concurrent/.git/objects/pack/pack-$packhash.pack.temp" && + test_copy_bytes 64 <"$HTTPD_DOCUMENT_ROOT_PATH/repo_pack.git/$p" >"$tmpfile" && + mkfifo first-ready first-continue && + exec 8<>first-ready && + exec 9<>first-continue && + write_script git-wait-index-pack <<-\EOF && + echo ready >"$GIT_TEST_WAIT_READY" && + read continue <"$GIT_TEST_WAIT_CONTINUE" && + exec git index-pack "$@" + EOF + { + ( + if ! PATH="$TRASH_DIRECTORY:$PATH" \ + GIT_TEST_WAIT_READY="$TRASH_DIRECTORY/first-ready" \ + GIT_TEST_WAIT_CONTINUE="$TRASH_DIRECTORY/first-continue" \ + GIT_TRACE_CURL="$TRASH_DIRECTORY/first.trace" \ + git -C packfileclient-concurrent http-fetch --packfile="$packhash" \ + --index-pack-arg=wait-index-pack \ + --index-pack-arg=--stdin --index-pack-arg=--keep \ + "$HTTPD_URL/dumb/repo_pack.git/$p" >first.out + then + echo failed >"$TRASH_DIRECTORY/first-ready" && + exit 1 + fi + ) & + first_pid=$! + } && + test_when_finished " + echo continue >&9 + kill $first_pid 2>/dev/null || : + wait $first_pid 2>/dev/null || : + exec 8>&- + exec 9>&- + rm -f first-ready first-continue git-wait-index-pack + " && + read ready <&8 && + test "$ready" = ready && + GIT_TRACE_CURL="$TRASH_DIRECTORY/second.trace" \ + git -C packfileclient-concurrent http-fetch --packfile="$packhash" \ + --index-pack-arg=index-pack \ + --index-pack-arg=--stdin --index-pack-arg=--keep \ + "$HTTPD_URL/dumb/repo_pack.git/$p" >second.out && + echo continue >&9 && + wait "$first_pid" && + printf "pack\t%s\n" "$packhash" >expect && + test_cmp expect first.out && + printf "keep\t%s\n" "$packhash" >expect && + test_cmp expect second.out && + test_grep "Range: bytes=64-" first.trace && + test_grep "Range: bytes=[0-9]*-" second.trace && + test_grep "HTTP/[0-9.]* 416" second.trace && + test_path_is_missing "$tmpfile" && + git -C packfileclient-concurrent cat-file -e "$HASH" +' + +test_expect_success PERL,PIPE 'concurrent http-fetch --packfile cannot corrupt an overlapping download' ' + git init packfileclient-overlap && + blob=$(test-tool genrandom pack-overlap 2m | + git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \ + hash-object -w --stdin) && + packhash=$(printf "%s\n" "$blob" | + git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \ + pack-objects "$TRASH_DIRECTORY/overlap-pack") && + pack="$TRASH_DIRECTORY/overlap-pack-$packhash.pack" && + tmpfile="packfileclient-overlap/.git/objects/pack/pack-$packhash.pack.temp" && + mkfifo server-ready first-ready && + exec 7<>server-ready && + exec 8<>first-ready && + write_script slow-pack-server "$PERL_PATH" <<-\EOF && + use strict; + use warnings; + use IO::Socket::INET; + + my ($packfile, $server_ready, $first_ready) = @ARGV; + open(my $in, "<:raw", $packfile) or die "open $packfile: $!"; + my $pack = do { local $/; <$in> }; + close($in) or die "close $packfile: $!"; + my $server = IO::Socket::INET->new(LocalAddr => "127.0.0.1", + LocalPort => 0, Proto => "tcp", Listen => 2, ReuseAddr => 1) + or die "listen: $!"; + + sub signal_ready { + my ($file, $value) = @_; + open(my $out, ">", $file) or die "open $file: $!"; + print $out "$value\n" or die "write $file: $!"; + close($out) or die "close $file: $!"; + } + + sub write_all { + my ($out, $data) = @_; + my $offset = 0; + while ($offset < length($data)) { + my $written = syswrite($out, $data, + length($data) - $offset, $offset); + defined($written) && $written or die "write response: $!"; + $offset += $written; + } + } + + sub start_response { + my $out = $server->accept() or die "accept: $!"; + <$out> or die "read request: $!"; + my $start = 0; + while (<$out>) { + last if /^\r?\n$/; + $start = $1 if /^Range: bytes=(\d+)-/i; + } + $start < length($pack) or die "invalid range $start"; + my $length = length($pack) - $start; + my $middle = int($length / 2); + my $status = $start ? "206 Partial Content" : "200 OK"; + my $headers = "HTTP/1.1 $status\r\n" . + "Content-Length: $length\r\n" . + ($start ? "Content-Range: bytes $start-" . + (length($pack) - 1) . "/" . length($pack) . "\r\n" : "") . + "Connection: close\r\n\r\n"; + write_all($out, $headers); + write_all($out, substr($pack, $start, $middle)); + return ($out, $start + $middle); + } + + signal_ready($server_ready, $server->sockport()); + my ($first, $first_pos) = start_response(); + signal_ready($first_ready, "ready"); + my ($second, $second_pos) = start_response(); + write_all($first, substr($pack, $first_pos)); + write_all($second, substr($pack, $second_pos)); + close($first) or die "close first response: $!"; + close($second) or die "close second response: $!"; + EOF + { + ( + if ! "$TRASH_DIRECTORY/slow-pack-server" "$pack" \ + "$TRASH_DIRECTORY/server-ready" \ + "$TRASH_DIRECTORY/first-ready" + then + echo failed >"$TRASH_DIRECTORY/server-ready" && + echo failed >"$TRASH_DIRECTORY/first-ready" && + exit 1 + fi + ) >server.log 2>&1 & + server_pid=$! + } && + test_when_finished " + kill $server_pid 2>/dev/null || : + wait $server_pid 2>/dev/null || : + exec 7>&- + exec 8>&- + rm -f server-ready first-ready slow-pack-server + " && + read port <&7 && + url="http://127.0.0.1:$port/pack" && + { + ( + if ! GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-first.trace" \ + GIT_TRACE_CURL_NO_DATA=1 \ + git -C packfileclient-overlap http-fetch --packfile="$packhash" \ + --index-pack-arg=index-pack \ + --index-pack-arg=--stdin --index-pack-arg=--keep \ + "$url" >first.out + then + echo failed >"$TRASH_DIRECTORY/first-ready" && + exit 1 + fi + ) & + first_pid=$! + } && + test_when_finished " + kill $first_pid 2>/dev/null || : + wait $first_pid 2>/dev/null || : + " && + read ready <&8 && + test "$ready" = ready && + test_path_is_file "$tmpfile" && + test -s "$tmpfile" && + { + GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-second.trace" \ + GIT_TRACE_CURL_NO_DATA=1 \ + git -C packfileclient-overlap http-fetch --packfile="$packhash" \ + --index-pack-arg=index-pack \ + --index-pack-arg=--stdin --index-pack-arg=--keep \ + "$url" >second.out & + second_pid=$! + } && + test_when_finished " + kill $second_pid 2>/dev/null || : + wait $second_pid 2>/dev/null || : + " && + wait "$server_pid" && + wait "$first_pid" && + wait "$second_pid" && + test_grep "HTTP/[0-9.]* 200" overlap-first.trace && + test_grep "Range: bytes=[1-9][0-9]*-" overlap-second.trace && + test_grep "HTTP/[0-9.]* 206" overlap-second.trace && + printf "keep\t%s\npack\t%s\n" "$packhash" "$packhash" | sort >expect && + sort first.out second.out >actual && + test_cmp expect actual && + test_path_is_missing "$tmpfile" && + git -C packfileclient-overlap cat-file -e "$blob" +' + 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 &&