From 42ef19a5936c6ef97c8b6bccc5882a9d85219331 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Thu, 16 Jul 2026 18:20:02 +0000 Subject: [PATCH 1/2] Isolate the stdio server's stdin from handler subprocesses 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. --- docs/migration.md | 26 +++ docs/run/index.md | 24 --- docs/troubleshooting.md | 35 ---- src/mcp/os/win32/utilities.py | 27 ++- src/mcp/server/stdio.py | 170 ++++++++++++++---- tests/server/test_stdio.py | 240 ++++++++++++++++++++++++- tests/transports/stdio/test_windows.py | 56 +++++- 7 files changed, 479 insertions(+), 99 deletions(-) diff --git a/docs/migration.md b/docs/migration.md index 876608db70..46c6f6c32a 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -1855,6 +1855,32 @@ group (spawned with `start_new_session=True`); the `getpgid()` lookup and the per-process terminate/kill fallback are gone. The win32 utilities logger is now named `mcp.os.win32.utilities` (was `client.stdio.win32`). +### `stdio_server` keeps the protocol stdin on a private descriptor + +While serving on the process's real stdin, the stdio server transport now duplicates +the protocol pipe to a private descriptor and points fd 0 — and, on Windows, the +standard input handle — at the null device, restoring both when the transport exits. +Subprocesses started by handler code therefore inherit the null device instead of the +protocol pipe. (The claim is best-effort: in the rare process whose descriptor table +cannot be rearranged, the transport serves stdin in place, exactly as v1 did.) + +In v1 a child inheriting the pipe could consume protocol bytes, and on Windows it +hung inside interpreter startup — behind the transport's pending read on the shared +pipe ([CPython gh-78961](https://github.com/python/cpython/issues/78961)) — until +the next request arrived: the +[#671](https://github.com/modelcontextprotocol/python-sdk/issues/671) hang, for +which passing `stdin=subprocess.DEVNULL` on every spawn was the required +workaround. On v2 the workaround is no longer needed, for any spawn API, +redirected or not. + +To migrate: nothing, unless handler code read `sys.stdin` (or called `input()`) +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. + ### WebSocket transport removed The WebSocket transport has been removed: `mcp.client.websocket.websocket_client`, `mcp.server.websocket.websocket_server`, and the `ws` optional dependency extra (`mcp[ws]`) no longer exist. WebSocket was never part of the MCP specification. Use the streamable HTTP transport instead (`mcp.client.streamable_http.streamable_http_client` on the client, `streamable_http_app()` on the server), which supports bidirectional communication with server-to-client streaming over standard HTTP. diff --git a/docs/run/index.md b/docs/run/index.md index f92090fee4..4ff8f53deb 100644 --- a/docs/run/index.md +++ b/docs/run/index.md @@ -41,30 +41,6 @@ Nothing prints, and it doesn't return. It is waiting on stdin for a host to spea 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)**. -On Windows, the same rule applies to child processes your tools start. A child -that inherits the stdio server's stdin can block behind the server's protocol -reader. If your tool starts a subprocess and you do not intend to feed it input, -redirect the child's stdin: - -```python -import asyncio -import subprocess -import sys - - -async def run_script() -> tuple[bytes, bytes]: - process = await asyncio.create_subprocess_exec( - sys.executable, - "script.py", - stdin=subprocess.DEVNULL, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - ) - return await process.communicate() -``` - -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)**. - ### Try it ```console diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 1279448359..621b32c6cd 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -141,41 +141,6 @@ There is no error string for this, which is exactly why it is hard to search. Th An "invalid" tool name is *not* on that list: a non-conforming name logs a warning but the tool is registered and listed anyway. -## My stdio tool hangs when it starts a subprocess on Windows - -Your server is running over `stdio`, and a tool starts another process with -`asyncio.create_subprocess_exec`, `asyncio.create_subprocess_shell`, or -`subprocess.Popen`. The tool call never returns on Windows, while the same code -works over an HTTP transport. - -The child inherited the server's stdin. In a stdio server, stdin is the protocol -pipe and the server is already waiting on it for the next JSON-RPC message. A -Python child process on Windows can block during startup when it inherits that -same pipe. - -If you do not intend to send input to the child, redirect its stdin: - -```python -import asyncio -import subprocess -import sys - - -async def run_script() -> tuple[bytes, bytes]: - process = await asyncio.create_subprocess_exec( - sys.executable, - "script.py", - stdin=subprocess.DEVNULL, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - ) - return await process.communicate() -``` - -Use the same idea with `subprocess.Popen(..., stdin=subprocess.DEVNULL)`. Also -capture or redirect the child's stdout. The stdio server's stdout is the MCP -wire, so a child that writes there can corrupt the connection. - ## `MCPError: Server returned an error response` The server refused the HTTP request outright, with a body that is not JSON-RPC, so the python `Client` has nothing better to show you than this stand-in. diff --git a/src/mcp/os/win32/utilities.py b/src/mcp/os/win32/utilities.py index 1cc867d4fa..39df0de050 100644 --- a/src/mcp/os/win32/utilities.py +++ b/src/mcp/os/win32/utilities.py @@ -1,4 +1,4 @@ -"""Windows-specific functionality for stdio client operations.""" +"""Windows-specific functionality for stdio transport operations.""" import logging import shutil @@ -20,14 +20,39 @@ import pywintypes import win32api import win32con + import win32file import win32job else: # Type stubs for non-Windows platforms win32api = None win32con = None + win32file = None win32job = None pywintypes = None + +def rebind_std_handle_to_fd(fd: int) -> None: + """Points the Win32 standard-handle slot for fd 0, 1, or 2 at fd's current OS handle. + + os.dup2 updates only the C runtime's descriptor table; anything that resolves + GetStdHandle — subprocess standard-handle inheritance, native code — reads the + Win32 slot, so after retargeting a standard descriptor the slot must be + repointed too. + + Raises: + OSError: The slot could not be set; callers treat descriptor + rearrangement as best-effort and fall back on failure. + """ + if sys.platform != "win32" or not win32api or not win32file or not pywintypes: + return + std_ids = {0: win32api.STD_INPUT_HANDLE, 1: win32api.STD_OUTPUT_HANDLE, 2: win32api.STD_ERROR_HANDLE} + try: + win32api.SetStdHandle(std_ids[fd], win32file._get_osfhandle(fd)) + except pywintypes.error as exc: + # Normalized so callers' OSError-based best-effort handling covers it. + raise OSError(f"SetStdHandle failed for fd {fd}") from exc + + # How often FallbackProcess polls the underlying Popen for exit. _EXIT_POLL_INTERVAL = 0.01 diff --git a/src/mcp/server/stdio.py b/src/mcp/server/stdio.py index 876d256ddb..4fe730b8d5 100644 --- a/src/mcp/server/stdio.py +++ b/src/mcp/server/stdio.py @@ -17,61 +17,159 @@ async def run_server(): ``` """ +import os import sys -from contextlib import asynccontextmanager +from collections.abc import Callable +from contextlib import asynccontextmanager, suppress from io import TextIOWrapper +from typing import BinaryIO, TextIO import anyio import anyio.lowlevel import mcp_types as types +from mcp.os.win32.utilities import rebind_std_handle_to_fd from mcp.shared._context_streams import create_context_streams from mcp.shared.message import SessionMessage +# True while a transport in this process has fd 0 pointed at the null device. +# A second concurrent stdio_server() must not claim again: it would duplicate +# the null device instead of the protocol pipe, and its restore would clobber +# the first transport's. +_stdin_claimed = False + + +def _is_backed_by_fd(stream: TextIO, fd: int) -> bool: + """Whether stream is a text wrapper over the process's real descriptor fd.""" + try: + return stream.buffer.fileno() == fd + except (AttributeError, OSError, ValueError): + # In-memory or injected streams (tests, embedders) have no usable + # descriptor; io.UnsupportedOperation is a subclass of OSError/ValueError. + return False + + +def _claim_stdin() -> tuple[BinaryIO, Callable[[], None] | None]: + """Returns the binary stream the transport reads the protocol from, and its undo. + + When running on the process's real stdin, moves the protocol pipe to a + private descriptor and points fd 0 (and, on Windows, the standard input + handle) at the null device for the transport's lifetime. Child processes + spawned by handlers then inherit the null device instead of the protocol + pipe: a child holding the protocol pipe could consume protocol bytes, and + on Windows a Python child hangs during interpreter startup while the + transport's blocking read is pending on the shared pipe + (https://github.com/python/cpython/issues/78961). + + Isolation is best-effort: when the descriptors cannot be rearranged, or a + transport in this process already claimed stdin, the returned stream is + sys.stdin.buffer read in place (the pre-isolation behavior) and the undo + callback is None. + """ + global _stdin_claimed + if _stdin_claimed or not _is_backed_by_fd(sys.stdin, 0): + return sys.stdin.buffer, None + # Set before touching the descriptor table so a second transport entering + # mid-claim serves in place instead of duplicating a half-moved fd 0. + _stdin_claimed = True + private_fd = None + try: + private_fd = os.dup(0) + devnull_fd = os.open(os.devnull, os.O_RDONLY) + try: + os.dup2(devnull_fd, 0) + finally: + os.close(devnull_fd) + if sys.platform == "win32": # pragma: no cover + rebind_std_handle_to_fd(0) + except OSError: + _stdin_claimed = False + # Isolation is best-effort: serve stdin in place, as before it existed. + 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 + + def restore() -> None: + global _stdin_claimed + _restore_fd(0, private_fd) + _stdin_claimed = False + + # closefd=False: the reader may sit in a blocking read on this descriptor + # in a worker thread past the transport's lifetime, so garbage collection + # of the wrapper must never close (and free for reuse) the fd under it. + return os.fdopen(private_fd, "rb", closefd=False), restore + + +def _restore_fd(fd: int, private_fd: int) -> None: + """Points fd back at the protocol stream the transport claimed. + + Best-effort: a failure must never mask whatever ended the transport, so it + is swallowed rather than raised out of stdio_server's finally. + """ + with suppress(OSError): + os.dup2(private_fd, fd) + if sys.platform == "win32": # pragma: no cover + rebind_std_handle_to_fd(fd) + @asynccontextmanager async def stdio_server(stdin: anyio.AsyncFile[str] | None = None, stdout: anyio.AsyncFile[str] | None = None): """Server transport for stdio: this communicates with an MCP client by reading from the current process' stdin and writing to stdout. + + While serving on the process's real stdin, the transport claims it: the + protocol pipe moves to a private descriptor and fd 0 (with the Windows + standard input handle) reads the null device, so handler code and its + child processes cannot touch the protocol stream. Both are restored when + the context exits. Passing an explicit stdin skips this entirely. """ # Purposely not using context managers for these, as we don't want to close # standard process handles. Encoding of stdin/stdout as text streams on # python is platform-dependent (Windows is particularly problematic), so we # re-wrap the underlying binary stream to ensure UTF-8. - if not stdin: - stdin = anyio.wrap_file(TextIOWrapper(sys.stdin.buffer, encoding="utf-8", errors="replace")) - if not stdout: - stdout = anyio.wrap_file(TextIOWrapper(sys.stdout.buffer, encoding="utf-8")) + restore_stdin: Callable[[], None] | None = None + try: + if not stdin: + stdin_buffer, restore_stdin = _claim_stdin() + stdin = anyio.wrap_file(TextIOWrapper(stdin_buffer, encoding="utf-8", errors="replace")) + if not stdout: + stdout = anyio.wrap_file(TextIOWrapper(sys.stdout.buffer, encoding="utf-8")) - read_stream_writer, read_stream = create_context_streams[SessionMessage | Exception](0) - write_stream, write_stream_reader = create_context_streams[SessionMessage](0) + read_stream_writer, read_stream = create_context_streams[SessionMessage | Exception](0) + write_stream, write_stream_reader = create_context_streams[SessionMessage](0) - async def stdin_reader(): - try: - async with read_stream_writer: - async for line in stdin: - try: - message = types.jsonrpc_message_adapter.validate_json(line, by_name=False) - except Exception as exc: - await read_stream_writer.send(exc) - continue - - session_message = SessionMessage(message) - await read_stream_writer.send(session_message) - except anyio.ClosedResourceError: # pragma: no cover - await anyio.lowlevel.checkpoint() - - async def stdout_writer(): - try: - async with write_stream_reader: - async for session_message in write_stream_reader: - json = session_message.message.model_dump_json(by_alias=True, exclude_unset=True) - await stdout.write(json + "\n") - await stdout.flush() - except anyio.ClosedResourceError: # pragma: no cover - await anyio.lowlevel.checkpoint() - - async with anyio.create_task_group() as tg: - tg.start_soon(stdin_reader) - tg.start_soon(stdout_writer) - yield read_stream, write_stream + async def stdin_reader(): + try: + async with read_stream_writer: + async for line in stdin: + try: + message = types.jsonrpc_message_adapter.validate_json(line, by_name=False) + except Exception as exc: + await read_stream_writer.send(exc) + continue + + session_message = SessionMessage(message) + await read_stream_writer.send(session_message) + except anyio.ClosedResourceError: # pragma: no cover + await anyio.lowlevel.checkpoint() + + async def stdout_writer(): + try: + async with write_stream_reader: + async for session_message in write_stream_reader: + json = session_message.message.model_dump_json(by_alias=True, exclude_unset=True) + await stdout.write(json + "\n") + await stdout.flush() + except anyio.ClosedResourceError: # pragma: no cover + await anyio.lowlevel.checkpoint() + + async with anyio.create_task_group() as tg: + tg.start_soon(stdin_reader) + tg.start_soon(stdout_writer) + yield read_stream, write_stream + finally: + if restore_stdin is not None: + restore_stdin() diff --git a/tests/server/test_stdio.py b/tests/server/test_stdio.py index 218e34d5ac..f11c6c55bd 100644 --- a/tests/server/test_stdio.py +++ b/tests/server/test_stdio.py @@ -1,11 +1,13 @@ import io +import os import sys import threading -from collections.abc import AsyncIterator -from contextlib import asynccontextmanager +from collections.abc import AsyncIterator, Iterator +from contextlib import asynccontextmanager, contextmanager from io import TextIOWrapper import anyio +import anyio.to_thread import pytest from mcp_types import ( CLIENT_CAPABILITIES_META_KEY, @@ -105,6 +107,240 @@ async def test_stdio_server_invalid_utf8(monkeypatch: pytest.MonkeyPatch) -> Non assert second.message == valid +@contextmanager +def _pipe_planted_on_fd0(monkeypatch: pytest.MonkeyPatch) -> Iterator[tuple[int, int]]: + """Plants a fresh pipe on the process's fd 0 and rebinds sys.stdin over it. + + Yields the pipe's (read_fd, write_fd). The caller owns the write end and + must close it exactly once (every test does, to EOF the transport); the + helper closes only what it created and still owns, so no descriptor is + ever closed twice - a second close lands on a recycled fd number and + destroys whatever unrelated file now lives there (pytest's capture files, + in practice). Captures the real os.dup2 up front so teardown restores + fd 0 even for tests that monkeypatch descriptor calls. + """ + real_dup2 = os.dup2 + in_r, in_w = os.pipe() + saved0 = os.dup(0) + stdin_double = TextIOWrapper(open(0, "rb", closefd=False), encoding="utf-8") + try: + os.dup2(in_r, 0) + monkeypatch.setattr(sys, "stdin", stdin_double) + yield in_r, in_w + finally: + # Closed while its descriptor is still valid, so a later garbage + # collection never does I/O on a recycled fd. + stdin_double.close() + real_dup2(saved0, 0) + os.close(saved0) + os.close(in_r) + + +def _frame(message: JSONRPCRequest | JSONRPCResponse) -> bytes: + """One JSON-RPC message as the newline-terminated wire line the transport reads.""" + return (message.model_dump_json(by_alias=True, exclude_none=True) + "\n").encode() + + +@pytest.mark.anyio +async def test_stdio_server_takes_stdin_off_the_descriptor_table_while_serving( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """On the real process stdin, the transport claims the protocol pipe and releases it on exit. + + SDK-defined behavior: while serving, fd 0 is the null device, so a child + process that inherits it cannot consume protocol bytes and, on Windows, + cannot hang at interpreter startup behind the transport's pending read + (CPython gh-78961). The protocol still flows over the original pipe, and on + exit fd 0 points back at it. Raw pipes are the subject here: the property + under test is what the process's descriptor table looks like while serving. + """ + with _pipe_planted_on_fd0(monkeypatch) as (in_r, in_w): + out_r, out_w = os.pipe() # captures responses via the sys.stdout double + stdout_double = TextIOWrapper(open(out_w, "wb", closefd=False), encoding="utf-8") + try: + monkeypatch.setattr(sys, "stdout", stdout_double) + + request = JSONRPCRequest(jsonrpc="2.0", id=1, method="ping") + response = JSONRPCResponse(jsonrpc="2.0", id=1, result={}) + with anyio.fail_after(5): + async with stdio_server() as (read_stream, write_stream): + async with read_stream: + # fd 0 is the null device: instant EOF instead of protocol bytes. In a + # worker thread because a regression would leave fd 0 on the (empty) + # protocol pipe, and a blocked read on the loop thread would outlive + # fail_after; abandoning turns that into a red TimeoutError instead. + assert await anyio.to_thread.run_sync(os.read, 0, 1, abandon_on_cancel=True) == b"" + + # The protocol still flows over the original pipe. + os.write(in_w, _frame(request)) + received = await read_stream.receive() + assert isinstance(received, SessionMessage) + assert received.message == request + + await write_stream.send(SessionMessage(response)) + # Worker thread + abandon for the same reason as the fd 0 read above. + line = await anyio.to_thread.run_sync(os.read, out_r, 65536, abandon_on_cancel=True) + assert jsonrpc_message_adapter.validate_json(line.decode().strip()) == response + + os.close(in_w) # EOF lets the reader finish so the context can exit + await write_stream.aclose() + + # Restored: fd 0 is a handle to the protocol pipe again, not the null + # device. samestat degrades to a trivially-true comparison for pipes on + # Windows (st_dev/st_ino are 0 there); the POSIX legs carry the assertion. + assert os.path.sameopenfile(0, in_r) + finally: + stdout_double.close() + os.close(out_r) + os.close(out_w) + + +@pytest.mark.anyio +async def test_a_nested_stdio_server_does_not_clobber_the_first_transports_claim( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Only the first default-stream transport claims stdin; a nested one serves in place. + + SDK-defined behavior: a second stdio_server() entered while the first is + serving must not re-claim fd 0 (it would duplicate the null device and its + restore would clobber the first transport's). The inner transport reads the + in-place stdin - the null device while the outer one serves - and sees + immediate EOF; after both exit, fd 0 is back on the protocol pipe. + """ + with _pipe_planted_on_fd0(monkeypatch) as (in_r, in_w): + monkeypatch.setattr(sys, "stdout", TextIOWrapper(io.BytesIO(), encoding="utf-8")) + + request = JSONRPCRequest(jsonrpc="2.0", id=1, method="ping") + with anyio.fail_after(5): + async with stdio_server() as (outer_read, outer_write): + async with outer_read: + async with stdio_server() as (inner_read, inner_write): + async with inner_read: + with pytest.raises(anyio.EndOfStream): + await inner_read.receive() + await inner_write.aclose() + + # The outer transport still owns the real pipe. + os.write(in_w, _frame(request)) + received = await outer_read.receive() + assert isinstance(received, SessionMessage) + assert received.message == request + + os.close(in_w) + await outer_write.aclose() + + assert os.path.sameopenfile(0, in_r) + + +@pytest.mark.anyio +@pytest.mark.parametrize("failing_call", ["dup", "dup2"]) +async def test_stdio_server_reads_stdin_in_place_when_descriptor_isolation_fails( + failing_call: str, monkeypatch: pytest.MonkeyPatch +) -> None: + """A descriptor-table failure while claiming stdin degrades to reading sys.stdin in place. + + SDK-defined behavior: isolation is best-effort; when duplicating fd 0 or + retargeting it fails, the transport serves over the original stdin exactly + as it did before isolation existed - observable as fd 0 still being the + protocol pipe while serving. The dup2 variant fails after the private + duplicate exists, covering its cleanup path. + """ + request = JSONRPCRequest(jsonrpc="2.0", id=1, method="ping") + with _pipe_planted_on_fd0(monkeypatch) as (in_r, in_w): + os.write(in_w, _frame(request)) + os.close(in_w) # EOF after the one frame lets the reader finish + monkeypatch.setattr(sys, "stdout", TextIOWrapper(io.BytesIO(), encoding="utf-8")) + + # Injections fire exactly once, at the transport's own call, then pass + # through: pytest's capture machinery also calls os.dup/os.dup2 at + # phase transitions, and a still-armed injector detonating there + # corrupts capture for every later test in the process. + if failing_call == "dup": + real_dup = os.dup + armed = [True] + + def failing_dup(fd: int) -> int: + if fd == 0 and armed[0]: + armed[0] = False + raise OSError("injected descriptor failure") + return real_dup(fd) + + monkeypatch.setattr(os, "dup", failing_dup) + else: + real_dup2 = os.dup2 + armed = [True] + + def failing_dup2(fd: int, fd2: int, inheritable: bool = True) -> int: + if armed[0]: + armed[0] = False + raise OSError("injected descriptor failure") + return real_dup2(fd, fd2, inheritable) + + monkeypatch.setattr(os, "dup2", failing_dup2) + + with anyio.fail_after(5): + async with stdio_server() as (read_stream, write_stream): # pragma: no branch + async with read_stream: # pragma: no branch + # Isolation was skipped: fd 0 is still the protocol pipe, + # not the null device. (samestat degrades to trivially-true + # for pipes on Windows; the POSIX legs carry the assertion.) + assert os.path.sameopenfile(0, in_r) + # The spent injector passes calls through untouched. + os.close(os.dup(0)) + received = await read_stream.receive() + assert isinstance(received, SessionMessage) + assert received.message == request + await write_stream.aclose() + + +@pytest.mark.anyio +async def test_stdio_server_exits_cleanly_when_the_stdin_restore_fails( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A failed fd 0 restore on exit is swallowed, not raised. + + SDK-defined behavior: the restore in stdio_server's finally must never mask + the exception (or clean exit) that ended the transport. The context exits + normally; the only trace is fd 0 remaining on the null device. + """ + request = JSONRPCRequest(jsonrpc="2.0", id=1, method="ping") + with _pipe_planted_on_fd0(monkeypatch) as (_, in_w): + os.write(in_w, _frame(request)) + os.close(in_w) + monkeypatch.setattr(sys, "stdout", TextIOWrapper(io.BytesIO(), encoding="utf-8")) + + # The claim's dup2 (first call) must succeed and only the restore's + # dup2 (second call) fails; later calls pass through because pytest's + # capture machinery also uses os.dup2 between test phases. + real_dup2 = os.dup2 + dup2_calls: list[tuple[int, int]] = [] + + def flaky_dup2(fd: int, fd2: int, inheritable: bool = True) -> int: + dup2_calls.append((fd, fd2)) + if len(dup2_calls) == 2: + raise OSError("injected restore failure") + return real_dup2(fd, fd2, inheritable) + + monkeypatch.setattr(os, "dup2", flaky_dup2) + + with anyio.fail_after(5): + async with stdio_server() as (read_stream, write_stream): # pragma: no branch + async with read_stream: # pragma: no branch + received = await read_stream.receive() + assert isinstance(received, SessionMessage) + assert received.message == request + await write_stream.aclose() + + # The restore was attempted (second dup2) and its failure swallowed: + # the context exited cleanly with fd 0 left on the null device. + assert dup2_calls[1] == (dup2_calls[1][0], 0) + devnull_probe = os.open(os.devnull, os.O_RDONLY) + try: + assert os.path.sameopenfile(0, devnull_probe) + finally: + os.close(devnull_probe) + + class _GatedStdin(io.RawIOBase): """Raw stdin double: serves its frames, then blocks until released before EOF. diff --git a/tests/transports/stdio/test_windows.py b/tests/transports/stdio/test_windows.py index 2d4eeac826..bba59c703e 100644 --- a/tests/transports/stdio/test_windows.py +++ b/tests/transports/stdio/test_windows.py @@ -15,12 +15,14 @@ import sys from contextlib import AsyncExitStack from pathlib import Path +from textwrap import dedent import anyio import anyio.abc import pytest -from mcp_types import JSONRPCRequest, JSONRPCResponse +from mcp_types import JSONRPCRequest, JSONRPCResponse, TextContent +from mcp.client.client import Client from mcp.client.stdio import StdioServerParameters, stdio_client from mcp.os.win32.utilities import FallbackProcess from mcp.shared.message import SessionMessage @@ -238,3 +240,55 @@ async def test_a_native_server_emitting_crlf_line_endings_round_trips_messages() # here instead of a parsed message. assert isinstance(received, SessionMessage) assert received.message == JSONRPCResponse(jsonrpc="2.0", id=1, result={}) + + +async def test_a_tool_spawned_python_child_with_default_stdin_completes_promptly() -> None: # pragma: no cover + """A tool that runs a Python subprocess without redirecting stdin returns promptly. + + Regression for #671 (SDK-defined isolation behavior): before `stdio_server` + pointed fd 0 at the null device, such a child inherited the protocol stdin + pipe and blocked inside interpreter startup behind the transport's pending + read (CPython gh-78961) until the next inbound message arrived - so these + calls, with no follow-up traffic, hung until the timeout. Covers both + reported shapes: output piped with stdin defaulted, and no redirection at + all (the console subsystem still propagates the standard handles). + """ + server = dedent( + """ + import subprocess, sys + from mcp.server import MCPServer + + mcp = MCPServer("spawner") + + @mcp.tool() + def run_child() -> str: + proc = subprocess.run([sys.executable, "-c", "print('ok')"], capture_output=True, timeout=20) + return proc.stdout.decode().strip() + + @mcp.tool() + def run_child_bare() -> str: + # No redirection at all: Windows still hands a console child the + # parent's standard handles, so pre-isolation this hung too. The + # child prints nothing, keeping the inherited stdout wire clean. + proc = subprocess.run([sys.executable, "-c", "pass"], timeout=20) + return str(proc.returncode) + + mcp.run() + """ + ) + transport = stdio_client(StdioServerParameters(command=sys.executable, args=["-c", server])) + + # Four interpreter cold starts on a loaded runner (the server also imports + # the SDK); healthy runs take ~3s. A regression hangs forever, so the bound + # only has to beat "never". + with anyio.fail_after(40.0): + async with Client(transport) as client: + result = await client.call_tool("run_child") + bare = await client.call_tool("run_child_bare") + + content = result.content[0] + assert isinstance(content, TextContent) + assert content.text == "ok" + bare_content = bare.content[0] + assert isinstance(bare_content, TextContent) + assert bare_content.text == "0" From f74f6084df1f11d3d8b723beb96e53718820bf2b Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Thu, 16 Jul 2026 20:46:41 +0000 Subject: [PATCH 2/2] Isolate the stdio server's stdout from handler code and subprocesses 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. --- docs/get-started/real-host.md | 2 +- docs/handlers/logging.md | 7 +- docs/migration.md | 55 ++-- docs/run/index.md | 2 +- docs/troubleshooting.md | 2 +- src/mcp/os/posix/utilities.py | 18 +- src/mcp/server/stdio.py | 178 +++++++--- tests/interaction/_requirements.py | 9 +- tests/interaction/transports/test_stdio.py | 3 +- tests/server/test_stdio.py | 359 ++++++++++++++++++++- tests/transports/stdio/test_lifecycle.py | 49 +++ tests/transports/stdio/test_windows.py | 3 +- 12 files changed, 600 insertions(+), 87 deletions(-) diff --git a/docs/get-started/real-host.md b/docs/get-started/real-host.md index 159c1f56f7..17e0e045e8 100644 --- a/docs/get-started/real-host.md +++ b/docs/get-started/real-host.md @@ -165,7 +165,7 @@ Once that command sits and waits, what's left is almost always one of three thin * **A relative path.** The host launches your server from *its* working directory, not the one you registered from. `server.py` where `/absolute/path/to/server.py` is needed is the single most common failure. If the host can't find `uv` either, that path has to be absolute too. * **The host is still running its old config.** Hosts read their config at launch. Claude Desktop in particular has to be *fully quit* (not just its window closed) and reopened before an edit to `claude_desktop_config.json` takes effect. -* **Something reached stdout.** On stdio, stdout *is* the protocol. One stray `print()` and the host reads a corrupt message and drops the connection. Log with the `logging` module, which writes to stderr. **[Logging](../handlers/logging.md)** has the whole story. +* **Something reached stdout before serving began.** On stdio, stdout *is* the protocol. The SDK diverts stray output to stderr while serving, but anything that reaches stdout before then -- a wrapper script echoing, an import-time `print()` in an unbuffered process -- hands the host a corrupt message and it drops the connection. Log with the `logging` module, which writes to stderr. **[Logging](../handlers/logging.md)** has the whole story. Claude Desktop keeps a log per server: `mcp-server-.log` is your server's stderr, next to `mcp.log` for connections, under `~/Library/Logs/Claude` on macOS and `%APPDATA%\Claude\logs` on Windows. diff --git a/docs/handlers/logging.md b/docs/handlers/logging.md index 945aa60d5e..c53aa79e47 100644 --- a/docs/handlers/logging.md +++ b/docs/handlers/logging.md @@ -33,8 +33,9 @@ For a **stdio** server this question matters more than usual. The host launched The standard library already does the right thing: log output goes to `sys.stderr` by default. Your `logger.info(...)` lines land in the terminal (or wherever the host collects the subprocess's stderr), and the protocol stream stays clean. !!! tip - 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. `logger.debug("got here")` is the same one line of effort and goes to the right place. @@ -72,7 +73,7 @@ went to standard error: the terminal, not the wire. * The MCP protocol's logging capability is deprecated by the 2026-07-28 spec and not replaced. Don't build on it. * `logger = logging.getLogger(__name__)` at module level, `logger.info(...)` in the tool. That's the whole pattern. * Log output never reaches the model. Only the value you `return` does. -* Standard error is yours; stdout belongs to the protocol. Never `print()` in a stdio server. +* Standard error is yours; stdout belongs to the protocol. The SDK diverts a stray `print()` to stderr while serving, but it arrives unlabeled -- use `logging`. * `MCPServer(..., log_level="DEBUG")` sets the level, and a logging configuration you made first is left alone. Telling connected clients that something on your server changed (the tool list, a resource) is **[Subscriptions](subscriptions.md)**. diff --git a/docs/migration.md b/docs/migration.md index 46c6f6c32a..24ca788930 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -1855,31 +1855,40 @@ group (spawned with `start_new_session=True`); the `getpgid()` lookup and the per-process terminate/kill fallback are gone. The win32 utilities logger is now named `mcp.os.win32.utilities` (was `client.stdio.win32`). -### `stdio_server` keeps the protocol stdin on a private descriptor - -While serving on the process's real stdin, the stdio server transport now duplicates -the protocol pipe to a private descriptor and points fd 0 — and, on Windows, the -standard input handle — at the null device, restoring both when the transport exits. -Subprocesses started by handler code therefore inherit the null device instead of the -protocol pipe. (The claim is best-effort: in the rare process whose descriptor table -cannot be rearranged, the transport serves stdin in place, exactly as v1 did.) - -In v1 a child inheriting the pipe could consume protocol bytes, and on Windows it -hung inside interpreter startup — behind the transport's pending read on the shared -pipe ([CPython gh-78961](https://github.com/python/cpython/issues/78961)) — until +### `stdio_server` keeps the protocol streams on private descriptors + +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 +therefore inherit the diversions instead of the protocol pipes, and a stray +`print()` lands on stderr rather than the wire. (The claim is best-effort: in the +rare process whose descriptor table cannot be rearranged, the transport serves in +place, exactly as v1 did.) + +In v1 a child inheriting the pipes could consume protocol bytes or corrupt the +outgoing stream with its own output, and on Windows it hung inside interpreter +startup — behind the transport's pending read on the shared pipe +([CPython gh-78961](https://github.com/python/cpython/issues/78961)) — until the next request arrived: the [#671](https://github.com/modelcontextprotocol/python-sdk/issues/671) hang, for -which passing `stdin=subprocess.DEVNULL` on every spawn was the required -workaround. On v2 the workaround is no longer needed, for any spawn API, -redirected or not. - -To migrate: nothing, unless handler code read `sys.stdin` (or called `input()`) -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. +which passing `stdin=subprocess.DEVNULL` on every spawn (and capturing the child's +stdout) was the required workaround. On v2 the workaround is no longer needed, for +any spawn API, redirected or not. + +To migrate: nothing, unless handler code used the standard streams directly during a +stdio session. Reading `sys.stdin` (or calling `input()`) now sees end-of-file +instead of racing the transport for protocol bytes; there was never a meaningful +value to read there. `print()` and other `sys.stdout` writes reach stderr instead of +corrupting the wire — code that deliberately wrote protocol frames to `sys.stdout` +must send them through the transport's write stream instead. A child that streams +a lot of output to its inherited stdout now streams it into the client's stderr +channel; capture output you don't want in the client's logs. Likewise, anything +that read ahead from `sys.stdin` before the server started keeps those bytes; they +no longer reach the transport (they never reliably did). Passing an explicit `stdin=`/`stdout=` stream to +`stdio_server(...)` skips the descriptor changes for that stream, as does any +environment where the sys stream is not backed by the process's real descriptor. ### WebSocket transport removed diff --git a/docs/run/index.md b/docs/run/index.md index 4ff8f53deb..7757fc888a 100644 --- a/docs/run/index.md +++ b/docs/run/index.md @@ -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 diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 621b32c6cd..e6613a2900 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -137,7 +137,7 @@ There is no error string for this, which is exactly why it is hard to search. Th * **Is the tool on the `mcp` the host is running?** A second `MCPServer(...)` in another module is a different, empty server. Check which object the host's command actually imports. * **Did two tools share a name?** Then one of them is gone. Look for `Tool already exists:` in the server log. * **Is the host's list stale?** Adding a tool after startup only reaches clients that handle `notifications/tools/list_changed`. Restarting the host is the blunt fix. -* **Did something write to `stdout`?** On a stdio transport, stdout *is* the protocol: one stray `print()` and the host drops the connection, which some hosts render as a server with nothing in it. Log with the `logging` module instead. The rest of the host-side checklist is on **[Connect to a real host](get-started/real-host.md)**. +* **Did something write to `stdout` before the server started serving?** While serving, the SDK diverts stray stdout to stderr (best-effort: an environment that replaces `sys.stdout`, or merges stderr into stdout on Windows, is served as-is), but output flushed to stdout earlier -- a wrapper script echoing, an import-time `print()` in an unbuffered process -- lands on the protocol stream, and one junk line can make the host drop the connection, which some hosts render as a server with nothing in it. Log with the `logging` module instead. The rest of the host-side checklist is on **[Connect to a real host](get-started/real-host.md)**. An "invalid" tool name is *not* on that list: a non-conforming name logs a warning but the tool is registered and listed anyway. diff --git a/src/mcp/os/posix/utilities.py b/src/mcp/os/posix/utilities.py index d15be17194..999fa3b757 100644 --- a/src/mcp/os/posix/utilities.py +++ b/src/mcp/os/posix/utilities.py @@ -1,8 +1,9 @@ -"""POSIX-specific functionality for stdio client operations.""" +"""POSIX-specific functionality for stdio transport operations.""" import logging import os import signal +import sys from contextlib import suppress import anyio @@ -10,6 +11,21 @@ logger = logging.getLogger(__name__) + +def same_open_file(fd_a: int, fd_b: int) -> bool: + """Whether two descriptors refer to the same open file. + + False on Windows - anonymous pipe handles carry no identity to compare - + and False when either descriptor cannot be interrogated. + """ + if sys.platform == "win32": + return False + try: + return os.path.sameopenfile(fd_a, fd_b) + except OSError: + return False + + # How often to probe for surviving group members between SIGTERM and SIGKILL. _GROUP_POLL_INTERVAL = 0.01 diff --git a/src/mcp/server/stdio.py b/src/mcp/server/stdio.py index 4fe730b8d5..ad430309fa 100644 --- a/src/mcp/server/stdio.py +++ b/src/mcp/server/stdio.py @@ -19,24 +19,28 @@ async def run_server(): import os import sys +import threading from collections.abc import Callable from contextlib import asynccontextmanager, suppress from io import TextIOWrapper -from typing import BinaryIO, TextIO +from typing import BinaryIO, Literal, TextIO import anyio import anyio.lowlevel import mcp_types as types +from mcp.os.posix.utilities import same_open_file from mcp.os.win32.utilities import rebind_std_handle_to_fd from mcp.shared._context_streams import create_context_streams from mcp.shared.message import SessionMessage -# True while a transport in this process has fd 0 pointed at the null device. -# A second concurrent stdio_server() must not claim again: it would duplicate -# the null device instead of the protocol pipe, and its restore would clobber -# the first transport's. -_stdin_claimed = False +# Descriptors a transport in this process currently has pointed away from +# their protocol pipes. A second concurrent stdio_server() must not claim an +# fd again: it would duplicate the diversion target instead of the protocol +# pipe, and its restore would clobber the first transport's. The lock makes +# the check-and-claim atomic for embedders running transports on threads. +_claimed: set[int] = set() +_claimed_lock = threading.Lock() def _is_backed_by_fd(stream: TextIO, fd: int) -> bool: @@ -49,58 +53,122 @@ def _is_backed_by_fd(stream: TextIO, fd: int) -> bool: return False -def _claim_stdin() -> tuple[BinaryIO, Callable[[], None] | None]: - """Returns the binary stream the transport reads the protocol from, and its undo. +def _dup_above_std(fd: int) -> int: + """Duplicates fd onto a descriptor above the standard range (fd > 2). - When running on the process's real stdin, moves the protocol pipe to a - private descriptor and points fd 0 (and, on Windows, the standard input - handle) at the null device for the transport's lifetime. Child processes - spawned by handlers then inherit the null device instead of the protocol - pipe: a child holding the protocol pipe could consume protocol bytes, and - on Windows a Python child hangs during interpreter startup while the - transport's blocking read is pending on the shared pipe - (https://github.com/python/cpython/issues/78961). + os.dup hands out the lowest free slot, so in a process started with a + standard descriptor closed the private wire duplicate would itself become + fd 0, 1, or 2 - handed to children as a standard stream, and, when it + lands on fd 2, silently made the target of the stdout diversion. - Isolation is best-effort: when the descriptors cannot be rearranged, or a - transport in this process already claimed stdin, the returned stream is - sys.stdin.buffer read in place (the pre-isolation behavior) and the undo - callback is None. + Raises: + OSError: propagated from os.dup; nothing is leaked into the standard + range - every duplicate made so far is closed first. """ - global _stdin_claimed - if _stdin_claimed or not _is_backed_by_fd(sys.stdin, 0): - return sys.stdin.buffer, None - # Set before touching the descriptor table so a second transport entering - # mid-claim serves in place instead of duplicating a half-moved fd 0. - _stdin_claimed = True + duplicates = [os.dup(fd)] + try: + while duplicates[-1] <= 2: + duplicates.append(os.dup(fd)) + except OSError: + for duplicate in duplicates: + os.close(duplicate) + raise + for below_std in duplicates[:-1]: + os.close(below_std) + return duplicates[-1] + + +def _open_stdin_diversion() -> int: + """What fd 0 reads while claimed: the null device, so readers see EOF.""" + return os.open(os.devnull, os.O_RDONLY) + + +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) + + +def _claim_fd( + fd: int, stream: TextIO, mode: Literal["rb", "wb"], open_diversion: Callable[[], int] +) -> tuple[BinaryIO, Callable[[], None] | None]: + """Returns the binary stream the transport uses for the protocol, and its undo. + + When stream is the process's real standard stream, moves the protocol pipe + to a private descriptor and points fd (and, on Windows, the matching + standard handle) at the diversion for the transport's lifetime, so handler + code and its child processes touch the diversion instead of the protocol + pipe; stdio_server's docstring describes the resulting behavior. + + The claim is best-effort: when the descriptors cannot be rearranged the + transport serves the sys stream's buffer in place, exactly as before + isolation existed, and when a transport in this process already claimed + fd it serves a fresh buffered view of wherever fd currently points. In + both cases the undo callback is None. A stream with no real descriptor + is served through its own buffer. + """ + if not _is_backed_by_fd(stream, fd): + return stream.buffer, None + # Claimed before touching the descriptor table so a second transport + # entering mid-claim serves in place instead of duplicating a half-moved + # descriptor. + 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 + # points - the diversion - through a fresh view rather than the sys + # stream's buffer: its cached seekability describes the pre-claim + # target, and interrogating it can fail on the retargeted descriptor. + return open(fd, mode, closefd=False), None private_fd = None try: - private_fd = os.dup(0) - devnull_fd = os.open(os.devnull, os.O_RDONLY) + private_fd = _dup_above_std(fd) + diversion_fd = open_diversion() try: - os.dup2(devnull_fd, 0) + os.dup2(diversion_fd, fd) finally: - os.close(devnull_fd) + os.close(diversion_fd) if sys.platform == "win32": # pragma: no cover - rebind_std_handle_to_fd(0) + rebind_std_handle_to_fd(fd) except OSError: - _stdin_claimed = False - # Isolation is best-effort: serve stdin in place, as before it existed. + with _claimed_lock: + _claimed.discard(fd) if private_fd is not None: - # A completed dup2 is undone; an untouched fd 0 is re-pointed at + # A completed dup2 is undone; an untouched fd is re-pointed at # the same pipe it already holds, which is harmless. - _restore_fd(0, private_fd) + _restore_fd(fd, private_fd) os.close(private_fd) - return sys.stdin.buffer, None + # 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: - global _stdin_claimed - _restore_fd(0, private_fd) - _stdin_claimed = False - - # closefd=False: the reader may sit in a blocking read on this descriptor - # in a worker thread past the transport's lifetime, so garbage collection - # of the wrapper must never close (and free for reuse) the fd under it. - return os.fdopen(private_fd, "rb", closefd=False), restore + # 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): + stream.flush() + _restore_fd(fd, private_fd) + with _claimed_lock: + _claimed.discard(fd) + + # closefd=False: an I/O call may sit blocked on this descriptor in a + # worker thread past the transport's lifetime, so garbage collection of + # the wrapper must never close (and free for reuse) the fd under it. The + # private descriptor is deliberately left open for the same reason - one + # descriptor per stream per session, restore() only points fd back at it. + return os.fdopen(private_fd, mode, closefd=False), restore def _restore_fd(fd: int, private_fd: int) -> None: @@ -120,23 +188,29 @@ async def stdio_server(stdin: anyio.AsyncFile[str] | None = None, stdout: anyio. """Server transport for stdio: this communicates with an MCP client by reading from the current process' stdin and writing to stdout. - While serving on the process's real stdin, the transport claims it: the - protocol pipe moves to a private descriptor and fd 0 (with the Windows - standard input handle) reads the null device, so handler code and its - child processes cannot touch the protocol stream. Both are restored when - the context exits. Passing an explicit stdin skips this entirely. + While serving on the process's real stdin and stdout, the transport claims + them: each protocol pipe moves to a private descriptor, fd 0 (with the + Windows standard input handle) reads the null device, and fd 1 (with the + standard output handle) is diverted to stderr — the null device if stderr + is unusable. Handler code and the child processes it spawns can therefore + neither consume protocol bytes nor corrupt the outgoing stream: reads see + end-of-file, and stray writes (a `print()`, a child's inherited stdout) + land on stderr. Both descriptors are restored when the context exits. + Passing an explicit stream skips the claim for that side. """ # Purposely not using context managers for these, as we don't want to close # standard process handles. Encoding of stdin/stdout as text streams on # python is platform-dependent (Windows is particularly problematic), so we # re-wrap the underlying binary stream to ensure UTF-8. restore_stdin: Callable[[], None] | None = None + restore_stdout: Callable[[], None] | None = None try: if not stdin: - stdin_buffer, restore_stdin = _claim_stdin() + stdin_buffer, restore_stdin = _claim_fd(0, sys.stdin, "rb", _open_stdin_diversion) stdin = anyio.wrap_file(TextIOWrapper(stdin_buffer, encoding="utf-8", errors="replace")) if not stdout: - stdout = anyio.wrap_file(TextIOWrapper(sys.stdout.buffer, encoding="utf-8")) + stdout_buffer, restore_stdout = _claim_fd(1, sys.stdout, "wb", _open_stdout_diversion) + stdout = anyio.wrap_file(TextIOWrapper(stdout_buffer, encoding="utf-8")) read_stream_writer, read_stream = create_context_streams[SessionMessage | Exception](0) write_stream, write_stream_reader = create_context_streams[SessionMessage](0) @@ -171,5 +245,7 @@ async def stdout_writer(): tg.start_soon(stdout_writer) yield read_stream, write_stream finally: + if restore_stdout is not None: + restore_stdout() if restore_stdin is not None: restore_stdin() diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index 752c17aa4f..98e0116f6b 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -3916,9 +3916,12 @@ def __post_init__(self) -> None: note="Only observable over stdio: stdin/stdout purity is stdio-specific.", divergence=Divergence( note=( - "stdio_server's own writes satisfy this, but it does not redirect or guard sys.stdout: " - "handler code that calls print() writes directly to the protocol stream and corrupts the " - "framing. The spec MUST is satisfied only as long as application code behaves." + "While serving, stdio_server moves the wire to private descriptors and diverts fd 0/1, so " + "handler code and its child processes can neither read protocol bytes nor write into the " + "stream (pinned by tests/server/test_stdio.py). Remaining gaps: output flushed to stdout " + "before the transport enters can still precede the first frame; the claim is best-effort " + "and skipped for explicitly injected streams; and a stderr merged into stdout (2>&1) is " + "detected and neutralized only on POSIX - Windows pipe handles carry no identity." ), ), ), diff --git a/tests/interaction/transports/test_stdio.py b/tests/interaction/transports/test_stdio.py index dbb7de3459..2c34f5a0bf 100644 --- a/tests/interaction/transports/test_stdio.py +++ b/tests/interaction/transports/test_stdio.py @@ -113,7 +113,8 @@ async def test_stdio_server_writes_one_jsonrpc_message_per_line() -> None: """Every `stdio_server` write is one valid JSON-RPC message on its own line. Each line is newline-terminated with payload newlines JSON-escaped. This proves the - transport's own framing; it does not guard `sys.stdout` against handler code (see the + transport's own framing over injected streams; the descriptor-level guard that keeps + handler code off the wire is pinned by tests/server/test_stdio.py (see the narrowed divergence on `transport:stdio:stream-purity`). """ captured = io.StringIO() diff --git a/tests/server/test_stdio.py b/tests/server/test_stdio.py index f11c6c55bd..73d39797fa 100644 --- a/tests/server/test_stdio.py +++ b/tests/server/test_stdio.py @@ -136,11 +136,69 @@ def _pipe_planted_on_fd0(monkeypatch: pytest.MonkeyPatch) -> Iterator[tuple[int, os.close(in_r) +@contextmanager +def _pipe_planted_on_fd1(monkeypatch: pytest.MonkeyPatch) -> Iterator[tuple[int, int]]: + """Plants a fresh pipe on the process's fd 1 and rebinds sys.stdout over it. + + Yields the pipe's (read_fd, write_fd); the test observes the wire through + the read end. The helper owns every descriptor it creates and closes each + exactly once (see _pipe_planted_on_fd0 for why double-closing is + destructive), closing the sys.stdout double first, while its descriptor is + still valid. Captures the real os.dup2 up front so teardown restores fd 1 + even for tests that monkeypatch descriptor calls. + """ + real_dup2 = os.dup2 + out_r, out_w = os.pipe() + saved1 = os.dup(1) + stdout_double = TextIOWrapper(open(1, "wb", closefd=False), encoding="utf-8") + try: + os.dup2(out_w, 1) + monkeypatch.setattr(sys, "stdout", stdout_double) + yield out_r, out_w + finally: + stdout_double.close() + real_dup2(saved1, 1) + os.close(saved1) + os.close(out_r) + os.close(out_w) + + +@contextmanager +def _pipe_planted_on_fd2() -> Iterator[int]: + """Plants a fresh pipe on the process's fd 2 to observe the stdout diversion. + + Yields the pipe's read end. Same exactly-once ownership rules as the other + planting helpers; nothing rebinds sys.stderr because the transport never + touches it - the diversion duplicates fd 2 directly. + """ + real_dup2 = os.dup2 + err_r, err_w = os.pipe() + saved2 = os.dup(2) + try: + os.dup2(err_w, 2) + yield err_r + finally: + real_dup2(saved2, 2) + os.close(saved2) + os.close(err_r) + os.close(err_w) + + def _frame(message: JSONRPCRequest | JSONRPCResponse) -> bytes: """One JSON-RPC message as the newline-terminated wire line the transport reads.""" return (message.model_dump_json(by_alias=True, exclude_none=True) + "\n").encode() +async def _read_from(fd: int) -> bytes: + """One os.read from fd, in a worker thread. + + A regression can leave the target pipe empty, and a blocked read on the + loop thread would outlive fail_after; abandoning turns that into a red + TimeoutError instead. + """ + return await anyio.to_thread.run_sync(os.read, fd, 65536, abandon_on_cancel=True) + + @pytest.mark.anyio async def test_stdio_server_takes_stdin_off_the_descriptor_table_while_serving( monkeypatch: pytest.MonkeyPatch, @@ -179,7 +237,7 @@ async def test_stdio_server_takes_stdin_off_the_descriptor_table_while_serving( await write_stream.send(SessionMessage(response)) # Worker thread + abandon for the same reason as the fd 0 read above. - line = await anyio.to_thread.run_sync(os.read, out_r, 65536, abandon_on_cancel=True) + line = await _read_from(out_r) assert jsonrpc_message_adapter.validate_json(line.decode().strip()) == response os.close(in_w) # EOF lets the reader finish so the context can exit @@ -341,6 +399,305 @@ def flaky_dup2(fd: int, fd2: int, inheritable: bool = True) -> int: os.close(devnull_probe) +@pytest.mark.anyio +async def test_stdio_server_takes_stdout_off_the_descriptor_table_while_serving( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """On the real process stdout, the transport claims the wire and diverts fd 1 to stderr. + + SDK-defined behavior: while serving, fd 1 points at stderr, so a stray + write - a print(), or a child process inheriting stdout - lands in the + client's log instead of corrupting the JSON-RPC stream. The transport's + own frames still flow over the original pipe, and on exit fd 1 points + back at it. Raw pipes are the subject here: the property under test is + what the process's descriptor table looks like while serving. + """ + response = JSONRPCResponse(jsonrpc="2.0", id=1, result={}) + with _pipe_planted_on_fd1(monkeypatch) as (out_r, out_w), _pipe_planted_on_fd2() as err_r: + with anyio.fail_after(5): + async with stdio_server(stdin=anyio.AsyncFile(io.StringIO())) as (read_stream, write_stream): + read_stream.close() + # A raw write to fd 1 - what a child process inheriting + # stdout does - lands on stderr, not the wire. + os.write(1, b"stray child output\n") + assert await _read_from(err_r) == b"stray child output\n" + + # sys.stdout follows fd 1 to the diversion as well: a stray + # print() in handler code cannot reach the wire. The text + # layer writes os.linesep, hence CRLF on Windows. + print("stray print", flush=True) + assert await _read_from(err_r) == b"stray print" + os.linesep.encode() + + # The transport's frames flow over the original pipe, unpolluted. + await write_stream.send(SessionMessage(response)) + line = await _read_from(out_r) + assert jsonrpc_message_adapter.validate_json(line.decode().strip()) == response + + await write_stream.aclose() + + # Restored: fd 1 is a handle to the protocol pipe again, not the + # diversion. (samestat degrades to trivially-true for pipes on + # Windows; the POSIX legs carry the assertion.) + assert os.path.sameopenfile(1, out_w) + + +@pytest.mark.anyio +async def test_a_nested_stdio_server_does_not_clobber_the_first_transports_stdout_claim( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Only the first default-stream transport claims stdout; a nested one writes in place. + + SDK-defined behavior: a second stdio_server() entered while the first is + serving must not re-claim fd 1 (it would duplicate the diversion and its + restore would clobber the first transport's), and must not write into the + outer transport's wire. The inner transport writes the in-place stdout - + wherever fd 1 currently points, here the stderr diversion, which is where + its frames observably land - and after both exit, fd 1 is back on the + protocol pipe. + """ + outer_response = JSONRPCResponse(jsonrpc="2.0", id=1, result={}) + inner_response = JSONRPCResponse(jsonrpc="2.0", id=2, result={}) + with _pipe_planted_on_fd1(monkeypatch) as (out_r, out_w), _pipe_planted_on_fd2() as err_r: + with anyio.fail_after(5): + async with stdio_server(stdin=anyio.AsyncFile(io.StringIO())) as (outer_read, outer_write): + outer_read.close() + async with stdio_server(stdin=anyio.AsyncFile(io.StringIO())) as (inner_read, inner_write): + inner_read.close() + await inner_write.send(SessionMessage(inner_response)) + line = await _read_from(err_r) + assert jsonrpc_message_adapter.validate_json(line.decode().strip()) == inner_response + await inner_write.aclose() + + # The outer transport still owns the real pipe. + await outer_write.send(SessionMessage(outer_response)) + line = await _read_from(out_r) + assert jsonrpc_message_adapter.validate_json(line.decode().strip()) == outer_response + await outer_write.aclose() + + assert os.path.sameopenfile(1, out_w) + + +@pytest.mark.anyio +async def test_stdio_server_diverts_stdout_to_the_null_device_when_stderr_is_unusable( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """When stderr cannot be duplicated, fd 1 is diverted to the null device instead. + + SDK-defined behavior: the diversion target is stderr so stray output stays + visible, but a process whose fd 2 is unusable still gets its stdout + claimed - pointed at the null device - and the wire stays pure. + """ + response = JSONRPCResponse(jsonrpc="2.0", id=1, result={}) + with _pipe_planted_on_fd1(monkeypatch) as (out_r, out_w): + # Fires exactly once, at the diversion's own dup of fd 2, then passes + # through: pytest's capture machinery also calls os.dup at phase + # transitions, and a still-armed injector detonating there corrupts + # capture for every later test in the process. + real_dup = os.dup + armed = [True] + + def failing_dup(fd: int) -> int: + if fd == 2 and armed[0]: + armed[0] = False + raise OSError("injected stderr failure") + return real_dup(fd) + + monkeypatch.setattr(os, "dup", failing_dup) + + with anyio.fail_after(5): + async with stdio_server(stdin=anyio.AsyncFile(io.StringIO())) as (read_stream, write_stream): + read_stream.close() + devnull_probe = os.open(os.devnull, os.O_WRONLY) + try: + assert os.path.sameopenfile(1, devnull_probe) + finally: + os.close(devnull_probe) + + # Stray output is discarded rather than blocking or reaching + # the wire; the transport's frames still flow over the pipe. + os.write(1, b"discarded\n") + await write_stream.send(SessionMessage(response)) + line = await _read_from(out_r) + assert jsonrpc_message_adapter.validate_json(line.decode().strip()) == response + await write_stream.aclose() + + assert os.path.sameopenfile(1, out_w) + + +@pytest.mark.anyio +async def test_the_claim_survives_a_process_started_with_stderr_closed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A free standard slot never captures the private wire duplicate. + + SDK-defined behavior: in a process started with fd 2 closed, os.dup hands + out slot 2 - without the guard, the private stdout duplicate would become + the process's "stderr" and the diversion would then point fd 1 straight + back at the wire. The claim must keep its own descriptors above the + standard range: stray writes are discarded (stderr is unusable, so the + diversion is the null device), the wire stays pure, and fd 2 is left as + it was found - closed. + """ + response = JSONRPCResponse(jsonrpc="2.0", id=1, result={}) + with _pipe_planted_on_fd1(monkeypatch) as (out_r, out_w): + saved2 = os.dup(2) + os.close(2) # the freed slot is what os.dup would hand out next + try: + with anyio.fail_after(5): + async with stdio_server(stdin=anyio.AsyncFile(io.StringIO())) as (read_stream, write_stream): + read_stream.close() + devnull_probe = os.open(os.devnull, os.O_WRONLY) + try: + assert os.path.sameopenfile(1, devnull_probe) + finally: + os.close(devnull_probe) + + os.write(1, b"discarded\n") + await write_stream.send(SessionMessage(response)) + line = await _read_from(out_r) + assert jsonrpc_message_adapter.validate_json(line.decode().strip()) == response + await write_stream.aclose() + + assert os.path.sameopenfile(1, out_w) + # No claim descriptor was left occupying the closed stderr slot. + with pytest.raises(OSError): + os.fstat(2) + finally: + os.dup2(saved2, 2) + os.close(saved2) + + +@pytest.mark.anyio +async def test_a_duplication_failure_midway_through_the_claim_leaks_nothing_into_the_standard_range( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A dup failure while skipping the standard range closes what was already made. + + SDK-defined behavior: with fd 2 free, the claim's first duplicate lands on + slot 2 and a second duplication is needed; when that one fails, the claim + degrades to serving in place and the slot-2 duplicate is closed rather + than left masquerading as the process's stderr. + """ + request = JSONRPCRequest(jsonrpc="2.0", id=1, method="ping") + with _pipe_planted_on_fd0(monkeypatch) as (in_r, in_w): + os.write(in_w, _frame(request)) + os.close(in_w) + monkeypatch.setattr(sys, "stdout", TextIOWrapper(io.BytesIO(), encoding="utf-8")) + + saved2 = os.dup(2) + os.close(2) + # Fires on the second dup of fd 0 - the escape from the standard + # range - then passes through (see the one-shot injector rationale in + # the descriptor-isolation failure test above). + real_dup = os.dup + dup_calls: list[int] = [] + + def failing_second_dup(fd: int) -> int: + if fd == 0: + dup_calls.append(fd) + if len(dup_calls) == 2: + raise OSError("injected descriptor failure") + return real_dup(fd) + + monkeypatch.setattr(os, "dup", failing_second_dup) + try: + with anyio.fail_after(5): + async with stdio_server() as (read_stream, write_stream): # pragma: no branch + async with read_stream: # pragma: no branch + # Isolation degraded to serving in place. + assert os.path.sameopenfile(0, in_r) + received = await read_stream.receive() + assert isinstance(received, SessionMessage) + assert received.message == request + await write_stream.aclose() + + # The injector leaves non-stdin descriptors untouched. + os.close(os.dup(in_r)) + # The slot-2 duplicate made before the failure was closed again. + with pytest.raises(OSError): + os.fstat(2) + finally: + real_dup2 = os.dup2 + real_dup2(saved2, 2) + os.close(saved2) + + +@pytest.mark.anyio +async def test_stdout_is_diverted_to_the_null_device_when_stderr_is_detected_as_the_wire( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A stderr that aliases stdout is never used as the diversion target. + + SDK-defined behavior: when stderr refers to the same open file as stdout + (a `2>&1` launch), diverting stray output "to stderr" would write straight + back into the wire, so the diversion falls back to the null device. The + detection itself is platform-dependent (see same_open_file), so it is + forced here; the POSIX twin below exercises the real detection. + """ + response = JSONRPCResponse(jsonrpc="2.0", id=1, result={}) + with _pipe_planted_on_fd1(monkeypatch) as (out_r, out_w): + + def merged(fd_a: int, fd_b: int) -> bool: + return True + + monkeypatch.setattr("mcp.server.stdio.same_open_file", merged) + + with anyio.fail_after(5): + async with stdio_server(stdin=anyio.AsyncFile(io.StringIO())) as (read_stream, write_stream): + read_stream.close() + devnull_probe = os.open(os.devnull, os.O_WRONLY) + try: + assert os.path.sameopenfile(1, devnull_probe) + finally: + os.close(devnull_probe) + + os.write(1, b"discarded\n") + await write_stream.send(SessionMessage(response)) + line = await _read_from(out_r) + assert jsonrpc_message_adapter.validate_json(line.decode().strip()) == response + await write_stream.aclose() + + assert os.path.sameopenfile(1, out_w) + + +@pytest.mark.anyio +@pytest.mark.skipif(sys.platform == "win32", reason="pipe identity is only comparable on POSIX") +# lax no cover: the per-job 100% coverage gate also runs on Windows, where this test is skipped. +async def test_a_stderr_merged_into_stdout_is_really_detected_on_posix( # pragma: lax no cover + monkeypatch: pytest.MonkeyPatch, +) -> None: + """With fd 2 genuinely pointing at the wire pipe, the diversion is the null device. + + The unmocked twin of the detection test above: fd 2 is planted on the same + pipe as fd 1 and the real same-open-file comparison must notice. + """ + response = JSONRPCResponse(jsonrpc="2.0", id=1, result={}) + with _pipe_planted_on_fd1(monkeypatch) as (out_r, out_w): + real_dup2 = os.dup2 + saved2 = os.dup(2) + os.dup2(out_w, 2) # stderr now IS the wire, as 2>&1 leaves it + try: + with anyio.fail_after(5): + async with stdio_server(stdin=anyio.AsyncFile(io.StringIO())) as (read_stream, write_stream): + read_stream.close() + devnull_probe = os.open(os.devnull, os.O_WRONLY) + try: + assert os.path.sameopenfile(1, devnull_probe) + finally: + os.close(devnull_probe) + + os.write(1, b"discarded\n") + await write_stream.send(SessionMessage(response)) + line = await _read_from(out_r) + assert jsonrpc_message_adapter.validate_json(line.decode().strip()) == response + await write_stream.aclose() + + assert os.path.sameopenfile(1, out_w) + finally: + real_dup2(saved2, 2) + os.close(saved2) + + class _GatedStdin(io.RawIOBase): """Raw stdin double: serves its frames, then blocks until released before EOF. diff --git a/tests/transports/stdio/test_lifecycle.py b/tests/transports/stdio/test_lifecycle.py index 8a370c10f6..21b3f5f5c2 100644 --- a/tests/transports/stdio/test_lifecycle.py +++ b/tests/transports/stdio/test_lifecycle.py @@ -15,12 +15,15 @@ import threading from contextlib import AsyncExitStack from pathlib import Path +from textwrap import dedent import anyio import anyio.abc import pytest +from mcp_types import TextContent from mcp.client import stdio +from mcp.client.client import Client from mcp.client.stdio import StdioServerParameters, stdio_client from mcp.os.win32.utilities import FallbackProcess from tests.transports.stdio._liveness import ( @@ -274,3 +277,49 @@ async def test_fallback_process_wait_is_cancellable_while_the_child_lives() -> N popen.wait() popen.stdin.close() popen.stdout.close() + + +@pytest.mark.anyio +async def test_a_tool_spawned_childs_stdout_writes_never_reach_the_wire(tmp_path: Path) -> None: + """A child writing to its inherited stdout pollutes the server's stderr, never the protocol. + + Regression for the stdout half of `stdio_server`'s descriptor isolation: + pre-isolation, a tool-spawned child that printed to its inherited stdout + wrote straight into the JSON-RPC stream ahead of the tool response. The + junk line arriving on the server's stderr is the whole assertion: fd 1 has + exactly one target, so stderr delivery proves the wire never saw it (the + byte-level wire-purity twin is in tests/server/test_stdio.py). + """ + server = dedent( + """ + import subprocess, sys + from mcp.server import MCPServer + + mcp = MCPServer("noisy-spawner") + + @mcp.tool() + def run_noisy_child() -> str: + # No redirection: the child inherits the server's stdout. + proc = subprocess.run([sys.executable, "-c", "print('this is not json')"], timeout=20) + return str(proc.returncode) + + mcp.run() + """ + ) + + with (tmp_path / "server-stderr.txt").open("w+") as errlog: + transport = stdio_client(StdioServerParameters(command=sys.executable, args=["-c", server]), errlog=errlog) + # Three interpreter cold starts (the server also imports the SDK) on a + # loaded runner; a regressed Windows leg hangs rather than corrupts, so + # the bound only has to beat "never". + with anyio.fail_after(40): + async with Client(transport) as client: + result = await client.call_tool("run_noisy_child") + errlog.seek(0) + server_stderr = errlog.read() + + content = result.content[0] + assert isinstance(content, TextContent) + assert content.text == "0" + # Diverted to the server's stderr instead of landing on the wire. + assert "this is not json" in server_stderr diff --git a/tests/transports/stdio/test_windows.py b/tests/transports/stdio/test_windows.py index bba59c703e..621e367ad0 100644 --- a/tests/transports/stdio/test_windows.py +++ b/tests/transports/stdio/test_windows.py @@ -269,7 +269,8 @@ def run_child() -> str: def run_child_bare() -> str: # No redirection at all: Windows still hands a console child the # parent's standard handles, so pre-isolation this hung too. The - # child prints nothing, keeping the inherited stdout wire clean. + # child prints nothing, keeping this test pinned on the hang; the + # noisy-child shape is owned by test_lifecycle.py. proc = subprocess.run([sys.executable, "-c", "pass"], timeout=20) return str(proc.returncode)