THRIFT-6052: Limit struct read/write recursion depth in Smalltalk library#3557
Open
Jens-G wants to merge 1 commit into
Open
THRIFT-6052: Limit struct read/write recursion depth in Smalltalk library#3557Jens-G wants to merge 1 commit into
Jens-G wants to merge 1 commit into
Conversation
2db5638 to
e4ec62a
Compare
…rary Client: st Smalltalk struct serialization is emitted inline by the generator (the struct_writer / struct_reader templates). These did not bound recursion depth, so a deeply nested message was read or written without a limit. Wrap the generated struct read/write bodies with incrementRecursionDepth ... ensure: [decrementRecursionDepth] and add the counter to TProtocol (limit 64, TProtocolError depthLimit on excess). While there, fix the struct reader, which emitted "oprot readStructEnd" in a read context where it should be "iprot" -- a latent bug that broke service struct reads independently of depth. Replace the isolated counter test with a round-trip test driving the generated DeepClient send/recv path (lib/st/test/TProtocolRecursionDepthTest.st + RecursionDepthTest.thrift): a chain of distinct struct types at the limit round-trips, while one level past it is rejected with the depth limit on both write and read. A finite chain of distinct types is used rather than a genuinely recursive type because the generator inline-expands nested struct serialization and recurses without bound at code-generation time on a recursive type -- a separate, pre-existing generator limitation, out of scope here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e4ec62a to
cd6d305
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-6052— bound the recursion depth of Smalltalk struct read/write.Smalltalk struct serialization is emitted inline by the generator (the
struct_writer/struct_readertemplates; there are no per-struct read/writemethods). On
masterthese do not bound recursion depth, so a deeply nestedmessage is read or written without a limit.
This PR wraps the generated struct read/write bodies with
incrementRecursionDepth…ensure: [decrementRecursionDepth]and adds thecounter to
TProtocol(limit 64,TProtocolError depthLimiton excess).Incidental fix: the struct reader emitted
oprot readStructEndin a readcontext where it should be
iprot. That was a latent bug that broke servicestruct reads independently of depth; it is corrected here (the
oprot→iprotchange in
struct_reader).Test
The previous test pumped
incrementRecursionDepth/decrementRecursionDepthdirectly and never serialized a struct. It is replaced by a round-trip test
(
lib/st/test/TProtocolRecursionDepthTest.st+RecursionDepthTest.thrift) thatdrives the generated
DeepClientsend/recv path:TProtocolError(depth limit)TProtocolError(depth limit)Why a finite chain instead of
Recursive.thriftThe Smalltalk generator inline-expands nested struct serialization, so a
genuinely recursive type (e.g.
CoRec/CoRec2/RecTree) makes it recursewithout bound at code-generation time and crash (stack overflow). That is a
separate, pre-existing generator limitation, out of scope for this ticket —
tracked separately in
THRIFT-6062. So the limit
is exercised with a finite chain of distinct
struct types (
A1..A65) deep enough to cross it. The runtime depth counter onlyincrements per struct level (not per container), so this finite chain is the
representable way to reach the limit through generated code.
Validation
Smalltalk is not in the CI matrix (build/docker/README.md), so this was
validated locally with GNU Smalltalk 3.2.5 (the suite also files into Squeak,
the documented target; gst needs a couple of trivial
Collection>>sum-stylecompat shims that Squeak does not):
master: the two over-limit cases stop passing — the over-limitwrite serializes with no error (the bound is absent) and the over-limit
read is not cleanly rejected — confirming the gap this change closes.
Note on exception/union coverage
The runtime guard reaches
struct,unionandexceptionalike: field dispatch routesis_struct() || is_xception()(a union is at_struct, so this covers unions too) through the same guardedstruct_reader/struct_writer. The committed test exercises a finite struct chain only — a recursive type cannot be generated in Smalltalk at all: the generator inline-expands struct serialization with no base case, so any recursive struct/union/exception overflows the C++ compiler's stack at code-generation time (filed as THRIFT-6062). A recursive-exception round-trip is therefore not expressible here; exception coverage rests on the shared guarded reader/writer path described above.🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com