diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index fc646447d5c038..f26e1594ab64ee 100644 --- a/t/lib-httpd.sh +++ b/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 diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh index b1682944e280e2..8ab97e882abb27 100644 --- a/t/lib-httpd/apply-one-time-script.sh +++ b/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 -then - LC_ALL=C - export LC_ALL +# +# 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. - "$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" + +# 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 + cat "$modified" else - "$GIT_EXEC_PATH/git-http-backend" + cat "$out" fi +rm -f "$out" "$modified" diff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh index c97b16145b7f92..904cdacbd0ede6 100644 --- a/t/lib-httpd/http-429.sh +++ b/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/// # -# 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 -# 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 +# Store state in the current directory (HTTPD_ROOT_PATH). Build a safe name +# from test_context, retry_after, and repo_name, so that all requests for one +# 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. It forwards every later +# 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 - rm -f "$state_file" + # Always return 429 printf "Retry-After: 1\r\n" printf "Content-Type: text/plain\r\n" printf "\r\n" diff --git a/t/meson.build b/t/meson.build index 3219264fe7d497..a118a4d7196b17 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 00000000000000..cd8e6560056160 --- /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