Repository navigation
Bound batch decode by item limit (PLT-820) - #98
Conversation
parseMessage decoded every element of a batch array before the item limit was checked in handleBatch, so a compact 10 MiB array of scalars expanded into one jsonrpcMessage per element and could lead to hundreds of MiB of heap per request. Push the limit into parseMessage, the one function both read paths pass through, and stop the decode one element past it. The handleBatch count check and the WS aggregate byte admission remain as defense in depth.
amir-deris
changed the title
PR SummaryMedium Risk Overview
Observable behavior: “batch too large” errors may use a null id when the only call sits past the decoded prefix; batches of exactly the limit are still served. Docs for Adds unit, allocation, HTTP/WS integration tests and testdata for truncation and at-limit batches. Reviewed by Cursor Bugbot for commit 4634ec8. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sound, well-scoped fix: pushing the batch item limit into parseMessage genuinely bounds the per-element jsonrpcMessage allocation on both the WS and HTTP read paths, and the deliberate "decode one past the limit" overshoot correctly preserves handleBatch's len(msgs) > limit rejection. No blockers; the notes below are a documented behavior change to the batch-too-large error id, the pre-existing decorated-codec gap Codex flagged, and some test-coverage gaps.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor review pass (
cursor-review.md) produced no output — that file is empty, so this synthesis merges only my own findings with Codex's single finding.REVIEW_GUIDELINES.mdis also empty, so no repo-specific standards were applied. - Test-coverage gap: neither
TestHTTPOversizeBatchRejectednorTestWSOversizeBatchRejectedAndConnectionSurvivesactually proves the fix. Both assert thebatch too largerejection, which the code already produced before this change (viahandleBatch). The only real regression guard isTestParseMessageBatchAllocationsBounded, which callsparseMessagedirectly and so does not cover the codec wiring. Consider an end-to-end assertion — e.g. assertcodec.(*jsonCodec).batchItemLimit() != 0afterserveSingleRequestwires the handler, or wrap the HTTP/WS assertions intesting.AllocsPerRun. Without that, a future change that dropsattachHandlerfromserveSingleRequest(server.go:226) would silently reintroduce the unbounded decode with every test still green. - No test covers the changed
respondWithBatchTooLargeid behavior (a batch whose firstitemLimit+1decoded elements are all notifications). Worth a table entry so the tradeoff is pinned down rather than only described in a comment. WithBatchItemLimit's doc comment (rpc/client_opt.go:130-133) says the option "does not affect batch requests sent by the client." That was already inaccurate —handleBatchappliedbatchRequestLimitto inbound batch responses too — and this PR makes the coupling tighter by also truncating the decode of those responses. Behavior is unchanged (both before and after, an over-limit response batch is rejected rather than dispatched), so this is not a regression, but the doc comment is now more misleading and is worth correcting while you are here.- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| // what handleBatch's own count check reads to reject the batch, and decoding | ||
| // past it would allocate a jsonrpcMessage per element of an array the server | ||
| // has already decided not to serve. | ||
| if itemLimit > 0 && len(msgs) > itemLimit { |
There was a problem hiding this comment.
[suggestion] The > itemLimit (rather than >= itemLimit) overshoot is correct and necessary — it is what keeps handleBatch's len(msgs) > h.batchRequestLimit check (handler.go:317) firing — but the correctness of this line now depends on a strict-> comparison in a different file. If anyone ever tightens handler.go:317 to >=, an over-limit batch would be silently truncated to itemLimit elements and executed rather than rejected, which is a far worse failure than the one being fixed here.
The comment explains the why but doesn't guard it. Two cheap options: reference handler.go's check explicitly by name in the comment, or make the contract explicit by having parseMessage return a truncated bool that handleBatch treats as an unconditional reject. Either removes the silent-execution failure mode.
(The limit source itself is safe by construction — jsonCodec.batchItemLimit() reads c.handler.batchRequestLimit from the very same handler that performs the check — so the two values can never diverge. It is only the comparison operator that is load-bearing across files.)
| // This is the best we can do, given that the protocol doesn't have a way | ||
| // of reporting an error for the entire batch. | ||
| // of reporting an error for the entire batch. The batch is only decoded up to | ||
| // the item limit, so a batch whose every decoded element is a notification is |
There was a problem hiding this comment.
[suggestion] Worth calling out in the PR description, not just this comment: this is a user-visible protocol regression, and the consequence is a client hang rather than a cosmetic id change.
For a batch whose first itemLimit+1 elements are all notifications and whose later elements include a call, the error response now carries a null id. On the receiving side, handleResponses matches by h.respWait[string(msg.ID)] (handler.go:551); a null id matches nothing, so the response is logged as "Unsolicited RPC response" and the caller's requestOp blocks until its context deadline instead of getting an immediate batch too large error.
Impact is limited in practice — geth's own BatchCallContext assigns an id to every element, so it can't hit this — but a third-party client that front-loads notifications can. The tradeoff looks acceptable given the memory bound it buys; please just make it explicit in the description/changelog rather than only in a code comment, and consider a table case in TestParseMessageBatchItemLimit pinning the behavior.
| // attachHandler wires h into codec so reads on it observe the handler's limits. A codec | ||
| // that does not implement handlerSetter reads unlimited, and the handler's own checks | ||
| // remain the backstop. | ||
| func attachHandler(codec ServerCodec, h *handler) { |
There was a problem hiding this comment.
[suggestion] Raised by Codex, kept with a lower severity. attachHandler's type assertion only matches codecs implementing the unexported handlerSetter, so a ServerCodec decorator that forwards readBatch without promoting setHandler leaves the wrapped jsonCodec.handler nil, batchItemLimit() returns 0, and the full batch is decoded and allocated before handleBatch rejects it.
Two things temper this relative to Codex's "Medium":
- It is pre-existing, not introduced here —
attachBudgetHandlerhad exactly the same gap, and the doc comment you added on this function states the fallback explicitly. - Nothing in-tree wraps
ServerCodec; the only decorator is thepassthroughCodeclocal type inws_admission_test.go. Both built-in codecs are covered (websocketCodecembeds*jsonCodec, sosetHandleris promoted).
So the exposure is external embedders only, and the handler-level check still rejects the batch — the DoS is degraded to "allocation happens, then rejection," not "batch executes." Still, since bounded decoding is now a security property rather than just a budgeting optimization, it's worth making it not depend on a private-interface assertion — e.g. plumb the item limit through codec construction, or fall back to a package-level default in parseMessage when no handler is wired.
| httpsrv := httptest.NewServer(srv) | ||
| defer httpsrv.Close() | ||
|
|
||
| resp, err := http.Post(httpsrv.URL, "application/json", strings.NewReader(makeCallBatch(50000))) |
There was a problem hiding this comment.
[nit] makeCallBatch(50000) builds a ~3.3MB request body, uncomfortably close to defaultBodyLimit (5MiB, rpc/http.go:36) — if the element template ever grows, this test starts failing with a 413 for reasons unrelated to what it's asserting. It also costs 50k fmt.Sprintf calls plus the join on every run.
Since the server only ever decodes itemLimit+1 = 5 elements, a few hundred elements would exercise the identical path. Suggest dropping the count substantially, or adding a comment noting the deliberate margin against the body limit.
|
|
||
| // TestHTTPOversizeBatchRejected covers the single-request path, which builds its handler | ||
| // separately from ServeCodec and so wires the codec up on its own. | ||
| func TestHTTPOversizeBatchRejected(t *testing.T) { |
There was a problem hiding this comment.
Assert the codec actually observed the limit? Same for TestWSOversizeBatchRejectedAndConnectionSurvive test.
| // TestParseMessageBatchAllocationsBounded is the regression guard for the report: a | ||
| // compact array of scalars must not allocate a jsonrpcMessage per element before the | ||
| // item limit is consulted. | ||
| // It cannot run in parallel: testing.AllocsPerRun panics in a parallel test. |
There was a problem hiding this comment.
Maybe time to upgrade the go module?
There was a problem hiding this comment.
Actually upgrading go.mod does not remove the need to keep this test non-parallel.
If this test calls t.Parallel():
- in go version 1.23 / 1.24 => No panic — results can be flaky
- 1.25+ =>
AllocsPerRuncan panic if other parallel tests in the package are actively running — and rpc package has manyt.Parallel()tests.
Review feedback: the HTTP and WS tests asserted only the "batch too large" rejection, which handleBatch already produced before the item limit reached parseMessage, so neither covered the codec wiring. Both now send a probe batch whose only call sits just past the decoded prefix. respondWithBatchTooLarge reports the first decoded call's id, so a null id is positive evidence the codec observed the limit; a paired case with a leading call still expects its id, so the null id reads as truncation rather than ids going missing. Pin the cross-file contract the overshoot depends on: parseMessage must return itemLimit+1 elements and handleBatch must reject on a strict '>'. Neither side was covered, so tightening either would have gone unnoticed. Add a reqresp-batch.js case for a batch of exactly the limit, an invalid-batch-toolarge.js case for the null-id behavior, and name both in the parseMessage comment. Also shrink makeCallBatch's element count, which built a ~3.3MB body against a 5MiB defaultBodyLimit for a path that only decodes itemLimit+1 elements, and correct WithBatchItemLimit's doc comment: the limit applies to inbound batches, including batched responses to the client's own requests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
The fix is sound: parseMessage now stops one element past the item limit, which correctly preserves handleBatch's strict len(msgs) > limit rejection while bounding per-element allocations, and it's wired into every built-in codec path (WS, IPC/stdio via ServeCodec, and HTTP via the new attachHandler in serveSingleRequest). No blocking issues found; a few non-blocking notes on latent budget coupling on the HTTP path, allocation-test robustness, and the unexported setHandler extension point.
Findings: 0 blocking | 7 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Observable protocol change: the
batch too largeerror can now carry a nullidwhere it previously carried the id of a call appearing later in the batch (only when the firstitemLimit+1elements are all notifications). This is documented inhandler.goand pinned intestdata/invalid-batch-toolarge.js, and the common all-calls batch is unaffected, but it is a divergence from upstream go-ethereum worth calling out in the PR description / release notes. - The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only my findings with Codex's.REVIEW_GUIDELINES.mdis also empty, so no repo-specific standards were applied. - Coverage note: the new tests cover WS and HTTP, but there is no direct test that the IPC/stdio
ServeCodecpath (plainNewCodec) is bounded. It is covered transitively byTestParseMessageBatchItemLimitplus theattachHandlerwiring, so this is optional. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| h := newHandler(ctx, codec, s.idgen, &s.services, s.batchItemLimit, s.batchResponseLimit, nil, s.readLimit, nil, s.wsAdmissionTimeout) | ||
| h.allowSubscribe = false | ||
| attachHandler(codec, h) |
There was a problem hiding this comment.
[suggestion] Attaching the handler here also switches on the other behavior c.handler != nil gates in jsonCodec.readBatch: the acquirePreDecode call. Today that is inert because serveSingleRequest passes a nil wsConcurrentBudget (acquirePreDecode returns early without setting preDecodeHeld), so this is safe as written.
It is a latent trap though: nothing on the HTTP single-request path ever calls commitFrameBudget or releasePreDecode after a successful read, so the moment a non-nil budget is passed to this newHandler call, every HTTP request would permanently leak readLimit bytes of semaphore weight. Worth a short comment here (or a defer h.releasePreDecode()) recording that the nil budget is load-bearing.
| ) | ||
| raw := json.RawMessage(makeScalarBatch(elements)) | ||
|
|
||
| allocs := testing.AllocsPerRun(2, func() { |
There was a problem hiding this comment.
[suggestion] testing.AllocsPerRun measures runtime.MemStats.Mallocs, which is process-wide, not per-goroutine. Package rpc leaves background goroutines alive from earlier tests (WS ping loops, httptest servers, handler call goroutines), and any allocation they make during the measurement window is attributed to parseMessage.
The 1000-alloc ceiling against an expected ~100-300 gives decent headroom, so this probably won't flake often — but when it does the failure will be confusing and unrelated to this code. Consider making it explicitly differential instead, e.g. measure parseMessage(smallBatch, itemLimit) as a baseline in the same run and assert the 200k-element case is within a small multiple, so ambient allocations cancel out.
| // handlerSetter is implemented by codecs that need the handler wired in to enforce its | ||
| // read limits (jsonCodec and, via embedding, websocketCodec). | ||
| type handlerSetter interface { | ||
| setHandler(h *handler) |
There was a problem hiding this comment.
[nit] Raised by the Codex pass, and it's a fair point though I'd rank it below blocking: setHandler is unexported, so a ServerCodec decorator defined outside package rpc (embedding the result of NewCodec and forwarding readBatch) cannot satisfy handlerSetter, and those deployments keep the unbounded decode this PR is fixing.
Two things soften it: the shape is pre-existing (it was setBudgetHandler before this rename), and the handler-side len(msgs) > batchRequestLimit check still rejects the batch — only the allocation bound is lost, not the limit itself. There are also no such wrappers in this repo. The doc comment below already states the fallback accurately; the only thing I'd add is that this now silently downgrades a security bound rather than just a performance one, so an exported opt-in (or documenting ServerCodec decorators as unsupported) would be worth a follow-up.
| // batch requests sent by the client. | ||
| // Note: this option applies to batches the client receives: both batch requests sent by | ||
| // the server on a bidirectional connection and batched responses to the client's own | ||
| // requests. A batch with more items than the limit is rejected instead of dispatched, and |
There was a problem hiding this comment.
[nit] "rejected instead of dispatched" undersells what happens for the response case: when the over-limit batch is a batch of responses to the client's own calls, handleBatch routes to respondWithBatchTooLarge, which writes a batch too large error frame back to the peer while the pending BatchElems all fail with ErrMissingBatchResponse. Worth naming that outcome explicitly so callers know setting this option can make their own batch calls fail rather than just protecting them.
3 participants
Summary
parseMessagedecoded every batch element beforehandleBatchchecked the item limit, so a compact array of scalars could allocate hundreds of MiB per request. The limit is now enforced insideparseMessage(the single decode path for HTTP and WS), stopping one element past the limit.The overshoot is deliberate and load-bearing:
handleBatchrejects onlen(msgs) > h.batchRequestLimit, so returning exactlyitemLimitelements would make an over-limit batch look in-limit and be executed.parseMessage's break and that comparison must stay in agreement; both directions are now pinned by tests (see below).Behavior change: null error id for notification-front-loaded batches
Because only the first
itemLimit+1elements are decoded, a call that appears after that prefix can no longer contribute itsidto thebatch too largeerror. Such a batch is now answered with a null id where it previously carried the id of the first call anywhere in the array.This is user-visible, and for a client it is a hang rather than a cosmetic difference: a null id matches nothing in
h.respWait(rpc/handler.go:551), so the response is logged as "Unsolicited RPC response" and the caller'srequestOpblocks until its context deadline instead of getting an immediatebatch too largeerror.Impact in practice is narrow. Geth's own
BatchCallContextassigns an id to every element, so in-tree callers cannot hit it; it requires a third-party client that front-loads more thanitemLimitnotifications ahead of its first call. Accepted as the cost of bounding the allocation, and pinned by a case intestdata/invalid-batch-toolarge.jsrather than left to a code comment.WithBatchItemLimit's doc comment is corrected while here — it claimed the option did not affect batches on the client side, which was already inaccurate (handleBatchapplies the limit to inbound response batches too) and is more misleading now that the limit also truncates their decode.Test plan
parseMessagelevel:makeTruncationProbesends a batch whose only call sits just past the decoded prefix, so a null error id is positive evidence that the codec passed the limit intoparseMessage. Thebatch too largerejection alone proves nothing — the handler already produced it before this change.attachHandlerfromserveSingleRequest-> HTTP probe reports id 99parseMessagebreaking at the limit instead of one past ->invalid-batch-toolarge.jssees the batch executed (the silent-execution failure mode)handleBatchtightened to>=->reqresp-batch.jssees an exactly-at-limit batch rejectedKnown gap (pre-existing, not addressed here)
attachHandlerwires the codec through an assertion on the unexportedhandlerSetterinterface, so an externalServerCodecdecorator that forwardsreadBatchwithout promotingsetHandlerleavesbatchItemLimit()at 0 and decodes unbounded. This gap is identical inattachBudgetHandlerbefore this PR, nothing in-tree wrapsServerCodec(both built-in codecs are covered —websocketCodecembeds*jsonCodec), and the handler-level check still rejects the batch, so the exposure degrades to "allocate, then reject" for external embedders. Worth plumbing the limit through codec construction as a follow-up now that bounded decoding is a security property rather than a budgeting optimization.