From cd6d305921beb83e38ad5a0131dcceeb05459573 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Thu, 28 May 2026 01:45:58 +0200 Subject: [PATCH] THRIFT-6052: Limit struct read/write recursion depth in Smalltalk library 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 --- .../cpp/src/thrift/generate/t_st_generator.cc | 18 ++- lib/st/test/RecursionDepthTest.thrift | 101 ++++++++++++ lib/st/test/TProtocolRecursionDepthTest.st | 153 ++++++++++++++++++ lib/st/thrift.st | 17 +- 4 files changed, 281 insertions(+), 8 deletions(-) create mode 100644 lib/st/test/RecursionDepthTest.thrift create mode 100644 lib/st/test/TProtocolRecursionDepthTest.st diff --git a/compiler/cpp/src/thrift/generate/t_st_generator.cc b/compiler/cpp/src/thrift/generate/t_st_generator.cc index c1ad35577e8..140f8cdefcf 100644 --- a/compiler/cpp/src/thrift/generate/t_st_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_st_generator.cc @@ -706,9 +706,10 @@ string t_st_generator::struct_writer(t_struct* tstruct, string sname) { const vector& fields = tstruct->get_sorted_members(); vector::const_iterator fld_iter; - out << "[oprot writeStructBegin: " - << "(TStruct new name: '" + tstruct->get_name() + "')." << '\n'; + out << "[oprot incrementRecursionDepth." << '\n'; indent_up(); + out << indent() << "[oprot writeStructBegin: " + << "(TStruct new name: '" + tstruct->get_name() + "')." << '\n'; for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) { bool optional = (*fld_iter)->get_req() == t_field::T_OPTIONAL; @@ -735,7 +736,8 @@ string t_st_generator::struct_writer(t_struct* tstruct, string sname) { out << "." << '\n'; } - out << indent() << "oprot writeFieldStop; writeStructEnd] value"; + out << indent() + << "oprot writeFieldStop; writeStructEnd] ensure: [oprot decrementRecursionDepth]] value"; indent_down(); return out.str(); @@ -759,10 +761,11 @@ string t_st_generator::struct_reader(t_struct* tstruct, string clsName = "") { // This is nasty, but without it we'll break things by prefixing TResult. string name = ((capitalize(clsName) == "TResult") ? capitalize(clsName) : prefix(clsName)); out << indent() << val << " := " << name << " new." << '\n'; + out << indent() << "iprot incrementRecursionDepth." << '\n'; - out << indent() << "iprot readStructBegin." << '\n' << indent() << "[" << desc - << " := iprot readFieldBegin." << '\n' << indent() << desc - << " type = TType stop] whileFalse: [|" << found << "|" << '\n'; + out << indent() << "[iprot readStructBegin." << '\n' + << indent() << "[" << desc << " := iprot readFieldBegin." << '\n' + << indent() << desc << " type = TType stop] whileFalse: [|" << found << "|" << '\n'; indent_up(); for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) { @@ -779,7 +782,8 @@ string t_st_generator::struct_reader(t_struct* tstruct, string clsName = "") { out << indent() << found << " ifNil: [iprot skip: " << desc << " type]]." << '\n'; indent_down(); - out << indent() << "oprot readStructEnd." << '\n' << indent() << val << "] value"; + out << indent() << "iprot readStructEnd] ensure: [iprot decrementRecursionDepth]." << '\n' + << indent() << val << "] value"; indent_down(); return out.str(); diff --git a/lib/st/test/RecursionDepthTest.thrift b/lib/st/test/RecursionDepthTest.thrift new file mode 100644 index 00000000000..bceaf4d8247 --- /dev/null +++ b/lib/st/test/RecursionDepthTest.thrift @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Input for TProtocolRecursionDepthTest. The generated struct read/write path is +// driven deep enough to cross the recursion limit (64) so that the limit added +// in thrift.st / the Smalltalk generator can be exercised end to end. +// +// A *finite* chain of distinct struct types is used rather than a genuinely +// recursive type (as in test/Recursive.thrift): the Smalltalk generator +// inline-expands nested struct serialization, so a self- or mutually-recursive +// type makes it recurse without bound at code-generation time and crash. That +// is a separate, pre-existing generator limitation, tracked apart from this fix. +// +// Field 'f' is optional so an instance (or a crafted payload) can stop at any +// depth, letting one chain exercise both the at-limit and over-limit cases. + +struct A1 { 1: optional A2 f } +struct A2 { 1: optional A3 f } +struct A3 { 1: optional A4 f } +struct A4 { 1: optional A5 f } +struct A5 { 1: optional A6 f } +struct A6 { 1: optional A7 f } +struct A7 { 1: optional A8 f } +struct A8 { 1: optional A9 f } +struct A9 { 1: optional A10 f } +struct A10 { 1: optional A11 f } +struct A11 { 1: optional A12 f } +struct A12 { 1: optional A13 f } +struct A13 { 1: optional A14 f } +struct A14 { 1: optional A15 f } +struct A15 { 1: optional A16 f } +struct A16 { 1: optional A17 f } +struct A17 { 1: optional A18 f } +struct A18 { 1: optional A19 f } +struct A19 { 1: optional A20 f } +struct A20 { 1: optional A21 f } +struct A21 { 1: optional A22 f } +struct A22 { 1: optional A23 f } +struct A23 { 1: optional A24 f } +struct A24 { 1: optional A25 f } +struct A25 { 1: optional A26 f } +struct A26 { 1: optional A27 f } +struct A27 { 1: optional A28 f } +struct A28 { 1: optional A29 f } +struct A29 { 1: optional A30 f } +struct A30 { 1: optional A31 f } +struct A31 { 1: optional A32 f } +struct A32 { 1: optional A33 f } +struct A33 { 1: optional A34 f } +struct A34 { 1: optional A35 f } +struct A35 { 1: optional A36 f } +struct A36 { 1: optional A37 f } +struct A37 { 1: optional A38 f } +struct A38 { 1: optional A39 f } +struct A39 { 1: optional A40 f } +struct A40 { 1: optional A41 f } +struct A41 { 1: optional A42 f } +struct A42 { 1: optional A43 f } +struct A43 { 1: optional A44 f } +struct A44 { 1: optional A45 f } +struct A45 { 1: optional A46 f } +struct A46 { 1: optional A47 f } +struct A47 { 1: optional A48 f } +struct A48 { 1: optional A49 f } +struct A49 { 1: optional A50 f } +struct A50 { 1: optional A51 f } +struct A51 { 1: optional A52 f } +struct A52 { 1: optional A53 f } +struct A53 { 1: optional A54 f } +struct A54 { 1: optional A55 f } +struct A55 { 1: optional A56 f } +struct A56 { 1: optional A57 f } +struct A57 { 1: optional A58 f } +struct A58 { 1: optional A59 f } +struct A59 { 1: optional A60 f } +struct A60 { 1: optional A61 f } +struct A61 { 1: optional A62 f } +struct A62 { 1: optional A63 f } +struct A63 { 1: optional A64 f } +struct A64 { 1: optional A65 f } +struct A65 { 1: optional i16 x } + +service Deep { + A1 echo(1: A1 a) +} diff --git a/lib/st/test/TProtocolRecursionDepthTest.st b/lib/st/test/TProtocolRecursionDepthTest.st new file mode 100644 index 00000000000..65b88339843 --- /dev/null +++ b/lib/st/test/TProtocolRecursionDepthTest.st @@ -0,0 +1,153 @@ +'Recursion-depth limit test for the *generated* Smalltalk struct read/write path. + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + + This drives the recursion-depth limit through the generated struct read/write + code (DeepClient sendEchoA: / recvEcho), not by poking the depth counter in + isolation. The recursive types in test/Recursive.thrift cannot be used: the + Smalltalk generator inline-expands nested struct serialization and recurses + without bound at code-generation time on a recursive type, so a finite chain of + distinct struct types (A1..A65) is used to nest past the limit (64) instead. + + To run (Squeak/Pharo, or GNU Smalltalk with SUnit): + thrift --gen st RecursionDepthTest.thrift + file in: thrift.st, the generated RecursionDepthTest.st, then this file. + Run the TProtocolRecursionDepthTest suite. +'! + +"A minimal in-memory transport so the round-trip needs no socket: writeByte: + hands us an Array, writeString: a String; we normalise both to bytes and replay + them as a ByteArray (first -> integer for readByte, asString -> String)." +TTransport subclass: #TMemoryBuffer + instanceVariableNames: 'bytes pos' + classVariableNames: '' + poolDictionaries: '' + category: 'Thrift-Tests'! + +!TMemoryBuffer class methodsFor: 'instance creation'! +new + ^ super new init! ! + +!TMemoryBuffer methodsFor: 'init'! +init + bytes := OrderedCollection new. + pos := 0! ! + +!TMemoryBuffer methodsFor: 'transport'! +write: aCollection + aCollection do: [:each | bytes add: each asInteger]! ! + +!TMemoryBuffer methodsFor: 'transport'! +read: anInteger + | chunk | + chunk := bytes copyFrom: pos + 1 to: pos + anInteger. + pos := pos + anInteger. + ^ chunk asByteArray! ! + +!TMemoryBuffer methodsFor: 'transport'! +flush! ! + +!TMemoryBuffer methodsFor: 'transport'! +open + ^ self! ! + +!TMemoryBuffer methodsFor: 'transport'! +close! ! + +!TMemoryBuffer methodsFor: 'transport'! +isOpen + ^ true! ! + +TestCase subclass: #TProtocolRecursionDepthTest + instanceVariableNames: '' + classVariableNames: '' + poolDictionaries: '' + category: 'Thrift-Tests'! + +!TProtocolRecursionDepthTest methodsFor: 'support'! +recursionLimit + "The limit enforced by TProtocol>>incrementRecursionDepth." + ^ 64! ! + +!TProtocolRecursionDepthTest methodsFor: 'support'! +clientOn: aTransport + ^ DeepClient new + inProtocol: (TBinaryProtocol new transport: aTransport); + yourself! ! + +!TProtocolRecursionDepthTest methodsFor: 'support'! +chainOfDepth: depth + "An A1->A2->..->A`depth` chain of distinct structs; deeper links stay nil. + Writing it drives `depth` nested struct writers, i.e. depth levels deep." + | node | + node := nil. + depth to: 1 by: -1 do: [:i | | cur | + cur := (Smalltalk at: ('A', i printString) asSymbol) new. + i = 65 ifTrue: [cur x: 1] ifFalse: [cur f: node]. + node := cur]. + ^ node! ! + +!TProtocolRecursionDepthTest methodsFor: 'support'! +craftReplyOfDepth: n + "Craft an echo_result REPLY whose success value nests n structs deep, using + raw protocol primitives so the depth counter is bypassed on the way in (a + normal write of an over-limit value would itself be rejected). Reading it + back drives the generated struct reader n (+1 for the result) levels deep." + | transport proto emit | + transport := TMemoryBuffer new. + proto := TBinaryProtocol new transport: transport. + proto writeMessageBegin: (TCallMessage new name: 'echo'; seqid: 0; yourself). + proto writeFieldBegin: (TField new name: 'success'; type: TType struct; id: 0). + emit := nil. + emit := [:i | + i < n ifTrue: [ + proto writeFieldBegin: (TField new name: 'f'; type: TType struct; id: 1). + emit value: i + 1]. + proto writeFieldStop]. + emit value: 1. + proto writeFieldStop. + proto writeMessageEnd. + ^ transport! ! + +!TProtocolRecursionDepthTest methodsFor: 'tests'! +testWriteAtLimitRoundTrips + "A chain exactly at the limit serializes without error." + self + shouldnt: [(self clientOn: TMemoryBuffer new) sendEchoA: (self chainOfDepth: self recursionLimit)] + raise: TProtocolError! ! + +!TProtocolRecursionDepthTest methodsFor: 'tests'! +testWriteOverLimitIsRejected + "A chain one level past the limit is rejected with TProtocolError." + self + should: [(self clientOn: TMemoryBuffer new) sendEchoA: (self chainOfDepth: self recursionLimit + 1)] + raise: TProtocolError! ! + +!TProtocolRecursionDepthTest methodsFor: 'tests'! +testReadWithinLimitRoundTrips + "A shallow payload is read back through the generated reader without error." + self + shouldnt: [(self clientOn: (self craftReplyOfDepth: 3)) recvEcho] + raise: TProtocolError! ! + +!TProtocolRecursionDepthTest methodsFor: 'tests'! +testReadOverLimitIsRejected + "A payload nested past the limit is rejected with TProtocolError on read." + self + should: [(self clientOn: (self craftReplyOfDepth: self recursionLimit + 6)) recvEcho] + raise: TProtocolError! ! diff --git a/lib/st/thrift.st b/lib/st/thrift.st index b2dbc9768e4..451d56dfa00 100644 --- a/lib/st/thrift.st +++ b/lib/st/thrift.st @@ -69,6 +69,10 @@ sizeLimit unknown ^ 0! ! +!TProtocolError class methodsFor: 'as yet unclassified'! +depthLimit + ^ 6! ! + TError subclass: #TTransportError instanceVariableNames: '' classVariableNames: '' @@ -191,7 +195,7 @@ type: anInteger type := anInteger! ! Object subclass: #TProtocol - instanceVariableNames: 'transport' + instanceVariableNames: 'transport recursionDepth' classVariableNames: '' poolDictionaries: '' category: 'Thrift-Protocol'! @@ -507,6 +511,17 @@ transport transport: aTransport transport := aTransport! ! +!TProtocol methodsFor: 'as yet unclassified'! +incrementRecursionDepth + recursionDepth := (recursionDepth ifNil: [0]) + 1. + recursionDepth > 64 ifTrue: [ + recursionDepth := recursionDepth - 1. + TProtocolError signal: 'Maximum recursion depth exceeded']! ! + +!TProtocol methodsFor: 'as yet unclassified'! +decrementRecursionDepth + recursionDepth := (recursionDepth ifNil: [0]) - 1! ! + !TProtocol methodsFor: 'writing' stamp: 'pc 10/24/2007 19:37'! writeBool: aBool! !