From bbab1dfbc66268b6d53186e4016311dd0c08b693 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 01:20:21 -0700 Subject: [PATCH 1/4] fix(remote): keep curl's stderr when the request fails (#850) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_remote_http_post_json` sent curl's stderr to /dev/null and reported "000" for every kind of failure alike. The caller sees only that code, so when a request failed there was nothing anywhere saying why. That cost a Windows run a long time: connect returned a bare 000, and the reason — curl could not open a path embedded in its own config file — was in the stderr this line was discarding. Removing the discard is what made the other two fixes findable at all, which is why it is worth landing on its own rather than as a detail of them. Captured to a file rather than passed straight through, and shown only when curl actually failed: on the success path `curl -sS` is already silent, and an unconditional pass-through would put curl's output in the middle of a caller's. No behaviour change on the success path, and none for any caller reading the printed HTTP code. --- scripts/remote.sh | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index e17de5846..dd159711f 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -317,11 +317,23 @@ _remote_http_post_json() { printf 'max-filesize = "2097152"\n' printf 'data = "@%s"\n' "$body_file" } > "$cfg" - if curl_output=$(curl -sS -o "$out_file" -w '%{http_code}' -K "$cfg" 2>/dev/null); then + # Do not discard curl's stderr. On failure it is the only record of WHY, and + # the caller only ever sees the HTTP code — which this function reports as + # "000" for every kind of failure alike. A Windows run spent a long time on a + # bare 000 whose cause (curl could not open a path embedded in the config) + # was sitting in the stderr this line was throwing away. + # + # Captured rather than passed through, and shown only when curl actually + # failed: on the success path curl -sS is already silent, and a stray write + # to stderr here would land in the middle of a caller's output. + local curl_err; curl_err="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-err.XXXXXX")" + if curl_output=$(curl -sS -o "$out_file" -w '%{http_code}' -K "$cfg" 2>"$curl_err"); then : else curl_status=$? fi + [ "$curl_status" -ne 0 ] && [ -s "$curl_err" ] && cat "$curl_err" >&2 + rm -f "$curl_err" if [ "$curl_status" -ne 0 ]; then kill "$copier_pid" 2>/dev/null || true wait "$copier_pid" 2>/dev/null || true From adbf2b34d74f36152b9b4d0e66f7021c9540a8f0 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 16:29:23 -0700 Subject: [PATCH 2/4] test(remote): bind the kept stderr on the POST path (#850) The stop condition was that reverting the production change leaves CI green. It does not now: putting 2>/dev/null back turns the first test red. tests/test_remote_curl_stderr.bats, 5 ok / 0 not ok, driving the production _remote_http_post_json. The helper's stderr is captured to a FILE rather than read out of bats's $output, which merges the two streams. A test that cannot tell stdout from stderr cannot check that a message went to the right one, and "the message appears somewhere" is not what this fix is about -- the http code goes to stdout and the diagnosis has to go to stderr. failing curl the diagnosis reaches the caller's stderr succeeding curl nothing does, AND THE STUB WROTE SOMETHING ANYWAY both the http code is what it always was, 200 and 000 both no scratch file left in the run's own TMPDIR control the leftover check can see a planted leftover The second case is the one worth explaining. Real curl -sS is quiet on success, so a stub that also stayed quiet would pass against a version that dumped stderr unconditionally -- and that version drops noise into the middle of a caller's output. Making the stub noisy on the success path is what tells "shown only when curl failed" apart from "the stream was empty". The last is the control on an absence. A glob that matches nothing looks exactly like a glob aimed at the wrong directory, so one gets planted and the same check has to fire. Each run gets a private TMPDIR, so "what is left behind" is a question about this call and not about the machine. M0 no mutation 0 red M1 stderr discarded again 1 -- the diagnosis reaches stderr M2 stderr shown on success too 1 -- success stays quiet M3 the error file never removed 1 -- no scratch left behind M4 failure reports curl's exit code 3 -- the code contract, and the two cases that read it NOT COVERED, and I would rather name it than let the matrix imply otherwise: the scratch file is removed on the two normal paths, and the trap set earlier in the function does not name it -- its list is the config and the fifo. The signal path is therefore unmeasured here. I tried to drive one and the probe hung on this host (the bounded copier keeps the run alive while curl is being waited on), so what I have is a reading of the trap's text, not a measurement. Adding curl_err to that list would be one line, but it is a change to the fix rather than a test, so it is the author's call and not mine. Production untouched: the diff is this one test file. Windows unverified by me. --- tests/test_remote_curl_stderr.bats | 149 +++++++++++++++++++++++++++++ 1 file changed, 149 insertions(+) create mode 100644 tests/test_remote_curl_stderr.bats diff --git a/tests/test_remote_curl_stderr.bats b/tests/test_remote_curl_stderr.bats new file mode 100644 index 000000000..e5b0e1f19 --- /dev/null +++ b/tests/test_remote_curl_stderr.bats @@ -0,0 +1,149 @@ +#!/usr/bin/env bats +# WHEN THE ONLY THING A CALLER SEES IS "000", THROWING AWAY curl's STDERR IS +# THROWING AWAY THE DIAGNOSIS (#850). +# +# `_remote_http_post_json` reports `000` for every kind of failure alike: a +# refused connection, a timeout, a path curl could not open. The reason existed +# each time -- curl wrote it to stderr -- and `2>/dev/null` discarded it. A +# Windows run spent an afternoon on a bare `000` whose cause was in that stream. +# +# WHAT HAS TO HOLD, and each is its own case here: +# +# on failure the diagnosis reaches the caller's stderr +# on success nothing does, even if curl wrote something -- the show is +# gated on curl having FAILED, not on the stream being empty +# either way the http code is exactly what it was before +# either way no scratch file is left behind +# +# The stderr of the helper is captured to a FILE rather than read from bats's +# `$output`, which merges the two streams: a test that cannot tell stdout from +# stderr cannot check that a message went to the right one, and "the message +# appears somewhere" is what this fix is not about. + +load test_helper + +SANDBOX_TOOLS=(bash dirname mktemp mkfifo chmod rm rmdir sed cp cat grep python3 uname) + +setup() { + setup_test_env + + STUB_SRC="$BATS_TEST_TMPDIR/stubs" + mkdir -p "$STUB_SRC" + + # A curl whose behaviour the test dictates: STUB_CURL_MODE says whether it + # succeeds, and STUB_CURL_STDERR is written to stderr either way. Writing on + # the success path too is the point of one of the cases below -- it is how + # "shown only when curl failed" is told apart from "the stream was empty". + cat > "$STUB_SRC/curl" <<'STUB' +#!/usr/bin/env bash +set -u +cfg=""; out=""; prev="" +for arg in "$@"; do + case "$prev" in + -K) cfg="$arg" ;; + -o) out="$arg" ;; + esac + prev="$arg" +done +[ -z "${STUB_CURL_STDERR:-}" ] || printf '%s\n' "$STUB_CURL_STDERR" >&2 +if [ "${STUB_CURL_MODE:-ok}" = "fail" ]; then + # A real curl that cannot open a config path exits non-zero and writes + # nothing to the output file. Exit 26 is curl's "read error". + exit 26 +fi +hdr="$(sed -n 's/^dump-header = "\(.*\)"$/\1/p' "$cfg")" +[ -z "$hdr" ] || printf 'HTTP/1.1 200 OK\r\n\r\n' > "$hdr" +[ -z "$out" ] || printf '{"ok":true}' > "$out" +printf '200' +STUB + chmod +x "$STUB_SRC/curl" +} + +teardown() { teardown_test_env; } + +sandbox_path() { + local dir tool src + dir="$(mktemp -d "$BATS_TEST_TMPDIR/sandbox.XXXXXX")" + for tool in "${SANDBOX_TOOLS[@]}"; do + src="$(command -v "$tool")" || { echo "host lacks $tool" >&2; return 1; } + ln -s "$src" "$dir/$tool" + done + ln -s "$STUB_SRC/curl" "$dir/curl" + printf '%s' "$dir" +} + +# Runs the helper with its own TMPDIR, so "what scratch files remain" is a +# question about this call and not about everything else on the machine. +# stdout (the http code) lands in $output; stderr lands in $ERR_FILE. +post_with_curl() { + local mode="$1" stderr_text="$2" + RUN_TMPDIR="$(mktemp -d "$BATS_TEST_TMPDIR/run.XXXXXX")" + ERR_FILE="$BATS_TEST_TMPDIR/helper-stderr" + local bin; bin="$(sandbox_path)" + local body="$RUN_TMPDIR/body.json" + printf '{"t":"secret"}' > "$body" + + run env PATH="$bin" TMPDIR="$RUN_TMPDIR" STUB_CURL_MODE="$mode" \ + STUB_CURL_STDERR="$stderr_text" bash -c ' + set -uo pipefail + . '"$SCRIPTS"'/remote.sh 2>/dev/null + _remote_http_post_json "https://example.invalid/v1/x" "'"$body"'" \ + "'"$RUN_TMPDIR"'/out-body" "'"$RUN_TMPDIR"'/out-header" 2>"'"$ERR_FILE"'" + ' +} + +@test "a failing curl's diagnosis reaches the caller's stderr (#850)" { + # The whole point. Without this the operator has "000" and nothing else, and + # the reason they need is written down and then deleted. + post_with_curl fail "curl: (26) Failed to open/read local data from file" + [ "$status" -eq 0 ] + [ "$output" = "000" ] + + grep -q 'Failed to open/read local data' "$ERR_FILE" +} + +@test "a successful curl's stderr is NOT shown, even when it wrote something (#850)" { + # Distinguishes "shown only when curl failed" from "the stream happened to be + # empty". curl -sS is quiet on success, so a test that let it stay quiet here + # would pass against a version that dumped stderr unconditionally -- and that + # version would drop noise into the middle of a caller's output. + post_with_curl ok "a progress line nobody asked for" + [ "$status" -eq 0 ] + [ "$output" = "200" ] + + [ ! -s "$ERR_FILE" ] +} + +@test "the http code is unchanged on both paths (#850)" { + # The contract this must not have altered while adding the diagnosis. + post_with_curl ok "" + [ "$output" = "200" ] + + post_with_curl fail "curl: (7) Failed to connect" + [ "$output" = "000" ] +} + +@test "no scratch file is left behind, on either path (#850)" { + # The helper writes a config, a fifo directory and now an error file. All of + # them are removed on the normal paths; this asserts it for the run's own + # TMPDIR, so nothing else on the machine can make the check pass or fail. + post_with_curl ok "" + [ "$output" = "200" ] + refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null + refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null + + post_with_curl fail "curl: (7) Failed to connect" + [ "$output" = "000" ] + refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null + refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null +} + +@test "the leftover check can see a leftover when there is one (#850)" { + # Control on the assertion above, which is an absence: a glob that matches + # nothing looks exactly like a glob pointed at the wrong directory. Plant one + # and confirm the same check fires. + post_with_curl ok "" + : > "$RUN_TMPDIR/agmsg-curl-err.planted" + run ls "$RUN_TMPDIR"/agmsg-curl-err.* + [ "$status" -eq 0 ] +} From d59284c11e38c244615b9324347bed955f4c8236 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 17:20:24 -0700 Subject: [PATCH 3/4] fix(remote): the EXIT trap never cleaned up an early exit, for any of its files (#850) Review asked for one line: put curl_err in the existing trap so an early exit cannot strand it. I wrote that line, added a test for it, and the test failed. WHAT THE MEASUREMENT SAID. An EXIT trap set inside a function runs after that function's frame is gone. A single-quoted trap body therefore expands $cfg in the CALLER's scope, where no local of that name exists -- so it removes the empty string, returns 0, and reads as a cleanup that worked. bash 3.2.57 TRAP SEES: [EMPTY] bash 5.3.15 TRAP SEES: [EMPTY] So this was never about curl_err. The pre-existing trap has never swept anything on an early exit. Driven with a stub curl that fails after writing its headers, and a cat that fails so errexit leaves the function between the mktemp and the tail cleanup, a real run left all three behind: agmsg-curl-cfg.cn24V2 a 0600 config naming the request body agmsg-curl-err.cJJHwm agmsg-header-pipe.2u0Xbp The config is the one that matters. It is the file this helper exists to keep out of curl's argv. THE FIX. Bake the paths into the trap at set time with printf %q instead of expanding them when it fires, and create curl_err with the other temporaries so it exists before the trap that must remove it. Same run afterwards: nothing left. This is more than the one line review asked for, and the reason is that the one line does not work. Reverting to it would leave a test failing and the leak open. If the author prefers a different shape -- non-local variables, or re-arming the trap after each mktemp -- the tests here bind the behaviour, not the mechanism. TESTS. tests/test_remote_curl_stderr.bats, 7 ok / 0 not ok. failing curl the diagnosis reaches the caller's stderr succeeding curl nothing does, and the stub wrote to stderr anyway both the http code is unchanged, 200 and 000 both no scratch file left in the run's own TMPDIR early exit errexit out of the middle sweeps ALL THREE files premise an EXIT trap cannot read the locals of the function that set it -- measured, so that the printf %q baking is not read as ceremony and simplified back control the leftover check fires on a planted leftover The early-exit case took three attempts and each failure is worth naming: 1 A signal. The probe hung: on that path the bounded copier is never reaped, so it outlives the shell holding the inherited stdout, and bats's capture waits on it. Review suggested errexit instead, which is bounded. 2 Writing the fake `cat` through the sandbox symlink. `>` follows a symlink, so it wrote to the system's own /bin/cat -- refused by the OS here. On a machine where it was not refused, the test would have replaced a system binary. The symlink is removed first now. 3 A failure stub that exited without writing the headers, which strands the copier on open() and hangs the run for the same reason as (1). The stub now writes headers and then fails, which is also what a real curl does when it dies on the body. Existing suite unchanged: tests/test_remote.bats 122 ok / 0 not ok. Windows unverified by me. --- scripts/remote.sh | 25 ++++++++-- tests/test_remote_curl_stderr.bats | 77 ++++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index dd159711f..f82cac8b1 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -291,13 +291,33 @@ cmd_doctor() { # config file is 0600 and removed immediately after the call. _remote_http_post_json() { local url="$1" body_file="$2" out_file="$3" header_file="$4" cfg http_code \ - fifo_dir header_fifo copier_pid curl_output curl_status=0 + fifo_dir header_fifo copier_pid curl_output curl_status=0 curl_err cfg="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-cfg.XXXXXX")" fifo_dir="$(mktemp -d "${TMPDIR:-/tmp}/agmsg-header-pipe.XXXXXX")" header_fifo="$fifo_dir/header" + # Made here rather than beside curl, so everything this function creates + # exists before the trap that has to remove it. + curl_err="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-err.XXXXXX")" mkfifo "$header_fifo" chmod 600 "$cfg" - trap 'rm -f "$cfg" "$header_fifo"; rmdir "$fifo_dir" 2>/dev/null || true' EXIT INT TERM + # THE PATHS ARE BAKED IN, NOT EXPANDED WHEN THE TRAP FIRES. + # + # An EXIT trap set inside a function runs after that function's frame is + # gone, so a single-quoted body expands `$cfg` in the CALLER's scope, where a + # local of that name does not exist. It removes "" and returns 0, and the + # cleanup reads as working. Measured both ways on bash 3.2.57 and 5.3.15: a + # local is EMPTY inside an EXIT trap fired by errexit from within the + # function. + # + # That was already true of `cfg`, `header_fifo` and `fifo_dir` before this + # change, so the pre-existing trap has never cleaned up an early exit -- a + # measured run left all three behind, including a 0600 config naming the + # request body. `printf %q` fixes the values at set time and survives a + # TMPDIR with spaces in it. + # + # The explicit cleanup at the tail stays: it is what runs on the normal + # paths, and it runs before `trap -` clears this. + trap "rm -f $(printf '%q %q %q' "$cfg" "$header_fifo" "$curl_err"); rmdir $(printf '%q' "$fifo_dir") 2>/dev/null || true" EXIT INT TERM # Reaped on both normal paths below (waited on success, killed and waited on # failure), so this is short-lived by construction -- but the EXIT trap only # removes files, it does not kill the copier. A signal arriving before curl @@ -326,7 +346,6 @@ _remote_http_post_json() { # Captured rather than passed through, and shown only when curl actually # failed: on the success path curl -sS is already silent, and a stray write # to stderr here would land in the middle of a caller's output. - local curl_err; curl_err="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-err.XXXXXX")" if curl_output=$(curl -sS -o "$out_file" -w '%{http_code}' -K "$cfg" 2>"$curl_err"); then : else diff --git a/tests/test_remote_curl_stderr.bats b/tests/test_remote_curl_stderr.bats index e5b0e1f19..edd15b23f 100644 --- a/tests/test_remote_curl_stderr.bats +++ b/tests/test_remote_curl_stderr.bats @@ -46,12 +46,16 @@ for arg in "$@"; do prev="$arg" done [ -z "${STUB_CURL_STDERR:-}" ] || printf '%s\n' "$STUB_CURL_STDERR" >&2 +hdr="$(sed -n 's/^dump-header = "\(.*\)"$/\1/p' "$cfg")" if [ "${STUB_CURL_MODE:-ok}" = "fail" ]; then - # A real curl that cannot open a config path exits non-zero and writes - # nothing to the output file. Exit 26 is curl's "read error". - exit 26 + # A failure AFTER the headers were written -- curl exceeding max-filesize on + # the body, say. The headers matter here: leaving the fifo without a writer + # strands the bounded copier on open(), and everything downstream of that + # waits on a process that will never finish. That is a real property of the + # failure path, and driving it is a different experiment from this one. + [ -z "$hdr" ] || printf 'HTTP/1.1 200 OK\r\n\r\n' > "$hdr" + exit 63 fi -hdr="$(sed -n 's/^dump-header = "\(.*\)"$/\1/p' "$cfg")" [ -z "$hdr" ] || printf 'HTTP/1.1 200 OK\r\n\r\n' > "$hdr" [ -z "$out" ] || printf '{"ok":true}' > "$out" printf '200' @@ -138,6 +142,71 @@ post_with_curl() { refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null } +@test "an early exit between the mktemp and the cleanup still sweeps the file (#850)" { + # The hole the explicit cleanup cannot cover: it only runs if the function + # GETS there. A signal is the obvious way out early and it is also the one I + # could not drive -- the probe hung, because the bounded copier keeps the run + # alive while curl is being waited on. A review pointed out that the signal is + # not the only exit, and errexit is a bounded one. + # + # `cat` is the last command of the `&& &&` chain that shows the diagnosis, so + # under `set -e` a failing cat leaves the function immediately -- after the + # mktemp, before the rm. Everything that survives that has to come from the + # trap, which is exactly the property under test. + RUN_TMPDIR="$(mktemp -d "$BATS_TEST_TMPDIR/run.XXXXXX")" + local bin; bin="$(sandbox_path)" + local body="$RUN_TMPDIR/body.json" + printf '{"t":"secret"}' > "$body" + + # A `cat` that always fails, shadowing the real one for this run only. The + # symlink is REMOVED first: `>` through a symlink writes to its target, which + # here is the system's own /bin/cat. The first attempt did exactly that and + # was refused by the OS -- on a machine where it was not refused, this test + # would have replaced a system binary. + rm -f "$bin/cat" + printf '#!/usr/bin/env bash\nexit 1\n' > "$bin/cat" + chmod +x "$bin/cat" + + # Output goes to FILES, not to bats's capture pipe. On this path the bounded + # copier is never reaped -- the trap removes files and does not kill it -- so + # it outlives the shell still holding the inherited stdout and stderr. Those + # being a pipe is what makes `run` wait forever; those being files is what + # makes this test finish. The orphan is a real property of the early-exit + # path and is reported alongside this test rather than papered over. + run env PATH="$bin" TMPDIR="$RUN_TMPDIR" STUB_CURL_MODE=fail \ + STUB_CURL_STDERR="curl: (26) Failed to open/read local data" bash -c ' + set -euo pipefail + . '"$SCRIPTS"'/remote.sh 2>/dev/null + _remote_http_post_json "https://example.invalid/v1/x" "'"$body"'" \ + "'"$RUN_TMPDIR"'/out-body" "'"$RUN_TMPDIR"'/out-header" + ' >"$RUN_TMPDIR/driver-stdout" 2>"$RUN_TMPDIR/driver-stderr" + # It leaves early: the function never reaches its `printf` of the code. + [ "$status" -ne 0 ] + [ ! -s "$RUN_TMPDIR/driver-stdout" ] + + # And nothing is stranded. This is the trap's work, not the tail's -- and all + # three are asserted, because measuring this path is what showed the trap had + # never swept ANY of them. It expanded function locals after the frame was + # gone, removed empty strings, and returned 0. + refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null + refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null + refute ls -d "$RUN_TMPDIR"/agmsg-header-pipe.* 2>/dev/null +} + +@test "an EXIT trap cannot read the locals of the function that set it (#850)" { + # The premise the trap's shape rests on, measured here rather than asserted + # in a comment. If a future bash made locals visible to an EXIT trap, the + # `printf %q` baking would look like pointless ceremony and someone would + # simplify it back into a single-quoted body -- reopening the leak. This test + # is what tells them the ceremony is load-bearing. + run bash -c ' + f() { local v="hello"; trap '"'"'printf "TRAP_SEES=[%s]\n" "${v:-EMPTY}"'"'"' EXIT; false; } + set -e + f + ' + [ "$output" = "TRAP_SEES=[EMPTY]" ] +} + @test "the leftover check can see a leftover when there is one (#850)" { # Control on the assertion above, which is an absence: a glob that matches # nothing looks exactly like a glob pointed at the wrong directory. Plant one From a38d6f8e1430325f1bbf5537348fbd9edeffb6d3 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 17:46:12 -0700 Subject: [PATCH 4/4] fix(remote): one directory, armed before anything else exists (#850) Review's remaining point: whatever is created BEFORE the trap is armed is unprotected, and the previous shape kept that window -- it made three things and then armed a trap over all three. A failure at the second or third leaves the first stranded, including a 0600 config naming the request body. The window cannot be closed by reordering, because there is always a first allocation. So there is now only one: work_dir="$(mktemp -d ...)" trap "rm -rf " EXIT INT TERM cfg="$work_dir/config"; curl_err="$work_dir/stderr"; header_fifo="$work_dir/header" Everything else is made inside a directory that is already condemned, the normal path removes the same directory, and cleanup is one rm -rf on a path this function minted. The printf %q baking stays for the reason measured earlier: an EXIT trap runs after the function's frame is gone and cannot expand its locals. New case: make mkfifo fail, which happens after the directory exists and after the config is inside it, and assert nothing survives. AND THE ABSENCE ASSERTIONS WERE AIMED AT NAMES THAT NO LONGER EXIST. The leftover checks globbed agmsg-curl-cfg.* and agmsg-curl-err.*, which this layout never creates. They would have passed on any behaviour at all, and gone on passing while the new directory leaked. Both they and their planted-leftover control now use the name the helper really mints. An absence assertion pointed at a name nothing uses is indistinguishable from a clean run -- and I wrote four of them without noticing the rename underneath. 8 ok / 0 not ok tests/test_remote_curl_stderr.bats 122 ok / 0 not ok tests/test_remote.bats, unchanged Separately, and NOT part of this change: on the failure path the bounded copier is not reaped when the function leaves early, so it can outlive the shell holding inherited descriptors. That predates the stderr work and is filed on its own rather than folded in here. Windows unverified by me. --- scripts/remote.sh | 60 ++++++++++++++++-------------- tests/test_remote_curl_stderr.bats | 57 +++++++++++++++++++++++----- 2 files changed, 80 insertions(+), 37 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index f82cac8b1..f094b73c8 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -291,33 +291,39 @@ cmd_doctor() { # config file is 0600 and removed immediately after the call. _remote_http_post_json() { local url="$1" body_file="$2" out_file="$3" header_file="$4" cfg http_code \ - fifo_dir header_fifo copier_pid curl_output curl_status=0 curl_err - cfg="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-cfg.XXXXXX")" - fifo_dir="$(mktemp -d "${TMPDIR:-/tmp}/agmsg-header-pipe.XXXXXX")" - header_fifo="$fifo_dir/header" - # Made here rather than beside curl, so everything this function creates - # exists before the trap that has to remove it. - curl_err="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-err.XXXXXX")" - mkfifo "$header_fifo" - chmod 600 "$cfg" - # THE PATHS ARE BAKED IN, NOT EXPANDED WHEN THE TRAP FIRES. + work_dir header_fifo copier_pid curl_output curl_status=0 curl_err + # ONE ALLOCATION BEFORE THE TRAP, AND EVERYTHING ELSE INSIDE IT. # - # An EXIT trap set inside a function runs after that function's frame is - # gone, so a single-quoted body expands `$cfg` in the CALLER's scope, where a - # local of that name does not exist. It removes "" and returns 0, and the - # cleanup reads as working. Measured both ways on bash 3.2.57 and 5.3.15: a - # local is EMPTY inside an EXIT trap fired by errexit from within the - # function. + # Two separate things go wrong with the obvious ordering, and this shape is + # the smallest one that closes both. # - # That was already true of `cfg`, `header_fifo` and `fifo_dir` before this - # change, so the pre-existing trap has never cleaned up an early exit -- a - # measured run left all three behind, including a 0600 config naming the - # request body. `printf %q` fixes the values at set time and survives a - # TMPDIR with spaces in it. + # First, a trap cannot expand what it cannot see. An EXIT trap set inside a + # function runs after that function's frame is gone, so a single-quoted body + # expands `$cfg` in the CALLER's scope, where no local of that name exists. + # It removes "" and returns 0, so the cleanup reads as working. Measured on + # bash 3.2.57 and 5.3.15: a local is EMPTY inside an EXIT trap fired by + # errexit from within the function. `printf %q` fixes the value at set time + # and survives a TMPDIR containing spaces. # - # The explicit cleanup at the tail stays: it is what runs on the normal - # paths, and it runs before `trap -` clears this. - trap "rm -f $(printf '%q %q %q' "$cfg" "$header_fifo" "$curl_err"); rmdir $(printf '%q' "$fifo_dir") 2>/dev/null || true" EXIT INT TERM + # Second, anything created BEFORE the trap is armed is unprotected. Creating + # the config, then a directory, then an error file, and only then arming the + # trap leaves a window where the second or third allocation fails and the + # first is stranded -- including a 0600 config naming the request body. The + # window cannot be closed by ordering alone, because there is always a first + # allocation. + # + # So: acquire ONE directory, arm the trap on it immediately, and create + # everything else inside. A failure at any later step leaves nothing outside + # a directory that is already condemned. `rm -rf` on a path this function + # minted is the whole cleanup. + work_dir="$(mktemp -d "${TMPDIR:-/tmp}/agmsg-curl.XXXXXX")" + trap "rm -rf $(printf '%q' "$work_dir")" EXIT INT TERM + cfg="$work_dir/config" + curl_err="$work_dir/stderr" + header_fifo="$work_dir/header" + : > "$cfg" + chmod 600 "$cfg" + mkfifo "$header_fifo" # Reaped on both normal paths below (waited on success, killed and waited on # failure), so this is short-lived by construction -- but the EXIT trap only # removes files, it does not kill the copier. A signal arriving before curl @@ -352,7 +358,6 @@ _remote_http_post_json() { curl_status=$? fi [ "$curl_status" -ne 0 ] && [ -s "$curl_err" ] && cat "$curl_err" >&2 - rm -f "$curl_err" if [ "$curl_status" -ne 0 ]; then kill "$copier_pid" 2>/dev/null || true wait "$copier_pid" 2>/dev/null || true @@ -362,8 +367,9 @@ _remote_http_post_json() { else http_code="000" fi - rm -f "$cfg" "$header_fifo" - rmdir "$fifo_dir" 2>/dev/null || true + # One directory holds the config, the error file and the fifo, so the normal + # path removes exactly what the trap would have. + rm -rf "$work_dir" trap - EXIT INT TERM printf '%s' "$http_code" } diff --git a/tests/test_remote_curl_stderr.bats b/tests/test_remote_curl_stderr.bats index edd15b23f..6bf3aaa8a 100644 --- a/tests/test_remote_curl_stderr.bats +++ b/tests/test_remote_curl_stderr.bats @@ -128,18 +128,21 @@ post_with_curl() { } @test "no scratch file is left behind, on either path (#850)" { - # The helper writes a config, a fifo directory and now an error file. All of - # them are removed on the normal paths; this asserts it for the run's own - # TMPDIR, so nothing else on the machine can make the check pass or fail. + # The config, the error file and the fifo now live in one directory the helper + # mints, so this is one glob rather than three. + # + # THE NAME MATTERS AND ALMOST GOT THIS WRONG. An earlier version of this test + # globbed agmsg-curl-cfg.* and agmsg-curl-err.*, which the new layout never + # creates -- the check would have passed on any behaviour whatsoever, and gone + # on passing if the directory leaked. An absence assertion aimed at a name + # nothing uses is indistinguishable from a clean run. post_with_curl ok "" [ "$output" = "200" ] - refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null - refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null + refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null post_with_curl fail "curl: (7) Failed to connect" [ "$output" = "000" ] - refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null - refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null + refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null } @test "an early exit between the mktemp and the cleanup still sweeps the file (#850)" { @@ -190,7 +193,39 @@ post_with_curl() { # gone, removed empty strings, and returned 0. refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null - refute ls -d "$RUN_TMPDIR"/agmsg-header-pipe.* 2>/dev/null + refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null +} + +@test "a failure while setting up leaves nothing behind either (#850)" { + # The window the previous shape could not close: anything created BEFORE the + # trap is armed is unprotected, and there is always a first allocation. The + # answer is that there is now only ONE allocation before the trap, and + # everything else is made inside it. + # + # Driven by making `mkfifo` fail, which happens after the directory exists and + # after the config has been written into it. Under `set -e` that leaves the + # function immediately -- before curl, before any cleanup the tail would do. + RUN_TMPDIR="$(mktemp -d "$BATS_TEST_TMPDIR/run.XXXXXX")" + local bin; bin="$(sandbox_path)" + local body="$RUN_TMPDIR/body.json" + printf '{"t":"secret"}' > "$body" + + rm -f "$bin/mkfifo" + printf '#!/usr/bin/env bash\nexit 1\n' > "$bin/mkfifo" + chmod +x "$bin/mkfifo" + + run env PATH="$bin" TMPDIR="$RUN_TMPDIR" bash -c ' + set -euo pipefail + . '"$SCRIPTS"'/remote.sh 2>/dev/null + _remote_http_post_json "https://example.invalid/v1/x" "'"$body"'" \ + "'"$RUN_TMPDIR"'/out-body" "'"$RUN_TMPDIR"'/out-header" + ' + [ "$status" -ne 0 ] + + # Nothing survives: not the directory, and so not the config inside it. The + # config is the file that matters -- it is what this helper exists to keep + # out of curl's argv, and a stranded copy names the request body. + refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null } @test "an EXIT trap cannot read the locals of the function that set it (#850)" { @@ -211,8 +246,10 @@ post_with_curl() { # Control on the assertion above, which is an absence: a glob that matches # nothing looks exactly like a glob pointed at the wrong directory. Plant one # and confirm the same check fires. + # Planted under the name the helper really uses, so this controls the glob + # that the absence assertions actually run. post_with_curl ok "" - : > "$RUN_TMPDIR/agmsg-curl-err.planted" - run ls "$RUN_TMPDIR"/agmsg-curl-err.* + mkdir -p "$RUN_TMPDIR/agmsg-curl.planted" + run ls -d "$RUN_TMPDIR"/agmsg-curl.* [ "$status" -eq 0 ] }