You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "kpumuk (via GitHub)" <gi...@apache.org> on 2026/07/31 22:22:24 UTC
[PR] THRIFT-6045: Add Ruby recursion depth limit [thrift]
kpumuk opened a new pull request, #3691:
URL: https://github.com/apache/thrift/pull/3691
<!-- Explain the changes in the pull request below: -->
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.
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([THRIFT-6045](https://issues.apache.org/jira/browse/THRIFT-6045))
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] 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.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6045: Add Ruby recursion depth limit [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on PR #3691:
URL: https://github.com/apache/thrift/pull/3691#issuecomment-5166672003
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.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6045: Add Ruby recursion depth limit [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3691:
URL: https://github.com/apache/thrift/pull/3691#discussion_r3704221449
##########
lib/rb/ext/struct.c:
##########
@@ -771,17 +820,26 @@ static VALUE rb_thrift_union_write(VALUE self, VALUE protocol) {
return Qnil;
}
+// cppcheck-suppress constParameterCallback
+static VALUE rb_thrift_union_write(int argc, VALUE *argv, VALUE self) {
+ VALUE protocol;
+ int remaining_depth = parse_recursive_args(argc, argv, &protocol);
+ return rb_thrift_union_write_recursive(self, protocol, remaining_depth);
+}
+
void Init_struct(void) {
VALUE struct_module = rb_const_get(thrift_module, rb_intern("Struct"));
- rb_define_method(struct_module, "write", rb_thrift_struct_write, 1);
- rb_define_method(struct_module, "read", rb_thrift_struct_read, 1);
+ recursion_limit = FIX2INT(rb_const_get(thrift_module, rb_intern("DEFAULT_RECURSION_DEPTH")));
+
+ rb_define_method(struct_module, "write", rb_thrift_struct_write, -1);
+ rb_define_method(struct_module, "read", rb_thrift_struct_read, -1);
Review Comment:
`Init_struct` assumes `Thrift::DEFAULT_RECURSION_DEPTH` is already defined at extension-load time. If the native extension is loaded before `thrift/exceptions` (or whichever file defines the constant), `rb_const_get` will raise and prevent the extension from loading. Consider making this robust by either requiring `thrift/exceptions` from C before reading the constant, or falling back to a hard-coded default (e.g., 64) if the constant is not defined.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6045: Add Ruby recursion depth limit [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3691:
URL: https://github.com/apache/thrift/pull/3691#discussion_r3704380569
##########
lib/rb/ext/struct.c:
##########
@@ -771,17 +820,26 @@ static VALUE rb_thrift_union_write(VALUE self, VALUE protocol) {
return Qnil;
}
+// cppcheck-suppress constParameterCallback
+static VALUE rb_thrift_union_write(int argc, VALUE *argv, VALUE self) {
+ VALUE protocol;
+ int remaining_depth = parse_recursive_args(argc, argv, &protocol);
+ return rb_thrift_union_write_recursive(self, protocol, remaining_depth);
+}
+
void Init_struct(void) {
VALUE struct_module = rb_const_get(thrift_module, rb_intern("Struct"));
- rb_define_method(struct_module, "write", rb_thrift_struct_write, 1);
- rb_define_method(struct_module, "read", rb_thrift_struct_read, 1);
+ recursion_limit = FIX2INT(rb_const_get(thrift_module, rb_intern("DEFAULT_RECURSION_DEPTH")));
+
+ rb_define_method(struct_module, "write", rb_thrift_struct_write, -1);
+ rb_define_method(struct_module, "read", rb_thrift_struct_read, -1);
Review Comment:
Native extension cannot be loaded before thrift, it fails on fetching module. You are drunk, Copilot
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6045: Add Ruby recursion depth limit [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3691:
URL: https://github.com/apache/thrift/pull/3691#discussion_r3694065954
##########
lib/rb/ext/struct.c:
##########
@@ -771,17 +820,26 @@ static VALUE rb_thrift_union_write(VALUE self, VALUE protocol) {
return Qnil;
}
+// cppcheck-suppress constParameterCallback
+static VALUE rb_thrift_union_write(int argc, VALUE *argv, VALUE self) {
+ VALUE protocol;
+ int remaining_depth = parse_recursive_args(argc, argv, &protocol);
+ return rb_thrift_union_write_recursive(self, protocol, remaining_depth);
+}
+
void Init_struct(void) {
VALUE struct_module = rb_const_get(thrift_module, rb_intern("Struct"));
- rb_define_method(struct_module, "write", rb_thrift_struct_write, 1);
- rb_define_method(struct_module, "read", rb_thrift_struct_read, 1);
+ recursion_limit = FIX2INT(rb_const_get(thrift_module, rb_intern("DEFAULT_RECURSION_DEPTH")));
Review Comment:
Init_struct assumes Thrift::DEFAULT_RECURSION_DEPTH is already defined and uses rb_const_get unconditionally. If the native extension is loaded before `thrift/exceptions` (e.g., via `require "thrift_native"` directly), this will raise and prevent the extension from loading.
Consider defaulting to 64 when the constant is not defined (or defining the constant from C if missing) so the extension can be loaded independently.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6045: Add Ruby recursion depth limit [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3691:
URL: https://github.com/apache/thrift/pull/3691#issuecomment-5157739313
### 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.
https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/lib/thrift/struct.rb#L84-L88
Same ordering in `Union#read`:
https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/lib/thrift/union.rb#L58-L62
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:
https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/ext/struct.c#L711-L716
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.
https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/ext/struct.c#L36-L40
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](https://github.com/apache/thrift/pull/3592#issuecomment-4674593282).
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.
https://github.com/apache/thrift/blob/c5761b70c0efde55bd8a7372d1567de8e64e28a5/lib/rb/lib/thrift/protocol/base_protocol.rb#L292-L299
🤖 Generated with [Claude Code](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub>
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6045: Add Ruby recursion depth limit [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3691:
URL: https://github.com/apache/thrift/pull/3691#discussion_r3694130834
##########
lib/rb/ext/struct.c:
##########
@@ -771,17 +820,26 @@ static VALUE rb_thrift_union_write(VALUE self, VALUE protocol) {
return Qnil;
}
+// cppcheck-suppress constParameterCallback
+static VALUE rb_thrift_union_write(int argc, VALUE *argv, VALUE self) {
+ VALUE protocol;
+ int remaining_depth = parse_recursive_args(argc, argv, &protocol);
+ return rb_thrift_union_write_recursive(self, protocol, remaining_depth);
+}
+
void Init_struct(void) {
VALUE struct_module = rb_const_get(thrift_module, rb_intern("Struct"));
- rb_define_method(struct_module, "write", rb_thrift_struct_write, 1);
- rb_define_method(struct_module, "read", rb_thrift_struct_read, 1);
+ recursion_limit = FIX2INT(rb_const_get(thrift_module, rb_intern("DEFAULT_RECURSION_DEPTH")));
Review Comment:
`thrift_native` is not a standalone entry point and already requires the Ruby runtime classes and constants to be initialized. Directly requiring it fails at the pre-existing `Thrift` lookup before reaching `Init_struct`. Both supported load paths load `thrift/exceptions`, which defines `DEFAULT_RECURSION_DEPTH`, before the extension. A C fallback would therefore duplicate the default without enabling independent loading.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6045: Add Ruby recursion depth limit [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk merged PR #3691:
URL: https://github.com/apache/thrift/pull/3691
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org