THRIFT-6054: Limit recursion depth in Kotlin struct, union and exception read/write#3559
Open
Jens-G wants to merge 1 commit into
Open
THRIFT-6054: Limit recursion depth in Kotlin struct, union and exception read/write#3559Jens-G wants to merge 1 commit into
Jens-G wants to merge 1 commit into
Conversation
7334003 to
997d45b
Compare
…ion read/write Client: java,kotlin Kotlin-generated struct and exception serialization routes through TProtocol.readStruct() and writeStruct() (the callback helpers in the shared Java runtime), which did not bound recursion depth, so a deeply nested or cyclic value was read or written without a limit. Kotlin-generated unions extend the shared org.apache.thrift.TUnion, whose read/write path calls readStructBegin()/writeStructBegin() directly and was likewise unbounded -- this affects Java-generated unions too. Wrap the readStruct()/writeStruct() bodies, and TUnion's standard- and tuple-scheme read/write bodies, in incrementRecursionDepth() / try ... finally / decrementRecursionDepth(), reusing the existing TProtocol infrastructure (limit from TConfiguration.getRecursionLimit(), default 64, TProtocolException.DEPTH_LIMIT on excess). The Java generator emits an inline StandardScheme that already increments separately and does not call the readStruct()/writeStruct() helpers, so Java struct serialization is unaffected (no double-counting); only Kotlin-generated code consumes them. A union's standard and tuple schemes are mutually exclusive per protocol, so a union increments exactly once per level. Replace the isolated counter test with a generated-code round-trip (lib/kotlin RecursionDepthTest): the recursive types CoRec/CoRec2/RecTree/RecUnion/RecError are generated from a new src/test/resources/RecursionDepthTest.thrift (mirroring test/Recursive.thrift) and driven through TBase.read()/write() -- chains at the limit round-trip, chains one past it (write and read) are rejected with DEPTH_LIMIT for structs, unions and exceptions, a wide structure confirms the counter is decremented per sibling, and a cyclic graph is rejected instead of recursing without bound. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
997d45b to
3abeffe
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.
THRIFT-6054: Limit recursion depth in Kotlin struct, union and exception read/write
Problem
Kotlin-generated struct and exception serialization routes through the shared Java runtime's
TProtocol.readStruct()/writeStruct()callback helpers, which did not bound recursion depth. Kotlin-generated unions extend the sharedorg.apache.thrift.TUnion, whose read/write path callsreadStructBegin()/writeStructBegin()directly and was likewise unbounded. A deeply nested or cyclic value could therefore be read or written without a limit.Change
readStruct()/writeStruct()callback bodies (consumed by Kotlin-generated structs and exceptions) inincrementRecursionDepth()/try … finally/decrementRecursionDepth().TUnion's standard- and tuple-scheme read/write bodies the same way, closing the union path.Both reuse the existing
TProtocolinfrastructure: limit fromTConfiguration.getRecursionLimit()(default 64),TProtocolException.DEPTH_LIMITon excess. The Java generator emits an inlineStandardSchemethat already increments separately and does not call thereadStruct()/writeStruct()helpers, so Java struct serialization is unaffected (no double-counting). A union's standard and tuple schemes are mutually exclusive per protocol, so a union increments exactly once per level.Notes for committer review
TUnionchange lives in the sharedlib/javaruntime, so it also bounds Java-generated union recursion (not only Kotlin). Consequently Java union writes become bounded, whereas Java struct writes remain unbounded (the Java generator guards reads only). This asymmetry is intentional here but flagged for review.TUniontuple-scheme guard is not exercised by the test (which drivesTBinaryProtocol, i.e. the standard scheme); it mirrors the verified standard-scheme pattern exactly.Test
Replaces the isolated counter test with a generated-code round-trip (
lib/kotlinRecursionDepthTest). Recursive typesCoRec/CoRec2/RecTree/RecUnion/RecErrorare generated fromsrc/test/resources/RecursionDepthTest.thrift(mirroringtest/Recursive.thrift) and driven throughTBase.read()/write():DEPTH_LIMIT— for structs, unions and exceptions;12/12 cases pass with the fix; reverting only
TUnion.javamakes exactly the two union over-limit cases fail while struct and exception cases still pass — confirming the union gap is real, the fix is load-bearing, and exceptions ride the existingTProtocolguard.🤖 Generated with Claude Code