Repository navigation
Windows: no socket clone a child process can inherit (sbx proxy, tool bridge) - #79
Merged
Merged
Conversation
Root cause of the flaky proxy tests (connection reset, os error 10054, at proxy.rs while reading a 403/502): serve() read the request through client.try_clone(), and on Windows that clone is created INHERITABLE (measured with GetHandleInformation: the accepted socket inherit=0, its clone inherit=1). Any process the same process started while the clone was open (Command with piped stdio inherits handles) took a copy of the connection; closing ours then sent nothing, and the child's exit reset the connection, discarding a reply the client had already received. In the test suite the children were the ACL tests' icacls; bisected: proxy tests fail 6 of 6 beside windows::acl::tests and never beside any other module. In real use sbx starts the sandboxed command and icacls from the process the proxy runs in, so a refused or finished connection could hang open inside the command or be reset. serve() now reads and writes through &TcpStream, and pipe() copies both ways on scoped threads borrowing the two sockets: no clone exists to leak. The same shape in incode's tool bridge (answer_connection read the request through try_clone while the toolbox may be starting shell commands, background jobs included) is fixed the same way. New test: a_refusal_reaches_the_client_while_processes_are_started_beside_it starts children in a loop while 200 requests are refused; every refusal must be read whole. With the clone put back it loses 1 or 2 of 60 on every run. Tested: incord-sandbox all targets (lib 76, escape_windows 23), the whole sandbox lib 10 of 10 runs with no failure (it failed on the first run before), proxy+acl paired 0 of 20; cargo test --lib 2415, tool_bridge 17; clippy --workspace --all-targets -D warnings; fmt.
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
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 the flaky Windows proxy test, which turned out to be a real bug: on Windows, a cloned socket can leak into child processes.
Root cause
The sandbox proxy read each request through
client.try_clone(). On Windows that clone is created inheritable.GetHandleInformationconfirms it: the accepted socket hasinherit=0, and its clone hasinherit=1.Any child process started from the same process while the clone was open inherited the connection. That includes the sandboxed command and
icacls, becauseCommandwith piped stdio inherits handles. Closing our side then sent nothing, and the child's exit reset the connection, throwing away a403/502the client had already received (os error 10054).icacls. Bisecting showed the proxy tests fail 6 of 6 runs besidewindows::acl::tests, and never beside any other module.sbxstarts processes from the same process its proxy runs in. A refused or finished connection could be held open inside the command, or reset.The same pattern is fixed in incode's tool bridge. It read each request through
try_clone, while the toolbox may be starting shell commands, background jobs included.Change
proxy.rs:servereads and writes through&TcpStream, andpipecopies both ways on scoped threads that borrow the two sockets. No clone exists.tool_bridge.rs: the request is read through a borrow.Tested
a_refusal_reaches_the_client_while_processes_are_started_beside_itstarts child processes in a loop while 200 requests are refused, and every refusal must be read whole. With the clone put back, it loses 1–2 of 60 on every run.cargo test --lib2415 passed,tool_bridge17 passed.clippy --workspace --all-targets -D warningsandfmtare clean.