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

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/lib-httpd: document writing concurrency-safe CGI helpers
  t/lib-httpd: make http-429 first-request check atomic
  t/lib-httpd: fix apply-one-time-script race under concurrent requests

t/lib-httpd.sh

@@ -159,6 +159,19 @@ prepare_httpd() {
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
 	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
 	cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH"
+	# Apache runs each of these CGI scripts once per request. Apache can run one
+	# script for several requests at the same time. A helper that keeps state
+	# between requests must update that state with one atomic operation. A check
+	# and then a separate action is not safe: two requests can both pass the
+	# check before either one acts. Test the exit status of one atomic operation
+	# instead:
+	#   - "mkdir dir" fails if the directory exists, so only one request
+	#     succeeds. http-429.sh selects the first request this way.
+	#   - "rm marker" (without "-f") fails if the marker is gone, so only one
+	#     request consumes it. apply-one-time-script.sh claims its one-shot
+	#     marker this way.
+	# A scratch file name includes the process ID ($$), so concurrent requests
+	# do not overwrite each other's files.
 	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,43 @@
 #
 # 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 several requests at the same time. For example, a
+# partial fetch lazily fetches a missing object while the first response is
+# still in flight. To stay correct, the helper removes the marker only after
+# the response changes, and only with "rm" (without "-f"). The "rm" fails for
+# every request except the one that removes the marker first. That request
+# serves the modified body. Every other request serves its response unchanged.
+# No request emits an empty body, which Apache would report as HTTP 500.
+#
+# A scratch file name includes the process ID ($$), so concurrent requests do
+# not overwrite each other's files.
+#
+# The helper can run one-time-script more than once. It consumes the marker
+# when the response changes (the "rm" after "cmp"), not when it runs the
+# script. A request whose response is not the target runs the script, finds no
+# change, and leaves the marker for a later request. This 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"
+
+# one-time-script can be gone here: a concurrent request may have consumed it
+# since the "test -f" above. Then "./one-time-script" fails, the exit status
+# selects the unmodified body, and "2>/dev/null" discards the expected
+# "no such file" message.
+if ./one-time-script "$out" 2>/dev/null >"$modified" &&
+   ! cmp -s "$out" "$modified" &&
+   rm one-time-script 2>/dev/null
 then
-	LC_ALL=C
+	cat "$modified"
-	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
 else
-	"$GIT_EXEC_PATH/git-http-backend"
+	cat "$out"
 fi
+rm -f "$out" "$modified"

t/lib-httpd/http-429.sh

@@ -3,7 +3,7 @@
 # Script to return HTTP 429 Too Many Requests responses for testing retry logic.
 # Usage: /http_429/<test-context>/<retry-after-value>/<repo-path>
 #
-# The test-context is a unique identifier for each test to isolate state files.
+# The test-context is a unique identifier for each test to isolate state directories.
 # The retry-after-value can be:
 #   - A number (e.g., "1", "2", "100") - sets Retry-After header to that many seconds
 #   - "none" - no Retry-After header
@@ -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
+# Store state in the current directory (HTTPD_ROOT_PATH). Build a safe name
-# Create a safe filename from test_context, retry_after and repo_name
+# from test_context, retry_after, and repo_name, so that all requests for one
-# This ensures all requests for the same test context share the same state file
+# 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)
+# This endpoint returns 429 to the first request. It forwards every later
-if test -f "$state_file"
+# request to git-http-backend, so the retry succeeds. Apache can run this CGI
+# for several requests at the same time. A single atomic "mkdir" selects the
+# first request, because only one "mkdir" succeeds. That request returns 429
+# and leaves the directory as the "already rate-limited" marker. Every later
+# "mkdir" fails, so the endpoint forwards those requests.
+#
+# "permanent" is the exception. It must return 429 to every request, so it
+# skips the "mkdir" and records no state. A leftover directory would let a
+# later "permanent" request find the marker. The endpoint would forward that
+# request, which "permanent" must not allow.
+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
+		# Always return 429
-		rm -f "$state_file"
 		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