Skip to content

Commit 9aa18f3

Browse files
soyukatonghuaroot
andauthored
Merge commit from fork
processInput() called fgets($this->input) with no length argument. PHP fgets() reads until a newline or EOF, so a peer that streams bytes without ever sending a newline forced the whole stream into a single allocation and exhausted the process memory limit (GHSA-vm75-qjc7-xh6w). Bound each read with fgets($this->input, $this->maxLineBytes) (default 4 MiB, configurable via the constructor). When a read fills the cap with no trailing newline the line exceeds the limit: discard it and keep discarding one bounded chunk per tick until its terminating newline, rather than buffering it. The discard is stateful so the run loop keeps servicing fibers and outgoing messages instead of blocking on a drain loop, and the over-length line is logged once. Reimplements tonghuaroot's advisory fix: makes the cap configurable rather than a hardcoded constant, drains the over-length line without an inner blocking loop (responsive run loop, single log line), and adds regression tests. Co-authored-by: tonghuaroot <23011166+tonghuaroot@users.noreply.github.com>
1 parent 529099f commit 9aa18f3

2 files changed

Lines changed: 143 additions & 1 deletion

File tree

src/Server/Transport/StdioTransport.php

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Mcp\Server\Transport;
1313

14+
use Mcp\Exception\InvalidArgumentException;
1415
use Mcp\Schema\JsonRpc\Error;
1516
use Mcp\Server\Transport\Stdio\RunnerControl;
1617
use Mcp\Server\Transport\Stdio\RunnerControlInterface;
@@ -24,17 +25,34 @@
2425
*/
2526
class StdioTransport extends BaseTransport
2627
{
28+
/**
29+
* Default cap on the bytes read for a single input line.
30+
*/
31+
public const DEFAULT_MAX_LINE_BYTES = 4 * 1024 * 1024;
32+
33+
/** Whether the current over-length line is still being drained and discarded. */
34+
private bool $discardingLine = false;
35+
2736
/**
2837
* @param resource $input
2938
* @param resource $output
39+
* @param int $maxLineBytes Maximum bytes read for a single input line. fgets() with no length reads until a
40+
* newline or EOF, so a peer that never sends a newline would buffer the whole stream
41+
* into one allocation and exhaust memory; a line exceeding this cap is discarded
42+
* instead.
3043
*/
3144
public function __construct(
3245
private $input = \STDIN,
3346
private $output = \STDOUT,
3447
?LoggerInterface $logger = null,
3548
private readonly RunnerControlInterface $runnerControl = new RunnerControl(),
49+
private readonly int $maxLineBytes = self::DEFAULT_MAX_LINE_BYTES,
3650
) {
3751
parent::__construct($logger);
52+
53+
if ($maxLineBytes < 1) {
54+
throw new InvalidArgumentException(\sprintf('The maximum line size must be a positive number of bytes, got %d.', $maxLineBytes));
55+
}
3856
}
3957

4058
public function send(string $data, array $context): void
@@ -68,13 +86,36 @@ public function listen(): int
6886

6987
protected function processInput(): void
7088
{
71-
$line = fgets($this->input);
89+
$line = fgets($this->input, $this->maxLineBytes);
7290
if (false === $line) {
7391
usleep(50000); // 50ms
7492

7593
return;
7694
}
7795

96+
$lineComplete = str_ends_with($line, "\n");
97+
98+
// A previous over-length line is still being drained: keep discarding
99+
// one bounded chunk per tick until its terminating newline is reached,
100+
// so the run loop stays responsive instead of blocking on a drain loop.
101+
if ($this->discardingLine) {
102+
$this->discardingLine = !$lineComplete;
103+
104+
return;
105+
}
106+
107+
// fgets() reads at most maxLineBytes - 1 bytes; a full read with no
108+
// trailing newline means the line exceeds the cap. Discard it rather
109+
// than buffering it, and keep discarding the remainder on later ticks.
110+
if (!$lineComplete && \strlen($line) >= $this->maxLineBytes - 1) {
111+
$this->discardingLine = true;
112+
$this->logger->warning('StdioTransport discarded an input line exceeding the maximum length.', [
113+
'max_line_bytes' => $this->maxLineBytes,
114+
]);
115+
116+
return;
117+
}
118+
78119
$trimmedLine = trim($line);
79120
if (!empty($trimmedLine)) {
80121
$this->handleMessage($trimmedLine, $this->sessionId);
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the official PHP MCP SDK.
5+
*
6+
* A collaboration between Symfony and the PHP Foundation.
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Mcp\Tests\Unit\Server\Transport;
13+
14+
use Mcp\Exception\InvalidArgumentException;
15+
use Mcp\Server\Transport\StdioTransport;
16+
use PHPUnit\Framework\Attributes\TestDox;
17+
use PHPUnit\Framework\TestCase;
18+
19+
final class StdioTransportTest extends TestCase
20+
{
21+
#[TestDox('a line exceeding the byte cap is discarded instead of buffered')]
22+
public function testOverlongLineIsDiscarded(): void
23+
{
24+
$messages = [];
25+
$transport = $this->createTransport(str_repeat('a', 100)."\n", $messages, maxLineBytes: 16);
26+
27+
$this->pumpToEof($transport);
28+
29+
self::assertSame([], $messages, 'the over-length line must never be dispatched');
30+
}
31+
32+
#[TestDox('processing resumes with the next line after an over-length line is discarded')]
33+
public function testRecoversAfterOverlongLine(): void
34+
{
35+
$messages = [];
36+
$transport = $this->createTransport(str_repeat('a', 100)."\n".'{"valid":1}'."\n", $messages, maxLineBytes: 16);
37+
38+
$this->pumpToEof($transport);
39+
40+
self::assertSame(['{"valid":1}'], $messages);
41+
}
42+
43+
#[TestDox('a normal line within the cap is dispatched')]
44+
public function testNormalLineIsDispatched(): void
45+
{
46+
$messages = [];
47+
$transport = $this->createTransport('{"jsonrpc":"2.0","id":1}'."\n", $messages);
48+
49+
$this->pumpToEof($transport);
50+
51+
self::assertSame(['{"jsonrpc":"2.0","id":1}'], $messages);
52+
}
53+
54+
#[TestDox('the line byte cap must be a positive number of bytes')]
55+
public function testRejectsNonPositiveCap(): void
56+
{
57+
$this->expectException(InvalidArgumentException::class);
58+
59+
new StdioTransport(input: $this->stream(''), output: $this->stream(''), maxLineBytes: 0);
60+
}
61+
62+
/**
63+
* @param list<string> $messages
64+
*/
65+
private function createTransport(string $input, array &$messages, int $maxLineBytes = 4 * 1024 * 1024): StdioTransport
66+
{
67+
$transport = new StdioTransport(
68+
input: $this->stream($input),
69+
output: $this->stream(''),
70+
maxLineBytes: $maxLineBytes,
71+
);
72+
73+
$transport->onMessage(static function ($transport, string $payload) use (&$messages): void {
74+
$messages[] = $payload;
75+
});
76+
77+
return $transport;
78+
}
79+
80+
/**
81+
* @return resource
82+
*/
83+
private function stream(string $contents)
84+
{
85+
$stream = fopen('php://temp', 'r+');
86+
fwrite($stream, $contents);
87+
rewind($stream);
88+
89+
return $stream;
90+
}
91+
92+
private function pumpToEof(StdioTransport $transport): void
93+
{
94+
$processInput = new \ReflectionMethod($transport, 'processInput');
95+
$input = (new \ReflectionProperty($transport, 'input'))->getValue($transport);
96+
97+
for ($i = 0; $i < 1000 && !feof($input); ++$i) {
98+
$processInput->invoke($transport);
99+
}
100+
}
101+
}

0 commit comments

Comments
 (0)