Skip to content

Bound exec-server JSON-RPC heap expansion#31807

Open
jif-oai wants to merge 2 commits into
mainfrom
jif/bound-exec-jsonrpc-decoding
Open

Bound exec-server JSON-RPC heap expansion#31807
jif-oai wants to merge 2 commits into
mainfrom
jif/bound-exec-jsonrpc-decoding

Conversation

@jif-oai

@jif-oai jif-oai commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Why

The exec-server transports cap serialized messages, but the shared JSON-RPC decoder could turn a small, valid message into a much larger heap allocation before method routing.

In a focused macOS parse using the production protocol type, the concrete reproduction was a notification whose params was a flat array of zeroes:

Measurement Result
Serialized message 4,194,303 bytes
Array elements 2,097,137
RSS before parsing 12,992 KiB
RSS after parsing 246,048 KiB
RSS increase 233,056 KiB / 56.9x

Each 0 costs about two bytes on the wire, but becomes a full serde_json::Value in memory. The untagged JSON-RPC envelope also needs an intermediate representation while it selects request, notification, response, or error. Method lookup happens later, so an unknown method does not protect the process. At the 64 MiB serialized ceiling, the same shape can require several GiB and terminate the orchestrator.

What changed

  • Decode every JSONRPCMessage through a shared, budgeted JSON-value visitor.
  • Stop decoding after 262,144 JSON values, before a compact array can grow into millions of heap objects.
  • Apply the same limit automatically to stdio, WebSocket, relay, and Noise because they all deserialize the shared protocol type.
  • Reject duplicate JSON object keys instead of changing typed envelope fields to last-wins behavior.
  • Limit same-version fs/readDirectory producers to 50,000 entries, keeping their responses below the shared decoder budget.
  • Preserve the existing untagged envelope selection after the bounded tree is built, so valid request, notification, response, and error messages keep the same wire shape.
  • Move request params into typed decoding instead of cloning the full JSON tree first.
  • Add the exact 4,194,303-byte reproduction and duplicate-key case as regression tests.

Why 262,144 values

The largest configured fs/walk response can contain 50,000 entries, which is roughly 150,000 JSON values before the small fixed envelope. A maximum same-version fs/readDirectory response contains about 200,004 values. A 262,144-value ceiling leaves headroom for both while putting the reproduction about 8x over budget.

This is a structural limit, not a byte limit. A large scalar string counts as one value, so base64 file payloads remain governed by the transport's serialized-message ceiling rather than this guard. The tests explicitly cover a scalar larger than 262,144 bytes.

Behavior and scope

Messages over the value budget or with duplicate object keys follow the existing malformed-message path. A same-version server returns a JSON-RPC error instead of producing an fs/readDirectory response with more than 50,000 entries. Serialization and the JSON-RPC wire schema do not change; this intentionally assumes the client and exec-server move together and adds no version negotiation.

This complements #31782: that PR bounds raw stdio framing at 64 MiB; this PR bounds the heap expansion that can happen inside an accepted frame. Existing Noise and WebSocket byte ceilings are unchanged.

This PR intentionally does not add method-specific limits beyond the producer guard above or a byte-weighted budget spanning every queued and in-flight RPC owner. That would require carrying ownership permits through notifications, pending responses, and typed decoding. It is a separate hardening layer; this change closes the demonstrated single-message structural amplification without introducing that machinery.

@jif-oai jif-oai requested a review from a team as a code owner July 9, 2026 15:05
// A maximum-size fs/walk response has at most 50,000 entries and needs roughly
// 150,000 JSON values. Keep ample headroom for legitimate protocol messages
// while preventing compact arrays from expanding into millions of heap values.
const MAX_JSONRPC_VALUE_NODES: usize = 256 * 1024;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex thinks process/read can return an unbounded number of (single byte even?) chunks inside the 1MB limit which would blow this up. Should be easy to rule out?

.map_err(map_fs_error)?
.map_err(map_fs_error)?;
let entry_count = entries.len();
if entry_count > MAX_READ_DIRECTORY_ENTRIES {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the math in the comment above is correct, when can we actually hit this path after a successful json parse? Maybe add a test if it's actually a useful guardrail?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants