THRIFT-6045: Add Ruby recursion depth limit

[kpumuk]

Ruby struct, union, and exception serialization previously had no recursion-depth limit, allowing deeply nested or cyclic values to recurse until the Ruby stack was exhausted.

This adds a call-local remaining-depth budget of 64 to the pure-Ruby and native read/write paths. Nested structs, unions, and exceptions consume the budget while containers pass it through unchanged; unknown and mismatched fields remain independently bounded by the existing protocol skip limit. The optional depth arguments and the migration required for custom exact-arity serialization overrides are documented for Ruby 0.25.0.

• Did you create an Apache Jira ticket? (THRIFT-6045)
• 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.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lib/rb/lib/thrift/exceptions.rb:21

• Thrift::ApplicationException#read/#write references Types::..., but this file doesn’t require thrift/types. If a consumer requires thrift/exceptions directly (or gets it indirectly), calling these methods can raise NameError: uninitialized constant Thrift::Types unless thrift/types was loaded elsewhere. Adding the explicit require here makes the file self-contained.

module Thrift
  DEFAULT_RECURSION_DEPTH = 64

lib/rb/lib/thrift/protocol/base_protocol.rb:23

• BaseProtocol uses Types::... extensively (e.g., in write_type / read_type), but this file doesn’t require thrift/types. Since it now requires thrift/exceptions, it’s more likely to be loaded directly; adding an explicit require 'thrift/types' avoids NameError when base_protocol is required standalone.

# this require is to make generated struct definitions happy
require 'set'
require 'thrift/exceptions'

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

lib/rb/lib/thrift/protocol/base_protocol.rb:253

• BaseProtocol#write_field always calls write_type with a third argument (remaining_depth), even when it is nil. That changes the call arity from 2→3 for the common case and will break custom protocol implementations that override write_type(field_info, value) with the previous 2-arg signature.

To preserve backward compatibility, only pass remaining_depth when it is non-nil (i.e., when the caller is explicitly propagating a depth budget).

      write_field_begin(field_info[:name], field_info[:type], fid)
      write_type(field_info, value, remaining_depth)
      write_field_end

[Jens-G]

Code review

No blocking issues. Depth accounting is symmetric between the pure-Ruby and the native path (both admit exactly 64 struct levels and raise on the 65th), containers pass the budget through unchanged on every path, and the guards added by THRIFT-6025 (negative/oversized container sizes), THRIFT-6104 (Set subclasses), THRIFT-6124 (reset of reused deserialization targets) and THRIFT-6013 (BaseProtocol#skip's independent bound) all survive the refactor.

Suggestions, none blocking:

1. The new depth check runs before the reused-target reset that THRIFT-6124 (d1d7280c3) made the unconditional first action of read. reused_obj.read(prot, 0) raises DEPTH_LIMIT with the previous deserialization's field values still in place, which breaks the invariant serializer_spec.rb's "does not retain previous struct state when reading fails" encodes — that test exercises EOFError, a failure mode that existed when THRIFT-6124 landed; DEPTH_LIMIT is a new one. Not reachable from a wire payload (Deserializer#deserialize always uses the default budget, and recursive descent always targets a freshly allocated object), but read(protocol, depth) is public API as of this PR. Moving the raise below the reset restores the invariant.

thrift/lib/rb/lib/thrift/struct.rb

Lines 84 to 88 in c5761b7

	def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH)
	raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0
	unless instance_variables.empty?
	defaults = fields_with_default_values

Same ordering in Union#read:

thrift/lib/rb/lib/thrift/union.rb

Lines 58 to 62 in c5761b7

	def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH)
	raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0
	@setfield = nil
	@value = nil

and in the extension, where parse_recursive_args raises before rb_thrift_struct_read_recursive's rb_check_frozen / reset_struct_field block (L664-L667) is entered:

thrift/lib/rb/ext/struct.c

Lines 711 to 716 in c5761b7

	// cppcheck-suppress constParameterCallback
	static VALUE rb_thrift_struct_read(int argc, VALUE *argv, VALUE self) {
	VALUE protocol;
	int remaining_depth = parse_recursive_args(argc, argv, &protocol);
	return rb_thrift_struct_read_recursive(self, protocol, remaining_depth);
	}

