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: 97687a49a5
ℹ️ 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".
|
|
||
| pub fn pair() -> std::io::Result<(Self, Self)> { | ||
| let (server, client) = Socket::pair(Domain::UNIX, Type::DGRAM, None)?; | ||
| let (server, client) = Socket::pair_raw(Domain::UNIX, Type::DGRAM, None)?; |
There was a problem hiding this comment.
Switching AsyncDatagramSocket::pair() to Socket::pair_raw makes both ends inheritable across exec, but EscalateServer::exec only intends to pass the client FD via ESCALATE_SOCKET_ENV_VAR. With this change, the spawned shell also inherits the server endpoint, so a child process can consume handshake datagrams (and their passed stream FDs) before escalate_task reads them, causing flaky escalation handshakes and policy-server bypass/DoS behavior in real command trees.
Useful? React with 👍 / 👎.
|
|
||
| pub fn pair() -> std::io::Result<(AsyncSocket, AsyncSocket)> { | ||
| let (server, client) = Socket::pair(Domain::UNIX, Type::STREAM, None)?; | ||
| let (server, client) = Socket::pair_raw(Domain::UNIX, Type::STREAM, None)?; |
There was a problem hiding this comment.
Changing AsyncSocket::pair() to Socket::pair_raw removes close-on-exec for the control channel used by escalate_client::run. In the EscalateAction::Run path, that process immediately calls execv, so the target command now inherits internal escalation stream FDs that were previously dropped on exec; long-lived/daemonized commands can retain these descriptors and unexpectedly keep internal channels alive.
Useful? React with 👍 / 👎.
bolinfest
force-pushed
the
pr12614
branch
4 times, most recently
from
a4a59f9 to
420df62
Compare
1 participant
Why
PR #12584 became difficult to review as a single feature diff. This PR isolates the
shell-escalationprep work so laterexec-serverandcorechanges can be reviewed against a stable API.The main goal is to make
shell-escalationreusable from multiple callers without hard-coding a dependency oncodex-coreinternals.What Changed
shell-escalation(ShellCommandExecutor) and thread it throughunix::escalate_server::run_escalate_server.shell-escalationinto a crate-localSandboxStatetype.unixmodule API surface needed by downstream callers (pub mod unix, publicShellEscalationPolicy).unix/socket.rs(explicittry_ioerror handling andSocket::pair_rawfor Unix socketpairs).shell-escalation/src/lib.rsso existing callers continue to build while the stacked follow-up PRs land.Verification
#12615and the remainingcore/test changes), which exercises the integration path.Stack created with Sapling. Best reviewed with ReviewStack.