Jens-G opened a new pull request, #3762: URL: https://github.com/apache/thrift/pull/3762
[THRIFT-5371](https://issues.apache.org/jira/browse/THRIFT-5371) `TFramedTransport::readFrame()` knows the exact size of the frame it hands to the protocol, but the budget the protocol is checked against stays at `TConfiguration::maxMessageSize` for the life of the connection. `updateKnownMessageSize()` had no caller anywhere in the C++ library: the mechanism was there and entirely unused. That has two consequences, and this closes both. **The one on the ticket.** The budget is a per-connection allowance that only `consume()` draws down and only `readEnd()` or `flush()` restores, so a long run of frames read without either exhausts it and starts refusing frames well within the limit. Sixty-four frames over a transport with room for any single one of them, but not for their sum, ends in `TTransportException: MaxMessageSize reached` — the symptom reported in 2021, still reproducible on master today. **The other direction.** A 68-byte frame could declare a 64 MB field, and `readStringBody()` would resize the caller's string to 64 MB before the following read discovered there was nothing behind it. A 5-byte frame could likewise declare an 8-million-element list. ### Change Three parts. - **`readFrame()` resets the budget and then binds it to the frame**, after the `maxFrameSize` check and before the buffer is allocated. The full reset first is not optional: `resetConsumedMessageSize()` refuses to grow a budget, so a frame larger than its predecessor would be rejected outright, and `updateKnownMessageSize()` on its own carries the previous frame's consumption forward. The reset belongs here rather than in `readEnd()` as the reporter suggested, because a oneway call never reaches `readEnd()`. - **`TBufferBase::read()` checks the budget against the bytes it can deliver** rather than the bytes asked for, for a transport that sets the new `budgetBoundToBuffer_`. `read()` may always return less than requested, so once the budget is one frame, an ordinary "give me up to N bytes" would otherwise be refused instead of short-read; `TransportTest` does exactly that with random chunk sizes. This does not loosen the bound the protocol is held to — every allocation-gating check, `readStringBody()` and the container element counts in all three protocols, calls `checkReadBytesAvailable()` directly with the size the wire declared and does not come through `read()`. - **`TNonblockingServer`** takes the frame apart itself and hands the payload to a `TMemoryBuffer`, with an identity transport factory by default, so the protocol reads straight from that buffer and none of the above reaches it. New public `TMemoryBuffer::bindMessageSizeToBuffer()` binds the budget to what the buffer holds, called at both `resetBuffer` sites. ### Scope `THeaderTransport` overrides `readFrame()` and so is unchanged; binding there is a separate question, since in unframed mode it hands out four bytes at a time. THRIFT-5464 is related but **not** closed by this. `TBufferBase::read()` still checks rather than decrements; what changes is that for a framed transport the budget it checks against is now one frame rather than the whole connection. ### Tests Twelve, written before the change. Five describe traffic that is legitimate today and pass unmodified; three fail unmodified — the field case asserts on the size of the string the protocol was asked to fill, because both outcomes raise `TTransportException(END_OF_FILE)` and only the allocation tells them apart. The remaining four cover the new `TMemoryBuffer` entry point. Against unmodified master the three failures are: ``` test_declared_field_larger_than_frame_is_rejected check str.size() < 1024u failed [67108864 >= 1024] test_declared_container_larger_than_frame_is_rejected exception TTransportException expected but not raised test_frame_budget_does_not_accumulate_across_frames TTransportException: MaxMessageSize reached ``` With the change, 12 of 12 pass and the full `UnitTests` suite is 101 cases with the one pre-existing `TServerSocketTest/test_bind_to_address` failure, which reproduces on unmodified master in the same container. ### Same defect elsewhere Java is [THRIFT-6165](https://issues.apache.org/jira/browse/THRIFT-6165), c_glib is [THRIFT-6166](https://issues.apache.org/jira/browse/THRIFT-6166). Separate PRs, one per binding. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
