Skip to content

Commit d2c25ff

Browse files
tnm-oaigitster
tnm-oai
authored and
gitster
committed
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. MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an existing file. Create a missing partial pack exclusively, close it, and reopen it without O_CREAT so every retained descriptor permits another downloader to unlink the staging path. Duplicate that descriptor for index-pack instead of reopening the path after closing the stream; index-pack installs its own pack and the shared staging file is only unlinked, never renamed. Accept HTTP 416 when a partial pack is already complete and let index-pack validate its contents. Exercise resumed transfers, EOF ranges, overlapping 200 and 206 responses, and unlinking the staging path while index-pack still holds its descriptor. Clarify the staging-key documentation. Signed-off-by: Ted Nyman <tnyman@openai.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 1090e9a commit d2c25ff

6 files changed

Lines changed: 295 additions & 25 deletions

File tree

‎Documentation/git-http-fetch.adoc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,9 @@ commit-id::
4848
line (which is not expected in
4949
this case), 'git http-fetch' fetches the packfile directly at the given
5050
URL and uses index-pack to generate corresponding .idx and .keep files.
51-
The hash is used to determine the name of the temporary file and is
52-
arbitrary. The output of index-pack is printed to stdout. Requires
51+
The hash is used to determine the name of the temporary file. It need
52+
not be the pack hash, but it must uniquely identify the pack contents
53+
for resumption. The output of index-pack is printed to stdout. Requires
5354
one or more --index-pack-arg options.
5455

5556
--index-pack-arg=<arg>::

‎http-fetch.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,8 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
7070

