forked from heavy-duty/box
Merge pull request #120 from dan-claude-bot/fix/wipe-sigpipe-shape
fix: drill/wipe.sh reads ufw into a capture, not into an early-exit reader
This commit is contained in:
commit
4fe91ef562
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
|
||||
`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
|
||||
|
||||
### 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
|
||||
sudo nft delete table $t >/dev/null 2>&1 && say "deleted nft table $t"
|
||||
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
|
||||
while sudo ufw status numbered | grep -q "on $net"; do
|
||||
n="$(sudo ufw status numbered | grep -m1 "on $net" | sed -E 's/^\[ *([0-9]+)\].*/\1/')"
|
||||
while :; do
|
||||
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"
|
||||
done
|
||||
done
|
||||
|
|
|
|||
50
test/cli.sh
50
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 \
|
||||
"--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
|
||||
# cannot exercise a UFW teardown, so the shape is pinned instead: no `ufw
|
||||
# status` may be piped into an early-exit reader here, because under this
|
||||
# file's pipefail the reader's match closes the pipe, ufw takes SIGPIPE, and
|
||||
# the branch silently reads false — skipping crumb removal on a host the
|
||||
# operator was told is clean. Both directions: the racing shape absent, the
|
||||
# capture present.
|
||||
# Comment lines are stripped before matching: the fix's own commentary quotes
|
||||
# #102's race, pinned as a CLASS rather than at the one site that had it
|
||||
# (#107). A daemon-free run cannot exercise a UFW teardown, so the shape is
|
||||
# pinned instead: nowhere under host/ or drill/ may `ufw status` be piped into
|
||||
# a reader that exits on its first match. `Status: active` is ufw's FIRST line,
|
||||
# so the reader matches, closes the pipe, ufw takes SIGPIPE, and the pipeline
|
||||
# yields 141 — under pipefail the branch silently reads false and the whole
|
||||
# firewall block is skipped on a host the operator was told is clean.
|
||||
#
|
||||
# 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
|
||||
# would fail on the very comment documenting why it exists.
|
||||
# shellcheck disable=SC2016 # "$1" is the subshell's positional, passed below
|
||||
check "teardown-host: no 'ufw status' piped into an early-exit reader" 0 "" \
|
||||
bash -c 'grep -vE "^[[:space:]]*#" "$1" | grep -qE "ufw status[^|]*\| *grep" && exit 1; exit 0' \
|
||||
_ "$ROOT/host/teardown-host.sh"
|
||||
# shellcheck disable=SC2016 # the $-strings are literals in the target file
|
||||
check "teardown-host: the UFW branch reads a captured snapshot" 0 "" \
|
||||
grep -qF 'if [[ "$ufw_status" == *"Status: active"* ]]; then' "$ROOT/host/teardown-host.sh"
|
||||
# shellcheck disable=SC2016 # ditto
|
||||
check "teardown-host: the numbered-delete loop breaks on absence, not on a pipe" 0 "" \
|
||||
grep -qF '[ -n "$line" ] || break' "$ROOT/host/teardown-host.sh"
|
||||
check "no 'ufw status' is piped into an early-exit reader under host/ or drill/" 0 "" \
|
||||
bash -c 'bad=""
|
||||
for f in "$1"/host/*.sh "$1"/drill/*.sh; do
|
||||
grep -vE "^[[:space:]]*#" "$f" | grep -qE "ufw status[^|]*\| *grep" && bad="$bad ${f#"$1"/}"
|
||||
done
|
||||
[ -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
|
||||
check "$f: the numbered-delete loop breaks on absence, not on a pipe" 0 "" \
|
||||
grep -qF '[ -n "$line" ] || break' "$ROOT/$f"
|
||||
done
|
||||
check "drill: reads the installed tree through current/" 0 "" \
|
||||
grep -qF '.local/share/box/current/VERSION' "$ROOT/drill/drill.sh"
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue