THRIFT-6145: Validate Ruby Compact decoder bounds - #3710
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
Aligns Ruby CompactProtocol’s pure-Ruby and native (C extension) decoders on strict varint32 and binary-length bounds, so malformed inputs are rejected consistently and large binary lengths are rejected before any payload read is attempted.
Changes:
- Tightened
read_varint32in both pure Ruby and the C extension to reject 5th-byte payload bits that would overflowuint32(while still allowing valid 5-byteuint32varints). - Added an early
INT32_MAXbound check for Compactread_binary, raisingProtocolException::SIZE_LIMITbefore callingread_all. - Expanded Ruby specs to cover the new binary size-limit behavior and the new varint32 overflow condition.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/rb/spec/compact_protocol_spec.rb | Renames shared varint32 size fixtures and adds coverage for binary size upper bounds and varint32 5th-byte overflow. |
| lib/rb/lib/thrift/protocol/compact_protocol.rb | Pure-Ruby decoder: adds binary size bound check and refines varint32 decoding to reject overflowing 5th-byte payload bits. |
| lib/rb/ext/compact_protocol.c | Native decoder: mirrors the pure-Ruby varint32 5th-byte overflow validation and adds the same binary size limit check. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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’s pure and native CompactProtocol readers accept fifth-byte varint payload bits outside the uint32 range, and they produce different values for the same input. Binary length prefixes above the signed int32 size domain are also passed to the transport instead of being rejected by the protocol.
This change gives both readers the same boundary behavior. Invalid fifth-byte payload bits now raise
ProtocolException::INVALID_DATA, while binary lengths aboveINT32_MAXraiseProtocolException::SIZE_LIMITbeforeread_allis called. Valid uint32 varints, signed i32 values, binary sizes throughINT32_MAX, and ordinary Compact messages remain supported.Benchmarks
I ran the repository’s small Compact read benchmark against current master (
c2def39207a73394420088da9b4b105571dd9036) and this change in the same Ruby container:Each result is the median of 11 independent warmed runs.
The ranges overlap. This focused small-structure read workload does not show a clear performance change.
[skip ci]anywhere in the commit message to free up build resources.