forked from heavy-duty/box
fix: teardown-host carried the same SIGPIPE race — it sets pipefail (#106 review)
The original diff claimed all three sibling `ufw status | grep -q` sites were safe because none set pipefail. That is true of drill/wipe.sh and drill/doctor.sh (both `set -u`) and FALSE of host/teardown-host.sh, whose line 12 is `set -euo pipefail`. Its line 60 was the identical pipeline, so the same race could read a plainly-active UFW as inactive and skip the whole crumb-removal block — leaving stale boxnet/claudenet rules on a host the operator was told is clean. Its numbered-delete loop had the same early-exit reader as its condition, so it could also end while rules remained. Both now read captures, matching box-firewall.sh's fix. The changelog claim is corrected rather than deleted: this repo's changelog is the record of what was proven, and shipping a disproven safety claim about a live defect is worse than the defect, because it tells the next reader not to look. Pinned in both directions, with comment lines stripped before matching — the fix's own commentary quotes the racing shape to explain it, and a pin that cannot tell prose from code fails on the comment documenting why it exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
dc09d54cbe
commit
57db9a56bb
3 changed files with 90 additions and 37 deletions
73
CHANGELOG.md
73
CHANGELOG.md
|
|
@ -5,40 +5,6 @@ which records not just what changed but what each drill run proved.
|
||||||
|
|
||||||
## Unreleased
|
## Unreleased
|
||||||
|
|
||||||
### Fixed
|
|
||||||
|
|
||||||
- **`box-firewall` could hand a UFW host the no-UFW firewall, ~2% of the
|
|
||||||
time** (#102) — filed as an intermittent test flake (`test/cli.sh`'s
|
|
||||||
fresh-UFW block going four-assertions-red on an unmodified `main`,
|
|
||||||
measured here at 5 failing runs in 40), it was not one. The branch that
|
|
||||||
decides the host's entire firewall stance read
|
|
||||||
`ufw status | grep -q "Status: active"`, and `Status: active` is the FIRST
|
|
||||||
line ufw prints: `grep -q` matches it and exits immediately, closing the
|
|
||||||
pipe while ufw is still writing the rest of the table, so ufw dies of
|
|
||||||
SIGPIPE. `grep` returned 0, but under this script's `set -o pipefail` the
|
|
||||||
PIPELINE returns 141 — the `if` reads false and a host with UFW plainly
|
|
||||||
active takes the nft-fallback branch, never building the DNS carve-out its
|
|
||||||
persisted rules depend on. A pure scheduling race, isolated at ~2% per
|
|
||||||
invocation (`PIPESTATUS` = `141 0`; a draining reader flakes 0/2000, a
|
|
||||||
reader whose match is on the last line flakes 0/2000). Real ufw is a
|
|
||||||
slower, longer writer than the test shim, so production had no reason to
|
|
||||||
be safer. `ufw status` is now read ONCE into a variable and matched with
|
|
||||||
`[[ ]]` — no reader, no race — and the stale-rule scan reads that same
|
|
||||||
snapshot, so the branch decision and the converge loop can no longer
|
|
||||||
disagree. The sibling `ufw status | grep -q` calls in `drill/wipe.sh`,
|
|
||||||
`drill/doctor.sh` and `host/teardown-host.sh` are the same shape but do
|
|
||||||
not set `pipefail`, so the SIGPIPE is discarded there and the branch holds.
|
|
||||||
- **A missing firewall log now diagnoses itself** (#102) — the four greps
|
|
||||||
reading `$WFW/*.log` used to fail together with empty output when the
|
|
||||||
driving run took the wrong branch, a signature that looks specific and
|
|
||||||
says nothing (#102 was filed reading it as "the log is not written";
|
|
||||||
the log existed, the mutations did not, and that distinction *was* the
|
|
||||||
diagnosis). `test/cli.sh` now asserts the precondition explicitly before
|
|
||||||
the content greps and, on failure, prints the contents of `$WFW`, the log
|
|
||||||
itself, and the stderr of the run that should have written it. It also
|
|
||||||
keeps `an agreeing UFW host deletes nothing` honest: that check asserts an
|
|
||||||
absence, which a run that did nothing at all passes for the wrong reason.
|
|
||||||
|
|
||||||
### Added
|
### Added
|
||||||
|
|
||||||
- **Merging the release PR IS the release — and the release re-arms main
|
- **Merging the release PR IS the release — and the release re-arms main
|
||||||
|
|
@ -68,6 +34,44 @@ which records not just what changed but what each drill run proved.
|
||||||
|
|
||||||
### Fixed
|
### Fixed
|
||||||
|
|
||||||
|
- **`box-firewall` could hand a UFW host the no-UFW firewall, ~2% of the
|
||||||
|
time** (#102) — filed as an intermittent test flake (`test/cli.sh`'s
|
||||||
|
fresh-UFW block going four-assertions-red on an unmodified `main`,
|
||||||
|
measured here at 5 failing runs in 40), it was not one. The branch that
|
||||||
|
decides the host's entire firewall stance read
|
||||||
|
`ufw status | grep -q "Status: active"`, and `Status: active` is the FIRST
|
||||||
|
line ufw prints: `grep -q` matches it and exits immediately, closing the
|
||||||
|
pipe while ufw is still writing the rest of the table, so ufw dies of
|
||||||
|
SIGPIPE. `grep` returned 0, but under this script's `set -o pipefail` the
|
||||||
|
PIPELINE returns 141 — the `if` reads false and a host with UFW plainly
|
||||||
|
active takes the nft-fallback branch, never building the DNS carve-out its
|
||||||
|
persisted rules depend on. A pure scheduling race, isolated at ~2% per
|
||||||
|
invocation (`PIPESTATUS` = `141 0`; a draining reader flakes 0/2000, a
|
||||||
|
reader whose match is on the last line flakes 0/2000). Real ufw is a
|
||||||
|
slower, longer writer than the test shim, so production had no reason to
|
||||||
|
be safer. `ufw status` is now read ONCE into a variable and matched with
|
||||||
|
`[[ ]]` — no reader, no race — and the stale-rule scan reads that same
|
||||||
|
snapshot, so the branch decision and the converge loop can no longer
|
||||||
|
disagree. **`host/teardown-host.sh` carried the same live defect** and is
|
||||||
|
fixed with it: that file does set `pipefail` (line 12), so its UFW
|
||||||
|
crumb-removal branch could read a plainly-active UFW as inactive and skip
|
||||||
|
silently, leaving stale `boxnet`/`claudenet` rules on a host the operator
|
||||||
|
was told is clean — and its numbered-delete loop had the same early-exit
|
||||||
|
reader as its condition, so it could end while rules remained. Both now
|
||||||
|
read captures. The sibling calls in `drill/wipe.sh` and `drill/doctor.sh`
|
||||||
|
are the same shape but set only `set -u`, so the SIGPIPE is discarded
|
||||||
|
there and the branch holds — latent, not live, until either gains
|
||||||
|
`pipefail`.
|
||||||
|
- **A missing firewall log now diagnoses itself** (#102) — the four greps
|
||||||
|
reading `$WFW/*.log` used to fail together with empty output when the
|
||||||
|
driving run took the wrong branch, a signature that looks specific and
|
||||||
|
says nothing (#102 was filed reading it as "the log is not written";
|
||||||
|
the log existed, the mutations did not, and that distinction *was* the
|
||||||
|
diagnosis). `test/cli.sh` now asserts the precondition explicitly before
|
||||||
|
the content greps and, on failure, prints the contents of `$WFW`, the log
|
||||||
|
itself, and the stderr of the run that should have written it. It also
|
||||||
|
keeps `an agreeing UFW host deletes nothing` honest: that check asserts an
|
||||||
|
absence, which a run that did nothing at all passes for the wrong reason.
|
||||||
- **`box grant` provisions an `incus-admin` member instead of refusing them**
|
- **`box grant` provisions an `incus-admin` member instead of refusing them**
|
||||||
(#99) — the refusal read "they already have the admin tier; there is
|
(#99) — the refusal read "they already have the admin tier; there is
|
||||||
nothing tighter to grant", which is true about *permission* and silent
|
nothing tighter to grant", which is true about *permission* and silent
|
||||||
|
|
@ -142,6 +146,7 @@ which records not just what changed but what each drill run proved.
|
||||||
opens as them, and dropping `incus-admin` leaves them in their own project
|
opens as them, and dropping `incus-admin` leaves them in their own project
|
||||||
with no re-grant.
|
with no re-grant.
|
||||||
|
|
||||||
|
|
||||||
## 0.7.0 — 2026-07-19
|
## 0.7.0 — 2026-07-19
|
||||||
|
|
||||||
### Added
|
### Added
|
||||||
|
|
|
||||||
|
|
@ -57,10 +57,37 @@ sudo systemctl daemon-reload
|
||||||
|
|
||||||
# Firewall crumbs — UFW rules mentioning either network (numbers shift after
|
# Firewall crumbs — UFW rules mentioning either network (numbers shift after
|
||||||
# each delete, so re-scan and remove the first match until none remain)
|
# each delete, so re-scan and remove the first match until none remain)
|
||||||
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 can exit early — the same discipline box-firewall.sh now uses, and for
|
||||||
|
# the same measured reason (#102). This file sets `pipefail` (line 12), so
|
||||||
|
# `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. A plainly-active UFW then reads as inactive
|
||||||
|
# and this entire block silently skips, leaving stale boxnet/claudenet rules
|
||||||
|
# on a host the operator was told is clean. It is a branch condition, so
|
||||||
|
# errexit never fires — there is no error to see, which is exactly why it
|
||||||
|
# went unnoticed here while the same shape was being measured next door.
|
||||||
|
#
|
||||||
|
# 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. 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 under
|
||||||
|
# pipefail+errexit that would kill a teardown instead of correctly deciding
|
||||||
|
# "no usable ufw here, nothing to clean".
|
||||||
|
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"
|
sudo ufw --force delete "$n"
|
||||||
done
|
done
|
||||||
done
|
done
|
||||||
|
|
|
||||||
21
test/cli.sh
21
test/cli.sh
|
|
@ -1695,6 +1695,27 @@ check "teardown-host: honors --yes/BOX_YES (CI runs it unattended)" 0 "" \
|
||||||
grep -qF 'BOX_YES' "$ROOT/host/teardown-host.sh"
|
grep -qF 'BOX_YES' "$ROOT/host/teardown-host.sh"
|
||||||
check "teardown-host: points at box uninstall when done" 0 "" \
|
check "teardown-host: points at box uninstall when done" 0 "" \
|
||||||
grep -qF "box uninstall" "$ROOT/host/teardown-host.sh"
|
grep -qF "box uninstall" "$ROOT/host/teardown-host.sh"
|
||||||
|
|
||||||
|
# #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
|
||||||
|
# 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 "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