You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "dxbjavid (via GitHub)" <gi...@apache.org> on 2026/06/05 09:44:44 UTC
[PR] limit recursion depth in c_glib thrift_protocol_skip [thrift]
dxbjavid opened a new pull request, #3585:
URL: https://github.com/apache/thrift/pull/3585
thrift_protocol_skip recurses through nested structs, lists, sets and maps with no depth guard, so a server skipping an unknown field (the dispatch processor calls it on every unrecognised method) can be driven into unbounded recursion by a deeply nested message and overflow the stack. The configuration already carries recursionLimit_ with a default of 64 but nothing reads it. I noticed this reading the c_glib skip next to the C++ one, which guards the same path with TInputRecursionTracker. This routes skip through an internal helper that tracks depth and stops at the configured limit.
--
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] limit recursion depth in c_glib thrift_protocol_skip [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G merged PR #3585:
URL: https://github.com/apache/thrift/pull/3585
--
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] limit recursion depth in c_glib thrift_protocol_skip [thrift]
Posted by "dxbjavid (via GitHub)" <gi...@apache.org>.
dxbjavid commented on PR #3585:
URL: https://github.com/apache/thrift/pull/3585#issuecomment-4659201794
Thanks for the merge! Appreciated
--
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] limit recursion depth in c_glib thrift_protocol_skip [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3585:
URL: https://github.com/apache/thrift/pull/3585#issuecomment-4636336366
### Code review
The code change itself looks correct: the guard is threaded through `thrift_protocol_skip_impl`, both `transport` and `configuration` are null-checked before reading `recursionLimit_`, the `-1` return propagates via `THRIFT_SKIP_RESULT_OR_RETURN`, all five recursive sites pass `recursion_depth + 1`, and the public `thrift_protocol_skip` signature is preserved. The gap is real and still present on `master` — `recursionLimit_` has existed since THRIFT-5237 but was never consulted in `skip`.
One item to address before merge: no test accompanies the change. AGENTS.md says "Provide tests for every submitted change", and the analogous per-language limit changes each added one. A test under `lib/c_glib/test/` that nests past the configured limit and asserts `THRIFT_PROTOCOL_ERROR_DEPTH_LIMIT` would cover it.
https://github.com/apache/thrift/blob/6934f8e3ad8b5de855efffd110673c75573855f5/lib/c_glib/src/thrift/c_glib/protocol/thrift_protocol.c#L436-L454
🤖 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] limit recursion depth in c_glib thrift_protocol_skip [thrift]
Posted by "dxbjavid (via GitHub)" <gi...@apache.org>.
dxbjavid commented on PR #3585:
URL: https://github.com/apache/thrift/pull/3585#issuecomment-4642003355
good point, added a test in testbinaryprotocol.c. it stands up a memory buffer with a small recursion_limit on its configuration, writes nested struct field headers a fair way past that limit, and asserts skip returns -1 with THRIFT_PROTOCOL_ERROR_DEPTH_LIMIT. there's also a shallow case nested within the limit that still skips cleanly, so the guard doesn't trip on normal input. used the binary protocol over the buffer so it stays self-contained, no socket needed.
--
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