SIGN IN SIGN UP

fix(processtree): harden the tree against process-id reuse (SUB-7846)

The kernel recycles pids, and the tree keeps an exited process's node for up
to exitCleanup::cleanupDelay. For that window a pid can be held by a LIVE
process while the tree still holds the DEAD one's node under it. Nothing
noticed, and two things went wrong. Both are reproduced in
pid_reuse_test.go, not inferred — the delayed-deletion half was found by
matthyx reviewing #873 and reproduced on that PR's head:

- handleForkEvent fills only EMPTY fields and a stale node is not empty, so
  a recycled pid kept the predecessor's Comm, Cmdline, Path, Cwd, Uid and
  Gid. A runtime alert of any type could name the wrong command.
- exitByPid matches on pid alone, so the dead process's delayed exit deleted
  whatever node held that pid by then — a live process — reparenting its
  children around it and taking its start time with it.

Three layers, each falling through to today's behaviour when its input is
unknown, and none of which ever KEEPS a node it cannot prove is newer:

1. A fork or exec on a pid with a pending exit retires the predecessor. The
   event proves a new live incarnation, since a zombie can neither fork nor
   exec, so no clock is needed. Deliberately NOT applied to procfs events:
   /proc lists zombies, so there a pending exit can belong to the same
   incarnation and proves nothing.
2. A delayed exit skips a node whose recorded creation postdates the exit
   event's arrival. Both sides are boot-relative — the side map is procfs
   ticks x 10^7, the arrival is CLOCK_BOOTTIME — so there is no btime skew
   and no wall-clock margin. pendingExit.Timestamp and .StartTimeNs stay
   wall-clock with their existing jobs and are not used here.
3. The procfs scan rebuilds a node whose start time changed. Two readings of
   one process yield identical ticks, so a different non-zero value is proof
   with no tolerance needed. The rebuild re-applies the Kubernetes
   host-process policy, which the first-sighting path enforces and an
   unconditional rebuild would bypass.

Layer 1 implements the shape matthyx proposed on SUB-7846, with one
difference. His snippet forces ok = false from the branch where a stale node
was found; the guard here runs before the lookup instead, so ok is false
naturally and no stale proc pointer exists. That also covers a case the
in-branch position misses: a pending exit can outlive its node, and the
recycled fork then builds a fresh node that the stale entry deletes at the
next cleanup. TestPidReuse_ForkAfterExit_ConsumesPendingExitWithNoNode fails
if the guard is gated on an existing node.

Retiring means the full teardown, NOT delete(pt.pendingExits, pid). That
simplification passes the obvious tests while leaving the dead process's
children linked to the pid, so the new process silently inherits them and
the tree claims an unrelated live process is their parent. It is quieter
than the bug it replaces.
TestPidReuse_ForkAfterExit_DoesNotInheritDeadChildren fails if anyone tries
it.

removeProcessNode extracts the teardown so the exit path and the procfs
rebuild cannot drift; it is independent of pendingExits because the rebuild
needs it for a pid with no pending exit at all. The extraction landed with
all 24 exit-manager and reparenting tests green before layer 3 used it. It
returns false when reparenting fails, and both callers then leave their
bookkeeping alone so the operation degrades to today's retry-next-tick. A
nil reparenting strategy takes the same failure path rather than being
dereferenced: unreachable today, since NewReparentingLogic ends in an
unconditional `return rl, nil`, but the agent has no recover() anywhere, so
the panic would end the process rather than one goroutine.

Under pt.mutex the additions are a map lookup and an integer comparison, and
no /proc read is added. The one syscall this work needs — CLOCK_BOOTTIME for
layer 2 — is read in handleExitEvent BEFORE the lock and passed into
addPendingExit, because unix.ClockGettime is a raw syscall rather than a vDSO
call and exits are about as frequent as forks. Same pattern, and same reason,
as the fork path's ~7.5 us start-time read.

forceCleanupOldest loses its second, redundant pass over the same pending
exits. That pass was silent while a repeat call always found the node gone;
now that a guard can keep a node and consume its pending entry, it would log
a warning for every node kept.

This is shared process-tree lifecycle: GetContainerProcessTree serves the
rule manager and every exporter, so this path sits behind every alert type.
Same-incarnation behaviour is therefore pinned by controls —
ProcfsSameStartTime_MergesAsToday keeps the merge semantics and children,
DelayedExit_NormalExitStillDeletes keeps ordinary exits deleting (without
it, an inverted comparison would leak every exited node and nothing would
catch it), and DelayedExit_UnknownStartTimeStillDeletes keeps the no-data
case byte-for-byte today's logic. Each guard was mutation-tested: deleting
it must turn a named test red.

The exit-manager lifecycle doc's SUB-7846 caveat is updated here rather than
in the preceding commit, where it was still accurate and where this doc does
not yet exist to link to.

Three limitations are documented rather than fixed. Layer 1 cannot
distinguish a recycle from a reordered exit, since the tracers do not
preserve kernel order across one queue. Layer 3 declines when the side map
holds no prior value, which lets layer 2 keep a node the scan merged rather
than rebuilt. And when a fork is processed BEFORE the exit it follows, all
three layers decline and both original bugs survive — confirmed against the
implementation, and left alone because the window is about one drain batch
and closing it would make the guards depend on ordering they cannot verify.

exitByPid settles the node-absent case before the arrival comparison, so the
guard cannot return while leaving an orphaned side-map entry. Unreachable
today, since side-map entries only exist alongside nodes, but it was the one
new branch relying on that invariant without stating it.

Verified in the Linux container, since macOS cannot build this package:
go vet ./pkg/processtree/... && go test -race -count=2 ./pkg/processtree/...
vet clean, 0 race reports, every package ok.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
A
Alon committed
dd16818c09301abf5720f6248d5c545cbaa8b988
Parent: 21a4fed