fix(transport): bound the incoming line buffer in AsyncRwTransport

[onatozmenn]

Fixes #1030

Motivation and Context

AsyncRwTransport::receive reads a line with read_until, which has no ceiling. A peer that never sends a newline makes line_buf grow until the process runs out of memory. That read path backs every stdio server built on rmcp::transport::io::stdio() and every client using TokioChildProcess, so one unterminated line from an untrusted peer is enough to take the process down.

JsonRpcMessageCodec already has max_length and enforces it correctly, but that code lives in Decoder::decode and this transport never calls the decoder, so the check was unreachable from here.

How

The limit can't be bolted onto read_until. It doesn't return until it hits a delimiter or EOF, so by the time you could look at the length the memory is already committed. receive now reads through fill_buf/consume, which lets the limit be checked before each append.

An oversized line is dropped and logged and the connection stays open, which is what the codec's decoder already does with is_discarding rather than tearing the session down.

The default is 16 MiB. I picked that to match DEFAULT_MAX_SSE_EVENT_SIZE, the bound the streamable HTTP client already applies to a single SSE event, so a payload that's fine over HTTP is also fine over stdio and callers sending large embedded resources don't get broken by the bound appearing. with_max_line_length overrides it, and usize::MAX gives back the old unbounded behaviour.

Cancellation safety is preserved, which matters because #947 depends on it. fill_buf consumes nothing if the future is dropped, and the copy into line_buf and the matching consume have no await between them, so a partially read line still survives to the next receive call.

How Has This Been Tested?

New crates/rmcp/tests/test_async_rw_max_line_length.rs, 7 tests:

• an oversized but valid message is dropped and the next one still arrives. Valid is the important part: unparsable junk gets discarded by the existing parse error handling either way, so only a well formed oversized message tells a bounded read apart from an unbounded one
• unbounded_limit_still_delivers_an_oversized_message is the control for that. With usize::MAX the exact same message comes through, so the assertion above is really testing the bound and not something incidental
• the shape from the report: 128 KiB with no newline anywhere, then a real message, which has to arrive
• a message right at the limit is still delivered
• a 1 MiB message passes on the default limit
• a line arriving in 97 byte chunks is reassembled, which guards the read_until to fill_buf swap
• two oversized messages back to back don't wedge the transport

Also run on Rust 1.96, the pinned toolchain:

• test_stdio_response_concurrency, 2 passed. That's the fix(transport): make AsyncRwTransport::receive cancel-safe (#941) #947 cancellation regression, and it's the one I most wanted to see stay green after touching this loop
• the CI "no local feature" job command. Everything green except test_with_js, which fails in my container only because there's no Node in it
• cargo clippy --all-targets --all-features -- -D warnings, clean
• cargo fmt clean on both changed files

Breaking Changes

No API break. It is a behaviour change though: a single message over 16 MiB used to be delivered and is now dropped. Anyone relying on that can call .with_max_line_length(usize::MAX).

If you'd rather have a different default, or would rather the bound be opt-in for now, that's an easy change. 16 MiB was the most defensible number I could find inside the repo rather than one I made up.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

_{Investigated and prepared with AI assistance.}

[alexhancock]

Could we just wrap the existing read in a FramedRead?

FramedRead::new(read, JsonRpcMessageCodec::new_with_max_length(DEFAULT_MAX_LINE_LENGTH))

https://docs.rs/tokio-util/latest/tokio_util/codec/struct.FramedRead.html

[onatozmenn]

I tried it, and it does not hold up here, though not for a reason that is visible from the outside. Writing up what I found since it took a while to pin down.

I swapped the whole hand-rolled read for exactly your line, deleted read_line and the LineRead/Step enums, and had receive consume self.read.next(). It compiles and the diff is much nicer. Then three of the new tests fail and two of the tests already on main hang.

1. FramedRead emits a None after a decoder error.

Small probe against the real codec, oversized line followed by a good one:

poll 0: Err(max line length exceeded)
poll 1: None
poll 2: Ok(id=2)
poll 3: None

receive reads None as EOF, so the first oversized line would close the session rather than be skipped. That is worse than the bug I am fixing: one long line from a peer takes the connection down instead of just being dropped.

I worked around that with a flag on the transport so the None straight after an error counts as a pause instead of EOF. The three new tests went green, and then the existing ones surfaced the real problem.

2. After an error, FramedRead will not decode what is already sitting in its buffer.

The pause path clears is_readable, so the poll after the error goes back to the IO instead of calling decode again. If the bad line and the next message arrived in the same write, that next message just sits in the buffer until the peer happens to send something else.

receive_ignores_parse_error and receive_responds_to_protocol_error catch this. Both are already on main, and both hang under the FramedRead version. They write

not json\n{"jsonrpc":"2.0","method":"notifications/initialized"}\n

in a single write and expect receive to skip the first line and return the second. With FramedRead it waits forever, because nothing else is ever sent.

Why it bites here in particular

JsonRpcMessageCodec reports a bad frame through Decoder::Error, and this transport's contract is to drop that one frame and carry straight on with whatever is already buffered. FramedRead treats a decoder error as a stream level event. Those two do not compose, and I suspect that is the actual reason receive hand-rolls its read loop today instead of reusing the decoder sitting right next to it.

So fill_buf/consume is not me preferring the manual version. It is the only way I found to apply the cap before the memory is committed while keeping "drop the frame, keep the connection, keep the buffer".

If you do want the duplication gone

The clean way would be to make the codec's Decoder::Item a Result instead of routing parse failures through Decoder::Error. Then FramedRead never errors, never pauses, never stops mid buffer, and receive collapses into a thin match. That is a breaking change to JsonRpcMessageCodec though, which felt like the wrong thing to bundle into a memory-exhaustion fix. Happy to open it separately if you like the shape.

I have left this PR as it was. If you would rather I push the FramedRead version so you can see the failures for yourself, I can put it on a branch.