Isolate the stdio server's stdin and stdout from handler subprocesses#3117
Isolate the stdio server's stdin and stdout from handler subprocesses#3117maxisbey wants to merge 2 commits into
Conversation
maxisbey
force-pushed
the
stdio-stdin-isolation
branch
from
118422b to
1eec799
Compare
📚 Documentation preview
|
maxisbey
force-pushed
the
stdio-stdin-isolation
branch
from
1eec799 to
e1d6e64
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/migration.md">
<violation number="1" location="docs/migration.md:1877">
P2: Supplying only `stdout=` still redirects fd 0, so this migration guidance incorrectly promises that an explicit `stdout=` stream skips descriptor changes. Say that only explicit `stdin=` bypasses the stdin isolation.</violation>
</file>
<file name="src/mcp/server/stdio.py">
<violation number="1" location="src/mcp/server/stdio.py:91">
P2: Each completed default `stdio_server()` session leaks its duplicated protocol descriptor. `closefd=False` prevents wrapper cleanup from closing `private_fd`, and restoration duplicates it back onto fd 0 without closing the source. Repeated start/stop sessions can therefore exhaust the process descriptor limit; close the private duplicate after restoration (the task group has already finished the reader before this `finally` runs).</violation>
</file>
<file name="docs_src/troubleshooting/tutorial009.py">
<violation number="1" location="docs_src/troubleshooting/tutorial009.py:15">
P1: This example exposes arbitrary Python execution to every MCP client that can call `run_script`, because the client-controlled `path` is passed directly to the interpreter. The subprocess example can retain the stdin guidance without teaching an unrestricted code-runner; use a server-configured script path or validate against a narrowly scoped allowlist before spawning.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| """Run a Python script and return what it printed.""" | ||
| process = await asyncio.create_subprocess_exec( | ||
| sys.executable, | ||
| path, |
There was a problem hiding this comment.
P1: This example exposes arbitrary Python execution to every MCP client that can call run_script, because the client-controlled path is passed directly to the interpreter. The subprocess example can retain the stdin guidance without teaching an unrestricted code-runner; use a server-configured script path or validate against a narrowly scoped allowlist before spawning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs_src/troubleshooting/tutorial009.py, line 15:
<comment>This example exposes arbitrary Python execution to every MCP client that can call `run_script`, because the client-controlled `path` is passed directly to the interpreter. The subprocess example can retain the stdin guidance without teaching an unrestricted code-runner; use a server-configured script path or validate against a narrowly scoped allowlist before spawning.</comment>
<file context>
@@ -0,0 +1,23 @@
+ """Run a Python script and return what it printed."""
+ process = await asyncio.create_subprocess_exec(
+ sys.executable,
+ path,
+ stdin=subprocess.DEVNULL,
+ stdout=subprocess.PIPE,
</file context>
| transport (they never reliably did). Passing explicit `stdin=`/`stdout=` streams to | ||
| `stdio_server(...)` skips the descriptor changes entirely, as does any environment | ||
| where `sys.stdin` is not backed by the process's real fd 0. |
There was a problem hiding this comment.
P2: Supplying only stdout= still redirects fd 0, so this migration guidance incorrectly promises that an explicit stdout= stream skips descriptor changes. Say that only explicit stdin= bypasses the stdin isolation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/migration.md, line 1877:
<comment>Supplying only `stdout=` still redirects fd 0, so this migration guidance incorrectly promises that an explicit `stdout=` stream skips descriptor changes. Say that only explicit `stdin=` bypasses the stdin isolation.</comment>
<file context>
@@ -1855,6 +1855,29 @@ group (spawned with `start_new_session=True`); the `getpgid()` lookup and the
+during a stdio session — it now sees end-of-file instead of racing the transport for
+protocol bytes; there was never a meaningful value to read there. Likewise, bytes
+something buffered out of `sys.stdin` before the server started no longer reach the
+transport (they never reliably did). Passing explicit `stdin=`/`stdout=` streams to
+`stdio_server(...)` skips the descriptor changes entirely, as does any environment
+where `sys.stdin` is not backed by the process's real fd 0.
</file context>
| transport (they never reliably did). Passing explicit `stdin=`/`stdout=` streams to | |
| `stdio_server(...)` skips the descriptor changes entirely, as does any environment | |
| where `sys.stdin` is not backed by the process's real fd 0. | |
| transport (they never reliably did). Passing an explicit `stdin=` stream to | |
| `stdio_server(...)` skips the descriptor changes entirely; an explicit `stdout=` | |
| stream alone does not. Environments where `sys.stdin` is not backed by the | |
| process's real fd 0 are likewise unaffected. |
| if private_fd is not None: | ||
| # A completed dup2 is undone; an untouched fd 0 is re-pointed at | ||
| # the same pipe it already holds, which is harmless. | ||
| _restore_fd(0, private_fd) |
There was a problem hiding this comment.
P2: Each completed default stdio_server() session leaks its duplicated protocol descriptor. closefd=False prevents wrapper cleanup from closing private_fd, and restoration duplicates it back onto fd 0 without closing the source. Repeated start/stop sessions can therefore exhaust the process descriptor limit; close the private duplicate after restoration (the task group has already finished the reader before this finally runs).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/stdio.py, line 91:
<comment>Each completed default `stdio_server()` session leaks its duplicated protocol descriptor. `closefd=False` prevents wrapper cleanup from closing `private_fd`, and restoration duplicates it back onto fd 0 without closing the source. Repeated start/stop sessions can therefore exhaust the process descriptor limit; close the private duplicate after restoration (the task group has already finished the reader before this `finally` runs).</comment>
<file context>
@@ -17,61 +17,159 @@ async def run_server():
+ if private_fd is not None:
+ # A completed dup2 is undone; an untouched fd 0 is re-pointed at
+ # the same pipe it already holds, which is harmless.
+ _restore_fd(0, private_fd)
+ os.close(private_fd)
+ return sys.stdin.buffer, None
</file context>
| _restore_fd(0, private_fd) | |
| _restore_fd(0, private_fd) | |
| with suppress(OSError): | |
| os.close(private_fd) |
maxisbey
force-pushed
the
stdio-stdin-isolation
branch
from
e1d6e64 to
bf24e5e
Compare
While serving on the process's real stdin, stdio_server() now reads the protocol from a private duplicate of fd 0 and points fd 0 (and, on Windows, the standard input handle) at the null device, restoring both on exit. Children spawned by handler code then inherit the null device instead of the protocol pipe. A child that inherited the pipe could consume protocol bytes on any platform, and on Windows a Python child hangs inside interpreter startup behind the transport's pending read (CPython gh-78961) until the next request arrives, so any tool that ran a subprocess without stdin=DEVNULL appeared to hang until timeout. Isolation engages only when sys.stdin is backed by the real fd 0, at most once per process, and degrades to reading stdin in place when the descriptor table cannot be rearranged. Fixes #671.
maxisbey
force-pushed
the
stdio-stdin-isolation
branch
from
bf24e5e to
42ef19a
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the nested-transport finding already flagged inline, this pass also examined and ruled out two candidates. The retained private_fd after restore() (the descriptor cubic's suggestion proposes closing) is deliberate, not a leak: an abandoned reader thread can still be blocked in read() on that descriptor past the transport's lifetime — the closefd=False comment documents this — so closing it would free the fd number for reuse under a live read; the cost is one retained fd per session, and a process typically runs one. Also checked the migration-guide claim that explicit streams skip the descriptor changes: the claim path is governed solely by the stdin= parameter, so the sentence holds for the both-streams injected usage it describes (an explicit stdout= alone does not skip it — a wording nit at most).
Extended reasoning...
This run's bug hunt surfaced no new findings; the prior inline finding on the nested-transport path stands unaddressed and no commits have landed since. The note above records what was additionally examined and refuted this pass — chiefly the private-fd retention, which two automated reviewers reached opposite conclusions on: closing it after restore would reintroduce the fd-recycling hazard the code's closefd=False comment guards against, since anyio's blocking file reads are abandoned (not cancelled) in worker threads and can outlive the transport. Recording this so the author and any later review pass don't re-litigate it or apply the suggested close.
While serving on the process's real stdout, stdio_server now moves the protocol pipe to a private descriptor and points fd 1 - and, on Windows, the standard output handle - at stderr, restoring it when the transport exits. A stray print() in handler code or a child process writing to its inherited stdout lands in the client's log instead of corrupting the JSON-RPC stream. The null device stands in when stderr is unusable or, on POSIX, is detected as merged into stdout (2>&1). The stdin claim generalizes into the shared _claim_fd mechanism: one lock-guarded sentinel table covers both descriptors, private wire duplicates are forced above the standard descriptor range so a process started with a standard descriptor closed cannot hand the wire out as its "stderr", and a failed claim degrades to serving the sys stream's buffer in place exactly as v1 did. Docs now describe the guarded behavior with its remaining gaps (output flushed before serving begins, injected streams, merged stderr on Windows), and the transport:stdio:stream-purity divergence narrows accordingly.
maxisbey
changed the title
There was a problem hiding this comment.
4 issues found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/handlers/logging.md">
<violation number="1" location="docs/handlers/logging.md:37">
P2: Windows processes launched with stderr merged into stdout can still send `print()` output to the protocol pipe, so this guarantee is misleading. Qualify the claim with the merged-stream caveat (or direct readers to the migration caveats).</violation>
</file>
<file name="docs/run/index.md">
<violation number="1" location="docs/run/index.md:42">
P2: Hosts that merge stderr into stdout can still receive diverted `print()` and child output on the protocol pipe. Qualify the guarantee here (and point readers to the merge caveat) so this does not promise isolation in that supported launch configuration.</violation>
</file>
<file name="src/mcp/server/stdio.py">
<violation number="1" location="src/mcp/server/stdio.py:125">
P1: A concurrently-entered transport can still access the protocol pipe during the claim window. `_claimed` is published before `dup2()` redirects the descriptor, so another thread takes the `already_claimed` path and uses fd 0/1 while it still refers to the wire; it can consume input or emit output into the outer protocol stream. Keep concurrent entrants from opening the descriptor until the diversion has been installed (and coordinate the claim lifetime as needed).</violation>
</file>
<file name="docs/migration.md">
<violation number="1" location="docs/migration.md:1862">
P2: Windows processes launched with stderr merged into stdout can still send stray handler or child output onto the protocol pipe, but this migration note says fd 1 is always diverted away from the wire and that capturing child stdout is no longer needed. Please document this Windows merge exception here (and retain the capture workaround for that launch shape), so users do not rely on isolation that the implementation deliberately cannot provide.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| with _claimed_lock: | ||
| already_claimed = fd in _claimed | ||
| if not already_claimed: | ||
| _claimed.add(fd) |
There was a problem hiding this comment.
P1: A concurrently-entered transport can still access the protocol pipe during the claim window. _claimed is published before dup2() redirects the descriptor, so another thread takes the already_claimed path and uses fd 0/1 while it still refers to the wire; it can consume input or emit output into the outer protocol stream. Keep concurrent entrants from opening the descriptor until the diversion has been installed (and coordinate the claim lifetime as needed).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/stdio.py, line 125:
<comment>A concurrently-entered transport can still access the protocol pipe during the claim window. `_claimed` is published before `dup2()` redirects the descriptor, so another thread takes the `already_claimed` path and uses fd 0/1 while it still refers to the wire; it can consume input or emit output into the outer protocol stream. Keep concurrent entrants from opening the descriptor until the diversion has been installed (and coordinate the claim lifetime as needed).</comment>
<file context>
@@ -49,58 +53,122 @@ def _is_backed_by_fd(stream: TextIO, fd: int) -> bool:
+ with _claimed_lock:
+ already_claimed = fd in _claimed
+ if not already_claimed:
+ _claimed.add(fd)
+ if already_claimed:
+ # An enclosing transport owns the claim. Serve wherever fd currently
</file context>
| Never `print()` in a stdio server. `print` writes to **stdout**, and stdout *is* the wire: one stray | ||
| line and the client is trying to parse it as JSON-RPC. | ||
| Don't `print()` in a stdio server. `print` writes to **stdout**, and stdout belongs to the protocol: | ||
| while serving, the SDK diverts stray stdout to stderr so it can't corrupt the wire, but that leaves |
There was a problem hiding this comment.
P2: Windows processes launched with stderr merged into stdout can still send print() output to the protocol pipe, so this guarantee is misleading. Qualify the claim with the merged-stream caveat (or direct readers to the migration caveats).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/handlers/logging.md, line 37:
<comment>Windows processes launched with stderr merged into stdout can still send `print()` output to the protocol pipe, so this guarantee is misleading. Qualify the claim with the merged-stream caveat (or direct readers to the migration caveats).</comment>
<file context>
@@ -33,8 +33,9 @@ For a **stdio** server this question matters more than usual. The host launched
- Never `print()` in a stdio server. `print` writes to **stdout**, and stdout *is* the wire: one stray
- line and the client is trying to parse it as JSON-RPC.
+ Don't `print()` in a stdio server. `print` writes to **stdout**, and stdout belongs to the protocol:
+ while serving, the SDK diverts stray stdout to stderr so it can't corrupt the wire, but that leaves
+ your line interleaved raw among the log output -- no level, no logger name, no way to filter it.
</file context>
| ``` | ||
|
|
||
| The matching troubleshooting entry is **[My stdio tool hangs when it starts a subprocess on Windows](../troubleshooting.md#my-stdio-tool-hangs-when-it-starts-a-subprocess-on-windows)**. | ||
| That also means stdout **is the wire**. While serving, the SDK moves the wire to a private descriptor and diverts stray output -- a `print()`, a subprocess writing to its inherited stdout -- to stderr, where it can't corrupt the stream. Output that reaches stdout *before* serving begins (a wrapper script echoing, an unbuffered import-time print) still lands on the wire. For output you actually want, the `logging` module is the right tool. That story is in **[Logging](../handlers/logging.md)**. |
There was a problem hiding this comment.
P2: Hosts that merge stderr into stdout can still receive diverted print() and child output on the protocol pipe. Qualify the guarantee here (and point readers to the merge caveat) so this does not promise isolation in that supported launch configuration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/run/index.md, line 42:
<comment>Hosts that merge stderr into stdout can still receive diverted `print()` and child output on the protocol pipe. Qualify the guarantee here (and point readers to the merge caveat) so this does not promise isolation in that supported launch configuration.</comment>
<file context>
@@ -39,7 +39,7 @@ python server.py
Nothing prints, and it doesn't return. It is waiting on stdin for a host to speak first.
-That also means stdout **is the wire**. A stray `print()` corrupts the stream; the `logging` module writes to stderr and is the right tool. That story is in **[Logging](../handlers/logging.md)**.
+That also means stdout **is the wire**. While serving, the SDK moves the wire to a private descriptor and diverts stray output -- a `print()`, a subprocess writing to its inherited stdout -- to stderr, where it can't corrupt the stream. Output that reaches stdout *before* serving begins (a wrapper script echoing, an unbuffered import-time print) still lands on the wire. For output you actually want, the `logging` module is the right tool. That story is in **[Logging](../handlers/logging.md)**.
### Try it
</file context>
| That also means stdout **is the wire**. While serving, the SDK moves the wire to a private descriptor and diverts stray output -- a `print()`, a subprocess writing to its inherited stdout -- to stderr, where it can't corrupt the stream. Output that reaches stdout *before* serving begins (a wrapper script echoing, an unbuffered import-time print) still lands on the wire. For output you actually want, the `logging` module is the right tool. That story is in **[Logging](../handlers/logging.md)**. | |
| That also means stdout **is the wire**. While serving, the SDK moves the wire to a private descriptor and normally diverts stray output -- a `print()`, a subprocess writing to its inherited stdout -- to stderr, where it can't corrupt the stream. Hosts that merge stderr into stdout can still expose that output on the wire. Output that reaches stdout *before* serving begins (a wrapper script echoing, an unbuffered import-time print) still lands on the wire. For output you actually want, the `logging` module is the right tool. That story is in **[Logging](../handlers/logging.md)**. |
|
|
||
| While serving on the process's real stdin and stdout, the stdio server transport now | ||
| duplicates each protocol pipe to a private descriptor and points the standard | ||
| descriptors — with their Windows standard handles — away from the wire, restoring |
There was a problem hiding this comment.
P2: Windows processes launched with stderr merged into stdout can still send stray handler or child output onto the protocol pipe, but this migration note says fd 1 is always diverted away from the wire and that capturing child stdout is no longer needed. Please document this Windows merge exception here (and retain the capture workaround for that launch shape), so users do not rely on isolation that the implementation deliberately cannot provide.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/migration.md, line 1862:
<comment>Windows processes launched with stderr merged into stdout can still send stray handler or child output onto the protocol pipe, but this migration note says fd 1 is always diverted away from the wire and that capturing child stdout is no longer needed. Please document this Windows merge exception here (and retain the capture workaround for that launch shape), so users do not rely on isolation that the implementation deliberately cannot provide.</comment>
<file context>
@@ -1855,31 +1855,40 @@ group (spawned with `start_new_session=True`); the `getpgid()` lookup and the
+
+While serving on the process's real stdin and stdout, the stdio server transport now
+duplicates each protocol pipe to a private descriptor and points the standard
+descriptors — with their Windows standard handles — away from the wire, restoring
+both when the transport exits: fd 0 reads the null device, and fd 1 writes to stderr
+(the null device if stderr is unusable). Subprocesses started by handler code
</file context>
There was a problem hiding this comment.
Beyond the inline findings, two candidates were examined and ruled out this run: (1) the per-session private-descriptor "leak" (also raised in an earlier bot review) — restore() deliberately leaves the private fd open because a worker thread can still sit blocked on it past the transport's lifetime, a one-fd-per-stream-per-session tradeoff documented in the closefd=False comment, not an accidental leak; (2) _claim_fd crashing on injected sys streams lacking a .buffer attribute — _is_backed_by_fd catches the AttributeError for the probe, and the subsequent stream.buffer access matches v1's unconditional sys.stdin.buffer access, so no regression.
Extended reasoning...
This run's finders raised the descriptor-leak and missing-.buffer candidates and verifiers refuted both; I confirmed the refutations against the diff (the leak is a deliberate, code-documented design choice tied to blocked worker-thread reads, and the .buffer access shape is identical to v1). Recording them here so the author and later review passes don't re-explore them from scratch — the descriptor-leak in particular was independently raised by another bot review on this PR. This is informational only, not a correctness guarantee; the two inline nits from this run stand on their own.
| def _open_stdout_diversion() -> int: | ||
| """What fd 1 receives while claimed: stderr, where stray output is at least | ||
| visible in the client's logs, or the null device when stderr is unusable | ||
| or is itself the wire (stderr merged into stdout, the 2>&1 launch shape - | ||
| detectable on POSIX; on Windows such a merge keeps its v1 behavior).""" | ||
| if not same_open_file(2, 1): | ||
| try: | ||
| return os.dup(2) | ||
| except OSError: | ||
| pass | ||
| return os.open(os.devnull, os.O_WRONLY) |
There was a problem hiding this comment.
🟡 The merged-stderr detection in _open_stdout_diversion misfires on every interactive terminal launch: a shell hands the child fd 1 and fd 2 backed by the same tty, so same_open_file(2, 1) is true for a plain python server.py run — not just the 2>&1 shape it targets — and stray stdout (a handler's print(), a child's inherited stdout) is silently discarded to the null device instead of appearing on the terminal via stderr. This contradicts the unconditional 'diverted to stderr' promise the PR's docs make and regresses v1 visibility in exactly the 'Try it' debugging flow; an os.isatty(1) exemption would separate the tty case (never a parsed JSON-RPC wire) from the genuine 2>&1-into-a-pipe merge.
Extended reasoning...
The bug
_open_stdout_diversion (src/mcp/server/stdio.py:86-96) decides where fd 1 points while the transport serves: normally a duplicate of stderr, but the null device when same_open_file(2, 1) reports that stderr is the wire (the 2>&1 launch shape). The check is os.path.sameopenfile(2, 1), which compares st_dev/st_ino — and that condition is true not only for a 2>&1 merge but for every process launched from an interactive shell: the shell hands the child fd 1 and fd 2 dup'd from the same tty open. Verified empirically on this branch: os.path.sameopenfile returns True for tty-backed fd pairs both when dup'd from one open (what a shell does) and even for two independent opens of the same tty device, since the stat identity is the device node's.
The code path
- A developer runs
python server.py(oruv run mcp run server.py) in a terminal — exactly the flow docs/run/index.md's "Try it" section walks through. - The shell gives the process fd 1 and fd 2 both pointing at the tty.
sys.stdoutis backed by real fd 1, sostdio_server()enters_claim_fd(1, ...)and calls_open_stdout_diversion(). same_open_file(2, 1)→os.path.sameopenfile(2, 1)→True(same tty device node), so the function skipsos.dup(2)and returnsos.open(os.devnull, os.O_WRONLY).- fd 1 is dup2'd onto the null device. From now on a handler's stray
print()and any tool-spawned child's stdout vanish without a trace, instead of appearing on the terminal (via the stderr diversion) as the docs promise.
Why nothing catches it
The tests force the merge with a monkeypatched same_open_file or a genuine 2>&1-style pipe plant; no test runs under a pty, so the tty false positive is never exercised. And it is not among the residual gaps the PR deliberately enumerates (pre-serving flushes, injected streams, Windows merge undetectability) — it is an unconsidered false positive of the new heuristic.
Impact
The behavior contradicts three unconditional statements this PR adds: docs/handlers/logging.md ("while serving, the SDK diverts stray stdout to stderr ... that leaves your line interleaved raw among the log output"), docs/run/index.md ("diverts stray output ... to stderr, where it can't corrupt the stream"), and the module's own rationale that the diversion target is stderr "so stray output stays visible." It is also a small visibility regression vs v1 for the interactive debugging flow: in v1 a stray print() during a manual terminal run simply appeared on the terminal; now it disappears, which will read as "my print vanished entirely" during the one workflow where developers are most likely to test it. In the tty case writing to "stderr" (the same tty) would be harmless — no parser reads a terminal, a human does — so the null-device fallback there discards output with zero protective benefit.
Severity is a nit, not blocking: real hosts spawn the server with distinct stdout/stderr pipes, so sameopenfile is False and the stderr diversion works exactly as documented; the wire stays pure in the tty case too (frames flow over the private descriptor). The only concrete harm is lost debug output during manual interactive runs plus docs that over-promise.
Fix
Exempt the tty case before the merge check: a tty is never a parsed JSON-RPC wire, so writing the diversion there is always safe. E.g. in _open_stdout_diversion:
if _isatty(1) or not same_open_file(2, 1):
try:
return os.dup(2)
except OSError:
pass
return os.open(os.devnull, os.O_WRONLY)(with os.isatty wrapped for OSError, mirroring same_open_file's own defensive shape). That keeps the null-device fallback for the genuine 2>&1-into-a-pipe merge while restoring visibility on terminal launches, and lets the docs' unconditional promise hold for the interactive case too.
| # the same pipe it already holds, which is harmless. | ||
| _restore_fd(fd, private_fd) | ||
| os.close(private_fd) | ||
| # fd still holds the protocol pipe, so the sys stream's buffer is | ||
| # target-consistent: serve it in place, exactly as v1 did - shared | ||
| # write ordering, no new descriptors to allocate in a process whose | ||
| # descriptor table is already failing. | ||
| return stream.buffer, None | ||
|
|
||
| def restore() -> None: | ||
| # Flush first: text buffered in the sys stream during the claim (a | ||
| # stray print() while stdout is claimed) drains to the diversion, not | ||
| # to the restored protocol pipe. | ||
| with suppress(OSError, ValueError): |
There was a problem hiding this comment.
🟡 When _claim_fd's descriptor rearrangement fails with OSError, the fallback path discards fd from _claimed even though the transport goes on serving the real fd 0/fd 1 in place for its whole lifetime — so a nested or concurrent stdio_server() entered afterward finds the fd unclaimed, claims it, and moves the wire out from under the outer transport (its reader sees EOF and the client's frames are delivered to the inner transport instead). Keeping fd in _claimed on the fallback path, with an undo that only discards the sentinel, makes a nested transport take the serve-in-place branch as the design intends.
Extended reasoning...
What the bug is
The _claimed sentinel table exists — per the module comment at src/mcp/server/stdio.py:36-41 — so that "a second concurrent stdio_server() must not claim an fd again ... and its restore would clobber the first transport's." The best-effort fallback breaks that invariant: in _claim_fd's except OSError handler (src/mcp/server/stdio.py:147-160), _claimed.discard(fd) runs and stream.buffer — the real fd 0/fd 1, still holding the wire — is returned with restore=None. The transport then serves the standard descriptor in place for its whole lifetime with no sentinel protecting the descriptor it is actively serving. The _claimed check at lines 121-127 is the only guard in the module against a second transport re-claiming; once the sentinel is discarded, nothing prevents it.
Step-by-step proof (reproduced experimentally against the real _claim_fd on this branch)
- Transport A enters
stdio_server(). Its claim of fd 0 fails transiently (e.g. an injectedOSErroron the firstos.dup(0)— descriptor exhaustion that later clears, exactly the environment the fallback exists for). A takes the fallback: it servessys.stdin.buffer(fd 0 = the wire) in place, and_claimedis empty. - Handler or embedder code enters a nested/concurrent
stdio_server()— the exact shape the sentinel is documented and tested to support (test_a_nested_stdio_server_does_not_clobber_the_first_transports_claim). The environment has recovered, so B's_claim_fd(0, ...)finds_is_backed_by_fdtrue and fd 0 unclaimed, and succeeds: the wire moves to B's private descriptor and fd 0 becomes the null device (verified viasameopenfileagainst a devnull probe). - Consequences, all observed: (a) A's in-place reader on fd 0 immediately sees EOF — the outer
stdin_readerends and the outer session dies; (b) a JSON-RPC frame written to the wire for A's session was read by B's private descriptor while A gotb''— B literally steals A's inbound frames; (c) the stdout twin is symmetric: a fallback stdout transport writes fd 1 in place, and a nested claim of fd 1 diverts fd 1 to stderr, so the outer transport's responses land on stderr instead of the wire for the nested transport's lifetime.
Why existing safeguards miss it
The nested-transport tests only cover the claim-succeeded outer shape: the outer transport holds a sentinel, so the inner one serves in place. No test enters a nested transport after a fallback, so the hole is untested. And this is not quite the v1-parity the fallback comment claims: v1 never repointed fd 0 at devnull, so a v1 nested transport raced the outer one for bytes rather than deterministically killing the outer session, moving fd 0 to the null device, and diverting every one of its inbound frames.
Addressing the refutation
One verifier argued this is a non-actionable known limitation: the trigger is a compound rarity, fallback mode is documented v1-parity where nesting was never safe, and the fix would merely have the inner transport open(fd, ...) on the actual wire, racing the outer one — v1's brokenness in another form. The rarity point is fair and is why this is a nit, not a blocker. But the other two points don't hold up: (1) the current behavior is strictly worse than v1 — a race for bytes can still deliver some frames to the right session, whereas the re-claim deterministically EOFs the outer reader and diverts all of its traffic; and (2) the fix does restore the module's stated contract — with the sentinel kept, the inner transport takes the serve-in-place branch (lines 121-127), which is precisely the degraded-but-documented shape the design already prescribes for nesting, and the outer transport's serving arrangement survives, which is the whole invariant the comment at lines 36-41 promises. Nesting on a shared wire is inherently degraded either way; the question is whether the sentinel's documented guarantee holds in the fallback shape, and today it doesn't.
The fix
On the fallback path, keep fd in _claimed for the transport's lifetime, returning a restore callback that only discards the sentinel (no descriptor work — there is nothing to restore). A nested transport then finds the fd claimed and serves in place, exactly as the design intends. The change is a few lines and slots naturally next to the existing restore() closure.
Severity
Nit: the trigger requires a transient OSError during descriptor rearrangement and a nested/concurrent stdio_server() in the same process and a descriptor table that recovers in between — no realistic deployment hits this. But nested/threaded transports are an explicitly supported scenario per this PR's own sentinel design and lock comment, the hole silently kills and hijacks the outer session when it does hit, and the fix is cheap.
1 participant
stdio_server()now serves the protocol from private duplicates of stdin and stdout and repoints the standard descriptors away from the wire while it runs, restoring both on exit: fd 0 (and the Windows standard input handle) reads the null device, and fd 1 (and the standard output handle) writes to stderr — the null device when stderr is unusable or, on POSIX, detected as merged into stdout. Subprocesses spawned by tool code therefore inherit the diversions instead of the protocol pipes, and a strayprint()lands in the client's log instead of corrupting the stream.Fixes #671.
Motivation and Context
stdin. The #671 hang is CPython gh-78961: Windows serializes operations on a synchronous pipe, a stdio server always has a blocking read pending on stdin, and a Python child that inherits that pipe freezes inside interpreter startup — before its first line of user code — until the pending read completes. That happens exactly when the next JSON-RPC message arrives, which is why hung tool calls "complete" once the client times out or sends another request. Skipping redirection entirely doesn't avoid it: Windows hands a console child the parent's standard handles even with
bInheritHandles=FALSE, so a plainsubprocess.run([...])hangs too. With fd 0 on the null device while serving, there is no shared pipe to block on. This is the default Go'sos/execships (nil stdin → the null device), and the reason this bug class doesn't exist in the TypeScript or Go SDKs. It also fixes the cross-platform variant of the same leak: a child that reads its inherited stdin was consuming protocol bytes on any platform.stdout. The same inheritance corrupts the other direction: a child that writes to its inherited stdout — or a stray
print()in handler code — writes straight into the JSON-RPC stream (the classic client-side symptom isUnexpected token ... is not valid JSON, e.g. #409). With fd 1 diverted to stderr while serving, that output shows up in the client's logs (every SDK's stdio client surfaces server stderr: this SDK'serrlog, TypeScript'sstderr: 'inherit'default, C#'s rolling stderr tail) instead of on the wire. This closes the documentedtransport:stdio:stream-puritydivergence, and is the direction go-sdk has queued for its v2 (modelcontextprotocol/go-sdk#572, where redirecting stdout to stderr after claiming the real fd is the maintainer-endorsed shape). Diverting to stderr rather than the null device keeps the output a human can see; the null device is the fallback when stderr can't be used.Both claims share one mechanism (
_claim_fd): a sentinel table (lock-guarded) so a nested or concurrent transport serves in place instead of clobbering the first claim, private duplicates forced above the standard descriptor range (in a process started with a standard descriptor closed,os.dupwould otherwise hand the wire duplicate out as the process's "stderr" and the diversion would point back at the wire), and best-effort degradation: anyOSErrorwhile re-pointing descriptors falls back to serving the sys stream's buffer in place, exactly as v1 did.With the collision gone, the workaround documentation #3079 added is removed — the docs describe v2, and v2 no longer exhibits the hang or the corruption (
stdin=subprocess.DEVNULLand captured child stdout still work fine where users already pass them). The behavior change and the v1 history are recorded indocs/migration.md, and the remaining honest gaps (output flushed to stdout before serving begins; explicitly injected streams; stderr-merged-into-stdout on Windows, where pipe handles carry no identity to compare) are documented where they belong.How Has This Been Tested?
windows-latestrunners against currentmainwith an instrumented harness (wall-clock-correlated events, timers around_winapi.DuplicateHandle/CreateProcess): spawn calls return in ~2 ms and never block; the child's first line executes only when the next protocol message lands, on Python 3.10 through 3.14. A real MCP session over stdio pipes shows the hang and that apingreleases it. With this change, the same unmodified repro returns in ~0.5 s.main(POSIX and Windows runners) and pass with this change.print(), post-pollution calls — all clean, all noise in the host-visible stderr), a Go SDK client doing the same, and this SDK's client against a TypeScript server. On unpatched code the same TS run shows the junk reaching the wire (TS clients survive only because their read buffer skips whole non-JSON lines — a workaround for exactly this bug class, typescript-sdk#700).python server.pysessions with a pre-run banner print (drains to stderr, wire clean), post-run prints (stdout restored, no Using transport="stdio" closes real stdio, causing ValueError after server exits #1933-styleValueError),python -u(documented pre-serving gap),2>&-and2>&1launches.Breaking Changes
None for typical servers. During a stdio session, handler code that read
sys.stdin(or calledinput()) now sees EOF instead of racing the transport for protocol bytes, andprint()/sys.stdoutwrites reach stderr instead of the wire — code that deliberately wrote protocol frames tosys.stdoutmust use the transport's write stream. A child that streams heavily to its inherited stdout now streams into the client's stderr channel. Documented indocs/migration.md. Injected-stream usage (stdio_server(stdin=..., stdout=...)) and environments where the sys streams aren't the real descriptors are unaffected.Types of changes
Checklist
Additional context
Isolation is best-effort and precisely scoped: it engages only when the sys stream is backed by its real descriptor, at most once per process per stream, and any failure to rearrange descriptors degrades to serving in place exactly as before. Known residuals, deliberately accepted at v1-parity: output flushed to stdout before the transport enters still precedes the first frame; a process launched with stderr merged into stdout keeps its (already broken in v1) merged behavior on Windows, where the merge is undetectable — on POSIX it is detected and the diversion falls back to the null device.
AI disclosure
AI assistance was used to investigate, implement, and validate this change; I reviewed the result and take responsibility for it.
AI Disclaimer