Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba675e723f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ba675e7 to
686ba5e
Compare
|
Claude finished @andypost's task in 5m 22s —— View job Review of #421 — pid-isolated worker record
The design holds up: main is indeed the only process that holds both names, the slot is appended so the wire numbering is stable, the teardown sequence in the handler is a faithful copy of the One thing survived that fix, on the other half of the pair. The zero payload resolves against the "unnamed" sentinel
nxt_queue_each(child, &pprocess->children, nxt_process_t, link) {
if (child->parent_ns_pid != pid) {
continue;
}
Children with The sharper case is the one the function's own comment rules out (
Not with a zero payload. Without The precedent for the fix is a few hundred lines away, for the same reason: /* nxt_port_process_ready_handler(), src/nxt_port.c:664 */
* Reject rather than compare, so that a cmsg_pid of 0 or the -1 that
* the shared-memory queue path leaves in place ... cannot match by accident.Rejecting Two smaller notesThe forger's reach is slightly wider than the comment says.
Otherwise
I could not build or run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 686ba5e4b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| nxt_main_test_run_name_child(task, proto, child, NXT_MAIN_RCP_NS_PID, | ||
| child_pid); | ||
|
|
||
| if (nxt_slow_path(child->parent_ns_pid != NXT_MAIN_RCP_NS_PID)) { |
There was a problem hiding this comment.
On macOS, NetBSD, OpenBSD, and illumos, NXT_USE_CMSG_PID is false, so nxt_main_process_name_child() is compiled as a no-op and child->parent_ns_pid remains zero. This unconditional assertion therefore makes build/tests fail whenever the test suite runs on those supported platforms; the naming/removal expectations need to be conditional on credential support, with the no-credential path instead verifying that no child name is recorded or resolved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b5a23d23. Forcing NXT_HAVE_UCRED and NXT_HAVE_SOCKOPT_SO_PASSCRED off locally, build/tests failed at exactly this line (exit 1, 41 passed) — and five assertions in the file were credential-dependent, not one.
The naming and resolution arms are now under #if (NXT_USE_CMSG_PID), and the #else arm asserts the inert shape you suggested rather than skipping: nothing is named, the report the prototype does send resolves nothing, the record stays and the router is told nothing (exit 0, 50 passed).
src/test/nxt_proto_creating_wedge_test.c is not affected. nxt_proto_report_child_pid() is gated on rt->is_pid_isolated, not on the credential, so the message is sent on every platform, and that test passed in the no-credential build.
Writing those inert assertions turned up a real gap, so this carries a one-line production change too: the handler matched child->parent_ns_pid == pid with no floor, and 0 is how "no name" is stored — so a report naming 0 retired the first child holding no name. It now refuses pid <= 0, mirroring the naming side, with a test case that reddens with credentials on as well as off.
Worth noting for the platform question: rt->is_pid_isolated can only be set where Linux namespaces and CLONE_NEWPID exist, and every such platform has SO_PASSCRED and struct ucred — so no prototype ever sends this message where the credential is missing, and the #310 window cannot open there. That reasoning is now in the handler comment.
Under "isolation": {"namespaces": {"pid": true}} a worker that died in the
CREATING state left the main process holding the record, the port and the
descriptor it had made for it at WHOAMI time, until the prototype itself
exited. With the start RPC now retired and requests retrying, a worker that
keeps dying in that window costs main one of each per attempt, and main is
the process that can least afford to run out of descriptors.
The gate that left it there is right about REMOVE_PID. A pid is that
message's whole payload, and the only pid a prototype has for a worker that
has not completed the PROCESS_CREATED handshake is the namespace-local one
it got from fork(). nxt_proc_remove_notify_matrix pairs a dying APP with
MAIN and the router, a prototype's local counter climbs with every worker
straight into the range the daemon's own pids occupy, and
nxt_port_remove_pid() closes the ports of whatever process holds the number
-- so broadcasting it would drop a live sibling's, the router's or the
controller's in-flight work. Losing requests is worse than the leak.
Main is the one process that can resolve that pid, because it is the one
that holds both names. A worker's WHOAMI arrives with the kernel-translated
global pid in SCM_CREDENTIALS, which is what main keys its record on, and
with the worker's own namespace-local pid in the message header -- workers
are forked with no clone flags of their own, so that is the same number the
prototype got from fork(). Measured, not assumed: main logs "whoami" from
global pid 29 with header pid 2 while the prototype logs fork() returning 2.
Main keeps the pair, and NXT_PORT_MSG_REMOVE_CHILD_PID then names the worker
by the only pid the prototype has.
The pair is kept whether or not the two numbers differ. The counters are
independent, so a global pid that has wrapped can land on the small number a
namespace-local one holds, and refusing the pair on that equality would
leave behind exactly the record this path exists to retire. What the pair
needs is two independently sourced pids, which is what NXT_USE_CMSG_PID
says: without it nxt_recv_msg_cmsg_pid() is the header pid itself, so
nothing is recorded and nothing can ever be resolved. Nothing is lost by
that: a pid namespace needs Linux unshare() and CLONE_NEWPID, and a platform
with both has SO_PASSCRED and struct ucred, so no prototype ever sends this
message there. The tests assert that inert shape rather than skipping it.
/proc/<pid>/status NSpid would give main the same number from the kernel
rather than from the sender, at the cost of an open and a parse per start.
It is not needed. The header pid is only ever looked for among the children
of the sender the kernel names, so a prototype can report its own workers
and nothing else, and a worker that names a live sibling instead of itself
is refused rather than believed -- which leaves a forger able to give up its
own record and nothing more. Both ends refuse a pid of 0 or less: zero is
how "no name" is stored, which a child keeps wherever the pair was refused,
so a report of it would retire the first such child of the sender.
What is not checked is that the worker is dead, and nothing in main can
check it: the pid names a process in a namespace main does not share, and
the prototype is its only reaper. A compromised prototype can therefore
orphan a live worker of its own from main's bookkeeping. That is not a new
trust boundary -- it is the real parent, it already kills its own children
on a failed start, and outside a pid namespace it can say the same thing
with REMOVE_PID -- but it is a new way to desync that bookkeeping, and it is
said where the handler trusts the report.
Main tells the router by the global pid once it has one, so the router sees
what an ordinary REMOVE_PID would have given it. It has nothing to retire
in practice -- a worker reaches it through the PROCESS_READY this one never
sent -- but it keeps the two paths identical from its side.
The message slot is appended, which is the only index-stable edit to
nxt_port_handlers_t: every _NXT_PORT_MSG_* value is that struct's offset.
Measured under rootlesskit with "pid": true and a worker killed inside the
WHOAMI round trip, five times: main's open descriptors went 14, 15, 16, 17,
18 without this and stayed at 14 with it.
Closes #310.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
686ba5e to
b5a23d2
Compare
1 participant
Under
"isolation": {"namespaces": {"pid": true}}a worker that died in theCREATING state left main holding the record, the port and the descriptor it made
at WHOAMI time until the prototype exited. Measured under
rootlesskitwith aworker killed inside the WHOAMI round trip, five times: main's open descriptors
went 14, 15, 16, 17, 18 without this and stayed at 14 with it. The bound is the
prototype's lifetime, which is the application's, so in practice this is an fd
leak in the most privileged, longest-lived process.
REMOVE_PIDcannot carry it -- a pid is its whole payload, and the only pid aprototype has for such a worker is the namespace-local one, which names an
unrelated process everywhere else. Main is the one process that holds both
names: the WHOAMI arrives with the global pid in
SCM_CREDENTIALSand theworker's namespace-local pid in the message header, and workers are forked with
no clone flags of their own, so that is the number the prototype got from
fork(). Main keeps the pair when the two disagree, andNXT_PORT_MSG_REMOVE_CHILD_PIDnames the worker by it./proc/<pid>/statusNSpid, which #310 proposed, is not needed. The header pidis only ever looked for among the children of the sender the kernel names, and a
worker naming a live sibling is refused, so a forger can give up its own record
and nothing more. On a platform with no credential nothing is recorded, so
nothing can be resolved.
The slot is appended, the only index-stable edit to
nxt_port_handlers_t.src/test/nxt_main_remove_child_pid_test.cdrives both halves, and thepid-isolated cases of
src/test/nxt_proto_creating_wedge_test.cnow assert themessage instead of its absence. Five mutations each fail the matching assertion.
Validation: debug and release builds, 0 warnings;
build/tests51 passed (50 onmaster);
build/unit_port_recv_test22 unchanged. Two failures intest_process_abrupt_teardown.pyandtest_process_teardown_churn.pyreproduceidentically on a clean master build here and are unrelated.
Closes #310.
🤖 Generated with Claude Code