Skip to content

THRIFT-6139: Validate Ruby CompactProtocol integer ranges - #3703

Open
kpumuk wants to merge 1 commit into
apache:masterfrom
kpumuk:rb-compact-integer-ranges
Open

THRIFT-6139: Validate Ruby CompactProtocol integer ranges#3703
kpumuk wants to merge 1 commit into
apache:masterfrom
kpumuk:rb-compact-integer-ranges

Conversation

@kpumuk

@kpumuk kpumuk commented Aug 5, 2026

Copy link
Copy Markdown
Member

Ruby's CompactProtocol writers accept integers outside the widths declared by the Thrift type system. An out-of-range value can therefore be serialized without an error, change when decoded, and behave differently depending on whether the native extension is loaded.

This change validates fixed-width integers, field identifiers, message sequence identifiers, and collection or binary sizes before writing bytes or changing protocol state. Native and pure-Ruby implementations now follow the same bounds, while protocol-owned header bytes continue to use their unsigned wire representation.

The pure-Ruby varint writers also collect multi-byte values into a single transport write. This avoids a performance penalty from the added validation and reduces temporary allocations without changing valid wire encodings.

Benchmarks

The protocol benchmark was run in seven fresh processes for both upstream/master and this change. Pure-Ruby runs additionally set THRIFT_BENCHMARK_SKIP_NATIVE=1.

ruby ../../test/rb/benchmarks/protocol_benchmark.rb \
  --small-runs 30000 \
  --large-runs 2 \
  --scenarios rb-cmp-write-small,rb-cmp-write-large,hdr-cmp-write-small \
  --json
Mode and scenario Master median Proposed median Change
Native Compact, small structures 0.155830s 0.154606s -0.79%
Native Compact, large structure 0.209833s 0.209450s -0.18%
Native Header/Compact, small structures 0.377205s 0.372075s -1.36%
Pure-Ruby Compact, small structures 0.467134s 0.450486s -3.56%
Pure-Ruby Compact, large structure 0.643928s 0.631595s -1.92%
Pure-Ruby Header/Compact, small structures 0.621219s 0.607185s -2.26%

A targeted allocation benchmark encoding 500,000 representative varints reduced temporary allocations from 4,500,009 to 1,400,004.

These measurements used Ruby 4.0.6 on aarch64 Linux; they do not represent a cross-version or cross-platform benchmark matrix.

  • Did you create an Apache Jira ticket? THRIFT-6139
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • 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.

@mergeable mergeable Bot added the ruby Pull requests that update Ruby code label Aug 5, 2026
Client: rb

Co-Authored-By: OpenAI Codex (GPT-5.6) <codex@openai.com>
@kpumuk
kpumuk force-pushed the rb-compact-integer-ranges branch from 240b717 to e84c0fe Compare August 5, 2026 14:39
@kpumuk
kpumuk marked this pull request as ready for review August 5, 2026 20:09
Copilot AI lite review requested due to automatic review settings August 5, 2026 20:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens Ruby CompactProtocol write-side validation so fixed-width integers, field IDs, sequence IDs, and container/binary sizes are checked against Thrift’s declared bounds before any bytes are written or protocol state is mutated. It also optimizes pure-Ruby varint emission by buffering multi-byte encodings into a single transport write.

Changes:

  • Add spec coverage for rejecting out-of-range / non-integer / nil values without writing partial output.
  • Add range/type validation for BYTE/I16/I32/I64, field IDs, sequence IDs, and size-prefixed values in the pure-Ruby CompactProtocol.
  • Reduce pure-Ruby varint write overhead by batching bytes into a single string write.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
lib/rb/spec/compact_protocol_spec.rb Adds behavioral specs ensuring invalid inputs raise and do not write/append bytes.
lib/rb/lib/thrift/protocol/compact_protocol.rb Implements pure-Ruby validation for integer widths/IDs/sizes and batches varint writes.
lib/rb/ext/compact_protocol.c Aligns native extension write-side validation with Thrift integer/size bounds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +450 to +452
def write_byte_direct(byte)
@trans.write([byte].pack('C'))
end
Comment on lines 198 to 202
def write_map_begin(ktype, vtype, size)
size = validate_size(size)
key_type = CompactTypes.get_compact_type(ktype)
value_type = CompactTypes.get_compact_type(vtype)
if (size == 0)
Comment on lines 281 to 285
VALUE rb_thrift_compact_proto_write_map_begin(VALUE self, VALUE ktype, VALUE vtype, VALUE size_value) {
int size = FIX2INT(size_value);
int size = checked_size_value(size_value);
int key_type = get_compact_type(ktype);
int value_type = get_compact_type(vtype);
VALUE transport = GET_TRANSPORT(self);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ruby Pull requests that update Ruby code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants