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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
