Merge branch 'mm/lib-httpd-cgi-safe' into jch

CGI helper scripts used by HTTP-related test scripts have been updated
to use atomic filesystem operations, preventing race conditions when
Apache handles concurrent requests.

* mm/lib-httpd-cgi-safe:
  t/README: document writing concurrency-safe helpers
  t/lib-httpd: make http-429 first-request check atomic
  t/lib-httpd: fix apply-one-time-script race under concurrent requests

t/README

@@ -854,6 +854,38 @@ from the test harness library.  At the end of the script, call
 'test_done'.
 
 
+Writing concurrency-safe helpers
+--------------------------------
+
+Some test code runs concurrently: a test may background work with '&',
+and the helper scripts installed for the web server (in t/lib-httpd) are
+run once per request, so the same script can execute for several
+requests at once.  Such code cannot assume it has exclusive access to a
+file.
+
+When exactly one of several concurrent processes needs to "win" a
+decision, a single atomic filesystem operation can make it, rather than
+a check followed by a separate action.  A "test -f X" then "touch X"
+(or "rm X") races: two processes can both pass the check before either
+acts.  Two atomic operations avoid this:
+
+ - "mkdir dir", which fails if the directory already exists, so that
+   exactly one caller wins, electing a first or only request (see
+   t/lib-httpd/http-429.sh).
+
+ - "mv src dst" (rename), which fails if the source is gone, so that
+   exactly one caller consumes it, claiming a planted one-shot marker
+   (see t/lib-httpd/apply-one-time-script.sh).
+
+A "$$" suffix on per-request scratch files keeps concurrent invocations
+from clobbering each other's fixed-name files.
+
+This is a standard shell locking idiom, and the same reasoning behind
+Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL,
+and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic
+operation whose failure indicates that another process got there first.
+
+
 Test harness library
 --------------------
 

t/lib-httpd.sh

@@ -159,6 +159,9 @@ prepare_httpd() {
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
 	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
 	cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
+	# The web server can run any of these CGI scripts for two requests at
+	# once; a helper that keeps state between requests must do so with an
+	# atomic operation. See "Writing concurrency-safe helpers" in t/README.
 	install_script incomplete-length-upload-pack-v2-http.sh
 	install_script incomplete-body-upload-pack-v2-http.sh
 	install_script error-no-report.sh

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
+#
+# 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.
+
+test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"
+
+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
-	LC_ALL=C
-	export LC_ALL
-
-	"$GIT_EXEC_PATH/git-http-backend" >out
-	./one-time-script out >out_modified
-
-	if cmp -s out out_modified
-	then
-		cat out
-	else
-		cat out_modified
-		rm one-time-script
-	fi
+	cat "$modified"
 else
-	"$GIT_EXEC_PATH/git-http-backend"
+	cat "$out"
 fi
+rm -f "$out" "$modified" one-time-script.$$

t/lib-httpd/http-429.sh

@@ -26,14 +26,24 @@ repo_path="${remaining#*/}"  # Get rest (repo path)
 # The repo name is the first component before any "/"
 repo_name="${repo_path%%/*}"
 
-# Use current directory (HTTPD_ROOT_PATH) for state file
-# Create a safe filename from test_context, retry_after and repo_name
-# This ensures all requests for the same test context share the same state file
+# Use current directory (HTTPD_ROOT_PATH) for state.
+# Create a safe name from test_context, retry_after and repo_name so that all
+# requests for the same test context share the same state.
 safe_name=$(echo "${test_context}-${retry_after}-${repo_name}" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-')
-state_file="http-429-state-${safe_name}"
+state="http-429-state-${safe_name}"
 
-# Check if this is the first call (no state file exists)
-if test -f "$state_file"
+# This endpoint returns 429 to the first request and forwards later ones to
+# git-http-backend, so the retry succeeds. Apache can run this CGI for several
+# requests at once, so a single atomic "mkdir" elects that first request: the
+# one whose mkdir succeeds returns 429 and leaves the directory behind as the
+# "already rate-limited" marker; every later request finds the directory (mkdir
+# fails) and is forwarded.
+#
+# "permanent" is the exception: it must return 429 to every request and never
+# succeed, so it skips the mkdir and records no state. A leftover directory
+# would make its own later requests find the marker and be forwarded, which is
+# exactly what "permanent" must not do.
+if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null
 then
 	# Already returned 429 once, forward to git-http-backend
 	# Set PATH_INFO to just the repo path (without retry-after value)
@@ -52,9 +62,6 @@ then
 	exec "$GIT_EXEC_PATH/git-http-backend"
 fi
 
-# Mark that we've returned 429
-touch "$state_file"
-
 # Output HTTP 429 response
 printf "Status: 429 Too Many Requests\r\n"
 
@@ -67,8 +74,7 @@ case "$retry_after" in
 		printf "Retry-After: invalid-format-123abc\r\n"
 		;;
 	permanent)
-		# Always return 429, don't set state file for success
-		rm -f "$state_file"
+		# Always return 429
 		printf "Retry-After: 1\r\n"
 		printf "Content-Type: text/plain\r\n"
 		printf "\r\n"

t/meson.build

@@ -717,6 +717,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',

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