THRIFT-6142: Enforce Ruby unframed HeaderTransport limits - #3706
Open
kpumuk wants to merge 1 commit into
Open
Conversation
Client: rb Co-Authored-By: OpenAI Codex (GPT-5.6) <codex@openai.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes a gap in Ruby HeaderTransport where unframed Binary/Compact reads bypassed max_frame_size, adding byte accounting and size-limit enforcement for unframed messages (including protocol signatures) and resetting budgets at message boundaries.
Changes:
- Track and enforce unframed message byte budgets in
HeaderTransport, raisingTransportException::SIZE_LIMITbefore exceedingmax_frame_size. - Reset unframed size budgets at message boundaries by notifying transports from pure-Ruby and native protocol
read_message_begin. - Add specs covering exact-limit acceptance, over-limit rejection, signature handling, partial reads, and sequential message budgeting (incl. accelerated binary).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/rb/spec/header_transport_spec.rb | Adds tests validating unframed size-limit enforcement and per-message budgeting. |
| lib/rb/lib/thrift/transport/header_transport.rb | Implements unframed byte accounting, limit checks, and message-boundary resets. |
| lib/rb/lib/thrift/protocol/compact_protocol.rb | Notifies transports of new-message boundaries to reset unframed budgets. |
| lib/rb/lib/thrift/protocol/binary_protocol.rb | Notifies transports of new-message boundaries to reset unframed budgets. |
| lib/rb/ext/thrift_native.c | Interns reset_message_size method ID for native protocol boundary notifications. |
| lib/rb/ext/constants.h | Exposes reset_message_size method ID for native extension usage. |
| lib/rb/ext/compact_protocol.c | Calls reset_message_size at message start in native compact reader. |
| lib/rb/ext/binary_protocol_accelerated.c | Calls reset_message_size at message start in accelerated binary reader. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+360
to
+366
| def read_unframed(size) | ||
| raise_unframed_size_limit if @unframed_bytes_read + size > @max_frame_size | ||
|
|
||
| data = @transport.read(size) | ||
| @unframed_bytes_read += data.bytesize | ||
| data | ||
| end |
Comment on lines
+214
to
+218
| expect { read_unframed_message(protocol) }.to raise_error( | ||
| Thrift::TransportException, | ||
| "Unframed message size exceeds maximum #{payload.bytesize - 1}" | ||
| ) do |error| | ||
| expect(error.type).to eq(Thrift::TransportException::SIZE_LIMIT) |
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.
Ruby HeaderTransport enforced
max_frame_sizefor Header and framed clients but passed unframed Binary and Compact reads directly to the underlying transport without applying the configured limit.This change counts bytes consumed by each unframed protocol message, including the initial protocol signature, and raises
TransportException::SIZE_LIMITbefore a read would exceed the configured maximum. Pure-Ruby and native protocol readers notify compatible transports at message boundaries so sequential messages receive independent budgets. Exact-limit messages and partial underlying reads remain supported.Benchmarks
Ruby 4.0.6 on aarch64 Linux, seven warmed trials per revision.
The repository protocol benchmark was run with and without the native extension:
A focused worst-case control decoded 100,000 sequential minimal unframed messages per trial through
HeaderTransport, using 19-byte Binary and 11-byte Compact messages. Pure mode usedruby -Ilib -rthrift; native mode added-Iextand usedBinaryProtocolAcceleratedfor the Binary reader.The focused control intentionally maximizes fixed per-message accounting overhead and uses an in-memory transport, so it does not include network or application work. The repository Header scenarios remain within trial noise.
[skip ci]anywhere in the commit message to free up build resources.