fix: drill/wipe.sh reads ufw into a capture, not into an early-exit reader
wipe.sh piped `ufw status` straight into `grep -q "Status: active"`. That is closes the pipe, ufw takes SIGPIPE, and the pipeline yields 141. It was correct today, and only by accident — the file is `set -u` with no `pipefail`, so the 141 was discarded and grep's 0 carried the branch. It was one line from wrong: adding `set -o pipefail` for unrelated robustness would have silently skipped every UFW removal on a host the operator was told is wiped, with no error and no red X. Measured on a shim: 5/5 runs took the wrong branch under pipefail, 3/3 the right one without. Transplant #106's pattern verbatim from host/teardown-host.sh: capture ufw_status once and match with `[[ ]]`; rewrite the numbered-delete loop — whose condition was itself an early-exit reader, plus an un-captured re-read to get the number — as a `while :` that reads one capture per iteration and breaks on absence. The re-scan stays per-delete, since numbers shift after each removal; it just no longer races. Removals keep the file's `cmd && say "did X"` idiom. Generalize the test/cli.sh pin from the one site to the class: sweep every host/*.sh and drill/*.sh for the racing shape and name the offenders, so a new script in either directory inherits the pin instead of being one more site to remember. Comment lines are stripped before matching — each fix's own commentary quotes the racing shape to explain it, and a prose-blind pin would fail on the comment documenting why it exists. The positive pins (the capture, the break-on-absence) now run per file over both, so the sweep cannot be satisfied by deleting a block instead of fixing it. drill/doctor.sh was checked and needs nothing: it already reads into `ufw_out` and is safe by construction, not by absent pipefail. Refs #107
This commit is contained in:
parent
239428bf88
commit
aec22277cd
3 changed files with 90 additions and 20 deletions
25
CHANGELOG.md
25
CHANGELOG.md
|
|
@ -374,6 +374,31 @@ which records not just what changed but what each drill run proved.
|
||||||
and *above* the first `incus` call, so the refusal costs no daemon and
|
and *above* the first `incus` call, so the refusal costs no daemon and
|
||||||
`test/cli.sh` drives it for real rather than grepping for it.
|
`test/cli.sh` drives it for real rather than grepping for it.
|
||||||
|
|
||||||
|
- **`drill/wipe.sh` no longer carries #102's SIGPIPE shape, and the pin now
|
||||||
|
sweeps the class** (#107) — the file piped `ufw status` straight into
|
||||||
|
`grep -q "Status: active"`. `Status: active` is ufw's FIRST line, so the
|
||||||
|
reader matches, closes the pipe, ufw dies of SIGPIPE, and the pipeline
|
||||||
|
yields 141. This was **correct today and only by accident**: `wipe.sh` is
|
||||||
|
`set -u` with no `pipefail`, so the 141 was discarded and grep's 0 carried
|
||||||
|
the branch. It was also one line from wrong — adding `set -o pipefail` for
|
||||||
|
unrelated robustness, the kind of tweak that reads as an obvious
|
||||||
|
improvement, would have silently skipped every UFW removal on a host the
|
||||||
|
operator was told is wiped, with no error and no red X (measured on a shim:
|
||||||
|
5/5 runs took the wrong branch under `pipefail`, 3/3 the right one
|
||||||
|
without). Now it captures `ufw_status` once and matches with `[[ ]]`, and
|
||||||
|
the numbered-delete loop — whose condition was itself an early-exit reader,
|
||||||
|
plus an un-captured re-read to get the number — reads one capture per
|
||||||
|
iteration and breaks on absence. That is #106's pattern transplanted
|
||||||
|
verbatim from `host/teardown-host.sh`, so both files now read alike.
|
||||||
|
The `test/cli.sh` pin is **generalized from the single site to the class**:
|
||||||
|
it sweeps every `host/*.sh` and `drill/*.sh` for the racing shape and names
|
||||||
|
the offenders, so a new script in either directory inherits the pin instead
|
||||||
|
of being one more site to remember. Comment lines are stripped before
|
||||||
|
matching — each fix's own commentary quotes the racing shape to explain it,
|
||||||
|
and a prose-blind pin would fail on the very comment documenting why it
|
||||||
|
exists. (`drill/doctor.sh` was checked and needs nothing: it already reads
|
||||||
|
into `ufw_out` and is safe by construction, not by absent `pipefail`.)
|
||||||
|
|
||||||
## 0.8.0 — 2026-07-19
|
## 0.8.0 — 2026-07-19
|
||||||
|
|
||||||
### Added
|
### Added
|
||||||
|
|
|
||||||
|
|
@ -117,10 +117,39 @@ for t in "inet box" "bridge box" "inet claudebox" "bridge claudebox"; do
|
||||||
# shellcheck disable=SC2086 # the table spec is two words by design
|
# shellcheck disable=SC2086 # the table spec is two words by design
|
||||||
sudo nft delete table $t >/dev/null 2>&1 && say "deleted nft table $t"
|
sudo nft delete table $t >/dev/null 2>&1 && say "deleted nft table $t"
|
||||||
done
|
done
|
||||||
if command -v ufw >/dev/null && sudo ufw status 2>/dev/null | grep -q "Status: active"; then
|
# Every ufw read is CAPTURED before it is matched, never piped into a reader
|
||||||
|
# that exits on its first match (#102, #107).
|
||||||
|
#
|
||||||
|
# `ufw status | grep -q "Status: active"` returns the WRITER's exit: grep
|
||||||
|
# matches on the first line ufw prints, closes the pipe, ufw takes SIGPIPE,
|
||||||
|
# and the pipeline yields 141. This file is 'set -u' with no pipefail, so
|
||||||
|
# that 141 is discarded, grep's 0 is the pipeline's result, and the branch
|
||||||
|
# held — the defect was latent here, never live. It was also one line from
|
||||||
|
# live: adding 'set -o pipefail' for unrelated robustness would silently turn
|
||||||
|
# this into #102, skipping every UFW removal on a host the operator was told
|
||||||
|
# is wiped, with no error and no red X to see. Captured and matched with
|
||||||
|
# [[ ]], it is correct under any future 'set' line.
|
||||||
|
#
|
||||||
|
# The numbered loop had the same defect for a different reason: its condition
|
||||||
|
# was also an early-exit reader, so it could end while rules remained, and it
|
||||||
|
# re-read un-captured to get the number. It now reads one capture per
|
||||||
|
# iteration and breaks on absence — the re-scan is still per-delete (numbers
|
||||||
|
# shift after each removal), just no longer racing.
|
||||||
|
ufw_status=""
|
||||||
|
if command -v ufw >/dev/null; then
|
||||||
|
# '|| true': ufw exits non-zero when it cannot read its config, and "no
|
||||||
|
# usable ufw here" is nothing-to-clean, not a failure to report.
|
||||||
|
ufw_status="$(sudo ufw status 2>/dev/null || true)"
|
||||||
|
fi
|
||||||
|
|
||||||
|
if [[ "$ufw_status" == *"Status: active"* ]]; then
|
||||||
for net in boxnet claudenet; do
|
for net in boxnet claudenet; do
|
||||||
while sudo ufw status numbered | grep -q "on $net"; do
|
while :; do
|
||||||
n="$(sudo ufw status numbered | grep -m1 "on $net" | sed -E 's/^\[ *([0-9]+)\].*/\1/')"
|
numbered="$(sudo ufw status numbered 2>/dev/null || true)"
|
||||||
|
line="$(printf '%s\n' "$numbered" | grep -m1 "on $net" || true)"
|
||||||
|
[ -n "$line" ] || break
|
||||||
|
n="$(printf '%s\n' "$line" | sed -E 's/^\[ *([0-9]+)\].*/\1/')"
|
||||||
|
[ -n "$n" ] || break
|
||||||
sudo ufw --force delete "$n" >/dev/null && say "deleted UFW rule on $net"
|
sudo ufw --force delete "$n" >/dev/null && say "deleted UFW rule on $net"
|
||||||
done
|
done
|
||||||
done
|
done
|
||||||
|
|
|
||||||
48
test/cli.sh
48
test/cli.sh
|
|
@ -2006,26 +2006,42 @@ check "teardown-host: points at box uninstall when done" 0 "" \
|
||||||
check "teardown-host: refuses without a TTY and names the override (#113)" 2 \
|
check "teardown-host: refuses without a TTY and names the override (#113)" 2 \
|
||||||
"--yes (or BOX_YES=1) means yes" env -u BOX_YES bash "$ROOT/host/teardown-host.sh" </dev/null
|
"--yes (or BOX_YES=1) means yes" env -u BOX_YES bash "$ROOT/host/teardown-host.sh" </dev/null
|
||||||
|
|
||||||
# #102's race, in the one other file that sets pipefail. A daemon-free run
|
# #102's race, pinned as a CLASS rather than at the one site that had it
|
||||||
# cannot exercise a UFW teardown, so the shape is pinned instead: no `ufw
|
# (#107). A daemon-free run cannot exercise a UFW teardown, so the shape is
|
||||||
# status` may be piped into an early-exit reader here, because under this
|
# pinned instead: nowhere under host/ or drill/ may `ufw status` be piped into
|
||||||
# file's pipefail the reader's match closes the pipe, ufw takes SIGPIPE, and
|
# a reader that exits on its first match. `Status: active` is ufw's FIRST line,
|
||||||
# the branch silently reads false — skipping crumb removal on a host the
|
# so the reader matches, closes the pipe, ufw takes SIGPIPE, and the pipeline
|
||||||
# operator was told is clean. Both directions: the racing shape absent, the
|
# yields 141 — under pipefail the branch silently reads false and the whole
|
||||||
# capture present.
|
# firewall block is skipped on a host the operator was told is clean.
|
||||||
# Comment lines are stripped before matching: the fix's own commentary quotes
|
#
|
||||||
|
# Swept, not per-file, because absence of pipefail is what made drill/wipe.sh
|
||||||
|
# survive the same shape: a file is only ever one `set -o pipefail` — the kind
|
||||||
|
# of robustness tweak that sails through review — from being #102 again. The
|
||||||
|
# sweep closes the class, so a new host/ or drill/ script inherits the pin for
|
||||||
|
# free instead of being one more site someone has to remember.
|
||||||
|
# Comment lines are stripped before matching: each fix's own commentary quotes
|
||||||
# the racing shape to explain it, and a pin that cannot tell prose from code
|
# the racing shape to explain it, and a pin that cannot tell prose from code
|
||||||
# would fail on the very comment documenting why it exists.
|
# would fail on the very comment documenting why it exists.
|
||||||
# shellcheck disable=SC2016 # "$1" is the subshell's positional, passed below
|
# shellcheck disable=SC2016 # "$1" is the subshell's positional, passed below
|
||||||
check "teardown-host: no 'ufw status' piped into an early-exit reader" 0 "" \
|
check "no 'ufw status' is piped into an early-exit reader under host/ or drill/" 0 "" \
|
||||||
bash -c 'grep -vE "^[[:space:]]*#" "$1" | grep -qE "ufw status[^|]*\| *grep" && exit 1; exit 0' \
|
bash -c 'bad=""
|
||||||
_ "$ROOT/host/teardown-host.sh"
|
for f in "$1"/host/*.sh "$1"/drill/*.sh; do
|
||||||
# shellcheck disable=SC2016 # the $-strings are literals in the target file
|
grep -vE "^[[:space:]]*#" "$f" | grep -qE "ufw status[^|]*\| *grep" && bad="$bad ${f#"$1"/}"
|
||||||
check "teardown-host: the UFW branch reads a captured snapshot" 0 "" \
|
done
|
||||||
grep -qF 'if [[ "$ufw_status" == *"Status: active"* ]]; then' "$ROOT/host/teardown-host.sh"
|
[ -z "$bad" ] || { printf "racing ufw reads in:%s\n" "$bad"; exit 1; }' \
|
||||||
|
_ "$ROOT"
|
||||||
|
|
||||||
|
# The other direction, per file that removes UFW rules: the capture present and
|
||||||
|
# the delete loop breaking on absence, so the sweep above cannot be satisfied by
|
||||||
|
# deleting the block instead of fixing it.
|
||||||
|
for f in host/teardown-host.sh drill/wipe.sh; do
|
||||||
|
# shellcheck disable=SC2016 # the $-strings are literals in the target files
|
||||||
|
check "$f: the UFW branch reads a captured snapshot" 0 "" \
|
||||||
|
grep -qF 'if [[ "$ufw_status" == *"Status: active"* ]]; then' "$ROOT/$f"
|
||||||
# shellcheck disable=SC2016 # ditto
|
# shellcheck disable=SC2016 # ditto
|
||||||
check "teardown-host: the numbered-delete loop breaks on absence, not on a pipe" 0 "" \
|
check "$f: the numbered-delete loop breaks on absence, not on a pipe" 0 "" \
|
||||||
grep -qF '[ -n "$line" ] || break' "$ROOT/host/teardown-host.sh"
|
grep -qF '[ -n "$line" ] || break' "$ROOT/$f"
|
||||||
|
done
|
||||||
check "drill: reads the installed tree through current/" 0 "" \
|
check "drill: reads the installed tree through current/" 0 "" \
|
||||||
grep -qF '.local/share/box/current/VERSION' "$ROOT/drill/drill.sh"
|
grep -qF '.local/share/box/current/VERSION' "$ROOT/drill/drill.sh"
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue