From e28c3dcce404f76fdd30e23490f3f7921006bc9d Mon Sep 17 00:00:00 2001 From: jknapp Date: Wed, 5 Aug 2026 16:03:08 -0700 Subject: [PATCH] fix(lsphp): correct the SIGPIPE explanation, drop the temp-dir boot precondition, close two scanner blind spots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- ext/cac-path-parity/tests/fpm-parity-check.sh | 17 +- scripts/entrypoint-litespeed.sh | 73 +++- scripts/entrypoint-lsphp.sh | 117 +++++-- scripts/entrypoint-shared-ols.sh | 8 +- scripts/render-shared-ols-config.sh | 29 +- scripts/tests/lsphp-info-probe.test.sh | 312 ++++++++++++++++-- 6 files changed, 467 insertions(+), 89 deletions(-) diff --git a/ext/cac-path-parity/tests/fpm-parity-check.sh b/ext/cac-path-parity/tests/fpm-parity-check.sh index cfa1f20..c4974e6 100755 --- a/ext/cac-path-parity/tests/fpm-parity-check.sh +++ b/ext/cac-path-parity/tests/fpm-parity-check.sh @@ -76,10 +76,19 @@ echo "extension: $EXT_SO" ## `grep -qx`. `grep -q` exits on its first match, and with `set -o pipefail` ## (line 39) a writer still writing at that moment dies 141 and the pipeline ## reads FALSE — announcing "cannot load the extension" *because* the extension -## was listed. `php-fpm -m` is ~1 KB and loses that race only rarely, but this -## pre-flight exists precisely to stop a harness malfunction being reported as -## an extension fault, so it must not have one of its own. (The same construct -## on 40 KB of `lsphp -i` is what broke entrypoint-lsphp.sh in production.) +## was listed. The reason to change it is structural, not that `php-fpm -m` is +## small: there is no payload size that makes this shape safe (41 KB SIGPIPEs +## about 11% of the time into a 64 KB pipe — see the note over the probe helpers +## in scripts/entrypoint-lsphp.sh), and this pre-flight exists precisely to stop +## a harness malfunction being reported as an extension fault, so it must not +## have one of its own. (The same construct on 40 KB of `lsphp -i` is what broke +## entrypoint-lsphp.sh in production.) +## +## A here-string, not the `[[ ]]` form those helpers use, on purpose: `<<<` +## spills to /tmp/sh-thd.XXXXXX above ~4-64 KB depending on the bash build, so +## it is a writable-temp-dir precondition, which is unacceptable on a boot path +## and irrelevant here — `php-fpm -m` is ~1 KB, and this harness has already +## created a docroot and a pool config by the time it runs. FPM_MODULES=$("$FPM_BIN" -n -d "extension=$EXT_SO" -m 2>/dev/null || true) if ! grep -qx 'cac_path_parity' <<<"$FPM_MODULES"; then echo "HARNESS FAILURE: $FPM_BIN cannot load $EXT_SO" >&2 diff --git a/scripts/entrypoint-litespeed.sh b/scripts/entrypoint-litespeed.sh index 88873bd..cfa3753 100644 --- a/scripts/entrypoint-litespeed.sh +++ b/scripts/entrypoint-litespeed.sh @@ -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" </dev/null) || return 1 diff --git a/scripts/entrypoint-lsphp.sh b/scripts/entrypoint-lsphp.sh index 1b17be7..ff45d47 100644 --- a/scripts/entrypoint-lsphp.sh +++ b/scripts/entrypoint-lsphp.sh @@ -178,44 +178,103 @@ validate_ini_num() { ## Everything between these two markers is extracted verbatim and executed by ## scripts/tests/lsphp-info-probe.test.sh — the markers are inert comments with ## no runtime effect, and they exist so the test exercises THE SHIPPED CODE -## rather than a copy of it that can drift away from it. +## rather than a copy of it that can drift away from it. Keep BOTH markers: the +## extractor refuses to emit anything unless it sees the END one, so a half- +## deleted pair is reported there as a marker error instead of silently +## sourcing the rest of this file. ## -## WHY THESE READ `$1` FROM A HERE-STRING AND NOT A PIPELINE. Both probes used -## to be `printf '%s\n' "$LSPHP_INFO" | `. `lsphp -i` is ~40 KB and both -## readers stop early — `grep -q` on first match, `awk` at `exit` — so the -## reader can close the pipe while printf is still writing to it. printf then -## takes SIGPIPE and dies 141, `set -o pipefail` (line 34) adopts 141 as the -## PIPELINE's status, and the test reads FALSE **because the thing it was +## WHY THESE MATCH `$1` IN THE SHELL AND NEVER PIPE IT INTO A READER. +## +## What broke. Both probes used to be `printf '%s\n' "$LSPHP_INFO" | `, +## and both readers stop early — `grep -q` at its first match, `awk` at `exit`. +## When the reader closes the pipe with the writer still writing, the writer +## takes SIGPIPE and dies 141; `set -o pipefail` (line 34) adopts 141 as the +## PIPELINE's status; and the branch reads FALSE **because the thing it was ## looking for was present early enough to stop the reader**. Measured on whp02 ## against the published cac-lsphp:php83: 5/5 runs status=141 with pipefail, ## 0 without. ## -## It reproduces on some hosts and not others, and the reason is the PIPE -## CAPACITY, not the payload alone. While the writer's whole output fits in the -## pipe it never blocks and always finishes before the reader can act; once it -## does not fit, the early exit is a guaranteed SIGPIPE. Linux gives a pipe -## 64 KiB by default — 40 KB fits, which is why this same image measured 0/10 -## on the build host here — but drops NEW pipes to a single page once a user -## passes fs.pipe-user-pages-soft, which is the state a busy production host -## lives in. Forcing the payload over the limit makes it deterministic -## everywhere: 3x this output = 122100 bytes gave 141 141 141 in this very -## image. "It worked when I ran it" was never evidence about this bug. +## THE RULE, and it is not about size. ANY `writer | early-exiting-reader` +## under pipefail is a latent 141. PAYLOAD SIZE IS NOT A SAFETY ARGUMENT. Each +## run is decided by a race — whether the reader's close lands before the +## writer's final write() returns — and the payload only sets how many write() +## syscalls the writer has to lose. Measured here against a default +## 65536-byte pipe (confirmed with F_GETPIPE_SZ): +## 41144 bytes -> 141 in 32/300 runs (11%) — well UNDER capacity +## 65012 bytes -> 141 in 25/30 runs — not 100% even AT capacity +## 500 KB into a 1 MiB pipe -> 200/200 SIGPIPE written 4096 bytes at a +## time, 0/200 written as one 500 KB write +## and strace caught printf dying having written 12086 of 40406 bytes into a +## 65536-byte pipe, i.e. losing with 53 KB of room to spare. The reason the same +## image failed 5/5 on whp02 and 10/10 clean in a dev container is the WRITER's +## syscall size: bash <= 5.2.15 pushes ~37 KB per write, bash >= 5.2.21 pushes +## 80-160 bytes, so the newer shell needs hundreds of chances to lose the race +## and the older one needs a couple. (An earlier draft of this comment blamed +## pipe capacity and fs.pipe-user-pages-soft. Both were wrong: that soft limit +## clamps new pipes to two pages rather than one, applies only once a single uid +## holds more than 1024 pipes, and is skipped entirely for CAP_SYS_RESOURCE.) ## -## A here-string is not a pipeline at all: the shell materialises the whole -## string first (temp file, or a pipe only when it provably fits the pipe -## buffer) and the command's status is the reader's own status, so there is no -## second status for pipefail to prefer and no writer left alive to signal. -## `case`/`[[ ]]` would also avoid the pipeline, but would mean re-expressing an -## anchored line match as a glob over embedded newlines; keeping grep/awk with -## the SAME patterns makes this a plumbing change and nothing else. +## The only SOUND reasons a call site is safe are structural: +## * the file does not set pipefail; or +## * the reader provably consumes to EOF (no `q`, `-q`, `-l`, `-m`, `exit`, +## `break`); or +## * the pipeline's status is discarded. +## The reader's implementation is not a defence either: at 248 KB, mawk, gawk, +## `grep -q` and `head -1` each returned 141 on 10/10, and these images have +## already drifted between mawk 1.3.4 (cac-lsphp) and gawk 5.2.1 +## (cac-litespeed:php83) — not a property this repo controls. ## -## Both return the reader's status, so a genuinely-absent extension is still a -## clean 1 and a genuinely-missing "Scan this dir" line is still empty output. +## WHY PURE-BASH MATCHING RATHER THAN A HERE-STRING. `<<<` does remove the +## pipeline, but it is not free: above a build-dependent size bash materialises +## the string as /tmp/sh-thd.XXXXXX, so it makes a writable temp dir a +## PRECONDITION OF BOOTING. Measured in this image (bash 5.2.21) the switch is +## at exactly 65536 bytes and `lsphp -i` is 39934, so the here-string form was +## not hitting disk here — but Debian's bash 5.2.15 switches somewhere between +## 4096 and 16384, where the same payload would. What that costs is not +## theoretical: +## docker run --read-only ... 'SCAN_DIR=$(awk ... <<<"$BIG")' +## -> bash: cannot create temp file for here-document: Read-only file +## system ... and the script is dead: exit 1, PID 1 gone. +## which is the exact boot failure this branch exists to remove, re-acquired +## from a different direction and gated on which bash the base image ships. +## `[[ ]]` and `${...}` allocate nothing and cannot fail that way. Where the +## subject is a couple of hundred bytes and provably cannot approach the +## threshold, a here-string is still fine — see `ols_running` in +## entrypoint-litespeed.sh, which says so at the call site. +## +## THE PATTERNS ARE THE OLD ONES RE-EXPRESSED, NOT APPROXIMATED. +## grep -q '^cac_path_parity support => enabled$' — an anchored whole-line +## match, so the subject is wrapped in a newline at BOTH ends and the glob +## matches \n\n; the wrapping is what keeps the first line and an +## unterminated last line matching exactly as grep matched them. +## grep -q '^PHP Version => ' — anchored at the start +## only, so only a leading newline is added. +## awk -F'=> ' '/^Scan this dir/ {print $2; exit}' — first matching line, +## then the text between the FIRST and SECOND '=> ' on it ($2), or empty if +## there is no separator. `${x#*'=> '}` then `${y%%'=> '*}` is that, exactly. +## scripts/tests/lsphp-info-probe.test.sh asserts this equivalence against the +## grep/awk originals over the edge cases (match on the first line, on the last +## line with no trailing newline, decoy substrings, a second separator, an empty +## value, a missing key), so "same answer as before" is checked, not asserted. +## +## Statuses are unchanged: a genuinely-absent extension is still a clean 1, and +## a genuinely-missing "Scan this dir" line is still empty output with status 0. lsphp_info_has_parity_ext() { - grep -q '^cac_path_parity support => enabled$' <<<"$1" + [[ $'\n'"$1"$'\n' == *$'\ncac_path_parity support => enabled\n'* ]] } lsphp_info_scan_dir() { - awk -F'=> ' '/^Scan this dir/ {print $2; exit}' <<<"$1" + local rest line val + rest=$'\n'"$1" + [[ $rest == *$'\nScan this dir'* ]] || return 0 + ## `#` takes the SHORTEST prefix, i.e. the FIRST matching line — awk's `exit`. + rest=${rest#*$'\nScan this dir'} + line="Scan this dir${rest%%$'\n'*}" + val="" + if [[ $line == *'=> '* ]]; then + val=${line#*'=> '} + val=${val%%'=> '*} + fi + printf '%s\n' "$val" } ## Did `lsphp -i` answer at all? Separates "the extension is not there" from ## "our probe produced nothing to look in", so neither gets reported as the @@ -223,7 +282,7 @@ lsphp_info_scan_dir() { ## (verified against lsphp83 8.3.32) and is not something LSPHP_INFO could ## contain from any other source. lsphp_info_is_usable() { - grep -q '^PHP Version => ' <<<"$1" + [[ $'\n'"$1" == *$'\nPHP Version => '* ]] } ## ---- CAC-TEST: probe helpers END ---- diff --git a/scripts/entrypoint-shared-ols.sh b/scripts/entrypoint-shared-ols.sh index 425f026..d376b05 100644 --- a/scripts/entrypoint-shared-ols.sh +++ b/scripts/entrypoint-shared-ols.sh @@ -78,8 +78,12 @@ trap term_handler TERM INT ## Variable + here-string, not a pipe into `grep -qi` — see the long note on the ## identical function in entrypoint-litespeed.sh: `grep -q` closing the pipe on ## a match can leave the writer dying 141, and `set -o pipefail` (line 14) turns -## that into "OLS is down" *because* the running line matched. A non-zero -## `lswsctrl` still counts as not running, as pipefail made it count before. +## that into "OLS is down" *because* the running line matched. The reason is +## structural (a pipefail script must not pipe into an early-exit reader), not +## that this particular output is small; and the here-string is safe here for +## the separate reason that `lswsctrl status` is far below the size at which +## bash spills a here-string to a temp file. A non-zero `lswsctrl` still counts +## as not running, as pipefail made it count before. ols_running() { local st st=$(/usr/local/lsws/bin/lswsctrl status 2>/dev/null) || return 1 diff --git a/scripts/render-shared-ols-config.sh b/scripts/render-shared-ols-config.sh index 63b0fc6..65a2d86 100644 --- a/scripts/render-shared-ols-config.sh +++ b/scripts/render-shared-ols-config.sh @@ -95,14 +95,27 @@ awk ' ## Measured in this image, `sed -n 's/^DOMAINS=//p' | head -1`: ## 400 matching lines (~6 KB of sed output) -> 0 0 0 0 0 ## 6000 matching lines (~90 KB of sed output) -> 141 141 141 -## The threshold is the PIPE CAPACITY, not "is the file small": while the -## writer's whole output fits, it never blocks and always finishes first; -## once it does not, the reader's early exit is a guaranteed SIGPIPE. Linux -## gives a pipe 64 KiB by default but drops NEW pipes to a single page once a -## user passes fs.pipe-user-pages-soft, which is the state a busy host gets -## into — and the reason a 40 KB probe failed 5/5 on whp02 and 0/10 here. -## So "a site.meta would never be that big" is not a bound worth resting on -## for panel-written input we do not validate. +## Do not read a threshold into those two rows. There is no size below which +## this is safe: each run is a RACE on whether `head` closes the pipe before +## `sed`'s final write() returns, and the payload only decides how many write() +## syscalls sed has to lose. Measured against a default 65536-byte pipe +## (F_GETPIPE_SZ), 41144 bytes SIGPIPEd on 32 of 300 runs — 11%, well under +## capacity — and 65012 bytes still only on 25 of 30, so it is neither safe +## below capacity nor certain at it; strace caught a writer dying having put +## 12086 of 40406 bytes into a 65536-byte pipe. What actually separated a host +## that failed 5/5 from one that passed 10/10 was the WRITER's syscall size +## (bash <= 5.2.15 writes ~37 KB at a time, >= 5.2.21 writes 80-160 bytes), not +## the host's pipe capacity. (An earlier draft of this comment blamed +## fs.pipe-user-pages-soft; that limit clamps new pipes to two pages, not one, +## only past 1024 pipes for one uid, and never for CAP_SYS_RESOURCE.) +## +## So the rule this file follows is structural, not statistical: under pipefail, +## a writer piped into a reader that can stop early (`head`, `grep -q`/`-l`/`-m`, +## `sed q`, `awk ... exit`, `read`) is a latent 141 — full stop. A call site is +## only sound when the file does not set pipefail, or the reader provably runs +## to EOF, or the status is thrown away. "A site.meta would never be that big" +## was never one of those, least of all for panel-written input we do not +## validate. ## ## awk reads the FILE directly and stops at the first hit: no pipeline, so ## nothing for pipefail to adopt. Same semantics as before, verified against diff --git a/scripts/tests/lsphp-info-probe.test.sh b/scripts/tests/lsphp-info-probe.test.sh index 74714b3..88fc467 100755 --- a/scripts/tests/lsphp-info-probe.test.sh +++ b/scripts/tests/lsphp-info-probe.test.sh @@ -8,12 +8,26 @@ ## printf '%s\n' "$LSPHP_INFO" | grep -q '^cac_path_parity support => enabled$' ## ## under `set -euo pipefail`. `grep -q` exits the instant it matches; printf is -## still pushing the remaining ~40 KB of `lsphp -i` into the pipe, takes -## SIGPIPE, and exits 141; pipefail prefers that over grep's 0. So the test read -## FALSE **because the extension was present** — present early enough in the -## output to stop the reader — and the container fell back to the degraded -## auto_prepend normaliser the extension exists to replace, while telling the -## operator the extension was "not loadable in this image". +## still pushing the rest of `lsphp -i` into the pipe, takes SIGPIPE, and exits +## 141; pipefail prefers that over grep's 0. So the test read FALSE **because +## the extension was present** — present early enough in the output to stop the +## reader — and the container fell back to the degraded auto_prepend normaliser +## the extension exists to replace, while telling the operator the extension was +## "not loadable in this image". +## +## THE RULE THIS FILE ENFORCES, stated so nobody re-derives a wrong one: ANY +## `writer | early-exiting-reader` under pipefail is a latent 141. PAYLOAD SIZE +## IS NOT A SAFETY ARGUMENT — each run is a race on whether the reader's close +## lands before the writer's final write() returns, and size only sets how many +## write() syscalls the writer must survive. Measured against a default +## 65536-byte pipe: 41144 bytes SIGPIPEd on 32/300 runs (11%, well UNDER +## capacity) and 65012 bytes on 25/30 (not certain even AT capacity); 500 KB +## into a 1 MiB pipe was 200/200 when written 4096 bytes at a time and 0/200 as +## a single write. A call site is sound only for a STRUCTURAL reason: the file +## does not set pipefail, or the reader provably consumes to EOF (no `q`, `-q`, +## `-l`, `-m`, `exit`, `break`), or the pipeline's status is discarded. Section 4 +## below prints this race happening at ~41 KB; if the numbers there ever read as +## "small payloads are fine", the numbers are right and the reading is wrong. ## ## WHY THE EXISTING SUITE DID NOT CATCH IT. The .phpt suite and ## fpm-parity-check.sh both test the EXTENSION; nothing executed the @@ -33,9 +47,17 @@ ## under a different Docker daemon), so no behavioural assertion about the ## broken code could be trusted to fail on every machine. Section 4 runs ## the old form anyway and prints what it did, for the record. +## 3. equivalence — the helpers' pure-bash matching answers exactly what the +## grep/awk patterns they replaced answer, checked case by case against +## those same patterns reading a FILE (a file, so the reference itself +## cannot SIGPIPE). Section 6. +## 4. the scan's own coverage — every reader shape section 5 claims to catch +## is caught, and a matched set of safe shapes is NOT flagged. Section 7. +## Without this the scan's regexes are unfalsified and can quietly stop +## matching; `grep -l` and `sed q` were both missed until section 7 existed. ## ## SCOPE LIMITS, stated rather than hidden. The structural scan is a text scan, -## so it under-reports in two known ways: +## so it under-reports in known ways: ## - it reads one line at a time. The repo's only multi-line pipeline ## (ols-htaccess-watcher.sh's `inotifywait … |` / `while read`) is invisible ## to it and was reviewed by hand: that reader loops until EOF, i.e. until @@ -43,6 +65,13 @@ ## - its quote stripping is flat, so a pipe nested inside a command ## substitution inside a quoted string (`echo "x ($(a | head -1))"`) is read ## as quoted text and skipped. +## - the reader list is an ENUMERATION, not a proof. It knows `grep` +## (-q/-l/-L/-m and their long forms), `head`, `read`, `awk … exit` and +## `sed` with a q/Q command; it does not know an early exit hidden in +## `perl -ne '… last'`, `python -c`, `jq`, `head -c`, or any project-local +## program that stops reading. A reader not on the list is not thereby safe. +## - it only inspects the FIRST word after a pipe, so `foo | LC_ALL=C grep -q` +## or `foo | { grep -q x; }` reads as an unknown reader and is skipped. ## It is a guard against reintroducing the shape, not a proof of its absence. ## ## Usage: scripts/tests/lsphp-info-probe.test.sh [REPO_ROOT] @@ -72,21 +101,52 @@ die() { echo "HARNESS FAILURE: $*" >&2; exit 2; } ## not stop the run: against a pre-fix checkout the probes are still inline ## pipelines, and section 5 below is what names them. Bailing out here would ## have replaced that report with "could not run". +## +## BOUNDED ON PURPOSE. The extraction buffers and emits NOTHING until it has +## seen the END marker, so deleting or mistyping that one line is a marker +## error (exit 4) rather than a slurp of every line after BEGIN into a file +## this harness then `source`s. That is not hypothetical: it happened during +## development and only failed loudly by luck — `set -u` tripped over an +## unbound variable two statements into the entrypoint's real boot code. An +## extractor whose failure mode is "execute arbitrary parts of the program +## under test" is not a safe thing to leave lying around, however careful the +## markers are today. +## exit 0 = both markers, block on stdout +## exit 3 = no BEGIN marker at all (pre-fix checkout, or block removed) +## exit 4 = BEGIN seen, END missing — refuse to emit, refuse to source ## --------------------------------------------------------------------------- HAVE_HELPERS=yes -awk '/^## ---- CAC-TEST: probe helpers BEGIN ----$/{f=1} f{print} /^## ---- CAC-TEST: probe helpers END ----$/{exit}' \ - "$ENTRYPOINT" > "$TMP/helpers.sh" -if [ -s "$TMP/helpers.sh" ]; then - for fn in lsphp_info_has_parity_ext lsphp_info_scan_dir lsphp_info_is_usable; do - if ! grep -q "^${fn}()" "$TMP/helpers.sh"; then - HAVE_HELPERS=no - bad "the probe-helper block in ${ENTRYPOINT#"$ROOT"/} defines no ${fn}()" - fi - done -else - HAVE_HELPERS=no - bad "no probe-helper markers in ${ENTRYPOINT#"$ROOT"/} — either they were removed, or this is a pre-fix checkout where the probes are still inline pipelines (trunk 9343a56). Section 5 says which lines." -fi +XRC=0 +awk ' + /^## ---- CAC-TEST: probe helpers BEGIN ----$/ { f = 1 } + f { buf = buf $0 ORS } + f && /^## ---- CAC-TEST: probe helpers END ----$/ { printf "%s", buf; found = 1; exit 0 } + END { if (found) exit 0; else if (f) exit 4; else exit 3 } +' "$ENTRYPOINT" > "$TMP/helpers.sh" || XRC=$? + +case "$XRC" in + 0) + for fn in lsphp_info_has_parity_ext lsphp_info_scan_dir lsphp_info_is_usable; do + if ! grep -q "^${fn}()" "$TMP/helpers.sh"; then + HAVE_HELPERS=no + bad "the probe-helper block in ${ENTRYPOINT#"$ROOT"/} defines no ${fn}()" + fi + done + ;; + 4) + HAVE_HELPERS=no + : > "$TMP/helpers.sh" + bad "${ENTRYPOINT#"$ROOT"/} has a 'CAC-TEST: probe helpers BEGIN' marker with no matching END marker. Nothing was extracted — an unterminated block would otherwise have pulled the whole rest of the entrypoint into a file this test sources and runs." + ;; + 3) + HAVE_HELPERS=no + bad "no probe-helper markers in ${ENTRYPOINT#"$ROOT"/} — either they were removed, or this is a pre-fix checkout where the probes are still inline pipelines (trunk 9343a56). Section 5 says which lines." + ;; + *) + HAVE_HELPERS=no + bad "extracting the probe-helper block from ${ENTRYPOINT#"$ROOT"/} failed (awk exit $XRC)" + ;; +esac ## --------------------------------------------------------------------------- ## 2. Fixtures. Shaped like real `lsphp -i`: the two lines the probes look for @@ -262,6 +322,35 @@ mapfile -t PIPEFAIL_FILES < <(grep -rl --include='*.sh' -E '^[[:space:]]*set[[:s [ "${#PIPEFAIL_FILES[@]}" -gt 0 ] || die "found no pipefail-enabled scripts under $ROOT — the scan would be vacuous" cat > "$TMP/scan.awk" <<'AWKPROG' +# Does this `grep …` invocation stop reading before EOF? -q/-l/-L/-m do; -c, +# -i, -v, -o, -n and the rest read the whole input and are none of our business. +# Walked option by option rather than pattern-matched in one go, because the +# letters have to be read the way grep reads them: a cluster like -im1 stops +# early, -e/-f/-A/-B/-C/-d/-D swallow the rest of their token as an ARGUMENT +# (so `grep -eq` is the pattern "q", not --quiet), and the first non-option word +# is the pattern, after which nothing is a flag any more. +function grep_stops_early(s, a, k, j, t, c, m) { + if (s !~ /^[[:space:]]*grep([[:space:];&)]|$)/) return 0 + sub(/^[[:space:]]*grep([[:space:]]+|$)/, "", s) + m = split(s, a, /[[:space:]]+/) + for (k = 1; k <= m; k++) { + t = a[k] + if (t == "") continue + if (t == "--") return 0 + if (t ~ /^--/) { + if (t ~ /^--(quiet|silent|max-count|files-with-match|files-without-match)/) return 1 + continue + } + if (t !~ /^-/) return 0 # the pattern; options are over + sub(/^-/, "", t) + for (j = 1; j <= length(t); j++) { + c = substr(t, j, 1) + if (c == "q" || c == "l" || c == "L" || c == "m") return 1 + if (c ~ /[efABCdD]/) break # rest of the token is its argument + } + } + return 0 +} { raw = $0 l = raw @@ -270,16 +359,37 @@ cat > "$TMP/scan.awk" <<'AWKPROG' while (match(l, /"[^"]*"/)) l = substr(l, 1, RSTART-1) "DQ" substr(l, RSTART+RLENGTH) if (l ~ /^[[:space:]]*#/) next # whole-line comment sub(/[[:space:]]#.*/, "", l) # trailing comment + + # `u` = the line with quote CHARACTERS dropped but their contents KEPT, used + # only to read a command's script argument. `l` cannot serve for that: it + # replaces a whole quoted span with a placeholder, so `sed 'q'` and + # `awk '{exit}'` lose the very token that makes them early-exit readers. + u = raw + gsub(/["']/, "", u) + sub(/[[:space:]]#.*/, "", u) + gsub(/\|\|/, " ", l) # || is not a pipeline if (l !~ /\|/) next n = split(l, seg, "|") for (i = 2; i <= n; i++) { r = seg[i] - if (r ~ /^[[:space:]]*grep[[:space:]]+-([[:alnum:]]*q|m)/) { print NR ": " raw; next } - if (r ~ /^[[:space:]]*head([[:space:]]|$)/) { print NR ": " raw; next } - if (r ~ /^[[:space:]]*(while[[:space:]]+)?read([[:space:]]|$)/) { print NR ": " raw; next } - if (r ~ /^[[:space:]]*awk([[:space:]]|$)/ && raw ~ /exit/) { print NR ": " raw; next } - if (r ~ /^[[:space:]]*sed([[:space:]]|$)/ && raw ~ /[;{\/][[:space:]]*[qQ][^[:alnum:]]/) { print NR ": " raw; next } + if (grep_stops_early(r)) { print NR ": " raw; next } + # The `[[:space:];&)]|$` tail rather than a bare `[[:space:]]|$`: a reader + # can be the last word of a compound command (`… | head; }`), which the + # whitespace-only form silently skipped. + if (r ~ /^[[:space:]]*head([[:space:];&)]|$)/) { print NR ": " raw; next } + if (r ~ /^[[:space:]]*(while[[:space:]]+)?read([[:space:];&)]|$)/) { print NR ": " raw; next } + if (r ~ /^[[:space:]]*awk([[:space:];&)]|$)/ && u ~ /exit/) { print NR ": " raw; next } + # sed: a q/Q command anywhere in the script — `/re/q`, `2q`, `$q`, `1p;q`, + # `{…;q}`, and the bare `sed q` / `sed 'q'` that the earlier pattern missed + # (it required a `;`, `{` or `/` in front of the q, which a lone script has + # none of). Anchored on what may PRECEDE the q and what may FOLLOW it, so a + # q inside a replacement — `sed s/a/q/` — is not read as the command (which + # is what the trailing `/` exclusion buys: a command q is never followed by + # another delimiter, a replacement q always is). + if (r ~ /^[[:space:]]*sed([[:space:];&)]|$)/ && + (u ~ /[;{\/][[:space:]]*[qQ]([^[:alnum:]\/]|$)/ || + u ~ /[[:space:]]([0-9]+|\$)?[qQ]([[:space:];}]|$)/)) { print NR ": " raw; next } } } AWKPROG @@ -296,10 +406,156 @@ if [ "$offenders" -eq 0 ]; then ok "${#PIPEFAIL_FILES[@]} pipefail-enabled scripts, no pipeline whose reader can outrun its writer" else bad "$offenders pipeline(s) above pipe into an early-exit reader under pipefail." - echo " Read the value into a variable and match it with a here-string (see" >&2 + echo " Read the value into a variable and match it in the shell (see" >&2 echo " lsphp_info_has_parity_ext in scripts/entrypoint-lsphp.sh), or give the" >&2 - echo " reader the file directly. A here-string is not a pipeline, so there is" >&2 - echo " no second exit status for pipefail to prefer." >&2 + echo " reader the file directly. Neither is a pipeline, so there is no second" >&2 + echo " exit status for pipefail to prefer. A here-string also works, but it" >&2 + echo " spills to a temp file above a build-dependent size, so it is not the" >&2 + echo " right shape on a boot path." >&2 +fi + +## --------------------------------------------------------------------------- +## 6. Equivalence. The helpers answer with `[[ ]]` and `${…}` what they used to +## answer with grep and awk, and "same patterns, different plumbing" is a +## claim that has to be checked rather than asserted — an anchored line match +## re-expressed as a glob is exactly where an off-by-one lives. +## +## The reference runs the ORIGINAL grep/awk patterns over a FILE, so the +## reference itself cannot SIGPIPE and cannot be accused of the bug it is +## refereeing. The file is written the way the old pipeline fed them, +## `printf '%s\n' "$SUBJECT"`, so the comparison is against the pre-fix +## behaviour byte for byte and not against a tidier reading of it. +## --------------------------------------------------------------------------- +echo "== pure-bash matching vs the grep/awk patterns it replaced ==" +if [ "$HAVE_HELPERS" = no ]; then + echo " (skipped — no probe helpers to compare)" +else + # shellcheck disable=SC1091 + source "$TMP/helpers.sh" + + NL=$'\n' + eq_fail=0 + eq_case() { # $1 = label, $2 = subject + local label="$1" subj="$2" f="$TMP/eq.txt" + local ref_has ref_use new_has new_use ref_scan new_scan + printf '%s\n' "$subj" > "$f" + + ref_has=0; grep -q '^cac_path_parity support => enabled$' "$f" || ref_has=$? + new_has=0; lsphp_info_has_parity_ext "$subj" || new_has=$? + ref_use=0; grep -q '^PHP Version => ' "$f" || ref_use=$? + new_use=0; lsphp_info_is_usable "$subj" || new_use=$? + ref_scan=$(awk -F'=> ' '/^Scan this dir/ {print $2; exit}' "$f") + new_scan=$(lsphp_info_scan_dir "$subj") + + if [ "$ref_has" = "$new_has" ] && [ "$ref_use" = "$new_use" ] && [ "$ref_scan" = "$new_scan" ]; then + ok "equivalent on: $label" + else + eq_fail=$((eq_fail+1)) + bad "NOT equivalent on: $label — has_ext grep=$ref_has bash=$new_has; usable grep=$ref_use bash=$new_use; scan_dir awk='$ref_scan' bash='$new_scan'" + fi + } + + SD='Scan this dir for additional .ini files' + PARITY='cac_path_parity support => enabled' + eq_case "empty subject" "" + eq_case "match on the first line" "${PARITY}${NL}tail line" + eq_case "match on the last line" "head line${NL}${PARITY}" + eq_case "match is the only line" "${PARITY}" + eq_case "match sandwiched" "a${NL}${PARITY}${NL}b" + eq_case "not at line start (decoy)" "x ${PARITY}${NL}b" + eq_case "trailing space defeats the \$" "${PARITY} ${NL}b" + eq_case "prefix-only line (decoy)" "cac_path_parity support => enabled but no${NL}b" + eq_case "genuinely absent" "a${NL}b${NL}c" + eq_case "embedded blank lines" "a${NL}${NL}${PARITY}${NL}${NL}b" + eq_case "banner first, scan dir second" "PHP Version => 8.3.27${NL}${SD} => /a/b" + eq_case "scan dir on the first line" "${SD} => /a/b${NL}PHP Version => 8.3.27" + eq_case "scan dir on the last line" "PHP Version => 8.3.27${NL}${SD} => /a/b" + eq_case "scan dir, second separator" "${SD} => /a => /b${NL}x" + eq_case "scan dir, empty value" "${SD} => ${NL}x" + eq_case "scan dir, no separator" "Scan this dir is broken${NL}x" + eq_case "scan dir, first of two wins" "${SD} => /first${NL}${SD} => /second" + eq_case "scan dir, value has spaces" "${SD} => /a b/c ${NL}x" + eq_case "scan-dir line is a prefix" "Scan this directory => /a/b${NL}x" + eq_case "banner not at line start" "x PHP Version => 8.3.27${NL}b" + eq_case "banner without trailing space" "PHP Version =>${NL}b" + eq_case "CR-terminated lines" $'PHP Version => 8.3.27\r'"${NL}${PARITY}"$'\r' + eq_case "glob metacharacters in body" "*${NL}?${NL}[a-z]${NL}${PARITY}${NL}][" + eq_case "realistic 40 KB body" "$(cat "$TMP/info-present.txt")" + [ "$eq_fail" -eq 0 ] || echo " (an inequivalence here means the shipped probe now answers something the old grep/awk did not)" >&2 +fi + +## --------------------------------------------------------------------------- +## 7. The scan's own coverage. Section 5 only proves something if its regexes +## actually match the shapes it claims to outlaw — an unfalsified scanner +## reports "clean" just as loudly when it has stopped matching anything. +## `grep -l foo` and `sed q` were both silently missed until this section +## existed; both are asserted below, alongside the safe forms that must NOT +## be reported, because a scanner that flags everything is no better. +## --------------------------------------------------------------------------- +echo "== the structural scan catches what it claims to ==" +cat > "$TMP/scan-bad.sh" <<'BADSH' +set -euo pipefail +a() { producer | grep -q needle; } +b() { producer | grep -qx needle; } +c() { producer | grep -m1 needle; } +d() { producer | grep -im1 needle; } +e() { producer | grep -l foo; } +f() { producer | grep -L foo; } +g() { producer | grep --quiet foo; } +h() { producer | grep --files-with-matches foo; } +i() { producer | head -1; } +j() { producer | head; } +k() { producer | read -r x; } +l() { producer | while read -r x; do :; done; } +m() { producer | awk '/x/ {print; exit}'; } +n() { producer | sed q; } +o() { producer | sed 'q'; } +p() { producer | sed 2q; } +q() { producer | sed -n '1p;q'; } +r() { producer | sed -n '/x/{p;q}'; } +s() { producer | sed '$q'; } +BADSH +cat > "$TMP/scan-good.sh" <<'GOODSH' +set -euo pipefail +a() { producer | grep -c needle; } +b() { producer | grep -i needle; } +c() { producer | grep -v needle; } +d() { producer | grep -o needle; } +e() { producer | awk '{print $1}'; } +f() { producer | sed -n 's/^K=//p'; } +g() { producer | sed 's/a/q/'; } +h() { producer | sed -e 'y/abc/xqz/'; } +i() { producer | wc -l; } +j() { producer | sort -u; } +k() { producer | tail -1; } +l() { case $x in a|b) : ;; esac; } +m() { echo "a | head -1"; } +n() { echo 'x | grep -q y'; } +o() { grep -q needle <<<"$1"; } +p() { grep -q needle "$file"; } +# q() { producer | grep -q commented-out; } +r() { producer | grep -eq foo; } +GOODSH +missed=""; falsely=""; hits_bad="" +while IFS= read -r line; do + fn=${line#*: }; fn=${fn%%(*} + hits_bad="$hits_bad $fn" +done < <(awk -f "$TMP/scan.awk" "$TMP/scan-bad.sh") +for want in a b c d e f g h i j k l m n o p q r s; do + case " ${hits_bad:-} " in *" $want "*) ;; *) missed="$missed $want" ;; esac +done +mapfile -t good_hits < <(awk -f "$TMP/scan.awk" "$TMP/scan-good.sh") +if [ -z "$missed" ]; then + ok "all 19 early-exit reader shapes are reported (incl. grep -l/-L/--quiet and bare 'sed q')" +else + bad "the scan misses these shapes in scan-bad.sh:$missed" + awk -f "$TMP/scan.awk" "$TMP/scan-bad.sh" >&2 +fi +if [ "${#good_hits[@]}" -eq 0 ]; then + ok "18 read-to-EOF / quoted / non-pipeline forms are not reported" +else + falsely=$(printf '%s; ' "${good_hits[@]}") + bad "the scan false-positives on: $falsely" fi echo