Bound buffer writes in JSON assembly and staging#2024
Merged
Conversation
1f83033 to
8819aa1
Compare
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
8819aa1 to
7f30491
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2017
Description
This change hardens buffer overflow protection in the MCP HTTP binding's JSON response assembly and reply staging logic by:
Adding bounds checking to JSON string escaping: Modified
putEscaped()to stop writing escape sequences before they would overflow the destination buffer's capacity. AddedputBounded()helper for single-byte writes that respect buffer limits, andescapedWidth()to predict escape sequence lengths. This prevents pathological inputs (e.g., control characters that expand 6x when escaped) from overflowing fixed-size buffers.Making staging operations return success/failure: Changed
stage()andstageBytes()methods to returnbooleanindicating 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.Propagating traceId through staging calls: Updated all staging methods to accept
traceIdparameter so they can properly callcleanup(traceId)when the pool is exhausted, ensuring correct resource cleanup and stream termination.Updating JSON generator plain write methods: Modified
write(CharSequence),writeKey(CharSequence), andwriteNumber(CharSequence)overloads to route through the bounded/Completion path, allowing them to truncate cleanly and report partial consumption viaconsumed()instead of overflowing.Adding comprehensive unit tests: New test class
McpHttpProxyFactoryTestvalidates 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
McpHttpProxyFactoryTestcovering:Existing JSON generator tests updated to verify partial consumption reporting and resumable plain writes.
https://claude.ai/code/session_01Nf6gRQmSySSoDLB9pcRW5M