diff --git a/REUSE.md b/REUSE.md index 75fa8d7..2f80a82 100644 --- a/REUSE.md +++ b/REUSE.md @@ -311,6 +311,7 @@ | `stacks.Manager.execCommand` / `composeExecCustomEnv` for NEW long-running calls | No context/timeout — a hung docker CLI blocks forever | `exec.CommandContext` + explicit timeout (copy `rsyncCopy` or appexport `composeExecEnv`) | | `config.LoadPermissive` | Skips validation — setup-mode only (customer.id/domain may be unset) | `config.Load` everywhere else | | `ExportDataMounts` / `ParseComposeHDDMounts` as **backup-classification** input | `ExportDataMounts` unions the `${USERDATA_PATH}` ROOT (export-capture logic, not per-bind); `ParseComposeHDDMounts` resolves absolutes AND drops the `:ro` flag — classification needs `${VAR}`-relative paths + read-only awareness | `ParseComposeClassifiableBinds` (controller/internal/stacks/classify_binds.go) | +| any settings getter/setter or helper that ends in one (`m.note`, `boxLang`) INSIDE a `settings.UpdateOffboxStatus`/`UpdateCrossDriveStatus` callback (R-578) | The callback runs under the settings WRITE lock and `sync.RWMutex` is not reentrant — it DEADLOCKS holding the lock and wedges settings.json for the box | Resolve the value BEFORE the callback (`lang := m.boxLang()`); `TestR578NothingTakesTheSettingsLockInsideASettingsCallback` (controller/internal/settings/r578_callback_lock_gate_test.go) convicts it in every package | | `docker compose restart` (any wrapper) | Does not pick up new images or env | `RedeployFromEnv` / composeExec `up -d` | | `backup.WholeOnTier` vs `UpdateCopyHolds` (R-659) | `UpdateCopyHolds` says what a copy HOLDS; it does not say the restore will ACCEPT it — a file app's second-drive copy holds the files and its unit restore still refuses | `WholeOnTier` (asks `DeclaredDriveFileLegs`, the refusal's own predicate) before a sentence names a copy as a way back | diff --git a/controller/internal/backup/offsite_diag_test.go b/controller/internal/backup/offsite_diag_test.go index 46a99e2..6ceb81f 100644 --- a/controller/internal/backup/offsite_diag_test.go +++ b/controller/internal/backup/offsite_diag_test.go @@ -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)\(`) diff --git a/controller/internal/settings/r578_callback_lock_gate_test.go b/controller/internal/settings/r578_callback_lock_gate_test.go new file mode 100644 index 0000000..ac0f172 --- /dev/null +++ b/controller/internal/settings/r578_callback_lock_gate_test.go @@ -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: 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 +}