@matheusbsbs / matheus-bagliano / commits / dcade13aa7

t/lib-httpd: fix apply-one-time-script race under concurrent requests

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. Running the one-time script more than once is fine; the only thing to avoid is serving a second, racing request's modified output. Two requests can both find the marker and run the script before either renames it away, but the rename is atomic, so exactly one of them wins: it serves its modified body and consumes the marker. The loser's rename fails because the marker is already gone, so it discards the modified output it produced and serves the unmodified body instead. The rename, not running the script, is what is serialized. 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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Michael Montalbo committed Jul 10, 2026 at 17:30 UTC dcade13aa712cafd36ab39a9935fa6aadd59b232

3 files changed +127 -14

t/lib-httpd/apply-one-time-script.sh

+30 -14

@@ -6,21 +6,37 @@

6 #

7 # This can be used to simulate the effects of the repository changing in

8 # between HTTP request-response pairs.

9 -if test -f one-time-script

10 -then

11 - LC_ALL=C

12 - export LC_ALL

9 +#

10 +# Apache can run this CGI for concurrent requests (for example a partial fetch

11 +# that lazily fetches a missing object while the first response is still in

12 +# flight), so the helper claims the marker atomically with a rename, and only

13 +# once it has decided to modify the response. A request that loses the race

14 +# finds the marker already gone and serves its response unchanged; no request

15 +# is left emitting an empty body, which the server would report as HTTP 500.

16 +# Scratch files are per-request ($$) so concurrent requests do not clobber each

17 +# other.

18 +#

19 +# The script may run more than once: the marker is consumed when the response

20 +# actually changes (the rename after "cmp"), not when the script runs, so a

21 +# request whose response is not the targeted one runs the script, sees no

22 +# change, and leaves the marker for a later request. That is safe because the

23 +# scripts are stateless filters over the captured response.

14 - "$GIT_EXEC_PATH/git-http-backend" >out

15 - ./one-time-script out >out_modified

25 +test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend"

17 - if cmp -s out out_modified

18 - then

19 - cat out

20 - else

21 - cat out_modified

22 - rm one-time-script

23 - fi

27 +LC_ALL=C

28 +export LC_ALL

29 +

30 +out=out.$$

31 +modified=out-modified.$$

32 +"$GIT_EXEC_PATH/git-http-backend" >"$out"

33 +

34 +if ./one-time-script "$out" 2>/dev/null >"$modified" &&

35 + ! cmp -s "$out" "$modified" &&

36 + mv one-time-script one-time-script.$$ 2>/dev/null

37 +then

38 + cat "$modified"

39 else

25 - "$GIT_EXEC_PATH/git-http-backend"

40 + cat "$out"

41 fi

42 +rm -f "$out" "$modified" one-time-script.$$

t/meson.build

@@ -707,6 +707,7 @@ integration_tests = [

707 't5564-http-proxy.sh',

708 't5565-push-multiple.sh',

709 't5566-push-group.sh',

710 + 't5567-one-time-script.sh',

711 't5570-git-daemon.sh',

712 't5571-pre-push-hook.sh',

713 't5572-pull-submodule.sh',

t/t5567-one-time-script.sh new

+96

@@ -0,0 +1,96 @@

1 +#!/bin/sh

2 +

3 +test_description='apply-one-time-script CGI helper is safe under concurrent requests'

4 +

5 +. ./test-lib.sh

6 +

7 +HELPER="$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh"

8 +

9 +test_expect_success PIPE 'concurrent requests: one rewritten, one passed through, neither empty' '

10 + mkdir workdir fakebin &&

11 + ENTERED="$PWD/entered" &&

12 + GATE="$PWD/gate" &&

13 + export ENTERED GATE &&

14 + mkfifo "$ENTERED" "$GATE" &&

15 +

16 + # Stand in for git-http-backend. The modify role returns a response

17 + # containing "packfile", which the one-time script rewrites. The

18 + # passthrough role returns a response that is left untouched, but first

19 + # announces that it has entered the helper and then blocks, so that it

20 + # is still in flight when the modify role claims and removes the marker.

21 + write_script fakebin/git-http-backend <<-\EOF &&

22 + printf "Status: 200 OK\r\n"

23 + printf "Content-Type: application/x-git-result\r\n"

24 + printf "\r\n"

25 + if test "$ROLE" = modify

26 + then

27 + printf "packfile\n"

28 + else

29 + echo entered >"$ENTERED"

30 + read -r released <"$GATE"

31 + printf "refs\n"

32 + fi

33 + EOF

34 +

35 + # The transform that replace_packfile would install as one-time-script:

36 + # rewrite responses that contain "packfile", leave the rest alone.

37 + write_script workdir/one-time-script <<-\EOF &&

38 + if grep packfile "$1" >/dev/null

39 + then

40 + sed "/packfile/q" "$1" &&

41 + printf "REPLACED\n"

42 + else

43 + cat "$1"

44 + fi

45 + EOF

46 +

47 + GIT_EXEC_PATH="$PWD/fakebin" &&

48 + export GIT_EXEC_PATH &&

49 +

50 + # Hold GATE open read-write on fd 9 for the duration, so releasing the

51 + # passthrough request below cannot block even if that request has

52 + # already exited (it keeps a reader on the FIFO).

53 + exec 9<>"$GATE" &&

54 +

55 + # Launch the passthrough request in the background. It enters the

56 + # helper, signals us through ENTERED, then blocks on GATE inside the

57 + # fake backend. The braces keep the && chain intact while backgrounding

58 + # only the subshell, so "wait" can reap it by pid; kill it on any exit

59 + # so a stray blocked child cannot hold the test output open and stall a

60 + # reader such as prove.

61 + { (

62 + cd workdir &&

63 + ROLE=passthrough sh "$HELPER" >../passthrough.out 2>../passthrough.err

64 + ) & } &&

65 + passthrough_pid=$! &&

66 + test_when_finished "kill $passthrough_pid 2>/dev/null || :" &&

67 +

68 + # Wait until the passthrough request is past the marker check.

69 + read -r entered <"$ENTERED" &&

70 +

71 + # Run the modifying request to completion while the passthrough request

72 + # is still blocked.

73 + (

74 + cd workdir &&

75 + ROLE=modify sh "$HELPER" >../modify.out 2>../modify.err

76 + ) &&

77 +

78 + # Release the passthrough request and let it finish. Ignore the helper

79 + # exit status here so a broken helper is diagnosed by the assertions

80 + # below rather than aborting the test.

81 + echo released >&9 &&

82 + { wait "$passthrough_pid" || :; } &&

83 +

84 + # Neither request may error out or produce an empty (HTTP 500) body,

85 + # and each must have played its role: the modify request rewrote its

86 + # response and the passthrough request came through untouched.

87 + test_must_be_empty passthrough.err &&

88 + test_must_be_empty modify.err &&

89 + test_grep "Status: 200 OK" passthrough.out &&

90 + test_grep "Status: 200 OK" modify.out &&

91 + test_grep REPLACED modify.out &&

92 + test_grep ! REPLACED passthrough.out &&

93 + test_grep refs passthrough.out

94 +'

95 +

96 +test_done