http: avoid concurrent appends to partial packs

Pack requests stage downloads in a predictable partial-pack file so an
interrupted transfer can be resumed. Both packfile URI and ordinary dumb
HTTP requests use this staging path. Opening it in append mode forces
each write to the current end of the file, so concurrent responses can
append duplicate data and corrupt the pack.

Open the partial pack read-write without O_APPEND and seek once to its
current end. Each downloader then retains the offset matching the Range
it requested. Because the staging key must uniquely identify immutable
pack contents, overlapping responses write the same bytes at the same
offsets instead of extending the file with duplicate data.

Duplicate the staging descriptor for index-pack instead of reopening the
path after closing the stream. Another downloader may unlink the staging
path before indexing begins, but index-pack can still read the retained
descriptor.

Exercise resumed transfers and overlapping 200 and 206 responses, and
clarify the staging-key documentation.

Signed-off-by: Ted Nyman <tnyman@openai.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
index 12036e65e9365153533e992b8a383b67eb3e1b8f..45e0d3d07c73cf40b9a0802e2e742b02ec94c9fb 100644 (file)
--- 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>::

diff --git a/http.c b/http.c
index 930e0d227fda73073305c6fb30a59fb9df2235ae..4965e892489ebf1acc07ca8192c9665a40bea750 100644 (file)
--- 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;
@@ -2733,22 +2736,30 @@ 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);
+       fd = open(preq->tmpfile.buf, O_RDWR | O_CREAT, 0666);
+       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();
@@ -2757,12 +2768,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",

diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 621f2fbf7327c56363b48655bd2ec6ce5d3d9cbb..76cfcb8006d7d3a4c03f39a03e766b203a1d768c 100755 (executable)
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -312,6 +312,170 @@ test_expect_success 'http-fetch --packfile accepts an already complete partial'
        git -C packfileclient-complete 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 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;
+       my $completed = 0;
+       END {
+               if (!$completed) {
+                       signal_ready($server_ready, "failed");
+                       signal_ready($first_ready, "failed");
+               }
+       }
+
+       $SIG{ALRM} = sub { die "timed out serving concurrent pack requests\n" };
+       alarm 60;
+
+       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: $!";
+       $completed = 1;
+       alarm 0;
+       EOF
+       {
+               "$TRASH_DIRECTORY/slow-pack-server" "$pack" \
+                       "$TRASH_DIRECTORY/server-ready" \
+                       "$TRASH_DIRECTORY/first-ready" >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" &&
+       {
+               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 "$second_pid" &&
+       wait "$first_pid" &&
+       wait "$server_pid" &&
+       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 &&