Skip to content

Bound buffer writes in JSON assembly and staging#2024

Merged
jfallows merged 1 commit into
developfrom
claude/github-issue-2017-u1jsid
Jul 5, 2026
Merged

Bound buffer writes in JSON assembly and staging#2024
jfallows merged 1 commit into
developfrom
claude/github-issue-2017-u1jsid

Conversation

@jfallows

@jfallows jfallows commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #2017

Description

This change hardens buffer overflow protection in the MCP HTTP binding's JSON response assembly and reply staging logic by:

  1. Adding bounds checking to JSON string escaping: Modified putEscaped() to stop writing escape sequences before they would overflow the destination buffer's capacity. Added putBounded() helper for single-byte writes that respect buffer limits, and escapedWidth() to predict escape sequence lengths. This prevents pathological inputs (e.g., control characters that expand 6x when escaped) from overflowing fixed-size buffers.

  2. Making staging operations return success/failure: Changed stage() and stageBytes() methods to return boolean indicating whether bytes were successfully staged or if the pooled buffer was exhausted. Callers now check the return value and stop staging further fragments once a write fails, preventing cascading writes to a released slot.

  3. Propagating traceId through staging calls: Updated all staging methods to accept traceId parameter so they can properly call cleanup(traceId) when the pool is exhausted, ensuring correct resource cleanup and stream termination.

  4. Updating JSON generator plain write methods: Modified write(CharSequence), writeKey(CharSequence), and writeNumber(CharSequence) overloads to route through the bounded/Completion path, allowing them to truncate cleanly and report partial consumption via consumed() instead of overflowing.

  5. Adding comprehensive unit tests: New test class McpHttpProxyFactoryTest validates that buffer writes stop at capacity boundaries, escape sequences don't split across limits, and partial writes can be resumed.

The changes ensure that even when upstream responses or tool outputs contain pathological content, the binding cannot overflow its fixed-size buffers—writes simply truncate at the boundary and the stream terminates cleanly.

Test Plan

Added unit tests in McpHttpProxyFactoryTest covering:

  • Buffer overflow prevention when escaped input exceeds capacity
  • Correct escaping when output fits capacity
  • Stopping at capacity boundary without splitting escape sequences
  • Bounded single-byte writes
  • Escaped width calculations

Existing JSON generator tests updated to verify partial consumption reporting and resumable plain writes.

https://claude.ai/code/session_01Nf6gRQmSySSoDLB9pcRW5M

@jfallows jfallows force-pushed the claude/github-issue-2017-u1jsid branch from 1f83033 to 8819aa1 Compare July 5, 2026 18:45
JsonGeneratorImpl's convenience write methods (write(CharSequence),
writeKey(CharSequence), writeNumber(CharSequence)) previously wrote through
an unbounded path with no real protection against exceeding the wrapped
limit: putRaw's only guard was a Java assert, which compiles out in a
packaged production build (no -ea), so oversized input would silently
overflow past the intended write region.

Fix by routing these plain overloads through their existing
(value, Completion.COMPLETE) counterparts, which already emit only as much
as fits the output bound -- checking each code point's escaped width before
committing to it, so no UTF-8 char or escape sequence is ever split -- and
report the true count via consumed(). A value too long for the wrapped limit
now truncates cleanly and leaves pending open (exactly like
Completion.INCOMPLETE) instead of overflowing; the caller can detect this
via consumed() and resume with the remainder, the same contract already
used by the SEGMENTABLE JsonSink delivery path.

putRaw/copy/wrap/writeRaw keep their original assert-only checks: these
verify invariants the calling code's own structure is responsible for
(buffer bounds, verbatim-block sizing, structural byte counts), not
attacker-length-dependent content, so they stay free in production and are
verified under -ea (Surefire's default) rather than paying a per-byte tax
on the hottest path in the file.

Delete the now-redundant unbounded writeString/writeStringBody(value)
helpers superseded by the bounded path.

Fixes #2017
@jfallows jfallows force-pushed the claude/github-issue-2017-u1jsid branch from 8819aa1 to 7f30491 Compare July 5, 2026 18:48

@jfallows jfallows left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LGTM

@jfallows jfallows merged commit 6fce57d into develop Jul 5, 2026
3 checks passed
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.

common-json: JsonGeneratorEx's convenience write API has no real bounds check against its wrap() limit

2 participants