THRIFT-6056: Limit recursion depth in Dart struct read/write#3561
Open
Jens-G wants to merge 1 commit into
Open
THRIFT-6056: Limit recursion depth in Dart struct read/write#3561Jens-G wants to merge 1 commit into
Jens-G wants to merge 1 commit into
Conversation
Client: dart
Add a _recursionDepth counter with incrementRecursionDepth() and
decrementRecursionDepth() to TProtocol, bounded at 64, and update the
Dart generator to bracket each generated struct read/write body with
try/finally so the counter is always restored. This bounds the work
performed for deeply nested or cyclic structs, which previously had no
limit in the generated read()/write() path.
Add a regression test under test/dart/recursion_depth_test that drives
the recursive IDL types from test/Recursive.thrift through the generated
read()/write() over the binary, compact and JSON protocols:
- chains at the limit round-trip; chains one past it are rejected with
DEPTH_LIMIT on both write and read,
- a wide (shallow) tree confirms decrementRecursionDepth() unwinds each
sibling back to depth 1,
- a cyclic object graph is rejected instead of recursing without bound.
The test needs a null-safe Dart SDK (>= 2.12); generation and execution
are wired via `make recursion-test` and kept out of the default cross
chain (the cross images currently pin an older Dart).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
103cd55 to
61ab25f
Compare
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.
Summary
THRIFT-6056— bound the recursion depth of Dart struct read/write.On
masterthe generated Dartread()/write()callreadStructBegin/writeStructBegindirectly with no depth bound, andTProtocolhas norecursion counter, so a deeply nested or cyclic struct is processed without a
limit in the generated path. (This is unlike netstd/Delphi, whose generated
code already brackets every read/write with a recursion tracker — there the
equivalent change would double-count; here there is no pre-existing bound.)
This PR:
_recursionDepthcounter withincrementRecursionDepth()/decrementRecursionDepth()toTProtocol, bounded at 64(
TProtocolErrorType.DEPTH_LIMITon excess);try { … } finally { decrementRecursionDepth(); }so the counter is alwaysrestored. The bound lives only in the generated path (one count per struct);
readStructBegin/writeStructBeginare left untouched, so there is nodouble-counting.
Unknown-field skipping during read is handled separately by
TProtocolUtil(which carries its own depth parameter) and is left unchanged here.
Test
The previous test exercised
increment/decrementRecursionDepth()in isolation,which does not cover the real generated read/write path. It is replaced by
test/dart/recursion_depth_test— a generated-code round-trip over therecursive IDL types in
test/Recursive.thrift(CoRec⇄CoRec2,RecTree),run across the binary, compact and JSON protocols:
DEPTH_LIMITDEPTH_LIMITDEPTH_LIMITinstead of unbounded recursionValidation with Dart 3.12 (
dart:stable):masterbaseline (unpatched generator +masterTProtocol):the 9 over-limit / cyclic cases fail — writes and reads are not bounded and
the cyclic case recurses to
StackOverflowError— confirming the gap thischange closes.
Generation and the test run are wired through
make -C test/dart recursion-test(mirroring the existing
thrift_teststub rules) and kept out of the defaultcross chain; the suite above was validated directly with
dart pub get && dart testunder Dart 3.x, since it needs a null-safe Dart ≥ 2.12 and the crossimages currently pin 2.7.2.
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com