7171
if (start_active_slot(preq->slot)) {
7272
run_active_slot(preq->slot);
73-
if (results.curl_result != CURLE_OK) {
73+
if (results.curl_result != CURLE_OK &&
74+
results.http_code != 416) {
7475
struct url_info url;
7576
char *nurl = url_normalize(preq->url, &url);
7677
if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {

‎http-push.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -595,7 +595,8 @@ static void finish_request(struct transfer_request *request)
595595

596596
} else if (request->state == RUN_FETCH_PACKED) {
597597
int fail = 1;
598-
if (request->curl_result != CURLE_OK) {
598+
if (request->curl_result != CURLE_OK &&
599+
request->http_code != 416) {
599600
fprintf(stderr, "Unable to get pack file %s\n%s",
600601
request->url, curl_errorstr);
601602
} else {

‎http-walker.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,8 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo,
451451

452452
if (start_active_slot(preq->slot)) {
453453
run_active_slot(preq->slot);
454-
if (results.curl_result != CURLE_OK) {
454+
if (results.curl_result != CURLE_OK &&
455+
results.http_code != 416) {
455456
error("Unable to get pack file %s\n%s", preq->url,
456457
curl_errorstr);
457458
goto abort;

‎http.c‎

Lines changed: 36 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2688,10 +2688,13 @@ int finish_http_pack_request(struct http_pack_request *preq)
26882688
int tmpfile_fd;
26892689
int ret = 0;
26902690

2691+
/* Another downloader may unlink the staging path while we index it. */
2692+
tmpfile_fd = xdup(fileno(preq->packfile));
26912693
fclose(preq->packfile);
26922694
preq->packfile = NULL;
2693-
2694-
tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
2695+
if (lseek(tmpfile_fd, 0, SEEK_SET) < 0)
2696+
die_errno("unable to seek local file %s for pack",
2697+
preq->tmpfile.buf);
26952698

26962699
ip.git_cmd = 1;
26972700
ip.in = tmpfile_fd;
@@ -2704,13 +2707,8 @@ int finish_http_pack_request(struct http_pack_request *preq)
27042707
else
27052708
ip.no_stdout = 1;
27062709

2707-
if (run_command(&ip)) {
2710+
if (run_command(&ip))
27082711
ret = -1;
2709-
goto cleanup;
2710-
}
2711-
2712-
cleanup:
2713-
close(tmpfile_fd);
27142712
unlink(preq->tmpfile.buf);
27152713
return ret;
27162714
}
@@ -2738,22 +2736,45 @@ struct http_pack_request *new_http_pack_request(
27382736
struct http_pack_request *new_direct_http_pack_request(
27392737
const unsigned char *packed_git_hash, char *url)
27402738
{
2741-
off_t prev_posn = 0;
2739+
off_t prev_posn;
27422740
struct http_pack_request *preq;
2741+
int fd;
27432742

27442743
CALLOC_ARRAY(preq, 1);
27452744
strbuf_init(&preq->tmpfile, 0);
2746-
27472745
preq->url = url;
27482746

27492747
odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
27502748
strbuf_addstr(&preq->tmpfile, ".temp");
2751-
preq->packfile = fopen(preq->tmpfile.buf, "a");
2752-
if (!preq->packfile) {
2753-
error("Unable to open local file %s for pack",
2754-
preq->tmpfile.buf);
2749+
/*
2750+
* MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an
2751+
* existing file; reopen a newly created file so others may unlink it.
2752+
*/
2753+
for (;;) {
2754+
fd = open(preq->tmpfile.buf, O_RDWR);
2755+
if (fd >= 0 || errno != ENOENT)
2756+
break;
2757+
fd = open(preq->tmpfile.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
2758+
if (fd >= 0) {
2759+
close(fd);
2760+
continue;
2761+
}
2762+
if (errno != EEXIST)
2763+
break;
2764+
}
2765+
if (fd < 0) {
2766+
error_errno("unable to open local file %s for pack",
2767+
preq->tmpfile.buf);
27552768
goto abort;
27562769
}
2770+
prev_posn = lseek(fd, 0, SEEK_END);
2771+
if (prev_posn < 0) {
2772+
error_errno("unable to seek local file %s for pack",
2773+
preq->tmpfile.buf);
2774+
close(fd);
2775+
goto abort;
2776+
}
2777+
preq->packfile = xfdopen(fd, "w");
27572778

27582779
preq->slot = get_active_slot();
27592780
preq->headers = object_request_headers();
@@ -2762,12 +2783,7 @@ struct http_pack_request *new_direct_http_pack_request(
27622783
curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);
27632784
curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER, preq->headers);
27642785

2765-
/*
2766-
* If there is data present from a previous transfer attempt,
2767-
* resume where it left off
2768-
*/
2769-
prev_posn = ftello(preq->packfile);
2770-
if (prev_posn>0) {
2786+
if (prev_posn > 0) {
27712787
if (http_is_verbose)
27722788
fprintf(stderr,
27732789
"Resuming fetch of pack %s at byte %"PRIuMAX"\n",

‎t/t5550-http-fetch-dumb.sh‎

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,256 @@ test_expect_success 'http-fetch --packfile' '
293293
git -C packfileclient cat-file -e "$HASH"
294294
'
295295

296+
test_expect_success 'http-fetch --packfile resumes a partial download' '
297+
git init packfileclient-resume &&
298+
p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
299+
ls objects/pack/pack-*.pack) &&
300+
tmpfile="packfileclient-resume/.git/objects/pack/pack-$ARBITRARY.pack.temp" &&
301+
test_copy_bytes 64 <"$HTTPD_DOCUMENT_ROOT_PATH/repo_pack.git/$p" >"$tmpfile" &&
302+
GIT_TRACE_CURL="$TRASH_DIRECTORY/resume.trace" \
303+
git -C packfileclient-resume http-fetch --packfile="$ARBITRARY" \
304+
--index-pack-arg=index-pack --index-pack-arg=--stdin \
305+
--index-pack-arg=--keep \
306+
"$HTTPD_URL/dumb/repo_pack.git/$p" >out &&
307+
test_grep "Range: bytes=64-" resume.trace &&
308+
test_path_is_missing "$tmpfile" &&
309+
git -C packfileclient-resume cat-file -e "$HASH"
310+
'
311+
312+
test_expect_success 'http-fetch --packfile permits unlink while indexing' '
313+
git init packfileclient-unlink &&
314+
p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
315+
ls objects/pack/pack-*.pack) &&
316+
tmpfile="packfileclient-unlink/.git/objects/pack/pack-$ARBITRARY.pack.temp" &&
317+
write_script git-unlink-index-pack <<-\EOF &&
318+
test -f "$GIT_TEST_PACK_TEMP" || exit 1
319+
rm "$GIT_TEST_PACK_TEMP" || exit 1
320+
exec git index-pack "$@"
321+
EOF
322+
test_when_finished "rm -f git-unlink-index-pack" &&
323+
PATH="$TRASH_DIRECTORY:$PATH" \
324+
GIT_TEST_PACK_TEMP="$TRASH_DIRECTORY/$tmpfile" \
325+
git -C packfileclient-unlink http-fetch --packfile="$ARBITRARY" \
326+
--index-pack-arg=unlink-index-pack \
327+
--index-pack-arg=--stdin --index-pack-arg=--keep \
328+
"$HTTPD_URL/dumb/repo_pack.git/$p" >out &&
329+
test_path_is_missing "$tmpfile" &&
330+
git -C packfileclient-unlink cat-file -e "$HASH"
331+
'
332+
333+
test_expect_success PIPE 'concurrent http-fetch --packfile accepts a complete partial' '
334+
git init packfileclient-concurrent &&
335+
p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
336+
ls objects/pack/pack-*.pack) &&
337+
packhash=$(basename "$p" .pack) &&
338+
packhash=${packhash#pack-} &&
339+
tmpfile="packfileclient-concurrent/.git/objects/pack/pack-$packhash.pack.temp" &&
340+
test_copy_bytes 64 <"$HTTPD_DOCUMENT_ROOT_PATH/repo_pack.git/$p" >"$tmpfile" &&
341+
mkfifo first-ready first-continue &&
342+
exec 8<>first-ready &&
343+
exec 9<>first-continue &&
344+
write_script git-wait-index-pack <<-\EOF &&
345+
echo ready >"$GIT_TEST_WAIT_READY" &&
346+
read continue <"$GIT_TEST_WAIT_CONTINUE" &&
347+
exec git index-pack "$@"
348+
EOF
349+
{
350+
(
351+
if ! PATH="$TRASH_DIRECTORY:$PATH" \
352+
GIT_TEST_WAIT_READY="$TRASH_DIRECTORY/first-ready" \
353+
GIT_TEST_WAIT_CONTINUE="$TRASH_DIRECTORY/first-continue" \
354+
GIT_TRACE_CURL="$TRASH_DIRECTORY/first.trace" \
355+
git -C packfileclient-concurrent http-fetch --packfile="$packhash" \
356+
--index-pack-arg=wait-index-pack \
357+
--index-pack-arg=--stdin --index-pack-arg=--keep \
358+
"$HTTPD_URL/dumb/repo_pack.git/$p" >first.out
359+
then
360+
echo failed >"$TRASH_DIRECTORY/first-ready" &&
361+
exit 1
362+
fi
363+
) &
364+
first_pid=$!
365+
} &&
366+
test_when_finished "
367+
echo continue >&9
368+
kill $first_pid 2>/dev/null || :
369+
wait $first_pid 2>/dev/null || :
370+
exec 8>&-
371+
exec 9>&-
372+
rm -f first-ready first-continue git-wait-index-pack
373+
" &&
374+
read ready <&8 &&
375+
test "$ready" = ready &&
376+
GIT_TRACE_CURL="$TRASH_DIRECTORY/second.trace" \
377+
git -C packfileclient-concurrent http-fetch --packfile="$packhash" \
378+
--index-pack-arg=index-pack \
379+
--index-pack-arg=--stdin --index-pack-arg=--keep \
380+
"$HTTPD_URL/dumb/repo_pack.git/$p" >second.out &&
381+
echo continue >&9 &&
382+
wait "$first_pid" &&
383+
printf "pack\t%s\n" "$packhash" >expect &&
384+
test_cmp expect first.out &&
385+
printf "keep\t%s\n" "$packhash" >expect &&
386+
test_cmp expect second.out &&
387+
test_grep "Range: bytes=64-" first.trace &&
388+
test_grep "Range: bytes=[0-9]*-" second.trace &&
389+
test_grep "416 Requested Range Not Satisfiable" second.trace &&
390+
test_path_is_missing "$tmpfile" &&
391+
git -C packfileclient-concurrent cat-file -e "$HASH"
392+
'
393+
394+
test_expect_success PERL,PIPE 'concurrent http-fetch --packfile cannot corrupt an overlapping download' '
395+
git init packfileclient-overlap &&
396+
blob=$(test-tool genrandom pack-overlap 2m |
397+
git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \
398+
hash-object -w --stdin) &&
399+
packhash=$(printf "%s\n" "$blob" |
400+
git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \
401+
pack-objects "$TRASH_DIRECTORY/overlap-pack") &&
402+
pack="$TRASH_DIRECTORY/overlap-pack-$packhash.pack" &&
403+
tmpfile="packfileclient-overlap/.git/objects/pack/pack-$packhash.pack.temp" &&
404+
mkfifo server-ready first-ready &&
405+
exec 7<>server-ready &&
406+
exec 8<>first-ready &&
407+
write_script slow-pack-server "$PERL_PATH" <<-\EOF &&
408+
use strict;
409+
use warnings;
410+
use IO::Socket::INET;
411+
412+
my ($packfile, $server_ready, $first_ready) = @ARGV;
413+
my $completed = 0;
414+
END {
415+
if (!$completed) {
416+
signal_ready($server_ready, "failed");
417+
signal_ready($first_ready, "failed");
418+
}
419+
}
420+
421+
$SIG{ALRM} = sub { die "timed out serving concurrent pack requests\n" };
422+
alarm 60;
423+
424+
open(my $in, "<:raw", $packfile) or die "open $packfile: $!";
425+
my $pack = do { local $/; <$in> };
426+
close($in) or die "close $packfile: $!";
427+
my $server = IO::Socket::INET->new(LocalAddr => "127.0.0.1",
428+
LocalPort => 0, Proto => "tcp", Listen => 2, ReuseAddr => 1)
429+
or die "listen: $!";
430+
431+
sub signal_ready {
432+
my ($file, $value) = @_;
433+
open(my $out, ">", $file) or die "open $file: $!";
434+
print $out "$value\n" or die "write $file: $!";
435+
close($out) or die "close $file: $!";
436+
}
437+
438+
sub write_all {
439+
my ($out, $data) = @_;
440+
my $offset = 0;
441+
while ($offset < length($data)) {
442+
my $written = syswrite($out, $data,
443+
length($data) - $offset, $offset);
444+
defined($written) && $written or die "write response: $!";
445+
$offset += $written;
446+
}
447+
}
448+
449+
sub start_response {
450+
my $out = $server->accept() or die "accept: $!";
451+
<$out> or die "read request: $!";
452+
my $start = 0;
453+
while (<$out>) {
454+
last if /^\r?\n$/;
455+
$start = $1 if /^Range: bytes=(\d+)-/i;
456+
}
457+
$start < length($pack) or die "invalid range $start";
458+
my $length = length($pack) - $start;
459+
my $middle = int($length / 2);
460+
my $status = $start ? "206 Partial Content" : "200 OK";
461+
my $headers = "HTTP/1.1 $status\r\n" .
462+
"Content-Length: $length\r\n" .
463+
($start ? "Content-Range: bytes $start-" .
464+
(length($pack) - 1) . "/" . length($pack) . "\r\n" : "") .
465+
"Connection: close\r\n\r\n";
466+
write_all($out, $headers);
467+
write_all($out, substr($pack, $start, $middle));
468+
return ($out, $start + $middle);
469+
}
470+
471+
signal_ready($server_ready, $server->sockport());
472+
my ($first, $first_pos) = start_response();
473+
signal_ready($first_ready, "ready");
474+
my ($second, $second_pos) = start_response();
475+
write_all($first, substr($pack, $first_pos));
476+
write_all($second, substr($pack, $second_pos));
477+
close($first) or die "close first response: $!";
478+
close($second) or die "close second response: $!";
479+
$completed = 1;
480+
alarm 0;
481+
EOF
482+
{
483+
"$TRASH_DIRECTORY/slow-pack-server" "$pack" \
484+
"$TRASH_DIRECTORY/server-ready" \
485+
"$TRASH_DIRECTORY/first-ready" >server.log 2>&1 &
486+
server_pid=$!
487+
} &&
488+
test_when_finished "
489+
kill $server_pid 2>/dev/null || :
490+
wait $server_pid 2>/dev/null || :
491+
exec 7>&-
492+
exec 8>&-
493+
rm -f server-ready first-ready slow-pack-server
494+
" &&
495+
read port <&7 &&
496+
url="http://127.0.0.1:$port/pack" &&
497+
{
498+
(
499+
if ! GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-first.trace" \
500+
GIT_TRACE_CURL_NO_DATA=1 \
501+
git -C packfileclient-overlap http-fetch --packfile="$packhash" \
502+
--index-pack-arg=index-pack \
503+
--index-pack-arg=--stdin --index-pack-arg=--keep \
504+
"$url" >first.out
505+
then
506+
echo failed >"$TRASH_DIRECTORY/first-ready" &&
507+
exit 1
508+
fi
509+
) &
510+
first_pid=$!
511+
} &&
512+
test_when_finished "
513+
kill $first_pid 2>/dev/null || :
514+
wait $first_pid 2>/dev/null || :
515+
" &&
516+
read ready <&8 &&
517+
test "$ready" = ready &&
518+
test_path_is_file "$tmpfile" &&
519+
test -s "$tmpfile" &&
520+
{
521+
GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-second.trace" \
522+
GIT_TRACE_CURL_NO_DATA=1 \
523+
git -C packfileclient-overlap http-fetch --packfile="$packhash" \
524+
--index-pack-arg=index-pack \
525+
--index-pack-arg=--stdin --index-pack-arg=--keep \
526+
"$url" >second.out &
527+
second_pid=$!
528+
} &&
529+
test_when_finished "
530+
kill $second_pid 2>/dev/null || :
531+
wait $second_pid 2>/dev/null || :
532+
" &&
533+
wait "$second_pid" &&
534+
wait "$first_pid" &&
535+
wait "$server_pid" &&
536+
test_grep "HTTP/[0-9.]* 200" overlap-first.trace &&
537+
test_grep "Range: bytes=[1-9][0-9]*-" overlap-second.trace &&
538+
test_grep "HTTP/[0-9.]* 206" overlap-second.trace &&
539+
printf "keep\t%s\npack\t%s\n" "$packhash" "$packhash" | sort >expect &&
540+
sort first.out second.out >actual &&
541+
test_cmp expect actual &&
542+
test_path_is_missing "$tmpfile" &&
543+
git -C packfileclient-overlap cat-file -e "$blob"
544+
'
545+
296546
test_expect_success 'fetch notices corrupt pack' '
297547
cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
298548
(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&

0 commit comments

Comments
 (0)