2. Minor: the comment "Structs consume this call-local budget" sits on static int recursion_limit, which is file-scope and assigned once in Init_struct. It describes the remaining_depth parameter, not this variable.

thrift/lib/rb/ext/struct.c

Lines 36 to 40 in c5761b7

	#define IS_CONTAINER(ttype) ((ttype) == TTYPE_MAP || (ttype) == TTYPE_LIST || (ttype) == TTYPE_SET)
	#define STRUCT_FIELDS(obj) rb_const_get(CLASS_OF(obj), fields_const_id)
	// Structs consume this call-local budget; containers pass it through unchanged.
	static int recursion_limit;

On the Init_struct thread: your reading is right. Init_thrift_native resolves Thrift, Thrift::Types and Thrift::ProtocolException::DEPTH_LIMIT unconditionally before it reaches Init_struct, so a standalone require "thrift_native" already fails earlier, and a C-side literal would only add a second copy of the default — the drift I flagged as issue 3 on the Python equivalent, #3592.

Copilot's suppressed note about write_type's 2→3 arity is worth a decision either way. If you keep the unconditional third argument, one sentence in the README migration note would help: omitting remaining_depth does not inherit the caller's budget, it restarts it at 64.

thrift/lib/rb/lib/thrift/protocol/base_protocol.rb

Lines 292 to 299 in c5761b7

	write_uuid(value)
	when Types::STRUCT
	if remaining_depth
	value.write(self, remaining_depth - 1)
	else
	value.write(self)
	end
	else

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[kpumuk]

Thanks for the review. The reset-order observation is correct: I reproduced that read(protocol, 0) leaves an existing object’s values intact in both the Ruby and native implementations.

I thought about whether this should count as the kind of failed read covered by THRIFT-6124. I am leaning towards protecting an object once deserialization has started: fields are cleared before reading the new payload, so an EOFError or another protocol failure cannot expose stale values. A zero depth is slightly different because the call is rejected before the protocol is touched.

In normal use, Deserializer always starts with the default budget. When recursive reading eventually reaches zero, the target is a newly allocated child, so there is no old state to preserve. The stale-state case therefore requires application code to call read(protocol, 0) directly on an already-used object. Resetting in that case would mean mutating the receiver even though the read was rejected before it began; it would also change the error for frozen objects from DEPTH_LIMIT to FrozenError. On balance, I think keeping the depth check first gives the cleaner behavior.

I did update the C comment—the original wording described the call-local remaining_depth value but was attached to the file-level default.

The write_type arity point is also fair. This is an intentional 0.25.0 compatibility change, and the README migration note asks exact-arity custom overrides to accept and forward the optional depth. There is deliberately no hidden protocol state from which an omitted argument could inherit a caller’s budget, so omission starts a fresh default budget while recursive custom writers must pass the current one explicitly.

And thanks for confirming the conclusion on standalone thrift_native loading.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

lib/rb/README.md:184

• This new behavior description is inserted immediately before the ### 0.24.0 heading but doesn’t have its own version heading, which makes it ambiguous which release it applies to. Add a ### 0.25.0 (or the intended version) heading above these lines so the changelog structure remains clear.

Ruby struct, union, and exception serialization methods now accept an optional
remaining struct depth: `read(protocol, depth = 64)` and
`write(protocol, depth = 64)`.
Unknown and mismatched fields remain independently bounded by
`Thrift::BaseProtocol#skip`. The `write_field` and `write_type` helpers accept
an optional `remaining_depth`: custom writers must use
`write_field(field_info, fid, value, remaining_depth)` or
`write_type(field_info, value, remaining_depth)` for struct values. Custom
protocols and custom struct, union, or exception serialization overrides with
exact argument counts must accept and forward the optional depth argument (or
use `*args`) before upgrading.

### 0.24.0

lib/rb/lib/thrift/struct.rb:86

• The error message string 'Maximum recursion depth exceeded' is duplicated across multiple Ruby files (and also in the C extension). To keep messages consistent and simplify future changes/localization, consider defining a single constant (e.g., under Thrift) for the message and referencing it from struct/union/exceptions and the native extension.

    def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH)
      raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0