THRIFT-6136: Handle Ruby container size conversion errors - #3700
Open
kpumuk wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
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.
Updates Ruby fuzzing and decoding to stop suppressing integer-conversion errors while correctly validating/handling Compact Protocol container sizes up to the full unsigned 32-bit range, reporting larger sizes as ProtocolException::SIZE_LIMIT.
Changes:
- Remove Ruby fuzz harness allowlist for integer-conversion errors; only ignore protocol/truncation errors.
- Add unsigned-32-bit container size validation in Ruby
BaseProtocol#validate_container_size. - Update native struct decoding (C extension) and Ruby struct decoding to use the shared container-size validation and add specs for the new behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/rb/test/fuzz/fuzz_common.rb | Tightens which exceptions are ignored by fuzzing to avoid hiding decoding bugs. |
| lib/rb/spec/struct_spec.rb | Adds specs ensuring reads accept >signed-32-bit sizes and reject >u32 sizes. |
| lib/rb/spec/base_protocol_spec.rb | Adds spec ensuring skip rejects sizes above u32. |
| lib/rb/lib/thrift/struct_union.rb | Uses protocol-level validate_container_size for map/list/set sizes. |
| lib/rb/lib/thrift/protocol/base_protocol.rb | Introduces MAX_CONTAINER_SIZE and enforces u32 upper bound. |
| lib/rb/ext/struct.c | Adds u32-safe container size conversion/validation in native implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Client: rb Co-Authored-By: OpenAI Codex (GPT-5.6) <codex@openai.com>
kpumuk
force-pushed
the
rb-fuzz-integer-errors
branch
from
August 3, 2026 14:34
ec0c562 to
affb1cf
Compare
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 fuzz targets currently suppress integer-conversion errors by matching Ruby exception text. That hides native protocol-decoding problems and is sensitive to message formatting changes between Ruby versions.
This change removes the conversion-error allowlist so the harness ignores only malformed-protocol and truncated-input exceptions. Native and pure Ruby protocol decoding now report Compact Protocol container counts above the signed 32-bit maximum as
ProtocolException::SIZE_LIMIT, matching the documented container-size bound.The native implementation keeps ordinary container-size validation on C fast paths. Focused Binary, accelerated Binary, and Compact container-read benchmarks remained unchanged from master.
[skip ci]anywhere in the commit message to free up build resources.