fix(unikontainers): verify pid identity before signaling or joining its netns - #900
Draft
Anand-240 wants to merge 1 commit into
Draft
fix(unikontainers): verify pid identity before signaling or joining its netns#900Anand-240 wants to merge 1 commit into
Anand-240 wants to merge 1 commit into
Conversation
✅ Deploy Preview for urunc canceled.
|
…ts netns isRunning(), Signal(), Kill(), and joinSandboxNetNs() all trusted a raw stored pid without checking whether it still identified the VMM process that was originally launched. Linux recycles pid numbers as soon as a process is reaped, so after the VMM exits and its pid gets reused by an unrelated process, urunc could end up sending SIGKILL to that unrelated process and joining its network namespace instead of the sandbox's, or could refuse to ever delete an already-dead container because isRunning() saw a live but unrelated process. Record the /proc/<pid>/stat starttime for the VMM pid at Create() time, since the kernel guarantees this value changes whenever a pid number is reused, and validate it before treating the pid as belonging to this container in isRunning(), Signal(), Kill(), and joinSandboxNetNs(). A mismatch is now treated the same as the process no longer existing. Fixes urunc-dev#899 Signed-off-by: Anand-240 <anandprakashsrivastava68@gmail.com>
Anand-240
force-pushed
the
fix/pid-reuse-identity-check
branch
from
August 4, 2026 13:15
bc66493 to
c6e53ee
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #899.
isRunning(), Signal(), Kill() and joinSandboxNetNs() were all trusting the raw pid stored in state without checking whether it still belonged to the VMM we actually launched. Since Linux reuses pids pretty fast once a process is reaped, if the VMM dies and something else on the host grabs that same pid before we get around to kill/delete, we'd end up signaling the wrong process and even joining its netns instead of the sandbox's.
Fix records the pid's /proc//stat starttime once at Create() time (that value changes whenever a pid gets reused, so it works as a cheap identity check), and checks it before acting on the pid anywhere we previously just assumed it was still ours. If it doesn't match anymore we just treat it like the process is already gone.
Added a few unit tests for the new starttime helper and the identity check itself, covering a live pid, a dead one, a mismatched starttime, negative pid, etc.
Ran go build and go test on pkg/unikontainers, everything passes except two tests (TestCopyFile, TestMoveFile) that were already failing on a clean upstream/main checkout too, they rely on read-only permission checks that don't trigger when running as root in a container, unrelated to this change.