[PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests

["Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com>]

From: Michael Montalbo <mmontalbo@gmail.com>

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.

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>
---
 t/lib-httpd/apply-one-time-script.sh | 38 +++++++----
 t/meson.build                        |  1 +
 t/t5567-one-time-script.sh           | 96 ++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+), 14 deletions(-)
 create mode 100755 t/t5567-one-time-script.sh

diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh
index b1682944e2..a298ae89ae 100644
--- a/t/lib-httpd/apply-one-time-script.sh
+++ b/t/lib-httpd/apply-one-time-script.sh
@@ -6,21 +6,31 @@
 #
 # 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.
+
+test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
 
-	"$GIT_EXEC_PATH/git-http-backend" >out
-	./one-time-script out >out_modified
+LC_ALL=C
+export LC_ALL
 
-	if cmp -s out out_modified
-	then
-		cat out
-	else
-		cat out_modified
-		rm one-time-script
-	fi
+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 3219264fe7..a118a4d719 100644
--- 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
index 0000000000..cd8e656005
--- /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
-- 
gitgitgadget