From a7270f00cbe010fb69fe58493334031e38429aab Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 01:10:56 +0200 Subject: [PATCH] Keep the walk's paths as bytes, forget a walk root superseded, and leave other mounts unread (hq issue 361 review) The review found a cursor of a name that is not UTF-8 came back changed from JSON, so a resumed walk skipped what it stood before; an ask to search again was lost to a restart, which carried the older walk on; a fresh search judged a filesystem it never walked; a mount point root cannot read failed or hung the walk; a found file gone before the package manager was asked failed every resume; and skipped or kept outcomes started a search on every reconcile, back to back with a long walk. --- internal/accounts/setuid_cache.go | 36 ++++++++-- internal/accounts/setuid_cache_test.go | 72 +++++++++++++++++--- internal/accounts/setuid_walk.go | 91 +++++++++++++++++++++++--- internal/accounts/setuid_walk_test.go | 29 ++++++++ internal/accounts/ways.go | 51 +++++++++++++-- internal/accounts/ways_test.go | 2 +- internal/apply/root_moved.go | 7 +- internal/apply/root_moved_test.go | 3 + 8 files changed, 259 insertions(+), 32 deletions(-) diff --git a/internal/accounts/setuid_cache.go b/internal/accounts/setuid_cache.go index 9c29877..3e189a0 100644 --- a/internal/accounts/setuid_cache.go +++ b/internal/accounts/setuid_cache.go @@ -58,6 +58,7 @@ type SetuidCache struct { running bool startedAt time.Time // when the search running, or the last one, was started (a resumed one: its start) attemptAt time.Time // when the running or last search was started or resumed + endedAt time.Time // when the last search ended, on the same clock again bool // asked to search again since the running or last one started failedAt time.Time failed error @@ -71,8 +72,8 @@ type setuidResult struct { Started time.Time `json:"started"` Finished time.Time `json:"finished"` // Mounts are the filesystems it walked; Found the setuid-root programs no package owns. - Mounts []string `json:"mounts"` - Found []string `json:"found"` + Mounts Paths `json:"mounts"` + Found Paths `json:"found"` } const ( @@ -145,6 +146,10 @@ func (c *SetuidCache) SearchAgain() { c.mu.Lock() defer c.mu.Unlock() c.again = true + // The walk under way, or kept, started before root acted: never carried on, even by an engine that + // restarts before the next search starts (the review of 2026-10-10). It keeps no progress from now on. + c.unfinished = nil + c.forgetProgress() } // SetuidSearch is one search: it carries p on to its end — walking what p has not walked, calling checkpoint @@ -176,10 +181,17 @@ func (c *SetuidCache) Look(now time.Time, mounts []string, search SetuidSearch) if c.last != nil { age = now.Sub(c.last.Started) } - fresh := c.last != nil && age > -clockSkew && age < c.freshFor() - asked := c.again || (c.last != nil && !covers(c.last.Mounts, mounts)) + // Fresh only while it walked every filesystem a search would walk now: one mounted since is not judged + // by a search that never saw it. + fresh := c.last != nil && age > -clockSkew && age < c.freshFor() && covers(c.last.Mounts, mounts) + // Asked for (an apply, a new filesystem): at most once an AgainAfter, counted from when the last search + // ended, so a walk of hours is not followed by another at once, apply after apply. + gap := now.Sub(c.attemptAt) + if !c.endedAt.IsZero() { + gap = now.Sub(c.endedAt) + } due := !c.running && (c.failed == nil || now.Sub(c.failedAt) >= c.backoff) && - (!fresh || age >= c.every() || (asked && (c.attemptAt.IsZero() || now.Sub(c.attemptAt) >= AgainAfter))) + (!fresh || age >= c.every() || (c.again && (c.attemptAt.IsZero() || gap >= AgainAfter))) if due { c.start(now, mounts, search) } @@ -250,6 +262,12 @@ func (c *SetuidCache) start(now time.Time, mounts []string, search SetuidSearch) if err == nil { unkept := false found, err = search(context.Background(), p, func() { + c.mu.Lock() + superseded := c.again + c.mu.Unlock() + if superseded { + return // root acted since this walk started: its progress is not to be carried on + } if kerr := c.keepProgress(p); kerr != nil && !unkept { unkept = true c.say("the search for setuid programs could not keep how far it got, and starts over if " + @@ -261,10 +279,14 @@ func (c *SetuidCache) start(now time.Time, mounts []string, search SetuidSearch) c.mu.Lock() defer c.mu.Unlock() c.running = false + c.endedAt = now.Add(took) if err != nil { - c.failed, c.failedAt = err, now + // The back-off counts from when it failed. + c.failed, c.failedAt = err, c.endedAt c.backoff = min(max(2*c.backoff, FailedBackoff), MaxBackoff) - c.unfinished = p + if !c.again { // superseded by an ask to search again: never carried on + c.unfinished = p + } c.say(fmt.Sprintf("the search for setuid programs started %s did not complete (%s this time): %v", p.Started.Local().Format("2006-01-02 15:04"), took.Round(time.Second), err)) return diff --git a/internal/accounts/setuid_cache_test.go b/internal/accounts/setuid_cache_test.go index 5d0a9be..6cf980d 100644 --- a/internal/accounts/setuid_cache_test.go +++ b/internal/accounts/setuid_cache_test.go @@ -139,7 +139,7 @@ func TestAnIncompleteSearchIsNeverFree(t *testing.T) { if starts(c) != 1 { t.Fatalf("tried again within its back-off: %d", starts(c)) } - c.Look(at.Add(FailedBackoff), root, g.search) + c.Look(at.Add(FailedBackoff+time.Second), root, g.search) if starts(c) != 2 { t.Fatalf("not tried again after its back-off: %d", starts(c)) } @@ -168,7 +168,10 @@ func TestAnApplyThatMovedRootSearchesAgain(t *testing.T) { c.Look(at, root, g.search) g.finish(c) c.SearchAgain() - if _, _, err := c.Look(at.Add(AgainAfter), root, g.search); err != nil { + if _, _, err := c.Look(at.Add(time.Minute), root, g.search); err != nil || starts(c) != 1 { + t.Fatalf("asked again within AgainAfter of the last search's end: %v, %d searches", err, starts(c)) + } + if _, _, err := c.Look(at.Add(AgainAfter+time.Second), root, g.search); err != nil { t.Fatalf("the last complete search judges while the new one runs: %v", err) } if starts(c) != 2 { @@ -180,20 +183,71 @@ func TestAnApplyThatMovedRootSearchesAgain(t *testing.T) { if starts(c) != 2 { t.Fatalf("a second search started beside the running one: %d", starts(c)) } - g.finish(c) - c.Look(at.Add(2*AgainAfter+time.Minute), root, g.search) + g.finish(c) // ends, on the cache's clock, at about AgainAfter+1s + c.Look(at.Add(2*AgainAfter), root, g.search) + if starts(c) != 2 { + t.Fatal("a search started again less than AgainAfter after the last one ended") + } + c.Look(at.Add(2*AgainAfter+2*time.Second), root, g.search) if starts(c) != 3 { t.Fatalf("the ask made while a search ran was dropped: %d", starts(c)) } g.finish(c) - // A filesystem the last search did not walk asks for one too. - c.Look(at.Add(4*AgainAfter), []string{"/", "/srv"}, g.search) - if starts(c) != 4 { + var nilCache *SetuidCache + nilCache.SearchAgain() +} + +// A filesystem the last complete search did not walk is not judged by it: not judged yet, and a search starts. +func TestAFilesystemTheSearchDidNotWalkIsNotJudgedByIt(t *testing.T) { + c := &SetuidCache{} + g := newGate() + c.Look(at, root, g.search) + g.finish(c) + _, _, err := c.Look(at.Add(time.Hour), []string{"/", "/srv"}, g.search) + notJudged(t, err, true) + if starts(c) != 2 { t.Fatalf("a new filesystem was not searched: %d", starts(c)) } g.finish(c) - var nilCache *SetuidCache - nilCache.SearchAgain() + if _, _, err := c.Look(at.Add(time.Hour), []string{"/", "/srv"}, g.search); err != nil { + t.Fatalf("once walked it judges: %v", err) + } +} + +// An ask to search again forgets the walk under way, so an engine restarted before the next search does not +// carry on a walk that started before root acted. +func TestAnAskToSearchAgainIsNotLostToARestart(t *testing.T) { + dir := t.TempDir() + paths := func() *SetuidCache { + return &SetuidCache{Path: filepath.Join(dir, SetuidSearchFile), ProgressPath: filepath.Join(dir, SetuidProgressFile)} + } + seen := make(chan *SetuidProgress, 1) + first := paths() + first.Look(at, root, interrupted(seen)) + <-seen + first.SearchAgain() // an apply moved root's things; then the engine restarts + restarted := paths() + var with SetuidProgress + restarted.Look(at.Add(time.Hour), root, func(ctx context.Context, p *SetuidProgress, _ func()) ([]string, error) { + with = *p + return nil, nil + }) + restarted.Wait() + if !with.Started.Equal(at.Add(time.Hour)) || len(with.Found) != 0 { + t.Fatalf("the walk from before the apply was carried on: %+v", with) + } +} + +// A cursor whose name is not UTF-8 is kept as its bytes: JSON strings would change it, and the resumed walk +// would skip what it stood before. +func TestACursorOfAnyBytesIsKeptExactly(t *testing.T) { + p := &SetuidProgress{Started: at, Mounts: Paths{"/"}, Cursor: Cursors{"/": "/home/agent/\x80/x"}, + Found: Paths{"/opt/\xff"}} + var back SetuidProgress + fromJSON(t, mustJSON(t, p), &back) + if back.Cursor["/"] != "/home/agent/\x80/x" || back.Found[0] != "/opt/\xff" { + t.Fatalf("kept as %q, %q", back.Cursor["/"], back.Found[0]) + } } func TestTheCompleteSearchIsKeptAndReadAfterARestart(t *testing.T) { diff --git a/internal/accounts/setuid_walk.go b/internal/accounts/setuid_walk.go index 86dde8e..1df3031 100644 --- a/internal/accounts/setuid_walk.go +++ b/internal/accounts/setuid_walk.go @@ -2,6 +2,7 @@ package accounts import ( "context" + "encoding/json" "errors" "fmt" "io/fs" @@ -34,12 +35,66 @@ type SetuidProgress struct { ID string `json:"id"` Started time.Time `json:"started"` // Mounts are the filesystems it walks, root first; Done those walked to their end. - Mounts []string `json:"mounts"` - Done []string `json:"done"` + Mounts Paths `json:"mounts"` + Done Paths `json:"done"` // Cursor is, for a filesystem being walked, the last path visited. - Cursor map[string]string `json:"cursor"` + Cursor Cursors `json:"cursor"` // Found is every setuid- or setgid-root regular file found so far, before the package manager is asked. - Found []string `json:"found"` + Found Paths `json:"found"` +} + +// Paths are paths kept as their bytes. **Never as JSON strings**: encoding/json replaces bytes that are not +// UTF-8 with U+FFFD, and a cursor so changed sorts after the names it stood before — a resumed walk would skip +// them unread (the review of 2026-10-10). A name of any bytes is any account's to make. +type Paths []string + +func (ps Paths) MarshalJSON() ([]byte, error) { + raw := make([][]byte, len(ps)) + for i, p := range ps { + raw[i] = []byte(p) + } + return json.Marshal(raw) +} + +func (ps *Paths) UnmarshalJSON(b []byte) error { + var raw [][]byte + if err := json.Unmarshal(b, &raw); err != nil { + return err + } + *ps = make(Paths, len(raw)) + for i, r := range raw { + (*ps)[i] = string(r) + } + return nil +} + +// Cursors are each filesystem's cursor, kept as bytes as Paths are. +type Cursors map[string]string + +type keptCursor struct { + Mount []byte `json:"mount"` + Path []byte `json:"path"` +} + +func (cs Cursors) MarshalJSON() ([]byte, error) { + raw := []keptCursor{} + for m, p := range cs { + raw = append(raw, keptCursor{Mount: []byte(m), Path: []byte(p)}) + } + slices.SortFunc(raw, func(a, b keptCursor) int { return strings.Compare(string(a.Mount), string(b.Mount)) }) + return json.Marshal(raw) +} + +func (cs *Cursors) UnmarshalJSON(b []byte) error { + var raw []keptCursor + if err := json.Unmarshal(b, &raw); err != nil { + return err + } + *cs = Cursors{} + for _, r := range raw { + (*cs)[string(r.Mount)] = string(r.Path) + } + return nil } // CheckpointEvery is how often a walk's progress is kept. @@ -49,10 +104,13 @@ const CheckpointEvery = time.Minute var prunedPaths = []string{"/proc", "/sys", "/var/lib/docker", "/var/lib/containers"} // walker walks for setuid programs. rootUID and rootGID are the owner and group a match has: root's (0), but -// for a test. +// for a test. mountPoints are every filesystem mounted on the machine (MountPoints): a directory that is one is +// another filesystem, left without being read — a FUSE mount root cannot read, or a network mount whose +// server is away, would fail or hang the walk (the review of 2026-10-10). type walker struct { rootUID, rootGID uint32 every time.Duration + mountPoints map[string]bool } // walk carries p on to its end, calling checkpoint every w.every and after each filesystem. It ends early only @@ -60,7 +118,7 @@ type walker struct { // A file or directory that vanished while it walked is not an error: what still exists was walked. func (w walker) walk(ctx context.Context, p *SetuidProgress, checkpoint func()) error { if p.Cursor == nil { - p.Cursor = map[string]string{} + p.Cursor = Cursors{} } last := time.Now() for _, m := range p.Mounts { @@ -75,8 +133,23 @@ func (w walker) walk(ctx context.Context, p *SetuidProgress, checkpoint func()) checkpoint() last = time.Now() } - slices.Sort(p.Found) - p.Found = slices.Compact(p.Found) + // Looked at again: a file found that is gone, or no longer setuid-root, is no answer the package manager + // can give, and would fail every search that resumes this walk (the review of 2026-10-10). + var still Paths + for _, f := range p.Found { + info, err := os.Lstat(f) + if errors.Is(err, fs.ErrNotExist) { + continue + } + if err != nil { + return fmt.Errorf("the walk for setuid programs could not read %s again: %w", f, err) + } + if w.setuidRoot(info) { + still = append(still, f) + } + } + slices.Sort(still) + p.Found = slices.Compact(still) return nil } @@ -113,7 +186,7 @@ func (w walker) walkMount(ctx context.Context, p *SetuidProgress, mount string, } switch { case d.IsDir(): - if path != mount && slices.Contains(prunedPaths, path) { + if path != mount && (slices.Contains(prunedPaths, path) || w.mountPoints[path]) { return filepath.SkipDir } info, err := os.Lstat(path) diff --git a/internal/accounts/setuid_walk_test.go b/internal/accounts/setuid_walk_test.go index 684a8b0..4d6a636 100644 --- a/internal/accounts/setuid_walk_test.go +++ b/internal/accounts/setuid_walk_test.go @@ -182,3 +182,32 @@ func TestTheSearchRunsOnALoweredThreadOfItsOwn(t *testing.T) { t.Fatalf("the thread could not be lowered: %v", err) } } + +// Another filesystem's mount point is left without being read, and a file found that is gone, or no longer +// setuid-root, when the walk ends is not asked about. +func TestTheWalkLeavesMountPointsAndDropsWhatWentMeanwhile(t *testing.T) { + top := tree(t, "fuse/secret!", "bin/x!", "bin/y!") + w := mine() + w.mountPoints = map[string]bool{filepath.Join(top, "fuse"): true} + w.every = 0 + p := &SetuidProgress{Mounts: []string{top}} + removed := false + if err := w.walk(t.Context(), p, func() { + if !removed && strings.HasSuffix(p.Cursor[top], "/bin/y") { + removed = true + os.Remove(filepath.Join(top, "bin/x")) + } + }); err != nil { + t.Fatal(err) + } + if got := rel(top, p.Found); strings.Join(got, ",") != "bin/y" { + t.Fatalf("found %v", got) + } +} + +func TestAMountPointIsReadAsTheKernelEscapesIt(t *testing.T) { + mp := MountPoints("/dev/a /mnt/a\\040b ext4 rw 0 0\n/dev/b /mnt/back\\134slash ext4 rw 0 0\n") + if !mp["/mnt/a b"] || !mp[`/mnt/back\slash`] { + t.Fatalf("read %v", mp) + } +} diff --git a/internal/accounts/ways.go b/internal/accounts/ways.go index ef4c0e8..100770a 100644 --- a/internal/accounts/ways.go +++ b/internal/accounts/ways.go @@ -355,7 +355,7 @@ func SuidMounts(text string) []string { if len(f) < 4 || pseudoFS[f[2]] || strings.HasPrefix(f[2], "fuse") { continue } - at := strings.ReplaceAll(strings.ReplaceAll(f[1], `\040`, " "), `\011`, "\t") + at := unescapeMount(f[1]) if strings.HasPrefix(at, "/var/lib/docker") || strings.HasPrefix(at, "/var/lib/containers") || strings.HasPrefix(at, "/proc") || strings.HasPrefix(at, "/sys") { continue @@ -370,6 +370,45 @@ func SuidMounts(text string) []string { return out } +// MountPoints is every mount point /proc/mounts lists, of any type: the walk leaves each it meets inside +// another filesystem without reading it. +func MountPoints(text string) map[string]bool { + out := map[string]bool{} + for _, line := range strings.Split(text, "\n") { + if f := strings.Fields(line); len(f) >= 2 { + out[unescapeMount(f[1])] = true + } + } + return out +} + +// unescapeMount is a mount point as /proc/mounts escapes it — a space, a tab, a newline and a backslash as +// three octal digits after a backslash — made a path again. +func unescapeMount(s string) string { + var b strings.Builder + for i := 0; i < len(s); i++ { + if s[i] == '\\' && i+3 < len(s) && isOctal(s[i+1:i+4]) { + b.WriteByte((s[i+1]-'0')<<6 | (s[i+2]-'0')<<3 | (s[i+3] - '0')) + i += 3 + continue + } + b.WriteByte(s[i]) + } + return b.String() +} + +func isOctal(s string) bool { + if len(s) != 3 { + return false + } + for i := 0; i < 3; i++ { + if s[i] < '0' || s[i] > '7' { + return false + } + } + return true +} + func contains(xs []string, s string) bool { for _, x := range xs { if x == s { @@ -384,10 +423,10 @@ func contains(xs []string, s string) bool { // search's runner (Exec.SearchRun). **No bound of its own** (novox/hq issue 361): it runs until it has walked // everything. A walk that could not complete — a directory it could not read — is an unanswered question, never // "none"; a file that vanished while it walked is not. -func (e Exec) setuidSearch(ctx context.Context, p *SetuidProgress, checkpoint func()) ([]string, error) { +func (e Exec) setuidSearch(ctx context.Context, p *SetuidProgress, checkpoint func(), mountPoints map[string]bool) ([]string, error) { walk := e.Walk if walk == nil { - walk = walker{every: CheckpointEvery}.walk + walk = walker{every: CheckpointEvery, mountPoints: mountPoints}.walk } if err := walk(ctx, p, checkpoint); err != nil { return nil, fmt.Errorf("the walk for setuid programs did not complete: %w", err) @@ -592,7 +631,11 @@ func (e Exec) moreWays(ctx context.Context, account string, uid int, groups []st } // In the background, the last complete result served while it is fresh (SetuidCache): never a stall. No // fresh complete result is said beside the other ways, which are judged all the same. - unowned, searchedAt, searchErr := e.Cache.Look(now(), mounts, e.setuidSearch) + mountPoints := MountPoints(string(mountsText)) + unowned, searchedAt, searchErr := e.Cache.Look(now(), mounts, + func(ctx context.Context, p *SetuidProgress, checkpoint func()) ([]string, error) { + return e.setuidSearch(ctx, p, checkpoint, mountPoints) + }) var notJudged *SetuidNotJudged if searchErr != nil && !errors.As(searchErr, ¬Judged) { return nil, searchErr diff --git a/internal/accounts/ways_test.go b/internal/accounts/ways_test.go index 2196db5..c8be804 100644 --- a/internal/accounts/ways_test.go +++ b/internal/accounts/ways_test.go @@ -275,7 +275,7 @@ func TestThePackageManagerIsAskedOnceForEveryPathFound(t *testing.T) { m.setuid = "/usr/bin/sudo\n/usr/bin/passwd\n/usr/local/bin/x\n/opt/y\n" m.packaged = map[string]bool{"/usr/bin/sudo": true, "/usr/bin/passwd": true} search := func(e Exec) ([]string, error) { - return e.setuidSearch(t.Context(), &SetuidProgress{Mounts: []string{"/"}}, func() {}) + return e.setuidSearch(t.Context(), &SetuidProgress{Mounts: []string{"/"}}, func() {}, nil) } found, err := search(Exec{Run: m.run, Walk: m.walk}) if err != nil { diff --git a/internal/apply/root_moved.go b/internal/apply/root_moved.go index af9bca2..b246c0a 100644 --- a/internal/apply/root_moved.go +++ b/internal/apply/root_moved.go @@ -16,11 +16,14 @@ var rootPaths = []string{"/etc/passwd", "/etc/shadow", "/etc/group", "/etc/gshad // MovesRoot says an apply changed something through which root's reach could have changed — an account, a // package, an action run as root, or a file in rootPaths (novox/hq issue 361). The engine then asks for a new // search for setuid programs: root acted, and a complete search is sound only until root acts. Outcomes that -// moved nothing (unchanged, held, waiting, held still) do not count. +// moved nothing (unchanged, held, waiting, held still, skipped, kept, forgotten) do not count. func MovesRoot(outcomes []Outcome) bool { for _, o := range outcomes { + // Only what moved the machine: skipped, kept and forgotten touch nothing, and counting them made one + // broken module start a search on every reconcile (the review of 2026-10-10). switch o.Action { - case "unchanged", "held", "waiting", heldStill: + case "created", "updated", "corrected", "removed": + default: continue } switch declaration.Type(o.Type) { diff --git a/internal/apply/root_moved_test.go b/internal/apply/root_moved_test.go index 683f088..f099087 100644 --- a/internal/apply/root_moved_test.go +++ b/internal/apply/root_moved_test.go @@ -21,6 +21,9 @@ func TestAnApplyMovesRootThroughAnAccountAPackageAnActionOrSudosRules(t *testing {Outcome{Type: "file", Target: "/etc/nginx/nginx.conf", Action: "updated"}, false}, {Outcome{Type: "container", Target: "plex", Action: "updated"}, false}, {Outcome{Type: "package", Target: "x", Action: heldStill}, false}, + {Outcome{Type: "action", Target: "setup", Action: "skipped"}, false}, + {Outcome{Type: "user", Target: "nox", Action: "forgotten"}, false}, + {Outcome{Type: "package", Target: "x", Action: "kept"}, false}, } { if got := MovesRoot([]Outcome{c.o}); got != c.want { t.Errorf("%+v: moves root %v, want %v", c.o, got, c.want)