t/lib-httpd: fix apply-one-time-script race under concurrent requests

apply-one-time-script.sh checks for the "one-time-script" marker, runs
it, captures the git-http-backend response in the fixed-name files "out"
and "out_modified", and removes the marker only after it has finished
serving the modified response. Because the client receives the response
body before that removal, it can start its next request while the marker
still exists. Apache can then run this CGI for two requests at once: a
partial fetch that receives a REF_DELTA against a missing promisor
object lazily fetches that base while the first response is still in
flight. The second request passes the marker check, the first request
then removes the marker, and the second fails to exec the now-missing
marker, emits no output, and the server answers HTTP 500:

  fatal: ... The requested URL returned error: 500
  fatal: could not fetch <oid> from promisor remote

This has been seen as a flaky failure of t5616.47 on the macOS CI
runners.

Claim the marker atomically with a rename, and only once the one-time
script has succeeded and actually changed the response; give the scratch
files per-request names. A request that loses the rename, or whose
script fails or leaves the response unchanged, serves the unmodified
body and keeps the marker for a later request. No path emits an empty
body, so the HTTP 500 no longer occurs.

Running the one-time script more than once is fine; the only thing to
avoid is serving a second, racing request's modified output. Two
requests can both find the marker and run the script before either
renames it away, but the rename is atomic, so exactly one of them wins:
it serves its modified body and consumes the marker. The loser's rename
fails because the marker is already gone, so it discards the modified
output it produced and serves the unmodified body instead. The rename,
not running the script, is what is serialized.

Add t5567 to lock this down. The overlap depends on timing, so a live
httpd test such as t5616.47 (the real code path) passes almost every
time even against the buggy helper; t5567 instead drives the helper
directly with a fake git-http-backend and forces the overlap with FIFOs.
Against the pre-fix helper it fails with the same shell error seen in
the field:

  ./one-time-script: No such file or directory

Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh
index b1682944e280e28fe48d7b1f96faa46fb7290b3a..adb9cec5284f1e4eb3c4a921f06f38c0789c883c 100644 (file)
--- a/t/lib-httpd/apply-one-time-script.sh
+++ b/t/lib-httpd/apply-one-time-script.sh
@@ -6,21 +6,37 @@
 #
 # This can be used to simulate the effects of the repository changing in
 # between HTTP request-response pairs.
-if test -f one-time-script
-then
-       LC_ALL=C
-       export LC_ALL
+#
+# Apache can run this CGI for concurrent requests (for example a partial fetch
+# that lazily fetches a missing object while the first response is still in
+# flight), so the helper claims the marker atomically with a rename, and only
+# once it has decided to modify the response. A request that loses the race
+# finds the marker already gone and serves its response unchanged; no request
+# is left emitting an empty body, which the server would report as HTTP 500.
+# Scratch files are per-request ($$) so concurrent requests do not clobber each
+# other.
+#
+# The script may run more than once: the marker is consumed when the response
+# actually changes (the rename after "cmp"), not when the script runs, so a
+# request whose response is not the targeted one runs the script, sees no
+# change, and leaves the marker for a later request. That is safe because the
+# scripts are stateless filters over the captured response.
 
-       "$GIT_EXEC_PATH/git-http-backend" >out
-       ./one-time-script out >out_modified
+test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
 
-       if cmp -s out out_modified
-       then
-               cat out
-       else
-               cat out_modified
-               rm one-time-script
-       fi
+LC_ALL=C
+export LC_ALL
+
+out=out.$$
+modified=out-modified.$$
+"$GIT_EXEC_PATH/git-http-backend" >"$out"
+
+if ./one-time-script "$out" 2>/dev/null >"$modified" &&
+   ! cmp -s "$out" "$modified" &&
+   mv one-time-script one-time-script.$$ 2>/dev/null
+then
+       cat "$modified"
 else
-       "$GIT_EXEC_PATH/git-http-backend"
+       cat "$out"
 fi
+rm -f "$out" "$modified" one-time-script.$$

