From 862c4258e596e411063808a9a68d0bf4db454ebf Mon Sep 17 00:00:00 2001 From: Michael Montalbo Date: Mon, 6 Jul 2026 20:06:57 -0700 Subject: [PATCH 1/3] t/lib-httpd: fix apply-one-time-script race under concurrent requests apply-one-time-script.sh is a CGI helper that, when the file "one-time-script" is present, runs it to rewrite the git-http-backend response. If "one-time-script" generates a response that differs from git-http-backend, the modified response is returned and "one-time-script" is deleted. Requests after the deletion return normal git-http-backend responses. The deletion is not safe under concurrency. The helper serves the modified body first and deletes "one-time-script" only afterward, so a client can issue its next request while the file still exists. Apache runs the CGI for both requests at once, for example when a partial fetch lazily fetches a missing promisor base while the first response is still in flight. Both requests find the file and try to run it; the first deletes it; the second then fails to exec the now-missing file, produces no output, and the server returns HTTP 500: fatal: ... The requested URL returned error: 500 fatal: could not fetch from promisor remote This is the flaky failure of t5616.47 on the macOS CI runners. Fix it by removing the file with "rm" only after the script has actually changed the response. Because "rm" without "-f" fails once the file is gone, exactly one request removes it and serves the modified body. Any other request serves the unmodified body. Running the script more than once is harmless; only its deletion is serialized, so exactly one request's modified response is ever served. Per-request scratch file names keep concurrent runs from overwriting each other, and no path emits an empty response body. t5616.47 exercises the real code path but, being timing-dependent, passes against the buggy helper almost every time. Add t5567, which 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 --- t/lib-httpd/apply-one-time-script.sh | 50 +++++++++++---- t/meson.build | 1 + t/t5567-one-time-script.sh | 96 ++++++++++++++++++++++++++++ 3 files changed, 133 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 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/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 From 8ed22c02a192e10ab46c7df61e92a3669faaf25a Mon Sep 17 00:00:00 2001 From: Michael Montalbo Date: Mon, 6 Jul 2026 20:32:10 -0700 Subject: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic http-429.sh returns 429 to the first request for an endpoint and forwards later ones to git-http-backend so the retry succeeds. It remembers that it has already answered 429 by checking for a shared state file with "test -f" and creating it with "touch". That "check-and-set" is not atomic. Apache runs the CGI for several requests at once, so two of them can pass the "test -f" before either "touch"es the file, and both then answer as the first request. The retry flow is mostly sequential, so this has not been observed to fail, but the race is latent. Replace the check and the "touch" with a single atomic "mkdir", which fails if the directory already exists, so exactly one of the concurrent requests is rate-limited and the rest are forwarded. The "permanent" mode needs one extra step, for correctness rather than tidiness. The marker means "429 already served, now forward", so it must never be visible to a request that must itself return 429. Since "permanent" returns 429 to every request, it must leave no marker. The original did not manage this. It ran the "touch" unconditionally and removed the file with "rm -f" in the "permanent" case, and that "create-then-remove" has the same racy window: a concurrent "permanent" request can see the marker before the "rm -f" and be wrongly forwarded. Skipping the "mkdir" entirely for "permanent" (the "!= permanent" guard) leaves no marker at all, so every "permanent" request rate-limits. There is no regression test. The check and the set are adjacent commands with nothing in between to synchronize on, so the overlap cannot be forced deterministically, only reproduced by chance; the fix is preventive. Signed-off-by: Michael Montalbo --- t/lib-httpd/http-429.sh | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) 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" From 374d148f43036077c31c5a55ddb1b59da4d3a923 Mon Sep 17 00:00:00 2001 From: Michael Montalbo Date: Mon, 6 Jul 2026 20:32:10 -0700 Subject: [PATCH 3/3] t/lib-httpd: document writing concurrency-safe CGI helpers The apply-one-time-script.sh and http-429.sh fixes share a root cause: a CGI helper assumed it had a file to itself, when Apache can run the helper for several requests at once. Document the atomic idioms that avoid this next to where lib-httpd.sh installs the CGI scripts, so the advice is in front of anyone adding another one. The note describes the anti-pattern, a "test -f" check followed by a separate action, and the two atomic alternatives these helpers now use: - "mkdir", which fails if the directory exists, to elect the first request (http-429.sh); and - "rm" without "-f", which fails once the file is gone, to consume a one-shot marker (apply-one-time-script.sh). Signed-off-by: Michael Montalbo --- t/lib-httpd.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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