fix(lsphp): correct the SIGPIPE explanation, drop the temp-dir boot precondition, close two scanner blind spots
Three non-blocking findings from the review of 8790b02. The fix itself is
unchanged in intent; this makes the reasoning around it true, removes a
regression the fix introduced on the boot path, and stops the new test from
under-reporting.
F1 — the shipped comments explained the bug wrongly, and a wrong rule is what
the next maintainer reasons from. entrypoint-lsphp.sh and
render-shared-ols-config.sh both said the race is decided by PIPE CAPACITY:
"while the output fits the pipe the writer always wins; once it doesn't, SIGPIPE
is guaranteed." Both halves are refuted by measurement against a default
65536-byte pipe (F_GETPIPE_SZ):
41144 bytes -> 141 in 32/300 runs (11%) — well UNDER capacity
65012 bytes -> 141 in 25/30 runs — not certain even AT capacity
500 KB into a 1 MiB pipe -> 200/200 with 4096-byte writes, 0/200 with one
500 KB write
and strace caught printf dying having written 12086 of 40406 bytes into a
65536-byte pipe. The mechanism is a race on whether the reader closes before the
writer's final write() returns; capacity only modulates how many syscalls the
writer needs. What actually separated whp02 (5/5 failures) from a dev container
(10/10 clean) is the WRITER's syscall size: bash <= 5.2.15 writes ~37 KB at a
time, bash >= 5.2.21 writes 80-160 bytes. The fs.pipe-user-pages-soft aside was
also wrong: it clamps to two pages not one, needs one uid holding >1024 pipes,
and is skipped for CAP_SYS_RESOURCE.
Both blocks now state the rule that is actually true — any
`writer | early-exiting-reader` under pipefail is a latent 141; payload size is
not a safety argument; the only sound reasons a call site is safe are structural
(no pipefail, reader provably reads to EOF, or the status is discarded) — and
the same correction is applied to the three other comments that leaned on size
(`ols_running` x2, fpm-parity-check.sh's pre-flight). Nor is the reader's
implementation a defence: at 248 KB, mawk, gawk, `grep -q` and `head -1` all
gave 141 on 10/10, and these images already differ (mawk 1.3.4 vs gawk 5.2.1).
Comment-only; the test file's own section-4 output no longer contradicts the
prose next to it.
F2 — `<<<` added a writable-temp-dir precondition to the boot path. Above a
build-dependent size bash materialises a here-string as /tmp/sh-thd.XXXXXX
(measured switch: 65536 in this image's bash 5.2.21, and Debian's 5.2.15
switches between 4096 and 16384, where a ~40 KB `lsphp -i` WOULD spill). On a
bare assignment a temp file it cannot create is `set -e` killing PID 1 — the
exact failure this branch exists to remove, re-acquired from a different
direction and gated on which bash the base image ships. In cac-lsphp:f1f2f3
under `docker run --read-only`, same payload, same statement shape:
OLD (here-string) : bash: cannot create temp file for here-document
-> exit 1, script dead
NEW (pure bash) : REACHED NEXT STATEMENT, SCAN=[…/mods-available/], exit 0
So the boot-critical sites — the three probe helpers in entrypoint-lsphp.sh and
the SCAN_DIR extraction in entrypoint-litespeed.sh — now match with `[[ ]]` and
parameter expansion, which allocate nothing. The non-boot sites keep their
here-strings and say why at the call site: `ols_running` in both OLS entrypoints
(`lswsctrl status` is under 100 bytes, orders below any spill threshold) and
fpm-parity-check.sh's `php-fpm -m` pre-flight (~1 KB, in a harness that has
already written a docroot and a pool config).
Matching semantics are preserved, not approximated: the anchored whole-line
grep becomes a glob over a subject wrapped in newlines at BOTH ends (so first
and unterminated-last lines still match), and awk's `-F'=> ' {print $2; exit}`
becomes first-matching-line then the text between the FIRST and SECOND
separator. Section 6 of the test asserts that against the original grep/awk
patterns reading a FILE — 24 cases incl. trailing-space, prefix decoys, CRLF,
a second separator, an empty value, two candidate lines, glob metacharacters in
the body, and the full 40 KB fixture. Mutations verify the assertions bite:
dropping the trailing-newline wrap fails 3 cases, taking the whole rest of the
line fails "second separator", `##` instead of `#` fails "first of two wins",
dropping the `^` anchor on the banner fails "banner not at line start".
F3 — the structural scan missed shapes it implied it caught, and the extractor
was unbounded.
* `grep -l`/`-L`/`--quiet`/`--files-with-matches`, `-im1`-style clusters, a
bare `head` before `;`, and `sed q` / `sed 'q'` / `sed 2q` / `sed '$q'` were
all invisible. grep is now walked option by option the way grep reads them
(so `grep -eq foo` stays the pattern "q", not --quiet), and the sed test
reads the script with quote characters stripped but their contents kept.
Replaying the old regexes against the new fixtures: 10 shapes missed and 2
false positives (`sed s/a/q/`, `grep -eq foo`) — both now correct.
* new section 7 pins that coverage from both sides: 19 early-exit shapes must
be reported, 18 read-to-EOF / quoted / non-pipeline forms must not. Without
it the scan's regexes are unfalsified and can quietly stop matching, which
is precisely how `grep -l` and `sed q` stayed missing.
* the helper extraction is bounded. It buffers and emits nothing until it has
seen the END marker (exit 4 = BEGIN without END, exit 3 = no markers), so a
half-deleted pair is a marker error instead of a slurp. Measured on this
entrypoint with the END marker removed: the old extractor produced 301 lines
including `mkdir -p "$SCAN_DIR"` and three `rm -f "$SCAN_DIR/…"` — which the
harness then sourced and ran. It failed loudly last time only because `set
-u` happened to trip two statements in. The new one emits 0 bytes and says
what is wrong.
* the stated scope limits now include what remains: the reader list is an
enumeration, not a proof (nothing knows about `perl -ne … last`, `jq`,
`head -c`), and only the first word after a pipe is inspected.
Verified: PHP 8.3 `--no-cache` build exit 0, 10/10 .phpt; cac-lsphp boots and
logs `path parity = extension` with `Rewriting => active` and .from/.to
populated from the rendered ini; cac-litespeed boots, resolves SCAN_DIR and
writes 99-user-error-log.ini, OLS reports "running with PID", /healthz 200. The
FPM parity harness — never executed by the previous review because no cac-fpm
image existed locally — was built (Dockerfile.fpm, PHPVER=83), the extension
compiled inside it, and it reports 9/9 ALL PASS, exit 0. The new test exits 0
here and exits 1 against a `git archive 9343a56` export naming all 9 offending
lines. `bash -n` clean repo-wide; `shellcheck -S warning` clean on the CI set;
`-S style` is byte-identical to before this commit (5 pre-existing info-level
findings, 0 added — the earlier report's claim of `-S style` clean was wrong).
No `.c`/`.h` file touched and the C fail-open invariant grep is still empty.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -67,19 +67,46 @@ fi
|
||||
## see lsphp's PHP errors in the exact same file on the new image.
|
||||
## Rendered as a tiny ini in lsphp's scan dir; PHP merges it after the
|
||||
## production-tuning overrides at startup.
|
||||
## Captured in two steps on purpose. As a single pipeline this was
|
||||
## Captured, then matched in the shell. As a single pipeline this was
|
||||
## `lsphp -i | awk '…{print;exit}'`: awk stops at the "Scan this dir" line,
|
||||
## which sits in the first few hundred bytes of ~40 KB of output, so lsphp can
|
||||
## still be writing when awk closes the pipe. It then dies 141, `set -o
|
||||
## pipefail` (line 12) makes that the pipeline's status, and because this is a
|
||||
## bare assignment `set -e` KILLS PID 1 — the container never starts, on a
|
||||
## machine where the race falls the wrong way. (Its twin in entrypoint-lsphp.sh
|
||||
## chose a degraded fallback instead; this one just exits.) Reading into a
|
||||
## variable first leaves awk's own status as the assignment's, and `|| true`
|
||||
## keeps a genuinely failing lsphp as an empty SCAN_DIR — which the `-n` test
|
||||
## below already handles — rather than as a boot failure.
|
||||
## which is near the top of the output, so lsphp can still be writing when awk
|
||||
## closes the pipe. lsphp then dies 141, `set -o pipefail` (line 12) makes that
|
||||
## the pipeline's status, and because this is a BARE ASSIGNMENT `set -e` KILLS
|
||||
## PID 1 — the container never starts. (Its twin in entrypoint-lsphp.sh chose a
|
||||
## degraded fallback instead; this one just exits.) That is a race on whether
|
||||
## the reader closes before the writer's last write() returns, not a function of
|
||||
## how big the output is: see the long note over the probe helpers in
|
||||
## entrypoint-lsphp.sh for the measurements. The rule is simply that no
|
||||
## `writer | early-exiting-reader` belongs in a pipefail script.
|
||||
##
|
||||
## A here-string would remove the pipeline, but bash spills a here-string to
|
||||
## /tmp/sh-thd.XXXXXX above a build-dependent size (65536 for the bash 5.2.21 in
|
||||
## this image, between 4096 and 16384 for Debian's 5.2.15) — and on this line,
|
||||
## a bare assignment, a temp file it cannot create is again `set -e` killing
|
||||
## PID 1: `docker run --read-only` reproduces exactly that. So the extraction is
|
||||
## done with parameter expansion, which allocates nothing.
|
||||
##
|
||||
## Same answer as the awk it replaces: first line starting "Scan this dir", then
|
||||
## the text between the FIRST and SECOND '=> ' on it (awk's $2 under -F'=> '),
|
||||
## empty if the line carries no separator, empty if there is no such line.
|
||||
## `|| true` on the capture keeps a genuinely failing lsphp as an empty
|
||||
## SCAN_DIR — which the `-n` test below already handles — not a boot failure.
|
||||
LSPHP_INFO=$(/usr/local/lsws/lsphp"${PHPVER}"/bin/lsphp -i 2>/dev/null || true)
|
||||
SCAN_DIR=$(awk -F'=> ' '/^Scan this dir/ {print $2; exit}' <<<"$LSPHP_INFO")
|
||||
SCAN_DIR=""
|
||||
## The leading newline is what makes a match on LINE 1 behave like every other
|
||||
## line, exactly as awk's `^` anchor does.
|
||||
scan_rest=$'\n'"$LSPHP_INFO"
|
||||
if [[ $scan_rest == *$'\nScan this dir'* ]]; then
|
||||
## `#` takes the SHORTEST prefix, i.e. the FIRST matching line — awk's `exit`.
|
||||
scan_rest=${scan_rest#*$'\nScan this dir'}
|
||||
scan_line="Scan this dir${scan_rest%%$'\n'*}"
|
||||
if [[ $scan_line == *'=> '* ]]; then
|
||||
SCAN_DIR=${scan_line#*'=> '}
|
||||
SCAN_DIR=${SCAN_DIR%%'=> '*}
|
||||
fi
|
||||
unset scan_line
|
||||
fi
|
||||
unset scan_rest
|
||||
if [ -n "$SCAN_DIR" ]; then
|
||||
cat > "$SCAN_DIR/99-user-error-log.ini" <<EOF
|
||||
; rendered at container start by entrypoint-litespeed.sh
|
||||
@@ -211,14 +238,24 @@ trap term_handler TERM INT
|
||||
## `grep -qi`: `grep -q` closes the pipe on its first match, and under the
|
||||
## `set -o pipefail` at the top of this file a writer that is still writing when
|
||||
## that happens dies 141 and the pipeline reports FALSE — i.e. "OLS is down"
|
||||
## precisely because the "running" line matched. (Same defect that shipped in
|
||||
## entrypoint-lsphp.sh's cac_path_parity probe.) `lswsctrl status` prints one
|
||||
## short line, so today it wins the race every time; the bound that makes that
|
||||
## true is a vendor script's output, not something this repo controls, and the
|
||||
## failure it would cause here — a spurious relaunch of a healthy OLS, five of
|
||||
## which trip the crash-loop cap and exit PID 1 — is expensive enough not to
|
||||
## rest on it. A non-zero `lswsctrl` still means "not running", exactly as
|
||||
## precisely because the "running" line matched, which here means a spurious
|
||||
## relaunch of a healthy OLS, five of which trip the crash-loop cap and exit
|
||||
## PID 1. (Same defect that shipped in entrypoint-lsphp.sh's cac_path_parity
|
||||
## probe.) The reason to change it is STRUCTURAL — a pipefail script must not
|
||||
## pipe into an early-exit reader, whatever the payload — because "lswsctrl
|
||||
## prints one short line so it always wins" is a size argument, and size
|
||||
## arguments about this race are wrong: see the measurements over the probe
|
||||
## helpers in entrypoint-lsphp.sh, where 41 KB SIGPIPEd 11% of the time into a
|
||||
## 64 KB pipe. A non-zero `lswsctrl` still means "not running", exactly as
|
||||
## pipefail made it mean before.
|
||||
##
|
||||
## A here-string is the right shape HERE, where those helpers use `[[ ]]`: the
|
||||
## reason to avoid `<<<` there is that bash spills a large here-string to
|
||||
## /tmp/sh-thd.XXXXXX and so makes a writable temp dir a boot precondition. The
|
||||
## threshold is 65536 bytes in this image's bash 5.2.21 and no lower than 4096
|
||||
## in any bash this repo has met; `lswsctrl status` prints well under 100 bytes
|
||||
## and cannot approach it, so no temp file is ever created and the case-
|
||||
## insensitive match stays a plain `grep -i` instead of a hand-rolled glob.
|
||||
ols_running() {
|
||||
local st
|
||||
st=$(/usr/local/lsws/bin/lswsctrl status 2>/dev/null) || return 1
|
||||
|
||||
Reference in New Issue
Block a user