fix(daemon): verify process identity before signaling the persisted PID - #789
fix(daemon): verify process identity before signaling the persisted PID#789iaroslav-reflection wants to merge 2 commits into
Conversation
Terminate sends SIGTERM to the PID persisted in the daemon record without checking what is running there. After a snapshotter restart the record can be stale, and on busy hosts PIDs are recycled quickly, so the signal can hit an unrelated process: another daemon, a shim, or a container process. Record the process start time (field 22 of /proc/<pid>/stat) next to the PID when nydusd is spawned; PID plus start time identify a process instance, since a recycled PID cannot have the same start time. Terminate refuses to signal when the start time does not match. This is the same guard runc uses against PID reuse when killing containers. Records without a start time (persisted by older snapshotter versions) keep the previous unverified behavior, so existing daemons are still torn down; every record written after this change is verified. Signed-off-by: Iaroslav Geraskin <iaroslav@reflection.ai>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #789 +/- ##
==========================================
+ Coverage 25.80% 26.39% +0.58%
==========================================
Files 134 134
Lines 12860 12912 +52
==========================================
+ Hits 3319 3408 +89
+ Misses 9130 9088 -42
- Partials 411 416 +5
🚀 New features to boost your workflow:
|
|
func (d *Daemon) RecordProcess(pid int) {
d.States.ProcessID = pid
startTime, err := tool.GetProcessStartTime(pid)
if err != nil {
log.L.WithError(err).Warnf(...)
return
}
d.States.ProcessStartTime = startTime
}
One more thing: |
…re acting on it RecordProcess left the previous process's start time in place when reading /proc failed. StartDaemon reuses the same Daemon on restart/failover, so the identity check compared the new PID against the stale value, always mismatched, and Terminate became a permanent no-op for that daemon. Reset the start time to zero in that branch so the record falls back to the unverified pre-check behavior. Centralize the check into VerifiedPid(), which returns the recorded PID only after verifying the process identity, and use it everywhere the snapshotter acts on the process: Terminate and CgroupMgr.AddProc in StartDaemon and recoverDaemons, where adding a recycled PID to the cgroup has real side effects on an unrelated process. Signed-off-by: Iaroslav Geraskin <iaroslav@reflection.ai>
|
Thanks! Both fixed. |
2 participants
Terminatesends SIGTERM to the PID persisted in the daemon record without checking what is actually running there. After a snapshotter restart the record can be stale, and on busy hosts PIDs are recycled quickly — the signal can land on an unrelated process (another nydusd, a shim, a container process).flowchart LR A["record: PID 1234"] --> B["nydusd 1234 dies,<br/>snapshotter restarts"] B --> C["kernel recycles 1234<br/>for an unrelated process"] C --> D["DestroyDaemon → Terminate"] D --> E["💥 SIGTERM to the wrong process"]Fix: record the process start time (field 22 of
/proc/<pid>/stat) next to the PID at spawn. PID + start time identify a process instance — a recycled PID cannot have the same start time.Terminateverifies identity before signaling and refuses on mismatch. This is the same guard runc uses against PID reuse when killing containers (hasInit/signalInit, comparinginit_process_startfrom its state file).Records persisted by older versions carry no start time and keep today's unverified behavior, so existing daemons are still torn down; every record written after this change is verified. The new
ConfigStatefield is JSON-serialized, so old records load cleanly.