R-578: settings-callback deadlock gate over every controller package
TestR578NothingTakesTheSettingsLockInsideASettingsCallback (internal/settings) derives from source the *Settings methods that take s.mu, the callback runners among them, and per package every helper that transitively reaches one; a call to any of those inside a func literal passed to a runner (or a non-literal callback) fails with file:line. Decoy: TestR578GateConvictsAHelperChain. REUSE.md lookalike row added. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS
This commit is contained in:
@@ -203,6 +203,9 @@ func TestR570SentenceStaysHungarian(t *testing.T) {
|
||||
//
|
||||
// RED-PROOF (REPORT): put `m.note(...)` back inside the final UpdateOffboxStatus callback → this test
|
||||
// names the file and the line, in a second, instead of the suite hanging for 25 minutes.
|
||||
//
|
||||
// R-578: the controller-wide successor is TestR578NothingTakesTheSettingsLockInsideASettingsCallback
|
||||
// (internal/settings) — it derives the lockers and helpers from source in every package.
|
||||
func TestNoteHelpersAreNotCalledUnderTheSettingsLock(t *testing.T) {
|
||||
callback := regexp.MustCompile(`\.Update\w*\(func\(`)
|
||||
note := regexp.MustCompile(`\b(m|s)\.(note|noteErr|boxLang)\(`)
|
||||
|
||||
@@ -0,0 +1,355 @@
|
||||
package settings
|
||||
|
||||
import (
|
||||
"go/ast"
|
||||
"go/parser"
|
||||
"go/token"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"sort"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// R-578 — the settings-callback deadlock, as a gate over EVERY package of the controller.
|
||||
//
|
||||
// `UpdateOffboxStatus` (and every other *Settings method that takes a func and the settings lock)
|
||||
// runs its callback while holding the settings WRITE lock. sync.RWMutex is not reentrant: anything
|
||||
// inside that callback that takes the settings lock again — a getter, a setter, or a package helper
|
||||
// that ends in one (`m.note` → `m.boxLang` → `GetLanguage`) — DEADLOCKS while holding the lock, and
|
||||
// wedges everything else on the box that touches settings.json. Release C of localisation slice 2
|
||||
// shipped exactly that; the only symptom was a 25-minute test timeout. The backup-only predecessor,
|
||||
// TestNoteHelpersAreNotCalledUnderTheSettingsLock, knew three helper names in one package.
|
||||
//
|
||||
// This test derives everything from source instead of a list:
|
||||
// - lockers: *Settings methods that take s.mu (directly or through another *Settings method);
|
||||
// - takers: lockers that have a func-typed parameter (the callback runners);
|
||||
// - tainted: per package, every func/method whose body calls an exported locker or another
|
||||
// tainted name of the same package (fixpoint) — so a new helper is covered the day it is written;
|
||||
// - violation: a call to a locker or a tainted name inside a func literal passed to a taker, OR a
|
||||
// taker given something that is not a literal (the gate could not see inside it).
|
||||
//
|
||||
// The matching is by NAME (no type checker — this must stay fast inside `go test ./...`). A false
|
||||
// positive is possible if an unrelated type has a method named like a settings locker and is called
|
||||
// inside a callback; rename it or hoist the call — both are cheap, a hang is not.
|
||||
//
|
||||
// RED-PROOF (R-578): put `lang := m.note("x")` inside the final UpdateOffboxStatus callback in
|
||||
// internal/backup/offbox.go → this test names internal/backup/offbox.go:<line> and the call chain.
|
||||
func TestR578NothingTakesTheSettingsLockInsideASettingsCallback(t *testing.T) {
|
||||
root := filepath.Join("..", "..") // controller/
|
||||
pkgs := parseControllerPackages(t, root)
|
||||
|
||||
own, ok := pkgs[filepath.Join(root, "internal", "settings")]
|
||||
if !ok {
|
||||
t.Fatal("internal/settings was not parsed — the walk no longer finds this package")
|
||||
}
|
||||
lockers, takers := settingsLockers(own)
|
||||
if len(lockers) < 20 || len(takers) == 0 {
|
||||
t.Fatalf("found %d settings lockers and %d callback runners — the pattern no longer matches the code", len(lockers), len(takers))
|
||||
}
|
||||
exported := map[string]bool{}
|
||||
for name := range lockers {
|
||||
if ast.IsExported(name) {
|
||||
exported[name] = true
|
||||
}
|
||||
}
|
||||
|
||||
var bad []string
|
||||
callbacks := 0
|
||||
backupTaint := map[string]bool{}
|
||||
for dir, files := range pkgs {
|
||||
tainted := taintedFuncs(files, exported)
|
||||
if dir == filepath.Join(root, "internal", "backup") {
|
||||
backupTaint = tainted
|
||||
}
|
||||
for _, pf := range files {
|
||||
ast.Inspect(pf.file, func(n ast.Node) bool {
|
||||
call, ok := n.(*ast.CallExpr)
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
sel, ok := call.Fun.(*ast.SelectorExpr)
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
idx, isTaker := takers[sel.Sel.Name]
|
||||
if !isTaker {
|
||||
return true
|
||||
}
|
||||
for _, i := range idx {
|
||||
if i >= len(call.Args) {
|
||||
continue
|
||||
}
|
||||
arg := call.Args[i]
|
||||
if lit, ok := arg.(*ast.FuncLit); ok {
|
||||
callbacks++
|
||||
for _, hit := range lockingCalls(lit.Body, exported, tainted) {
|
||||
bad = append(bad, pf.pos(hit.pos)+" "+hit.name+"() inside "+sel.Sel.Name+"(func…)")
|
||||
}
|
||||
continue
|
||||
}
|
||||
bad = append(bad, pf.pos(arg.Pos())+" "+sel.Sel.Name+" is passed a non-literal callback — the gate cannot see inside it; pass a func literal")
|
||||
}
|
||||
return true
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// Positive controls: the instrument is looking at something, and the transitive taint reaches
|
||||
// the helpers that actually caused the 2026-09-18 deadlock.
|
||||
if callbacks < 10 {
|
||||
t.Fatalf("found only %d settings callbacks across the controller — the walk no longer matches the code", callbacks)
|
||||
}
|
||||
for _, h := range []string{"boxLang", "note", "noteErr"} {
|
||||
if !backupTaint[h] {
|
||||
t.Errorf("internal/backup %s() is not seen as taking the settings lock — the taint analysis lost the chain that deadlocked release C", h)
|
||||
}
|
||||
}
|
||||
if len(bad) > 0 {
|
||||
sort.Strings(bad)
|
||||
t.Errorf("a settings callback (which holds the settings WRITE lock) calls something that takes "+
|
||||
"the settings lock again — sync.RWMutex is not reentrant, so this DEADLOCKS and wedges "+
|
||||
"settings.json for the whole box. Hoist the call before the callback (%d):\n %s",
|
||||
len(bad), strings.Join(bad, "\n "))
|
||||
}
|
||||
t.Logf("%d packages, %d settings lockers, %d callback runners, %d callbacks examined", len(pkgs), len(lockers), len(takers), callbacks)
|
||||
}
|
||||
|
||||
// TestR578GateConvictsAHelperChain is the decoy: a synthetic package whose callback reaches the
|
||||
// settings lock only through two local helpers. If the taint fixpoint stops following calls, this
|
||||
// fails before the real gate quietly goes blind.
|
||||
func TestR578GateConvictsAHelperChain(t *testing.T) {
|
||||
src := `package decoy
|
||||
type M struct{ settings *S }
|
||||
func (m *M) lang() string { return m.settings.GetLanguage() }
|
||||
func (m *M) msg() string { return "x" + m.lang() }
|
||||
func (m *M) clean() string { return "y" }
|
||||
func (m *M) run() {
|
||||
_ = m.settings.UpdateOffboxStatus(func(o *T) { o.A = m.msg(); o.B = m.clean() })
|
||||
}`
|
||||
fset := token.NewFileSet()
|
||||
f, err := parser.ParseFile(fset, "decoy.go", src, 0)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
files := []parsedFile{{fset: fset, file: f, path: "decoy.go"}}
|
||||
exported := map[string]bool{"GetLanguage": true}
|
||||
tainted := taintedFuncs(files, exported)
|
||||
if !tainted["msg"] || !tainted["lang"] || tainted["clean"] {
|
||||
t.Fatalf("taint = %v; want lang+msg tainted, clean not", tainted)
|
||||
}
|
||||
var lit *ast.FuncLit
|
||||
ast.Inspect(f, func(n ast.Node) bool {
|
||||
if l, ok := n.(*ast.FuncLit); ok {
|
||||
lit = l
|
||||
}
|
||||
return true
|
||||
})
|
||||
hits := lockingCalls(lit.Body, exported, tainted)
|
||||
if len(hits) != 1 || hits[0].name != "msg" {
|
||||
t.Fatalf("hits = %+v; want exactly msg()", hits)
|
||||
}
|
||||
}
|
||||
|
||||
type parsedFile struct {
|
||||
fset *token.FileSet
|
||||
file *ast.File
|
||||
path string
|
||||
}
|
||||
|
||||
func (p parsedFile) pos(pos token.Pos) string {
|
||||
ps := p.fset.Position(pos)
|
||||
return filepath.ToSlash(p.path) + ":" + strconv.Itoa(ps.Line)
|
||||
}
|
||||
|
||||
// parseControllerPackages parses every non-test .go file under controller/, grouped by directory.
|
||||
func parseControllerPackages(t *testing.T, root string) map[string][]parsedFile {
|
||||
t.Helper()
|
||||
fset := token.NewFileSet()
|
||||
out := map[string][]parsedFile{}
|
||||
err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if info.IsDir() {
|
||||
base := info.Name()
|
||||
if path != root && (strings.HasPrefix(base, ".") || base == "testdata" || base == "vendor") {
|
||||
return filepath.SkipDir
|
||||
}
|
||||
return nil
|
||||
}
|
||||
if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") {
|
||||
return nil
|
||||
}
|
||||
f, perr := parser.ParseFile(fset, path, nil, 0)
|
||||
if perr != nil {
|
||||
return perr
|
||||
}
|
||||
dir := filepath.Dir(path)
|
||||
rel, _ := filepath.Rel(root, path)
|
||||
out[dir] = append(out[dir], parsedFile{fset: fset, file: f, path: rel})
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// settingsLockers returns the *Settings methods that take s.mu (directly, or by calling another
|
||||
// *Settings method that does), and the subset that also take a func-typed parameter (with the
|
||||
// argument positions of those parameters).
|
||||
func settingsLockers(files []parsedFile) (lockers map[string]bool, takers map[string][]int) {
|
||||
type method struct {
|
||||
recv string
|
||||
decl *ast.FuncDecl
|
||||
}
|
||||
var methods []method
|
||||
for _, pf := range files {
|
||||
for _, d := range pf.file.Decls {
|
||||
fd, ok := d.(*ast.FuncDecl)
|
||||
if !ok || fd.Recv == nil || len(fd.Recv.List) != 1 || fd.Body == nil {
|
||||
continue
|
||||
}
|
||||
star, ok := fd.Recv.List[0].Type.(*ast.StarExpr)
|
||||
if !ok {
|
||||
continue
|
||||
}
|
||||
if id, ok := star.X.(*ast.Ident); !ok || id.Name != "Settings" {
|
||||
continue
|
||||
}
|
||||
recv := "_"
|
||||
if len(fd.Recv.List[0].Names) == 1 {
|
||||
recv = fd.Recv.List[0].Names[0].Name
|
||||
}
|
||||
methods = append(methods, method{recv, fd})
|
||||
}
|
||||
}
|
||||
lockers = map[string]bool{}
|
||||
for changed := true; changed; {
|
||||
changed = false
|
||||
for _, m := range methods {
|
||||
if lockers[m.decl.Name.Name] {
|
||||
continue
|
||||
}
|
||||
found := false
|
||||
ast.Inspect(m.decl.Body, func(n ast.Node) bool {
|
||||
call, ok := n.(*ast.CallExpr)
|
||||
if !ok || found {
|
||||
return !found
|
||||
}
|
||||
sel, ok := call.Fun.(*ast.SelectorExpr)
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
// s.mu.Lock() / s.mu.RLock()
|
||||
if (sel.Sel.Name == "Lock" || sel.Sel.Name == "RLock") && isRecvField(sel.X, m.recv, "mu") {
|
||||
found = true
|
||||
}
|
||||
// s.OtherLockingMethod()
|
||||
if id, ok := sel.X.(*ast.Ident); ok && id.Name == m.recv && lockers[sel.Sel.Name] {
|
||||
found = true
|
||||
}
|
||||
return !found
|
||||
})
|
||||
if found {
|
||||
lockers[m.decl.Name.Name] = true
|
||||
changed = true
|
||||
}
|
||||
}
|
||||
}
|
||||
// takers: locker -> the argument positions that are func-typed.
|
||||
takers = map[string][]int{}
|
||||
for _, m := range methods {
|
||||
if !lockers[m.decl.Name.Name] {
|
||||
continue
|
||||
}
|
||||
pos := 0
|
||||
for _, p := range m.decl.Type.Params.List {
|
||||
n := len(p.Names)
|
||||
if n == 0 {
|
||||
n = 1
|
||||
}
|
||||
if _, ok := p.Type.(*ast.FuncType); ok {
|
||||
for k := 0; k < n; k++ {
|
||||
takers[m.decl.Name.Name] = append(takers[m.decl.Name.Name], pos+k)
|
||||
}
|
||||
}
|
||||
pos += n
|
||||
}
|
||||
}
|
||||
return lockers, takers
|
||||
}
|
||||
|
||||
func isRecvField(x ast.Expr, recv, field string) bool {
|
||||
sel, ok := x.(*ast.SelectorExpr)
|
||||
if !ok || sel.Sel.Name != field {
|
||||
return false
|
||||
}
|
||||
id, ok := sel.X.(*ast.Ident)
|
||||
return ok && id.Name == recv
|
||||
}
|
||||
|
||||
// calledName is the name a call expression invokes: `f(…)` → f, `x.y.f(…)` → f.
|
||||
func calledName(call *ast.CallExpr) string {
|
||||
switch fn := call.Fun.(type) {
|
||||
case *ast.Ident:
|
||||
return fn.Name
|
||||
case *ast.SelectorExpr:
|
||||
return fn.Sel.Name
|
||||
}
|
||||
return ""
|
||||
}
|
||||
|
||||
// taintedFuncs returns, for one package, the names of funcs/methods whose body (transitively, within
|
||||
// the package) calls an exported settings locker.
|
||||
func taintedFuncs(files []parsedFile, exported map[string]bool) map[string]bool {
|
||||
bodies := map[string][]*ast.BlockStmt{}
|
||||
for _, pf := range files {
|
||||
for _, d := range pf.file.Decls {
|
||||
if fd, ok := d.(*ast.FuncDecl); ok && fd.Body != nil {
|
||||
bodies[fd.Name.Name] = append(bodies[fd.Name.Name], fd.Body)
|
||||
}
|
||||
}
|
||||
}
|
||||
tainted := map[string]bool{}
|
||||
for changed := true; changed; {
|
||||
changed = false
|
||||
for name, bs := range bodies {
|
||||
if tainted[name] {
|
||||
continue
|
||||
}
|
||||
for _, b := range bs {
|
||||
if len(lockingCalls(b, exported, tainted)) > 0 {
|
||||
tainted[name] = true
|
||||
changed = true
|
||||
break
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
return tainted
|
||||
}
|
||||
|
||||
type lockHit struct {
|
||||
name string
|
||||
pos token.Pos
|
||||
}
|
||||
|
||||
func lockingCalls(body ast.Node, exported, tainted map[string]bool) []lockHit {
|
||||
var hits []lockHit
|
||||
ast.Inspect(body, func(n ast.Node) bool {
|
||||
call, ok := n.(*ast.CallExpr)
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
name := calledName(call)
|
||||
_, isSel := call.Fun.(*ast.SelectorExpr)
|
||||
if (isSel && exported[name]) || tainted[name] {
|
||||
hits = append(hits, lockHit{name, call.Pos()})
|
||||
}
|
||||
return true
|
||||
})
|
||||
return hits
|
||||
}
|
||||
Reference in New Issue
Block a user