You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "kpumuk (via GitHub)" <gi...@apache.org> on 2026/08/05 15:38:45 UTC
[PR] THRIFT-6142: Enforce Ruby unframed HeaderTransport limits [thrift]
kpumuk opened a new pull request, #3706:
URL: https://github.com/apache/thrift/pull/3706
<!-- Explain the changes in the pull request below: -->
Ruby HeaderTransport enforced `max_frame_size` for 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_LIMIT` before 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:
```sh
ruby test/rb/benchmarks/protocol_benchmark.rb \
--small-runs 10000 \
--scenarios hdr-bin-read-small,hdr-cmp-read-small \
--json
THRIFT_BENCHMARK_SKIP_NATIVE=1 ruby \
test/rb/benchmarks/protocol_benchmark.rb \
--small-runs 10000 \
--scenarios hdr-bin-read-small,hdr-cmp-read-small \
--json
```
| Scenario | master median (range) | this change median (range) | Delta |
| --- | ---: | ---: | ---: |
| Pure Header Binary | 0.292063 s (0.289148–0.294775) | 0.293295 s (0.287210–0.299563) | +0.42% |
| Pure Header Compact | 0.283662 s (0.279885–0.296288) | 0.282497 s (0.277990–0.292476) | -0.41% |
| Native-loaded Header Binary | 0.241813 s (0.238484–0.245131) | 0.242277 s (0.238585–0.254646) | +0.19% |
| Native-loaded Header Compact | 0.185282 s (0.184773–0.189132) | 0.183141 s (0.182184–0.193690) | -1.16% |
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 used `ruby -Ilib -rthrift`; native mode added `-Iext` and used `BinaryProtocolAccelerated` for the Binary reader.
| Scenario | master median (range) | this change median (range) | Delta |
| --- | ---: | ---: | ---: |
| Pure unframed Binary | 0.517964 s (0.510047–0.532928) | 0.544925 s (0.535836–0.550106) | +5.21% |
| Pure unframed Compact | 0.482962 s (0.481741–0.491921) | 0.522609 s (0.517260–0.526999) | +8.21% |
| Native accelerated unframed Binary | 0.462682 s (0.459623–0.478828) | 0.488974 s (0.486336–0.506783) | +5.68% |
| Native unframed Compact | 0.413511 s (0.408578–0.423356) | 0.452232 s (0.447174–0.465880) | +9.36% |
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.
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/browse/THRIFT-6142) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6142: Enforce Ruby unframed HeaderTransport limits [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3706:
URL: https://github.com/apache/thrift/pull/3706#discussion_r3745610288
##########
lib/rb/lib/thrift/transport/header_transport.rb:
##########
@@ -336,15 +345,33 @@ def read_frame(req_sz)
# Handles unframed protocol - puts first_word back in buffer
def handle_unframed(first_word, req_sz)
+ @unframed_bytes_read = first_word.bytesize
+ raise_unframed_size_limit if @unframed_bytes_read > @max_frame_size
+
bytes_left = req_sz - 4
if bytes_left > 0
- rest = @transport.read(bytes_left)
+ rest = read_unframed(bytes_left)
@read_buffer = StringIO.new(first_word + rest)
else
@read_buffer = StringIO.new(first_word)
end
end
+ 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
Review Comment:
Addressed
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6142: Enforce Ruby unframed HeaderTransport limits [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3706:
URL: https://github.com/apache/thrift/pull/3706#discussion_r3722055302
##########
lib/rb/lib/thrift/transport/header_transport.rb:
##########
@@ -336,15 +345,33 @@ def read_frame(req_sz)
# Handles unframed protocol - puts first_word back in buffer
def handle_unframed(first_word, req_sz)
+ @unframed_bytes_read = first_word.bytesize
+ raise_unframed_size_limit if @unframed_bytes_read > @max_frame_size
+
bytes_left = req_sz - 4
if bytes_left > 0
- rest = @transport.read(bytes_left)
+ rest = read_unframed(bytes_left)
@read_buffer = StringIO.new(first_word + rest)
else
@read_buffer = StringIO.new(first_word)
end
end
+ 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
Review Comment:
`read_unframed` pre-checks the limit using the *requested* `size`, not the number of bytes actually returned. For transports that can return short reads, this can raise `SIZE_LIMIT` even when the read would not exceed the configured maximum (and conflicts with the intent to support partial underlying reads). A safer approach is to compute remaining budget (`@max_frame_size - @unframed_bytes_read`), raise only when remaining is <= 0, and cap the `read` call to `min(size, remaining)` so the transport can never return more bytes than allowed; then increment by `data.bytesize`.
##########
lib/rb/spec/header_transport_spec.rb:
##########
@@ -178,6 +196,85 @@ def framed(message)
@trans.write("12345")
expect { @trans.flush }.to raise_error(Thrift::TransportException, /frame that is too large/)
end
+
+ {
+ "binary" => Thrift::BinaryProtocol,
+ "compact" => Thrift::CompactProtocol
+ }.each do |protocol_name, protocol_class|
+ it "enforces max frame size for unframed #{protocol_name} messages" do
+ payload = unframed_message(protocol_class)
+
+ exact_limit = Thrift::HeaderTransport.new(Thrift::MemoryBufferTransport.new(payload))
+ exact_limit.set_max_frame_size(payload.bytesize)
+ expect(read_unframed_message(Thrift::HeaderProtocol.new(exact_limit))).to eq("legacy_unframed")
+
+ over_limit = Thrift::HeaderTransport.new(Thrift::MemoryBufferTransport.new(payload))
+ over_limit.set_max_frame_size(payload.bytesize - 1)
+ protocol = Thrift::HeaderProtocol.new(over_limit)
+ 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)
Review Comment:
These assertions match the full exception message string, which makes the spec brittle to minor message wording changes. Consider matching with a regex (or only asserting on `error.type` plus that the message includes the limit value) to keep the test focused on behavior rather than exact phrasing.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6142: Enforce Ruby unframed HeaderTransport limits [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3706:
URL: https://github.com/apache/thrift/pull/3706#discussion_r3741989273
##########
lib/rb/spec/header_transport_spec.rb:
##########
@@ -178,6 +196,85 @@ def framed(message)
@trans.write("12345")
expect { @trans.flush }.to raise_error(Thrift::TransportException, /frame that is too large/)
end
+
+ {
+ "binary" => Thrift::BinaryProtocol,
+ "compact" => Thrift::CompactProtocol
+ }.each do |protocol_name, protocol_class|
+ it "enforces max frame size for unframed #{protocol_name} messages" do
+ payload = unframed_message(protocol_class)
+
+ exact_limit = Thrift::HeaderTransport.new(Thrift::MemoryBufferTransport.new(payload))
+ exact_limit.set_max_frame_size(payload.bytesize)
+ expect(read_unframed_message(Thrift::HeaderProtocol.new(exact_limit))).to eq("legacy_unframed")
+
+ over_limit = Thrift::HeaderTransport.new(Thrift::MemoryBufferTransport.new(payload))
+ over_limit.set_max_frame_size(payload.bytesize - 1)
+ protocol = Thrift::HeaderProtocol.new(over_limit)
+ 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)
Review Comment:
Thanks. Exact exception-message assertions are an established convention in the Ruby specs, including neighboring HeaderTransport cases for undersized and fragmented frames. Keeping these assertions exact also verifies that the configured limit remains in the diagnostic, while the block independently checks `TransportException::SIZE_LIMIT`.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6142: Enforce Ruby unframed HeaderTransport limits [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk merged PR #3706:
URL: https://github.com/apache/thrift/pull/3706
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org