diff --git a/t/meson.build b/t/meson.build
index 3219264fe7d4973ff96436b5c5ea2f6a9b9468a1..a118a4d7196b17dfce00ccd43041f9c7450ff2ae 100644 (file)
--- a/t/meson.build
+++ b/t/meson.build
@@ -707,6 +707,7 @@ integration_tests = [
   't5564-http-proxy.sh',
   't5565-push-multiple.sh',
   't5566-push-group.sh',
+  't5567-one-time-script.sh',
   't5570-git-daemon.sh',
   't5571-pre-push-hook.sh',
   't5572-pull-submodule.sh',

diff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh
new file mode 100755 (executable)
index 0000000..cd8e656
--- /dev/null
+++ b/t/t5567-one-time-script.sh
@@ -0,0 +1,96 @@
+#!/bin/sh
+
+test_description='apply-one-time-script CGI helper is safe under concurrent requests'
+
+. ./test-lib.sh
+
+HELPER="$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh"
+
+test_expect_success PIPE 'concurrent requests: one rewritten, one passed through, neither empty' '
+       mkdir workdir fakebin &&
+       ENTERED="$PWD/entered" &&
+       GATE="$PWD/gate" &&
+       export ENTERED GATE &&
+       mkfifo "$ENTERED" "$GATE" &&
+
+       # Stand in for git-http-backend. The modify role returns a response
+       # containing "packfile", which the one-time script rewrites. The
+       # passthrough role returns a response that is left untouched, but first
+       # announces that it has entered the helper and then blocks, so that it
+       # is still in flight when the modify role claims and removes the marker.
+       write_script fakebin/git-http-backend <<-\EOF &&
+       printf "Status: 200 OK\r\n"
+       printf "Content-Type: application/x-git-result\r\n"
+       printf "\r\n"
+       if test "$ROLE" = modify
+       then
+               printf "packfile\n"
+       else
+               echo entered >"$ENTERED"
+               read -r released <"$GATE"
+               printf "refs\n"
+       fi
+       EOF
+
+       # The transform that replace_packfile would install as one-time-script:
+       # rewrite responses that contain "packfile", leave the rest alone.
+       write_script workdir/one-time-script <<-\EOF &&
+       if grep packfile "$1" >/dev/null
+       then
+               sed "/packfile/q" "$1" &&
+               printf "REPLACED\n"
+       else
+               cat "$1"
+       fi
+       EOF
+
+       GIT_EXEC_PATH="$PWD/fakebin" &&
+       export GIT_EXEC_PATH &&
+
+       # Hold GATE open read-write on fd 9 for the duration, so releasing the
+       # passthrough request below cannot block even if that request has
+       # already exited (it keeps a reader on the FIFO).
+       exec 9<>"$GATE" &&
+
+       # Launch the passthrough request in the background. It enters the
+       # helper, signals us through ENTERED, then blocks on GATE inside the
+       # fake backend. The braces keep the && chain intact while backgrounding
+       # only the subshell, so "wait" can reap it by pid; kill it on any exit
+       # so a stray blocked child cannot hold the test output open and stall a
+       # reader such as prove.
+       { (
+               cd workdir &&
+               ROLE=passthrough sh "$HELPER" >../passthrough.out 2>../passthrough.err
+       ) & } &&
+       passthrough_pid=$! &&
+       test_when_finished "kill $passthrough_pid 2>/dev/null || :" &&
+
+       # Wait until the passthrough request is past the marker check.
+       read -r entered <"$ENTERED" &&
+
+       # Run the modifying request to completion while the passthrough request
+       # is still blocked.
+       (
+               cd workdir &&
+               ROLE=modify sh "$HELPER" >../modify.out 2>../modify.err
+       ) &&
+
+       # Release the passthrough request and let it finish. Ignore the helper
+       # exit status here so a broken helper is diagnosed by the assertions
+       # below rather than aborting the test.
+       echo released >&9 &&
+       { wait "$passthrough_pid" || :; } &&
+
+       # Neither request may error out or produce an empty (HTTP 500) body,
+       # and each must have played its role: the modify request rewrote its
+       # response and the passthrough request came through untouched.
+       test_must_be_empty passthrough.err &&
+       test_must_be_empty modify.err &&
+       test_grep "Status: 200 OK" passthrough.out &&
+       test_grep "Status: 200 OK" modify.out &&
+       test_grep REPLACED modify.out &&
+       test_grep ! REPLACED passthrough.out &&
+       test_grep refs passthrough.out
+'
+
+test_done