fix(tests): running the suite deleted the router's own state — four packages did it
shater/stats/store_test.go ended with
_ = os.Remove(statsFilePath())
and statsFilePath() is not a test path. It is THE product path: /etc/shater/stats.db
on every host where that directory exists, which is the testbed and the router. So
`go test ./shater/...` deleted the accumulated query and connection log of whatever
machine ran it. The test passed. It had always passed — damage done by a test is a
side effect, not a wrong answer, and no instrument in this tree could see one.
A filesystem sweep (the new scripts/check-test-fs-isolation.sh: seed a router-shaped
canary tree in a container, run the whole gated suite, diff) found it was not alone.
Six packages, by measurement, not by reading:
shater/stats DELETED /etc/shater/stats.db (the line above; also
TestComboBackendSwitchSequence opened and pruned the
live DB, which the delete had been hiding)
shater/logsink DELETED /etc/shater/shaterd.log and /var/log/shaterd.log —
New()/Reconfigure() purge BOTH product locations when
the file toggle is off, so Config.Path (which every test
here already set) never protected them. The daemon's own
log, the one an operator reads after an outage.
shater/apply DELETED /var/run/shater.active — the ONE token hotplug and cron
check before touching the data plane. Clearing it on a
live router makes both stand down on a box that is up.
holdstate_test.go's `t.Cleanup(os.Remove(ActiveFlag))`
was not a cleanup; it was the delete.
shater/panel REWROTE /etc/shater/stats.db — stats.NewStore("sqlite") from
TestStatsEndpointsAcrossBackends resolves the product
path too.
shater/model CREATED /etc/shater/config.pre-v{0,1,2}.bak, config.pre-unreadable.bak
shater/generate REWROTE /etc/shater/cache.db
The last two are NOT fixed here — another agent is working in those trees. Both are
one TestMain away: model already has liveConfigPath/configBackupDir as vars, and
generate already has cacheFilePersistent; what leaks is product code (backupBeforeChange,
the engine's cache_file) called from tests that do not redirect them.
THE FIX is the seam generate/cache.go and generate/ruleset.go already use — the path
becomes a package-level var that only tests assign — plus, in each case, a test that
still pins the SHIPPED value, because an isolation that leaves the real decision
untested has only moved the defect:
stats: statsDirPersistent/statsFilePersistent/statsFileFallback + the exported
SetPathsForTest (exported because shater/panel needs it from outside).
New TestStatsFilePathPrefersPersistentDir covers both branches.
logsink: PersistPath/TmpfsPath + a TestMain, since the hazard is in New(), which
every test calls. New TestLogPathsAreTheShippedOnes.
apply: ActiveFlag + the existing TestMain. New TestActiveFlagIsTheShippedPath,
which also records WHY /var/run: tmpfs, so a reboot clears it.
TestNewStoreSelection got stronger rather than weaker. Its "sqlite" case used to
accept "sqlite" OR "memory" because the real path might not open on this host — an
expected value that depended on the machine. At a private path there is no excuse:
a writable directory MUST report "sqlite", and a new control at an unopenable path
MUST report "memory" (the honest "persistence is not active" signal) without a crash.
TWO GUARDS, because one of them cannot see half of it:
shater/testguard/fsisolation_test.go — parses every _test.go under shater/ and
fails BY NAME when a filesystem-mutating call gets a path that is not PROVABLY
temp-rooted. Positive and closed: what it cannot prove is a failure, not a
default, which is the only rule that catches a path built by a function call.
It follows local vars, closures, filepath.Join/Sprintf/+, helper parameters via
their call sites, helper return values, and the save/override/restore idiom.
Four waivers, each keyed on file+function+callee, each with the reason printed on
every run, each a struct field traced by hand; a waiver that stops matching fails
the test as STALE. Runs inside [2/7] and [4/7] — no new gate step, no new minute.
Blind spot, stated: damage done by PRODUCT code a test merely calls (which is
exactly logsink, model and generate above).
scripts/check-test-fs-isolation.sh — the dynamic half, for that blind spot. It
refuses to run outside a container unless told twice, because its method is to
let the damage happen and then look, and it seeds/unseeds only what was missing.
Verified:
- mutation, task 1: statsFilePath forced to the fallback -> the new path test
fails ("with ... present = .../fallback-stats.db, want the persistent ...");
newPersistent forced to memory -> "Backend = \"memory\", want \"sqlite\"";
the fallback made to report "sqlite" -> "Backend = \"sqlite\", want \"memory\"".
Green again after each revert.
- mutation, the guard: the original os.Remove(statsFilePath()) put back -> named
at store_test.go:154 with "the path comes out of statsFilePath(), which this
check cannot follow"; a planted test writing "/etc/config/network" -> named as
a literal path; the walk pointed at one package -> its own <150-file control
fires ("reading a blank page"); a waiver matching nothing -> STALE WAIVER.
- control, the sweep: with a planted violator it reports DELETED /etc/shater/stats.db
and MODIFIED /etc/config/network; without it, those are gone and only the two
foreign packages remain. Its bisect named shater/stats.TestComboBackendSwitchSequence
on its own.
- counts, declared vs executed (go test -list against top-level verdicts):
stats 112/112, panel 121/121, apply 122/122, logsink 26/26, testguard 1/1,
0 skips, 0 failures.
- scripts/run-tests.sh: [1/7][2/7][3/7][5/7][6/7][7/7] green. [4/7] -race fails on
four TestByeDPI* in shater/panel — a data race between byedpi.go's background
probe and byedpi_test.go's forceByeDPIBinary cleanup, in another agent's
uncommitted work (shater/panel/byedpi_cache_test.go is untracked). Proven not
ours: a pristine HEAD tree carrying ONLY this commit's files passes -race over
all 35 packages, and the same run with -skip ^TestByeDPI is green on the live
tree too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BHw89tdWddzhjUc4bAH4tS
This commit is contained in:
Claude Opus 5
@@ -0,0 +1,297 @@
|
||||
#!/usr/bin/env bash
|
||||
#
|
||||
# check-test-fs-isolation.sh — does running the test suite DAMAGE the machine?
|
||||
#
|
||||
# WHY THIS EXISTS (2026-07-27)
|
||||
# shater/stats/store_test.go called
|
||||
#
|
||||
# _ = os.Remove(statsFilePath())
|
||||
#
|
||||
# and statsFilePath() is not a test path — it is THE product path,
|
||||
# /etc/shater/stats.db on any host where /etc/shater exists. So on the testbed
|
||||
# (local_openwrt) and on the router itself, `go test ./shater/...` deleted the
|
||||
# accumulated statistics database. Nothing in the tree noticed, because the
|
||||
# test passed either way: the damage is not an assertion failure, it is a side
|
||||
# effect.
|
||||
#
|
||||
# No amount of grepping for "/etc" finds that one — the path is BUILT BY A
|
||||
# FUNCTION. The only instrument that sees it is the filesystem itself.
|
||||
#
|
||||
# WHAT IT DOES
|
||||
# 1. seeds a router-shaped canary tree (every persistent path the product code
|
||||
# names: /etc/shater/*, /etc/config/*, /var/run/*, /tmp/shater-*, ...),
|
||||
# because a delete of a file that does not exist leaves no trace at all —
|
||||
# an unseeded run is an instrument that cannot see the very defect it was
|
||||
# written for;
|
||||
# 2. snapshots /etc /var /usr /root /home /opt /srv /run /tmp (path, type,
|
||||
# size, mtime);
|
||||
# 3. runs the whole Go suite under the SHIPPED tags with TMPDIR pointed at a
|
||||
# private directory, so t.TempDir()/os.MkdirTemp land somewhere the
|
||||
# snapshot ignores and everything left in /tmp is a HARDCODED name;
|
||||
# 4. snapshots again and reports every created / deleted / modified path BY
|
||||
# NAME. Any difference fails.
|
||||
# 5. on a difference, BISECTS: re-runs package by package and then test by
|
||||
# test inside the guilty package, so the report names the test, not just
|
||||
# the file it ate. That costs time only on a failing run.
|
||||
#
|
||||
# CONTAINER ONLY, AND THAT IS THE POINT
|
||||
# The check works by letting the tests do their worst and then looking. Doing
|
||||
# that on a real host means doing the damage on that host — on the router,
|
||||
# this script's own method would eat the stats DB it is meant to protect. So
|
||||
# it refuses to run outside a container unless SHATER_FS_CHECK_I_KNOW=1.
|
||||
#
|
||||
# Usage:
|
||||
# scripts/check-test-fs-isolation.sh # re-execs into docker
|
||||
# SHATER_FS_CHECK_IN_DOCKER=1 ... # set by the re-exec; run in place
|
||||
#
|
||||
# Env:
|
||||
# SHATER_GO_IMAGE docker image for the re-exec (default golang:1.26)
|
||||
# SHATER_FS_CHECK_SKIP_BISECT=1 report paths only, do not hunt the test
|
||||
set -euo pipefail
|
||||
|
||||
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||
REPO="$(cd "$SCRIPT_DIR/.." && pwd)"
|
||||
cd "$REPO"
|
||||
|
||||
# shellcheck source=router-tags.sh
|
||||
. "$SCRIPT_DIR/router-tags.sh"
|
||||
|
||||
# The same ROOTS run-tests.sh gates, so this check covers exactly the suite that
|
||||
# gate runs — no more (an untested package cannot damage anything) and no less.
|
||||
# common/ is run separately, minus its CAP_NET_ADMIN integration tests: without
|
||||
# the capability those FAIL rather than skip (see run-tests.sh SKIP_COMMON), and
|
||||
# a run that dies early is a run whose filesystem verdict covers less than it
|
||||
# claims.
|
||||
PKG_ROOTS=(./shater/... ./protocol/... ./transport/... ./adapter/... ./route/... ./option/... ./dns/...)
|
||||
PKG_ROOTS_COMMON=(./common/...)
|
||||
SKIP_COMMON='^TestIntegration'
|
||||
|
||||
# --- re-exec into a container ------------------------------------------------
|
||||
if [ "${SHATER_FS_CHECK_IN_DOCKER:-0}" != "1" ]; then
|
||||
if [ "${SHATER_FS_CHECK_I_KNOW:-0}" = "1" ]; then
|
||||
echo "== fs-isolation check: running IN PLACE on this host (SHATER_FS_CHECK_I_KNOW=1) =="
|
||||
echo " Any test that writes outside its temp dir will do so HERE, for real."
|
||||
else
|
||||
image="${SHATER_GO_IMAGE:-golang:1.26}"
|
||||
echo "== fs-isolation check: re-exec into docker ($image) =="
|
||||
host_repo="$REPO"
|
||||
command -v cygpath >/dev/null 2>&1 && host_repo="$(cygpath -w "$REPO")"
|
||||
exec env MSYS2_ARG_CONV_EXCL='*' MSYS_NO_PATHCONV=1 docker run --rm \
|
||||
-v "$host_repo":/src \
|
||||
-v shater-tagcheck-gomod:/go/pkg/mod \
|
||||
-v shater-tagcheck-gocache:/root/.cache/go-build \
|
||||
-w /src \
|
||||
-e SHATER_FS_CHECK_IN_DOCKER=1 \
|
||||
-e SHATER_FS_CHECK_SKIP_BISECT="${SHATER_FS_CHECK_SKIP_BISECT:-0}" \
|
||||
"$image" bash scripts/check-test-fs-isolation.sh
|
||||
fi
|
||||
fi
|
||||
|
||||
WATCH=(/etc /var /usr /root /home /opt /srv /run /tmp)
|
||||
PRIVATE_TMP=/tmp/shater-fscheck-tmp
|
||||
STATE=/tmp/shater-fscheck-state
|
||||
mkdir -p "$PRIVATE_TMP" "$STATE"
|
||||
|
||||
# Snapshot. -xdev keeps the docker volume mounts (/go, /root/.cache/go-build,
|
||||
# /src) out by construction; the prunes cover what lives on the container's own
|
||||
# filesystem and legitimately churns.
|
||||
# PRUNED, each for a stated reason — this list is the instrument's blind spot and
|
||||
# every entry widens it:
|
||||
# $PRIVATE_TMP where TMPDIR sends t.TempDir()/os.MkdirTemp. Excluding it is the
|
||||
# whole trick: what is left in /tmp is a HARDCODED name.
|
||||
# $STATE this script's own scratch.
|
||||
# /root/.config the Go TOOLCHAIN's telemetry counters (…/go/telemetry/*.count),
|
||||
# rewritten by every `go` invocation including this script's own.
|
||||
# Not a test, and not something a test can be blamed for.
|
||||
# /root/.cache /root/go /var/cache /var/lib/apt /var/log/apt
|
||||
# build caches and the package manager: churn owned by the image.
|
||||
# -xdev keeps the docker volume mounts (/go, /root/.cache/go-build, /src) out by
|
||||
# construction.
|
||||
snapshot() { # $1 = output file
|
||||
find "${WATCH[@]}" -xdev \
|
||||
\( -path "$PRIVATE_TMP" -o -path "$STATE" -o -path /var/cache -o -path /var/lib/apt \
|
||||
-o -path /var/log/apt -o -path /root/.cache -o -path /root/go -o -path /root/.config \) -prune -o \
|
||||
-printf '%p\t%y\t%s\t%T@\n' 2>/dev/null \
|
||||
| awk -F'\t' 'BEGIN{OFS="\t"} $2=="d"{$3="-";$4="-"} {print}' | sort >"$1"
|
||||
}
|
||||
# ...why a directory's size and mtime are dropped: a directory's mtime moves
|
||||
# whenever anything inside it is created or removed, so keeping it would report
|
||||
# /etc/shater a second time for the child that is already named on its own line,
|
||||
# and would report /root every run because the Go toolchain writes telemetry
|
||||
# counters under it. Directories are still reported when they are CREATED or
|
||||
# DELETED. The blind spot this leaves is a file created and removed again inside
|
||||
# the same run — which the file-level snapshot cannot see either way.
|
||||
|
||||
# run_suite_quiet runs the whole gated suite the way run-tests.sh does. Its exit
|
||||
# status is deliberately NOT this check's verdict — assertions are run-tests.sh's
|
||||
# job; here the only question is what the run left behind.
|
||||
run_suite_quiet() { # $1 = log file (appended)
|
||||
TMPDIR="$PRIVATE_TMP" go test -count=1 -timeout 20m \
|
||||
-tags "$SHATER_ROUTER_TAGS" -ldflags "$SHATER_ROUTER_LDFLAGS" \
|
||||
"${PKG_ROOTS[@]}" >>"$1" 2>&1 || true
|
||||
TMPDIR="$PRIVATE_TMP" go test -count=1 -timeout 20m -skip "$SKIP_COMMON" \
|
||||
-tags "$SHATER_ROUTER_TAGS" -ldflags "$SHATER_ROUTER_LDFLAGS" \
|
||||
"${PKG_ROOTS_COMMON[@]}" >>"$1" 2>&1 || true
|
||||
}
|
||||
|
||||
# --- the canary tree ---------------------------------------------------------
|
||||
# Every persistent path named by a string literal in the product code (grep for
|
||||
# "/etc|/var|/tmp|/usr in shater/**/*.go, minus _test.go) plus the directories
|
||||
# they live in. A path that exists can be deleted, truncated or overwritten
|
||||
# VISIBLY; a path that does not exist absorbs os.Remove without a trace.
|
||||
CANARY_FILES=(
|
||||
/etc/shater/stats.db
|
||||
/etc/shater/cache.db
|
||||
/etc/shater/shaterd.log
|
||||
/etc/shater/alert-state.json
|
||||
/etc/shater/boot.nft
|
||||
/etc/config/shater
|
||||
/etc/config/network
|
||||
/etc/config/byedpi
|
||||
/etc/config/firewall
|
||||
/etc/config/dhcp
|
||||
/var/run/shaterd.pid
|
||||
/var/run/shaterd.ctl
|
||||
/var/run/shater.restarting
|
||||
/var/run/shater.active
|
||||
/var/log/shaterd.log
|
||||
/var/lock/shater.lock
|
||||
/usr/bin/ciadpi
|
||||
/tmp/dhcp.leases
|
||||
/tmp/shater-stats.db
|
||||
/tmp/shater-cache.db
|
||||
/tmp/ads.lst
|
||||
)
|
||||
CANARY_DIRS=(
|
||||
/etc/shater/lists
|
||||
/etc/shater/subs
|
||||
/etc/shater/history
|
||||
/tmp/shater-subs
|
||||
/tmp/shater-lists
|
||||
/tmp/.uci
|
||||
)
|
||||
|
||||
# Only ever CREATE what is missing, and record it, so the canaries can be taken
|
||||
# away again on exit. That matters for the SHATER_FS_CHECK_I_KNOW=1 path: seeding
|
||||
# a router with a fake /etc/shater/stats.db and walking away would be this
|
||||
# script committing the very sin it hunts. A path that already existed is never
|
||||
# written and never removed.
|
||||
seed_canaries() {
|
||||
local f d
|
||||
for d in "${CANARY_DIRS[@]}"; do
|
||||
[ -d "$d" ] || { mkdir -p "$d" && echo "$d" >>"$STATE/seeded-dirs"; }
|
||||
done
|
||||
for f in "${CANARY_FILES[@]}"; do
|
||||
mkdir -p "$(dirname "$f")"
|
||||
[ -e "$f" ] || {
|
||||
printf 'canary: if this file changed, a test wrote outside its temp dir\n' >"$f"
|
||||
echo "$f" >>"$STATE/seeded-files"
|
||||
}
|
||||
done
|
||||
}
|
||||
|
||||
# unseed removes exactly what seed_canaries created, newest first for the dirs.
|
||||
unseed() {
|
||||
local p
|
||||
if [ -f "$STATE/seeded-files" ]; then
|
||||
while read -r p; do [ -n "$p" ] && rm -f "$p"; done < <(sort -u "$STATE/seeded-files")
|
||||
fi
|
||||
if [ -f "$STATE/seeded-dirs" ]; then
|
||||
while read -r p; do [ -n "$p" ] && rmdir "$p" 2>/dev/null || true; done < <(sort -ur "$STATE/seeded-dirs")
|
||||
fi
|
||||
}
|
||||
trap unseed EXIT
|
||||
|
||||
echo "== [1/3] seeding the router-shaped canary tree =="
|
||||
seed_canaries
|
||||
echo " ${#CANARY_FILES[@]} files, ${#CANARY_DIRS[@]} dirs seeded under /etc /var /usr /tmp"
|
||||
echo " tags: $SHATER_ROUTER_TAGS"
|
||||
echo
|
||||
|
||||
echo "== [2/3] snapshot -> run the suite -> snapshot =="
|
||||
snapshot "$STATE/before"
|
||||
echo " before: $(wc -l <"$STATE/before" | tr -d ' ') paths"
|
||||
: >"$STATE/testlog"
|
||||
run_suite_quiet "$STATE/testlog"
|
||||
echo " suites run: $(grep -cE '^(ok|FAIL|---)' "$STATE/testlog" || true) package verdicts (assertions are run-tests.sh's job)"
|
||||
snapshot "$STATE/after"
|
||||
echo " after : $(wc -l <"$STATE/after" | tr -d ' ') paths"
|
||||
echo
|
||||
|
||||
echo "== [3/3] verdict =="
|
||||
# The control: a snapshot that came back empty would make every run "clean".
|
||||
if [ "$(wc -l <"$STATE/before" | tr -d ' ')" -lt 100 ]; then
|
||||
echo " FAILED: the before-snapshot has fewer than 100 paths — find(1) is not" >&2
|
||||
echo " reading the tree, so this check's silence means nothing." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
diff_out="$(diff "$STATE/before" "$STATE/after" || true)"
|
||||
if [ -z "$diff_out" ]; then
|
||||
echo " CLEAN: the whole suite ran and not one path under ${WATCH[*]} changed."
|
||||
echo " (t.TempDir/os.MkdirTemp went to $PRIVATE_TMP, which is excluded"
|
||||
echo " by design — everything else is a hardcoded, persistent name.)"
|
||||
exit 0
|
||||
fi
|
||||
|
||||
# Name what moved, by path, split into the three ways it can move.
|
||||
awk -F'\t' '/^</ {print $1}' <<<"$diff_out" | sed 's/^< //' | cut -f1 | sort >"$STATE/gone_or_changed"
|
||||
awk -F'\t' '/^>/ {print $1}' <<<"$diff_out" | sed 's/^> //' | cut -f1 | sort >"$STATE/new_or_changed"
|
||||
comm -23 "$STATE/gone_or_changed" "$STATE/new_or_changed" >"$STATE/deleted"
|
||||
comm -13 "$STATE/gone_or_changed" "$STATE/new_or_changed" >"$STATE/created"
|
||||
comm -12 "$STATE/gone_or_changed" "$STATE/new_or_changed" >"$STATE/modified"
|
||||
|
||||
echo " THE SUITE CHANGED THE MACHINE. On the router or the testbed these are not"
|
||||
echo " scratch files — they are the product's state."
|
||||
while read -r p; do [ -n "$p" ] && echo " DELETED $p" >&2; done <"$STATE/deleted"
|
||||
while read -r p; do [ -n "$p" ] && echo " CREATED $p" >&2; done <"$STATE/created"
|
||||
while read -r p; do [ -n "$p" ] && echo " MODIFIED $p" >&2; done <"$STATE/modified"
|
||||
echo
|
||||
|
||||
if [ "${SHATER_FS_CHECK_SKIP_BISECT:-0}" = "1" ]; then
|
||||
echo " (bisect disabled by SHATER_FS_CHECK_SKIP_BISECT=1 — the paths above are"
|
||||
echo " the whole report)"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# --- bisect: which package, then which test ----------------------------------
|
||||
# Only on a failing run, so the cost is paid exactly when it buys something.
|
||||
echo " hunting the culprit — package by package, then test by test:"
|
||||
pkgs="$(go list -tags "$SHATER_ROUTER_TAGS" \
|
||||
-f '{{if or .TestGoFiles .XTestGoFiles}}{{.ImportPath}}{{end}}' \
|
||||
"${PKG_ROOTS[@]}" "${PKG_ROOTS_COMMON[@]}" | grep -v '^$')"
|
||||
found=0
|
||||
while read -r pkg; do
|
||||
[ -n "$pkg" ] || continue
|
||||
seed_canaries
|
||||
snapshot "$STATE/b1"
|
||||
TMPDIR="$PRIVATE_TMP" go test -count=1 -timeout 10m \
|
||||
-tags "$SHATER_ROUTER_TAGS" -ldflags "$SHATER_ROUTER_LDFLAGS" "$pkg" >/dev/null 2>&1 || true
|
||||
snapshot "$STATE/a1"
|
||||
if diff -q "$STATE/b1" "$STATE/a1" >/dev/null; then
|
||||
continue
|
||||
fi
|
||||
echo " PACKAGE $pkg" >&2
|
||||
found=1
|
||||
# ...and now the tests of that package, one at a time.
|
||||
tests="$(go test -list '.*' -tags "$SHATER_ROUTER_TAGS" -ldflags "$SHATER_ROUTER_LDFLAGS" "$pkg" 2>/dev/null | grep -E '^(Test|Fuzz|Example)' || true)"
|
||||
while read -r tn; do
|
||||
[ -n "$tn" ] || continue
|
||||
seed_canaries
|
||||
snapshot "$STATE/b2"
|
||||
TMPDIR="$PRIVATE_TMP" go test -count=1 -timeout 10m -run "^${tn}\$" \
|
||||
-tags "$SHATER_ROUTER_TAGS" -ldflags "$SHATER_ROUTER_LDFLAGS" "$pkg" >/dev/null 2>&1 || true
|
||||
snapshot "$STATE/a2"
|
||||
if ! diff -q "$STATE/b2" "$STATE/a2" >/dev/null; then
|
||||
echo " TEST ${pkg}.${tn}" >&2
|
||||
diff "$STATE/b2" "$STATE/a2" | grep -E '^[<>]' | sed 's/^/ | /' | cut -f1 | sort -u >&2
|
||||
fi
|
||||
done <<<"$tests"
|
||||
done <<<"$pkgs"
|
||||
if [ "$found" -eq 0 ]; then
|
||||
echo " no single package reproduced it — the change needs the packages running" >&2
|
||||
echo " together, or something outside go test moved. Read $STATE/before vs after." >&2
|
||||
fi
|
||||
echo
|
||||
echo "== FS ISOLATION CHECK FAILED — a test writes outside its own temp directory. ==" >&2
|
||||
exit 1
|
||||
@@ -53,6 +53,25 @@
|
||||
# A guard that silently runs nothing is worse than no guard (same rule as
|
||||
# scripts/check-router-tags.sh).
|
||||
#
|
||||
# WHAT IT DOES NOT SEE, AND WHAT DOES (2026-07-27)
|
||||
# Everything above is about whether a test RAN and what it ASSERTED. None of it
|
||||
# can see what a test DID to the machine. shater/stats/store_test.go ended with
|
||||
# `os.Remove(statsFilePath())` — the PRODUCT path — so every run of this gate on
|
||||
# the testbed or the router deleted /etc/shater/stats.db, and every instrument
|
||||
# here reported `ok shater/stats`. Six packages were doing something of the kind.
|
||||
# Two instruments now cover it, and neither is inside the steps below:
|
||||
# - shater/testguard/fsisolation_test.go — a Go test that reads the SOURCE of
|
||||
# every _test.go under shater/ and fails BY NAME when a filesystem-mutating
|
||||
# call is handed a path that is not provably a temp dir. It runs as part of
|
||||
# [2/7] and [4/7] like any other test, so it needs no step of its own; what
|
||||
# it cannot see is damage done by PRODUCT code that a test merely calls.
|
||||
# - scripts/check-test-fs-isolation.sh — the dynamic half, for exactly that
|
||||
# blind spot: it seeds a router-shaped canary tree in a container, runs this
|
||||
# whole suite, and diffs. NOT run from here on purpose — it costs a second
|
||||
# full suite, and its method (let the damage happen, then look) must never
|
||||
# be pointed at a real /etc. Run it by hand when tests touch anything that
|
||||
# resolves a product path.
|
||||
#
|
||||
# Usage:
|
||||
# scripts/run-tests.sh # full gate (~3 min warm on the runner)
|
||||
# scripts/run-tests.sh --no-race # skip the -race pass (faster; local loop)
|
||||
|
||||
+10
-1
@@ -44,7 +44,16 @@ import (
|
||||
// ActiveFlag is raised on a successful enabled apply and cleared on teardown. It
|
||||
// is the ONLY token that lets hotplug/cron touch the data plane (tmpfs => cleared
|
||||
// by reboot, so nothing reconciles before the init has run at boot).
|
||||
const ActiveFlag = "/var/run/shater.active"
|
||||
//
|
||||
// A VAR, not a const, and only so this package's tests can point it at a temp
|
||||
// directory (apply_test.go's TestMain); production never assigns it. The reason:
|
||||
// the tests here run applyLocked and Teardown for real, which raise and clear
|
||||
// this flag — so with the shipped value compiled in, `go test ./shater/apply/`
|
||||
// removed /var/run/shater.active on whatever machine ran it. On a live router
|
||||
// that is the token hotplug and cron check before reconciling, and clearing it
|
||||
// makes them stand down on a box that IS active. Caught 2026-07-27 by
|
||||
// scripts/check-test-fs-isolation.sh.
|
||||
var ActiveFlag = "/var/run/shater.active"
|
||||
|
||||
// Applier owns the shared engine and the last-good model for rollback. All
|
||||
// exported methods are safe for concurrent use and serialize through both an
|
||||
|
||||
@@ -36,13 +36,41 @@ import (
|
||||
// performs still run for real from here. They are left alone because nothing
|
||||
// else in the gate reads those tables, so unlike the devices they have no
|
||||
// observed victim — not because they are harmless on a machine that matters.
|
||||
// It ALSO points ActiveFlag at a private directory, for a second reason of the same
|
||||
// shape. applyLocked raises that flag and Teardown clears it, both for real from here,
|
||||
// and the shipped value is /var/run/shater.active — the ONE token hotplug and cron check
|
||||
// before they touch the data plane. So `go test ./shater/apply/` used to delete it on
|
||||
// whatever machine ran it: on the testbed and on the router, that tells the reconcilers
|
||||
// the stack is down while it is up, and they stand down. The old
|
||||
// `t.Cleanup(func() { _ = os.Remove(ActiveFlag) })` in holdstate_test.go was not a
|
||||
// cleanup at all — it was the delete. Caught 2026-07-27 by
|
||||
// scripts/check-test-fs-isolation.sh; realActiveFlag below keeps the shipped value so
|
||||
// the product decision is still asserted (TestActiveFlagIsTheShippedPath).
|
||||
var realActiveFlag = ActiveFlag
|
||||
|
||||
func TestMain(m *testing.M) {
|
||||
restore := netplane.L3StubKernelForTest()
|
||||
dir, err := os.MkdirTemp("", "apply-active-flag")
|
||||
if err != nil {
|
||||
panic("apply TestMain: cannot make a private dir for ActiveFlag: " + err.Error())
|
||||
}
|
||||
ActiveFlag = dir + "/shater.active"
|
||||
code := m.Run()
|
||||
_ = os.RemoveAll(dir)
|
||||
restore()
|
||||
os.Exit(code)
|
||||
}
|
||||
|
||||
// TestActiveFlagIsTheShippedPath pins what TestMain took away. The location is not
|
||||
// cosmetic: /var/run is tmpfs, so a reboot clears the flag and nothing reconciles the
|
||||
// data plane before the init script has run. Moving it to persistent storage would let a
|
||||
// cron tick touch the plane on a box that has not started yet.
|
||||
func TestActiveFlagIsTheShippedPath(t *testing.T) {
|
||||
if realActiveFlag != "/var/run/shater.active" {
|
||||
t.Errorf("ActiveFlag = %q, want /var/run/shater.active (tmpfs: cleared by a reboot, so nothing reconciles before the init has run)", realActiveFlag)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCanRollback pins the signal the panel gates its rollback control on:
|
||||
// canRollback is false on a fresh applier (no armed commit-confirm snapshot AND
|
||||
// the engine holds no last-good predecessor), and flips to true once a snapshot
|
||||
|
||||
@@ -99,6 +99,10 @@ func TestHoldingSeesAPlaneThisProcessDidNotInstall(t *testing.T) {
|
||||
func TestSuccessfulApplyStillClearsTheHold(t *testing.T) {
|
||||
a := New(engine.New(), nil)
|
||||
t.Cleanup(func() { _ = a.eng.Close() })
|
||||
// ActiveFlag points into this binary's private temp dir (apply_test.go's TestMain),
|
||||
// so removing it here removes a test artifact — which is what this line always
|
||||
// claimed to be doing and, until 2026-07-27, was not: it was `rm /var/run/shater.active`
|
||||
// on the machine running the suite.
|
||||
t.Cleanup(func() { _ = os.Remove(ActiveFlag) })
|
||||
|
||||
// A REAL started instance: the derived half asks the engine, so a stub would
|
||||
|
||||
@@ -63,13 +63,23 @@ import (
|
||||
"github.com/sagernet/sing-box/shater/model"
|
||||
)
|
||||
|
||||
const (
|
||||
// PersistPath / TmpfsPath are the two file locations Globals.LogPersist
|
||||
// selects between: /etc/shater survives a reboot (and wears flash),
|
||||
// /var/log is tmpfs (free writes, lost on reboot).
|
||||
// PersistPath / TmpfsPath are the two file locations Globals.LogPersist selects
|
||||
// between: /etc/shater survives a reboot (and wears flash), /var/log is tmpfs
|
||||
// (free writes, lost on reboot).
|
||||
//
|
||||
// VARS, not consts, and only so this package's own tests can point them at a
|
||||
// temp tree; production never assigns them. The reason is specific: New() and
|
||||
// Reconfigure() PURGE both locations (purgeLogFiles) when the file toggle is
|
||||
// off, so with the shipped values compiled in, `go test ./shater/logsink/`
|
||||
// deleted the daemon's real log on whatever machine ran it — the testbed and
|
||||
// the router included. Same seam generate/cache.go uses, for the same class of
|
||||
// defect.
|
||||
var (
|
||||
PersistPath = "/etc/shater/shaterd.log"
|
||||
TmpfsPath = "/var/log/shaterd.log"
|
||||
)
|
||||
|
||||
const (
|
||||
// maxPartialLine caps the un-terminated tail buffered while waiting for a
|
||||
// '\n'. A producer that never sends one (should not happen — both log
|
||||
// formatters terminate every message) would otherwise grow the buffer
|
||||
|
||||
@@ -17,6 +17,72 @@ import (
|
||||
"github.com/sagernet/sing-box/option"
|
||||
)
|
||||
|
||||
// realPersistPath / realTmpfsPath keep the SHIPPED values of PersistPath and TmpfsPath,
|
||||
// which TestMain below replaces for the rest of the binary. Nothing else may read them:
|
||||
// they exist so TestLogPathsAreTheShippedOnes can still check the product decision after
|
||||
// the isolation, because an isolation that leaves the real decision untested has only
|
||||
// moved the defect.
|
||||
var realPersistPath, realTmpfsPath = PersistPath, TmpfsPath
|
||||
|
||||
// TestMain points the two PRODUCT log locations at a private directory for the whole test
|
||||
// binary.
|
||||
//
|
||||
// WHY. Config.Path already redirects the file a Sink WRITES, and every test here uses it —
|
||||
// but that is not the only thing this package touches. New() and Reconfigure() call
|
||||
// purgeLogFiles(cfg.path(), PersistPath, TmpfsPath), which unlinks `<p>` and `<p>.1` at
|
||||
// BOTH product locations, deliberately: a daemon booting with the file toggle off must not
|
||||
// present segments from a previous life. With the real values compiled in, that made
|
||||
// `go test ./shater/logsink/` run
|
||||
//
|
||||
// rm -f /etc/shater/shaterd.log /etc/shater/shaterd.log.1 \
|
||||
// /var/log/shaterd.log /var/log/shaterd.log.1
|
||||
//
|
||||
// on whatever machine ran it — the testbed and the router included, where those are the
|
||||
// daemon's own log, the one an operator reads after an outage. Caught 2026-07-27 by
|
||||
// scripts/check-test-fs-isolation.sh; no assertion in this package could have seen it,
|
||||
// because deleting the wrong file is a side effect, not a wrong answer.
|
||||
//
|
||||
// A TestMain rather than a per-test helper because the hazard is not in any one test: it
|
||||
// is in New(), which every test calls, including the ones written next year.
|
||||
func TestMain(m *testing.M) {
|
||||
dir, err := os.MkdirTemp("", "logsink-product-paths")
|
||||
if err != nil {
|
||||
fmt.Fprintf(os.Stderr, "logsink TestMain: cannot make a private dir for the product log paths: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
PersistPath = filepath.Join(dir, "flash", "shaterd.log")
|
||||
TmpfsPath = filepath.Join(dir, "tmpfs", "shaterd.log")
|
||||
for _, d := range []string{filepath.Dir(PersistPath), filepath.Dir(TmpfsPath)} {
|
||||
if err := os.MkdirAll(d, 0o755); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "logsink TestMain: mkdir %s: %v\n", d, err)
|
||||
os.Exit(1)
|
||||
}
|
||||
}
|
||||
code := m.Run()
|
||||
_ = os.RemoveAll(dir)
|
||||
os.Exit(code)
|
||||
}
|
||||
|
||||
// TestLogPathsAreTheShippedOnes pins what TestMain took away: the two locations
|
||||
// Globals.LogPersist selects between, and the selection itself. /etc/shater survives a
|
||||
// reboot and wears flash; /var/log is tmpfs. Swapping them is a silent product change —
|
||||
// the persistent choice would stop persisting — and after the isolation above nothing
|
||||
// else in this package would notice.
|
||||
func TestLogPathsAreTheShippedOnes(t *testing.T) {
|
||||
if realPersistPath != "/etc/shater/shaterd.log" {
|
||||
t.Errorf("PersistPath = %q, want /etc/shater/shaterd.log (the flash location that survives a reboot)", realPersistPath)
|
||||
}
|
||||
if realTmpfsPath != "/var/log/shaterd.log" {
|
||||
t.Errorf("TmpfsPath = %q, want /var/log/shaterd.log (tmpfs: free writes, lost on reboot)", realTmpfsPath)
|
||||
}
|
||||
if got := FilePath(true); got != PersistPath {
|
||||
t.Errorf("FilePath(persist=true) = %q, want PersistPath %q", got, PersistPath)
|
||||
}
|
||||
if got := FilePath(false); got != TmpfsPath {
|
||||
t.Errorf("FilePath(persist=false) = %q, want TmpfsPath %q", got, TmpfsPath)
|
||||
}
|
||||
}
|
||||
|
||||
// fileCfg returns a Config writing to a temp file, with the given toggles.
|
||||
func fileCfg(t *testing.T, toSyslog, toFile bool, maxKB int) Config {
|
||||
t.Helper()
|
||||
|
||||
@@ -5,6 +5,8 @@ import (
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
@@ -279,10 +281,23 @@ func arraysNonNull(t *testing.T, label string, body []byte) {
|
||||
// combination, extended to every backend and every stats route. Each must answer 200
|
||||
// with a JSON ARRAY (never null) for the log routes and a fully-populated snapshot
|
||||
// object for /api/stats — regardless of which store is attached, including none.
|
||||
//
|
||||
// The "sqlite" leg builds a REAL persistent store, so the DB path is pointed at this
|
||||
// test's own directory first. Without that, stats.NewStore("sqlite") resolves the
|
||||
// PRODUCT path — /etc/shater/stats.db on the testbed and on the router — and this test
|
||||
// opens, writes and prunes the live query and connection log of the machine running it.
|
||||
// That is not hypothetical: the filesystem sweep of 2026-07-27
|
||||
// (scripts/check-test-fs-isolation.sh) caught this package rewriting that file.
|
||||
func TestStatsEndpointsAcrossBackends(t *testing.T) {
|
||||
backends := []string{"off", "memory", "sqlite"}
|
||||
for _, backend := range backends {
|
||||
t.Run(backend, func(t *testing.T) {
|
||||
dbDir := filepath.Join(t.TempDir(), "shater")
|
||||
if err := os.MkdirAll(dbDir, 0o755); err != nil {
|
||||
t.Fatalf("mkdir %s: %v", dbDir, err)
|
||||
}
|
||||
t.Cleanup(stats.SetPathsForTest(dbDir))
|
||||
|
||||
s := newTestServer(t)
|
||||
store := stats.NewStore(backend, nil, nil, stats.Config{RingSize: 32})
|
||||
defer store.Close()
|
||||
|
||||
@@ -503,7 +503,13 @@ func TestComboUnlimitedRingWithRetentionDisabled(t *testing.T) {
|
||||
// admin does by editing Globals.StatsBackend and restarting the daemon), asserting each
|
||||
// store stands up, serves reads and closes cleanly, and that closing one never disturbs
|
||||
// the next.
|
||||
//
|
||||
// The "sqlite" leg opens a REAL bbolt DB, so the path is isolated first: without that it
|
||||
// opens /etc/shater/stats.db — the router's live query and connection log — and lets
|
||||
// retention run against it. The damage was invisible here because a neighbouring test in
|
||||
// this package deleted the same file afterwards.
|
||||
func TestComboBackendSwitchSequence(t *testing.T) {
|
||||
isolateStatsPaths(t)
|
||||
for _, backend := range []string{"memory", "sqlite", "off", "memory", "", "nonsense"} {
|
||||
st := NewStore(backend, nil, nil, Config{RingSize: 16})
|
||||
st.Start()
|
||||
|
||||
@@ -8,6 +8,7 @@ import (
|
||||
"io"
|
||||
"math"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
@@ -22,7 +23,19 @@ var _ logRing = (*boltRing)(nil)
|
||||
// DB-path selection, mirroring generate/cache.go. The persistent /etc/shater dir survives
|
||||
// a reboot (so the two logs persist across a daemon restart); we fall back to tmpfs when
|
||||
// that directory does not exist yet, because the DB engine creates the FILE but not the DIR.
|
||||
const (
|
||||
//
|
||||
// The three paths are VARS, not consts, and only so a test can point them at a temp tree
|
||||
// (SetPathsForTest below); production never assigns them. Same seam generate/cache.go and
|
||||
// generate/ruleset.go already use, and for a sharper reason than either: these are the
|
||||
// paths of the router's LIVE statistics database, and until 2026-07-27
|
||||
// shater/stats/store_test.go ended with
|
||||
//
|
||||
// _ = os.Remove(statsFilePath())
|
||||
//
|
||||
// which on the testbed and on the router itself is `rm /etc/shater/stats.db` — the whole
|
||||
// accumulated query and connection log, deleted by running `go test`. The test passed
|
||||
// either way: the damage is a side effect, not an assertion.
|
||||
var (
|
||||
statsDirPersistent = "/etc/shater"
|
||||
statsFilePersistent = "/etc/shater/stats.db"
|
||||
statsFileFallback = "/tmp/shater-stats.db"
|
||||
@@ -37,6 +50,28 @@ func statsFilePath() string {
|
||||
return statsFileFallback
|
||||
}
|
||||
|
||||
// SetPathsForTest points the DB-path decision above at dir and returns the function that
|
||||
// restores the real paths. TEST-ONLY: nothing in the daemon calls it, and it is not a
|
||||
// configuration knob — the location is a product decision, not an operator's.
|
||||
//
|
||||
// It is EXPORTED because the caller that needs it most is in another package:
|
||||
// shater/panel's TestStatsEndpointsAcrossBackends builds a real persistent store through
|
||||
// stats.NewStore("sqlite", ...) to check the HTTP contract, and without this seam that
|
||||
// test opens, writes and prunes the router's live stats.db.
|
||||
//
|
||||
// dir is used as the "persistent" directory; it must exist for statsFilePath to choose the
|
||||
// persistent branch, which is what a caller wanting a working DB wants. The tmpfs fallback
|
||||
// is pointed inside dir as well, so neither branch can escape into the real filesystem.
|
||||
func SetPathsForTest(dir string) (restore func()) {
|
||||
prevDir, prevFile, prevFallback := statsDirPersistent, statsFilePersistent, statsFileFallback
|
||||
statsDirPersistent = dir
|
||||
statsFilePersistent = filepath.Join(dir, "stats.db")
|
||||
statsFileFallback = filepath.Join(dir, "fallback-stats.db")
|
||||
return func() {
|
||||
statsDirPersistent, statsFilePersistent, statsFileFallback = prevDir, prevFile, prevFallback
|
||||
}
|
||||
}
|
||||
|
||||
// Async-writer tuning. The DNS/conn hot path never blocks on disk: appends do a
|
||||
// non-blocking send onto writeCh (dropping + counting on overflow), and a single writer
|
||||
// goroutine batches puts into one bbolt write transaction, committing at batchSize rows
|
||||
|
||||
@@ -2,9 +2,57 @@ package stats
|
||||
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// isolateStatsPaths points the DB-path decision (boltring.go) at a private directory
|
||||
// for the duration of one test.
|
||||
//
|
||||
// WHY IT EXISTS. statsFilePath() is not a test path — it is THE product path,
|
||||
// /etc/shater/stats.db on every host where that directory exists, which is the testbed
|
||||
// and the router. Until 2026-07-27 TestNewStoreSelection below opened a real persistent
|
||||
// store at it and then finished with `os.Remove(statsFilePath())`, i.e. `go test`
|
||||
// deleted the router's accumulated statistics database. Nothing failed: the damage is a
|
||||
// side effect, and a side effect is invisible to every assertion in the tree.
|
||||
//
|
||||
// The directory is CREATED, so the persistent branch is the one taken and the store this
|
||||
// returns is a real bbolt-backed one — an isolation that quietly forced the memory
|
||||
// fallback would leave the persistent path untested, which is the same defect pointed
|
||||
// the other way.
|
||||
func isolateStatsPaths(t *testing.T) string {
|
||||
t.Helper()
|
||||
dir := filepath.Join(t.TempDir(), "shater")
|
||||
if err := os.MkdirAll(dir, 0o755); err != nil {
|
||||
t.Fatalf("mkdir %s: %v", dir, err)
|
||||
}
|
||||
t.Cleanup(SetPathsForTest(dir))
|
||||
return dir
|
||||
}
|
||||
|
||||
// TestStatsFilePathPrefersPersistentDir pins the decision the seam above overrides, which
|
||||
// otherwise nothing would test any more: an existing /etc/shater wins, an absent one falls
|
||||
// back to tmpfs. Both halves matter — a statsFilePath that returned the fallback
|
||||
// unconditionally would put the router's query log in tmpfs, where a reboot eats it, and
|
||||
// every other test here would still pass.
|
||||
func TestStatsFilePathPrefersPersistentDir(t *testing.T) {
|
||||
base := t.TempDir()
|
||||
dir := filepath.Join(base, "shater")
|
||||
t.Cleanup(SetPathsForTest(dir))
|
||||
|
||||
// The directory does not exist yet => tmpfs fallback.
|
||||
if got := statsFilePath(); got != statsFileFallback {
|
||||
t.Fatalf("statsFilePath() with %s absent = %q, want the fallback %q", dir, got, statsFileFallback)
|
||||
}
|
||||
// ...and once it exists, the persistent file wins. THE CONTROL.
|
||||
if err := os.MkdirAll(dir, 0o755); err != nil {
|
||||
t.Fatalf("mkdir %s: %v", dir, err)
|
||||
}
|
||||
if got := statsFilePath(); got != statsFilePersistent {
|
||||
t.Fatalf("statsFilePath() with %s present = %q, want the persistent %q", dir, got, statsFilePersistent)
|
||||
}
|
||||
}
|
||||
|
||||
// TestNoopStore pins the OFF backend contract: empty-but-non-nil reads, a Snapshot
|
||||
// marked "off", and Start/Close/Resubscribe that never panic (and never subscribe).
|
||||
func TestNoopStore(t *testing.T) {
|
||||
@@ -37,10 +85,15 @@ func TestNoopStore(t *testing.T) {
|
||||
// reporting "memory". The "sqlite" request (the persistent backend's historical selector
|
||||
// value, now bbolt-implemented) returns a real Aggregator backed by the persistent log
|
||||
// store when the DB opens, reporting "sqlite"; if the DB cannot be opened it falls back to
|
||||
// the in-memory ring and honestly reports "memory". Both outcomes are valid, so the
|
||||
// persistent case is asserted separately (below). Deterministic behavior at a controlled
|
||||
// path is covered by the boltRing tests.
|
||||
// the in-memory ring and honestly reports "memory".
|
||||
//
|
||||
// Both of those are asserted EXACTLY, not as "either is fine". They used to be an
|
||||
// or-assertion because the DB path was the real one and the test could not know whether
|
||||
// this host would let it open — a test whose expected value depends on the machine.
|
||||
// Pointing the path at a private directory removes that excuse: a writable directory MUST
|
||||
// yield "sqlite", and an unopenable one MUST yield "memory" without a crash.
|
||||
func TestNewStoreSelection(t *testing.T) {
|
||||
dir := isolateStatsPaths(t)
|
||||
cases := []struct {
|
||||
backend string
|
||||
wantNoop bool
|
||||
@@ -66,16 +119,36 @@ func TestNewStoreSelection(t *testing.T) {
|
||||
_ = s.Close()
|
||||
}
|
||||
|
||||
// "sqlite" => an Aggregator whose effective backend is "sqlite" (DB opened) or "memory"
|
||||
// (honest fallback when the default DB path is unopenable on this host). Never a noop,
|
||||
// never a crash. Clean up any DB file the successful path may have created.
|
||||
// "sqlite" at a writable path => an Aggregator really backed by the persistent store,
|
||||
// reporting "sqlite". Never a noop, never a crash, and never the memory fallback: the
|
||||
// directory exists and is writable, so a "memory" answer here would mean persistence
|
||||
// silently did not happen — exactly the lie the honest-fallback branch exists to avoid
|
||||
// telling when it DOES happen.
|
||||
s := NewStore("sqlite", nil, nil)
|
||||
if _, isAgg := s.(*Aggregator); !isAgg {
|
||||
t.Fatalf("NewStore(sqlite): concrete type %T, want *Aggregator", s)
|
||||
}
|
||||
if got := s.Snapshot().Backend; got != "sqlite" && got != "memory" {
|
||||
t.Fatalf("NewStore(sqlite).Snapshot().Backend = %q, want \"sqlite\" or \"memory\"", got)
|
||||
if got := s.Snapshot().Backend; got != "sqlite" {
|
||||
t.Fatalf("NewStore(sqlite) at writable %s: Snapshot().Backend = %q, want \"sqlite\"", dir, got)
|
||||
}
|
||||
_ = s.Close()
|
||||
if _, err := os.Stat(statsFilePath()); err != nil {
|
||||
t.Fatalf("NewStore(sqlite) reported backend \"sqlite\" but left no DB at %s: %v", statsFilePath(), err)
|
||||
}
|
||||
|
||||
// THE CONTROL for the fallback half: an unopenable path (parent directory does not
|
||||
// exist) must produce an Aggregator that says "memory" — the honest signal that
|
||||
// persistence is NOT active — rather than a crash or a "sqlite" that is not true.
|
||||
// Without this the whole selection test is satisfied by a newPersistent that never
|
||||
// falls back, and the daemon would die on a router whose /etc/shater is unwritable.
|
||||
restore := SetPathsForTest(filepath.Join(t.TempDir(), "no", "such", "dir"))
|
||||
defer restore()
|
||||
s = NewStore("sqlite", nil, nil)
|
||||
if _, isAgg := s.(*Aggregator); !isAgg {
|
||||
t.Fatalf("NewStore(sqlite) at an unopenable path: concrete type %T, want *Aggregator", s)
|
||||
}
|
||||
if got := s.Snapshot().Backend; got != "memory" {
|
||||
t.Fatalf("NewStore(sqlite) at an unopenable path: Snapshot().Backend = %q, want \"memory\"", got)
|
||||
}
|
||||
_ = s.Close()
|
||||
_ = os.Remove(statsFilePath()) // bbolt keeps everything in the single DB file
|
||||
}
|
||||
|
||||
@@ -0,0 +1,10 @@
|
||||
// Package testguard holds checks the test suite runs ON ITSELF.
|
||||
//
|
||||
// It contains no product code and is imported by nothing. What lives here are
|
||||
// tests whose subject is the other tests in this tree — properties that no
|
||||
// individual test can assert about itself, because the thing that goes wrong is
|
||||
// not a wrong answer but a side effect.
|
||||
//
|
||||
// Today that is one property: a test may not write, delete or rename anything
|
||||
// outside its own temporary directory (fsisolation_test.go).
|
||||
package testguard
|
||||
@@ -0,0 +1,633 @@
|
||||
package testguard
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"go/ast"
|
||||
"go/parser"
|
||||
"go/token"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"sort"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestNoTestWritesOutsideItsTempDir is the sentinel for a defect class that every
|
||||
// other instrument in this repo is blind to BY CONSTRUCTION.
|
||||
//
|
||||
// WHAT HAPPENED (2026-07-27). shater/stats/store_test.go ended with
|
||||
//
|
||||
// _ = os.Remove(statsFilePath())
|
||||
//
|
||||
// and statsFilePath() is not a test path — it is THE product path,
|
||||
// /etc/shater/stats.db wherever that directory exists, which is the testbed and the
|
||||
// router. So running the suite deleted the router's accumulated statistics database.
|
||||
// The test PASSED. It had always passed. Damage done by a test is not an assertion
|
||||
// failure, it is a side effect, and nothing in a green test run reports one.
|
||||
//
|
||||
// It was not alone: a filesystem sweep (scripts/check-test-fs-isolation.sh, which
|
||||
// seeds a router-shaped canary tree and diffs it around a full run) found six
|
||||
// packages doing it — /etc/shater/stats.db and /etc/shater/shaterd.log and
|
||||
// /var/log/shaterd.log deleted, /var/run/shater.active deleted, /etc/shater/cache.db
|
||||
// rewritten, /etc/shater/config.pre-*.bak created.
|
||||
//
|
||||
// WHY A SECOND INSTRUMENT, when that sweep exists. The sweep works by letting the
|
||||
// tests do their worst and then looking at what changed — so it can only run where
|
||||
// the damage is acceptable (a container), and on the machine that matters it would
|
||||
// report the loss after taking it. This check reads the SOURCE, so it fires on the
|
||||
// commit that introduces the next one, before it has run anywhere.
|
||||
//
|
||||
// THE RULE, positive and closed: in a _test.go file under shater/, the path handed
|
||||
// to a filesystem-mutating call must be PROVABLY rooted in a temporary directory —
|
||||
// t.TempDir(), os.MkdirTemp/os.CreateTemp, or something derived from them. Anything
|
||||
// this check cannot prove is a failure, not a default. That is deliberate and it is
|
||||
// the whole point: the case that cost us the stats DB was a path produced by a
|
||||
// function call, which no blocklist of "/etc" prefixes would ever have matched.
|
||||
//
|
||||
// The escape hatch is allowedNonTemp below: named, with the reason, printed on
|
||||
// every run.
|
||||
func TestNoTestWritesOutsideItsTempDir(t *testing.T) {
|
||||
root := repoRoot(t)
|
||||
scanRoot := filepath.Join(root, "shater")
|
||||
files := testFilesUnder(t, scanRoot)
|
||||
|
||||
// THE CONTROL. This check walks the tree with filepath.WalkDir and parses what it
|
||||
// finds; a wrong root, a renamed directory or a parser that silently returned
|
||||
// nothing would make it report a clean tree having looked at nothing at all — the
|
||||
// exact failure mode ("a guard that silently runs nothing is worse than no
|
||||
// guard") this repo's gate is built around. 150 is far below the ~190 test files
|
||||
// that exist and far above zero.
|
||||
if len(files) < 150 {
|
||||
t.Fatalf("found only %d _test.go files under %s — this check is reading a blank page, "+
|
||||
"so its verdict means nothing. Fix the walk before trusting a green run.",
|
||||
len(files), scanRoot)
|
||||
}
|
||||
t.Logf("scanned %d _test.go files under %s", len(files), scanRoot)
|
||||
|
||||
fset := token.NewFileSet()
|
||||
var findings []finding
|
||||
byPkg := map[string][]*ast.File{}
|
||||
parsed := map[string]*ast.File{}
|
||||
for _, f := range files {
|
||||
af, err := parser.ParseFile(fset, f, nil, 0)
|
||||
if err != nil {
|
||||
t.Fatalf("parse %s: %v", f, err)
|
||||
}
|
||||
parsed[f] = af
|
||||
dir := filepath.Dir(f)
|
||||
byPkg[dir] = append(byPkg[dir], af)
|
||||
}
|
||||
|
||||
for _, f := range files {
|
||||
a := &analyzer{fset: fset, pkgFiles: byPkg[filepath.Dir(f)], file: parsed[f], path: relTo(root, f)}
|
||||
findings = append(findings, a.run()...)
|
||||
}
|
||||
|
||||
// Report the allowlist on every run, whether or not it fired: a waiver nobody
|
||||
// re-reads is how the last one stopped being true.
|
||||
used := map[string]bool{}
|
||||
var kept []finding
|
||||
for _, fd := range findings {
|
||||
if reason, ok := allowedNonTemp[fd.key()]; ok {
|
||||
used[fd.key()] = true
|
||||
t.Logf("ALLOWED %s\n -> %s", fd.key(), reason)
|
||||
continue
|
||||
}
|
||||
kept = append(kept, fd)
|
||||
}
|
||||
for k, reason := range allowedNonTemp {
|
||||
if !used[k] {
|
||||
t.Errorf("STALE WAIVER %s\n"+
|
||||
" allowedNonTemp still waives this, but the check no longer flags it. Either the\n"+
|
||||
" call moved (the waiver is keyed on file+function+callee, so it is now waiving\n"+
|
||||
" nothing) or it was fixed. Delete the entry. Reason on record: %s", k, reason)
|
||||
}
|
||||
}
|
||||
|
||||
sort.Slice(kept, func(i, j int) bool { return kept[i].key() < kept[j].key() })
|
||||
for _, fd := range kept {
|
||||
t.Errorf("%s:%d: %s(%s) — %s\n"+
|
||||
" A test may only write, delete or rename inside its OWN temp directory\n"+
|
||||
" (t.TempDir(), os.MkdirTemp). This path is not provably one, so on the testbed\n"+
|
||||
" or the router this call operates on whatever it really names. If the path comes\n"+
|
||||
" from the product, give that path the same seam generate/cache.go, logsink and\n"+
|
||||
" stats now use — a package-level VAR the test redirects — and point it at\n"+
|
||||
" t.TempDir(). If it is genuinely safe, add it to allowedNonTemp with the reason.",
|
||||
fd.file, fd.line, fd.callee, fd.arg, fd.why)
|
||||
}
|
||||
}
|
||||
|
||||
// allowedNonTemp waives a call this check flags, keyed on
|
||||
// "<repo-relative file>:<enclosing top-level func>:<callee>". The key deliberately
|
||||
// does NOT include the line number: a waiver that expires on every edit above it
|
||||
// teaches people to re-add it without reading. It DOES include the function, so a
|
||||
// waiver cannot silently spread to a new call elsewhere in the file.
|
||||
//
|
||||
// An entry here is a hole. Write the reason for someone deciding whether this
|
||||
// check proved what they think it proved — it is printed on every run, and a
|
||||
// waiver that stops matching anything fails the test as STALE.
|
||||
//
|
||||
// Every entry today is the same shape and the same limit: the path is a STRUCT
|
||||
// FIELD, and this check does not track fields. Each was traced by hand to the
|
||||
// t.TempDir() that fills it, and each was independently cleared by the dynamic
|
||||
// sweep (scripts/check-test-fs-isolation.sh saw these packages leave the canary
|
||||
// tree untouched at those paths). If field tracking is ever added, these should
|
||||
// disappear on their own — and the STALE check above will say so.
|
||||
var allowedNonTemp = map[string]string{
|
||||
|
||||
"shater/alert/expiry_test.go:TestExpiryFuturePoisonedLedgerRecovers:os.WriteFile": "e.path is the ExpiryNotifier's ledger, set by expiryFixture (expiry_test.go:25) to " +
|
||||
"filepath.Join(t.TempDir(), \"alert-state.json\"). The product default, /etc/shater/alert-state.json, is never reached from a test.",
|
||||
|
||||
"shater/generate/ruleset_test.go:TestURLBlocklistKeepsOldArtifactOnRefreshFailure:os.Chtimes": "rs.LocalOptions.Path is the compiled blocklist artifact, and the test sets " +
|
||||
"listsDirOverride = t.TempDir() before generating (ruleset_test.go:1516), which is the seam listsDir() honours. The real overlay path is unreachable here.",
|
||||
|
||||
"shater/logsink/logsink_test.go:TestReconfigureFileOffDeletesSegments:os.WriteFile": "cfg.Path comes from fileCfg(), which is filepath.Join(t.TempDir(), \"shaterd.log\"). " +
|
||||
"The two PRODUCT locations this package also purges are redirected for the whole binary by its TestMain.",
|
||||
|
||||
"shater/logsink/logsink_test.go:TestNewFileOffPurgesLeftovers:os.WriteFile": "same: cfg.Path is fileCfg()'s temp file, and the segments planted here are <that>.1.",
|
||||
}
|
||||
|
||||
type finding struct {
|
||||
file, fn, callee, arg, why string
|
||||
line int
|
||||
}
|
||||
|
||||
func (f finding) key() string { return f.file + ":" + f.fn + ":" + f.callee }
|
||||
|
||||
// mutators is the CLOSED list of calls that change the filesystem, with the argument
|
||||
// indexes that are paths. Closed and positive on purpose: a call that is not here is
|
||||
// not checked, so adding to this list is how the check grows — never a default.
|
||||
//
|
||||
// os.MkdirTemp/os.CreateTemp take a PARENT directory: "" means TMPDIR (safe), a
|
||||
// literal path means somebody chose a fixed location (not safe).
|
||||
// os.Symlink/os.Link create their SECOND argument; the first is only named.
|
||||
var mutators = map[string][]int{
|
||||
"os.Remove": {0},
|
||||
"os.RemoveAll": {0},
|
||||
"os.WriteFile": {0},
|
||||
"os.Create": {0},
|
||||
"os.CreateTemp": {0},
|
||||
"os.MkdirTemp": {0},
|
||||
"os.Mkdir": {0},
|
||||
"os.MkdirAll": {0},
|
||||
"os.OpenFile": {0},
|
||||
"os.Rename": {0, 1},
|
||||
"os.Symlink": {1},
|
||||
"os.Link": {1},
|
||||
"os.Chmod": {0},
|
||||
"os.Chown": {0},
|
||||
"os.Lchown": {0},
|
||||
"os.Chtimes": {0},
|
||||
"os.Truncate": {0},
|
||||
"os.Mkfifo": {0},
|
||||
"ioutil.WriteFile": {0},
|
||||
"ioutil.TempDir": {0},
|
||||
"ioutil.TempFile": {0},
|
||||
}
|
||||
|
||||
type analyzer struct {
|
||||
fset *token.FileSet
|
||||
pkgFiles []*ast.File // every _test.go of the same directory, for call-site lookup
|
||||
file *ast.File
|
||||
path string
|
||||
}
|
||||
|
||||
func (a *analyzer) run() []finding {
|
||||
var out []finding
|
||||
for _, decl := range a.file.Decls {
|
||||
fd, ok := decl.(*ast.FuncDecl)
|
||||
if !ok || fd.Body == nil {
|
||||
continue
|
||||
}
|
||||
ast.Inspect(fd.Body, func(n ast.Node) bool {
|
||||
call, ok := n.(*ast.CallExpr)
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
idxs, ok := mutators[calleeName(call.Fun)]
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
for _, i := range idxs {
|
||||
if i >= len(call.Args) {
|
||||
continue
|
||||
}
|
||||
if why, bad := a.notTempRooted(call.Args[i], fd, 0, map[string]bool{}); bad {
|
||||
out = append(out, finding{
|
||||
file: a.path,
|
||||
line: a.fset.Position(call.Pos()).Line,
|
||||
fn: fd.Name.Name,
|
||||
callee: calleeName(call.Fun),
|
||||
arg: exprText(a.fset, call.Args[i]),
|
||||
why: why,
|
||||
})
|
||||
}
|
||||
}
|
||||
return true
|
||||
})
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// notTempRooted answers the only question that matters: can the leftmost element of
|
||||
// this path expression be traced to a temporary directory? It returns ("", false)
|
||||
// when it can, and (reason, true) when it cannot — including when it simply does not
|
||||
// know, which is the closed half of the rule.
|
||||
func (a *analyzer) notTempRooted(e ast.Expr, scope *ast.FuncDecl, depth int, seen map[string]bool) (string, bool) {
|
||||
if depth > 12 {
|
||||
return "the path expression nests deeper than this check follows", true
|
||||
}
|
||||
switch x := e.(type) {
|
||||
case *ast.CallExpr:
|
||||
name := calleeName(x.Fun)
|
||||
switch {
|
||||
case strings.HasSuffix(name, ".TempDir") && !strings.HasPrefix(name, "os."):
|
||||
return "", false // t.TempDir(), b.TempDir(), tb.TempDir()
|
||||
case name == "os.MkdirTemp", name == "os.CreateTemp", name == "ioutil.TempDir", name == "ioutil.TempFile":
|
||||
return "", false // the RESULT of these is a fresh temp path
|
||||
case name == "filepath.Join", name == "path.Join", name == "filepath.Clean",
|
||||
name == "filepath.Dir", name == "filepath.Abs", name == "filepath.ToSlash",
|
||||
name == "filepath.FromSlash", name == "filepath.EvalSymlinks":
|
||||
if len(x.Args) == 0 {
|
||||
return "an empty " + name + "()", true
|
||||
}
|
||||
return a.notTempRooted(x.Args[0], scope, depth+1, seen)
|
||||
case name == "fmt.Sprintf":
|
||||
if len(x.Args) >= 2 {
|
||||
if lit, ok := stringLit(x.Args[0]); ok && (strings.HasPrefix(lit, "%s") || strings.HasPrefix(lit, "%v") || strings.HasPrefix(lit, "%q")) {
|
||||
return a.notTempRooted(x.Args[1], scope, depth+1, seen)
|
||||
}
|
||||
}
|
||||
return "the path is Sprintf'd from a format that does not start with the directory", true
|
||||
case name == "os.TempDir":
|
||||
return "os.TempDir() is the SHARED /tmp, not this test's own directory — a fixed name under it " +
|
||||
"collides with the router's real /tmp files and with a concurrent run", true
|
||||
}
|
||||
// Any other call: maybe a local helper that returns a temp path — follow it.
|
||||
if fn := a.findFunc(name); fn != nil {
|
||||
return a.notTempRootedReturns(fn, depth+1, seen)
|
||||
}
|
||||
return fmt.Sprintf("the path comes out of %s(), which this check cannot follow. "+
|
||||
"If that is a product path helper (statsFilePath() was exactly this), the call is operating "+
|
||||
"on the real machine", name), true
|
||||
|
||||
case *ast.BasicLit:
|
||||
lit, _ := stringLit(x)
|
||||
if lit == "" {
|
||||
return "", false // os.MkdirTemp("", ...) => TMPDIR
|
||||
}
|
||||
return fmt.Sprintf("%q is a literal path, so it names the same place on every machine "+
|
||||
"this suite ever runs on", lit), true
|
||||
|
||||
case *ast.BinaryExpr:
|
||||
if x.Op == token.ADD {
|
||||
return a.notTempRooted(x.X, scope, depth+1, seen)
|
||||
}
|
||||
return "the path is built by an operator this check does not follow", true
|
||||
|
||||
case *ast.ParenExpr:
|
||||
return a.notTempRooted(x.X, scope, depth+1, seen)
|
||||
|
||||
case *ast.Ident:
|
||||
return a.identNotTempRooted(x, scope, depth, seen)
|
||||
|
||||
case *ast.SelectorExpr:
|
||||
return fmt.Sprintf("%s is a field or another package's variable; this check cannot see where it points",
|
||||
exprText(a.fset, x)), true
|
||||
|
||||
case *ast.IndexExpr:
|
||||
return a.notTempRooted(x.X, scope, depth+1, seen)
|
||||
}
|
||||
return "this check cannot tell where the path is rooted", true
|
||||
}
|
||||
|
||||
// identNotTempRooted resolves a bare name: a local assignment inside the enclosing
|
||||
// top-level function (closures included — they are inside its body), else a parameter
|
||||
// resolved through every call site in the same package, else unknown.
|
||||
func (a *analyzer) identNotTempRooted(id *ast.Ident, scope *ast.FuncDecl, depth int, seen map[string]bool) (string, bool) {
|
||||
if scope == nil {
|
||||
return id.Name + " has no enclosing function to resolve it in", true
|
||||
}
|
||||
// The save/override/restore idiom assigns a package path var TWICE inside one
|
||||
// test — once from a temp dir, once back from the saved original:
|
||||
//
|
||||
// orig := restartHandoffPath
|
||||
// restartHandoffPath = filepath.Join(t.TempDir(), "restarting")
|
||||
// defer func() { restartHandoffPath = orig }()
|
||||
//
|
||||
// Following both assignments walks in a circle (restartHandoffPath -> orig ->
|
||||
// restartHandoffPath). The cycle is not evidence of anything, so a name already
|
||||
// being resolved is DROPPED from the judgement rather than counted against it;
|
||||
// the other assignment still has to prove itself. Stated as a limit: a var whose
|
||||
// only assignment in the test is a restore is judged on the restore's source.
|
||||
key := scope.Name.Name + "." + id.Name
|
||||
if seen[key] {
|
||||
return "", false
|
||||
}
|
||||
seen[key] = true
|
||||
defer delete(seen, key)
|
||||
|
||||
var rhs []ast.Expr
|
||||
ast.Inspect(scope.Body, func(n ast.Node) bool {
|
||||
switch s := n.(type) {
|
||||
case *ast.RangeStmt:
|
||||
// for _, d := range []string{a, b} — the loop variable is each element.
|
||||
if vi, ok := s.Value.(*ast.Ident); ok && vi.Name == id.Name {
|
||||
if cl, ok := s.X.(*ast.CompositeLit); ok {
|
||||
rhs = append(rhs, cl.Elts...)
|
||||
} else {
|
||||
rhs = append(rhs, s.X)
|
||||
}
|
||||
}
|
||||
case *ast.AssignStmt:
|
||||
for i, lhs := range s.Lhs {
|
||||
if li, ok := lhs.(*ast.Ident); ok && li.Name == id.Name {
|
||||
// x, y := f() — one call feeding several names: judge the call.
|
||||
if len(s.Rhs) == 1 && len(s.Lhs) > 1 {
|
||||
rhs = append(rhs, s.Rhs[0])
|
||||
} else if i < len(s.Rhs) {
|
||||
rhs = append(rhs, s.Rhs[i])
|
||||
}
|
||||
}
|
||||
}
|
||||
case *ast.ValueSpec:
|
||||
for i, nm := range s.Names {
|
||||
if nm.Name == id.Name && i < len(s.Values) {
|
||||
rhs = append(rhs, s.Values[i])
|
||||
}
|
||||
}
|
||||
}
|
||||
return true
|
||||
})
|
||||
if len(rhs) > 0 {
|
||||
// EVERY assignment must be temp-rooted: a variable that is a temp dir on one
|
||||
// branch and /etc on another is not isolated.
|
||||
for _, r := range rhs {
|
||||
if why, bad := a.notTempRooted(r, scope, depth+1, seen); bad {
|
||||
return fmt.Sprintf("%s is assigned from something that is not temp-rooted: %s", id.Name, why), true
|
||||
}
|
||||
}
|
||||
return "", false
|
||||
}
|
||||
if isParam(scope, id.Name) {
|
||||
return a.paramNotTempRooted(scope, id.Name, depth, seen)
|
||||
}
|
||||
// A package-level name. It is still acceptable if the TESTS of this package
|
||||
// redirect it — the seam this whole exercise installed:
|
||||
//
|
||||
// var ActiveFlag = "/var/run/shater.active" // product
|
||||
// func TestMain(m *testing.M) { ActiveFlag = dir + "/shater.active"; ... }
|
||||
//
|
||||
// so look for assignments to the name anywhere in this package's test files and
|
||||
// hold every one of them to the same rule. A name nothing assigns is the shape
|
||||
// that ate the stats DB, and stays a failure.
|
||||
if why, bad, found := a.packageLevelReassignment(id.Name, depth, seen); found {
|
||||
if bad {
|
||||
return why, true
|
||||
}
|
||||
return "", false
|
||||
}
|
||||
return fmt.Sprintf("%s is not a local variable of %s and no test in this package redirects it — "+
|
||||
"a product path used as-is (a product path constant is exactly this shape)",
|
||||
id.Name, scope.Name.Name), true
|
||||
}
|
||||
|
||||
// packageLevelReassignment looks for `name = <expr>` at the top level of any test
|
||||
// function in this package (TestMain being the usual one) and judges every such
|
||||
// assignment. found=false means nothing in the tests touches the name.
|
||||
func (a *analyzer) packageLevelReassignment(name string, depth int, seen map[string]bool) (why string, bad, found bool) {
|
||||
for _, f := range a.pkgFiles {
|
||||
for _, decl := range f.Decls {
|
||||
fn, ok := decl.(*ast.FuncDecl)
|
||||
if !ok || fn.Body == nil {
|
||||
continue
|
||||
}
|
||||
ast.Inspect(fn.Body, func(n ast.Node) bool {
|
||||
as, ok := n.(*ast.AssignStmt)
|
||||
if !ok || as.Tok == token.DEFINE {
|
||||
return true // `x := name` reads it, it does not redirect it
|
||||
}
|
||||
for i, lhs := range as.Lhs {
|
||||
li, ok := lhs.(*ast.Ident)
|
||||
if !ok || li.Name != name || i >= len(as.Rhs) {
|
||||
continue
|
||||
}
|
||||
found = true
|
||||
if w, b := a.notTempRooted(as.Rhs[i], fn, depth+1, seen); b {
|
||||
why, bad = fmt.Sprintf("%s is redirected in %s, but to something that is not temp-rooted: %s",
|
||||
name, fn.Name.Name, w), true
|
||||
}
|
||||
}
|
||||
return true
|
||||
})
|
||||
}
|
||||
}
|
||||
return why, bad, found
|
||||
}
|
||||
|
||||
// paramNotTempRooted judges a helper's string parameter by what every caller in this
|
||||
// package passes for it. No callers means the helper is dead, and a dead helper's
|
||||
// waiver would be worthless — so that is a failure too.
|
||||
func (a *analyzer) paramNotTempRooted(fn *ast.FuncDecl, name string, depth int, seen map[string]bool) (string, bool) {
|
||||
idx, ok := paramIndex(fn, name)
|
||||
if !ok {
|
||||
return name + " is a parameter this check could not index", true
|
||||
}
|
||||
callSites := 0
|
||||
for _, f := range a.pkgFiles {
|
||||
for _, decl := range f.Decls {
|
||||
caller, ok := decl.(*ast.FuncDecl)
|
||||
if !ok || caller.Body == nil {
|
||||
continue
|
||||
}
|
||||
var why string
|
||||
bad := false
|
||||
ast.Inspect(caller.Body, func(n ast.Node) bool {
|
||||
call, ok := n.(*ast.CallExpr)
|
||||
if !ok || calleeName(call.Fun) != fn.Name.Name || idx >= len(call.Args) {
|
||||
return true
|
||||
}
|
||||
callSites++
|
||||
if w, b := a.notTempRooted(call.Args[idx], caller, depth+1, seen); b {
|
||||
why, bad = fmt.Sprintf("%s is called from %s with a path that is not temp-rooted: %s",
|
||||
fn.Name.Name, caller.Name.Name, w), true
|
||||
}
|
||||
return true
|
||||
})
|
||||
if bad {
|
||||
return why, true
|
||||
}
|
||||
}
|
||||
}
|
||||
if callSites == 0 {
|
||||
return fmt.Sprintf("%s takes %s but nothing in this package calls it — nobody can say what it operates on",
|
||||
fn.Name.Name, name), true
|
||||
}
|
||||
return "", false
|
||||
}
|
||||
|
||||
// notTempRootedReturns follows a local helper that hands back a path.
|
||||
func (a *analyzer) notTempRootedReturns(fn *ast.FuncDecl, depth int, seen map[string]bool) (string, bool) {
|
||||
if fn.Body == nil {
|
||||
return fn.Name.Name + " has no body here", true
|
||||
}
|
||||
found := false
|
||||
var why string
|
||||
bad := false
|
||||
ast.Inspect(fn.Body, func(n ast.Node) bool {
|
||||
ret, ok := n.(*ast.ReturnStmt)
|
||||
if !ok || len(ret.Results) == 0 {
|
||||
return true
|
||||
}
|
||||
found = true
|
||||
if w, b := a.notTempRooted(ret.Results[0], fn, depth+1, seen); b {
|
||||
why, bad = fmt.Sprintf("%s returns something that is not temp-rooted: %s", fn.Name.Name, w), true
|
||||
}
|
||||
return true
|
||||
})
|
||||
if bad {
|
||||
return why, true
|
||||
}
|
||||
if !found {
|
||||
return fn.Name.Name + " returns no path this check could read", true
|
||||
}
|
||||
return "", false
|
||||
}
|
||||
|
||||
func (a *analyzer) findFunc(name string) *ast.FuncDecl {
|
||||
if strings.Contains(name, ".") {
|
||||
return nil // another package's function: not ours to follow
|
||||
}
|
||||
for _, f := range a.pkgFiles {
|
||||
for _, decl := range f.Decls {
|
||||
if fd, ok := decl.(*ast.FuncDecl); ok && fd.Recv == nil && fd.Name.Name == name {
|
||||
return fd
|
||||
}
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// --- small helpers -----------------------------------------------------------
|
||||
|
||||
func calleeName(e ast.Expr) string {
|
||||
switch x := e.(type) {
|
||||
case *ast.Ident:
|
||||
return x.Name
|
||||
case *ast.SelectorExpr:
|
||||
if id, ok := x.X.(*ast.Ident); ok {
|
||||
return id.Name + "." + x.Sel.Name
|
||||
}
|
||||
return "." + x.Sel.Name
|
||||
}
|
||||
return ""
|
||||
}
|
||||
|
||||
func stringLit(e ast.Expr) (string, bool) {
|
||||
lit, ok := e.(*ast.BasicLit)
|
||||
if !ok || lit.Kind != token.STRING {
|
||||
return "", false
|
||||
}
|
||||
s, err := strconv.Unquote(lit.Value)
|
||||
if err != nil {
|
||||
return "", false
|
||||
}
|
||||
return s, true
|
||||
}
|
||||
|
||||
func exprText(fset *token.FileSet, e ast.Expr) string {
|
||||
pos, end := fset.Position(e.Pos()), fset.Position(e.End())
|
||||
if pos.Filename != end.Filename || pos.Line != end.Line {
|
||||
return "…"
|
||||
}
|
||||
b, err := os.ReadFile(pos.Filename)
|
||||
if err != nil {
|
||||
return "…"
|
||||
}
|
||||
if end.Offset > len(b) || pos.Offset < 0 || pos.Offset > end.Offset {
|
||||
return "…"
|
||||
}
|
||||
return string(b[pos.Offset:end.Offset])
|
||||
}
|
||||
|
||||
func isParam(fn *ast.FuncDecl, name string) bool {
|
||||
_, ok := paramIndex(fn, name)
|
||||
return ok
|
||||
}
|
||||
|
||||
func paramIndex(fn *ast.FuncDecl, name string) (int, bool) {
|
||||
i := 0
|
||||
if fn.Type.Params == nil {
|
||||
return 0, false
|
||||
}
|
||||
for _, field := range fn.Type.Params.List {
|
||||
if len(field.Names) == 0 {
|
||||
i++
|
||||
continue
|
||||
}
|
||||
for _, nm := range field.Names {
|
||||
if nm.Name == name {
|
||||
return i, true
|
||||
}
|
||||
i++
|
||||
}
|
||||
}
|
||||
return 0, false
|
||||
}
|
||||
|
||||
func relTo(root, p string) string {
|
||||
r, err := filepath.Rel(root, p)
|
||||
if err != nil {
|
||||
return p
|
||||
}
|
||||
return filepath.ToSlash(r)
|
||||
}
|
||||
|
||||
// repoRoot walks up from this source file to the directory holding go.mod.
|
||||
func repoRoot(t *testing.T) string {
|
||||
t.Helper()
|
||||
_, self, _, ok := runtime.Caller(0)
|
||||
if !ok {
|
||||
t.Fatal("runtime.Caller failed — cannot locate this file, so cannot locate the tree to check")
|
||||
}
|
||||
dir := filepath.Dir(self)
|
||||
for i := 0; i < 12; i++ {
|
||||
if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
|
||||
return dir
|
||||
}
|
||||
parent := filepath.Dir(dir)
|
||||
if parent == dir {
|
||||
break
|
||||
}
|
||||
dir = parent
|
||||
}
|
||||
t.Fatalf("no go.mod above %s — cannot locate the repository root", self)
|
||||
return ""
|
||||
}
|
||||
|
||||
func testFilesUnder(t *testing.T, root string) []string {
|
||||
t.Helper()
|
||||
var out []string
|
||||
err := filepath.WalkDir(root, func(p string, d os.DirEntry, err error) error {
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if d.IsDir() {
|
||||
if d.Name() == "testdata" || d.Name() == "node_modules" || d.Name() == ".git" {
|
||||
return filepath.SkipDir
|
||||
}
|
||||
return nil
|
||||
}
|
||||
if strings.HasSuffix(p, "_test.go") {
|
||||
out = append(out, p)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("walk %s: %v", root, err)
|
||||
}
|
||||
sort.Strings(out)
|
||||
return out
|
||||
}
|
||||
Reference in New Issue
Block a user