diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e5d3713c70d..34c0554baf5 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -201,12 +201,16 @@ jobs: mkdir -p ./lib/php/test/Resources/packages/phpvo mkdir -p ./lib/php/test/Resources/packages/phpjs mkdir -p ./lib/php/test/Resources/packages/phpcm + mkdir -p ./lib/php/test/Resources/packages/phprec + mkdir -p ./lib/php/test/Resources/packages/phprecoop compiler/cpp/thrift --gen php:nsglobal="Basic" -r --out ./lib/php/test/Resources/packages/php lib/php/test/Resources/ThriftTest.thrift compiler/cpp/thrift --gen php:inlined,nsglobal="BasicInline" -r --out ./lib/php/test/Resources/packages/phpi lib/php/test/Resources/ThriftTest.thrift compiler/cpp/thrift --gen php:validate,nsglobal="Validate" -r --out ./lib/php/test/Resources/packages/phpv lib/php/test/Resources/ThriftTest.thrift compiler/cpp/thrift --gen php:validate,oop,nsglobal="ValidateOop" -r --out ./lib/php/test/Resources/packages/phpvo lib/php/test/Resources/ThriftTest.thrift compiler/cpp/thrift --gen php:json,nsglobal="Json" -r --out ./lib/php/test/Resources/packages/phpjs lib/php/test/Resources/ThriftTest.thrift compiler/cpp/thrift --gen php:classmap,server,rest,nsglobal="Classmap" -r --out ./lib/php/test/Resources/packages/phpcm lib/php/test/Resources/ThriftTest.thrift + compiler/cpp/thrift --gen php:nsglobal="PhpRec" -r --out ./lib/php/test/Resources/packages/phprec lib/php/test/Resources/RecursionDepth.thrift + compiler/cpp/thrift --gen php:oop,nsglobal="PhpRecOop" -r --out ./lib/php/test/Resources/packages/phprecoop lib/php/test/Resources/RecursionDepth.thrift - name: Run Tests run: vendor/bin/phpunit -c lib/php/phpunit.xml diff --git a/compiler/cpp/src/thrift/generate/t_php_generator.cc b/compiler/cpp/src/thrift/generate/t_php_generator.cc index 3b704a5a175..073bb9eb445 100644 --- a/compiler/cpp/src/thrift/generate/t_php_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_php_generator.cc @@ -1144,6 +1144,9 @@ void t_php_generator::generate_php_struct_reader(ostream& out, t_struct* tstruct // Declare stack tmp variables if (!binary_inline_) { + indent(out) << "$input->incrementRecursionDepth();" << '\n'; + indent(out) << "try {" << '\n'; + indent_up(); indent(out) << "$xfer += $input->readStructBegin($fname);" << '\n'; } @@ -1222,6 +1225,12 @@ void t_php_generator::generate_php_struct_reader(ostream& out, t_struct* tstruct if (!binary_inline_) { indent(out) << "$xfer += $input->readStructEnd();" << '\n'; + indent_down(); + indent(out) << "} finally {" << '\n'; + indent_up(); + indent(out) << "$input->decrementRecursionDepth();" << '\n'; + indent_down(); + indent(out) << "}" << '\n'; } if (needs_php_read_validator(tstruct, is_result)) { @@ -1265,6 +1274,9 @@ void t_php_generator::generate_php_struct_writer(ostream& out, t_struct* tstruct indent(out) << "$xfer = 0;" << '\n'; if (!binary_inline_) { + indent(out) << "$output->incrementRecursionDepth();" << '\n'; + indent(out) << "try {" << '\n'; + indent_up(); indent(out) << "$xfer += $output->writeStructBegin('" << name << "');" << '\n'; } @@ -1317,6 +1329,12 @@ void t_php_generator::generate_php_struct_writer(ostream& out, t_struct* tstruct } else { out << indent() << "$xfer += $output->writeFieldStop();" << '\n' << indent() << "$xfer += $output->writeStructEnd();" << '\n'; + indent_down(); + out << indent() << "} finally {" << '\n'; + indent_up(); + out << indent() << "$output->decrementRecursionDepth();" << '\n'; + indent_down(); + out << indent() << "}" << '\n'; } out << '\n'; diff --git a/lib/php/lib/Base/TBase.php b/lib/php/lib/Base/TBase.php index ae575a1461b..f0a7447e848 100644 --- a/lib/php/lib/Base/TBase.php +++ b/lib/php/lib/Base/TBase.php @@ -216,47 +216,52 @@ protected function readStruct(string $class, array $spec, TProtocol $input): int $fname = null; $ftype = 0; $fid = 0; - $xfer += $input->readStructBegin($fname); - while (true) { - $xfer += $input->readFieldBegin($fname, $ftype, $fid); - if ($ftype == TType::STOP) { - break; - } - if (isset($spec[$fid])) { - $fspec = $spec[$fid]; - $var = $fspec['var']; - if ($ftype == $fspec['type']) { - $xfer = 0; - if (isset(TBase::$tmethod[$ftype])) { - $func = 'read' . TBase::$tmethod[$ftype]; - $xfer += $input->$func($this->$var); - } else { - switch ($ftype) { - case TType::STRUCT: - $class = $fspec['class']; - $this->$var = new $class(); - $xfer += $this->$var->read($input); - break; - case TType::MAP: - $xfer += $this->readMap($this->$var, $fspec, $input); - break; - case TType::LST: - $xfer += $this->readList($this->$var, $fspec, $input, false); - break; - case TType::SET: - $xfer += $this->readList($this->$var, $fspec, $input, true); - break; + $input->incrementRecursionDepth(); + try { + $xfer += $input->readStructBegin($fname); + while (true) { + $xfer += $input->readFieldBegin($fname, $ftype, $fid); + if ($ftype == TType::STOP) { + break; + } + if (isset($spec[$fid])) { + $fspec = $spec[$fid]; + $var = $fspec['var']; + if ($ftype == $fspec['type']) { + $xfer = 0; + if (isset(TBase::$tmethod[$ftype])) { + $func = 'read' . TBase::$tmethod[$ftype]; + $xfer += $input->$func($this->$var); + } else { + switch ($ftype) { + case TType::STRUCT: + $class = $fspec['class']; + $this->$var = new $class(); + $xfer += $this->$var->read($input); + break; + case TType::MAP: + $xfer += $this->readMap($this->$var, $fspec, $input); + break; + case TType::LST: + $xfer += $this->readList($this->$var, $fspec, $input, false); + break; + case TType::SET: + $xfer += $this->readList($this->$var, $fspec, $input, true); + break; + } } + } else { + $xfer += $input->skip($ftype); } } else { $xfer += $input->skip($ftype); } - } else { - $xfer += $input->skip($ftype); + $xfer += $input->readFieldEnd(); } - $xfer += $input->readFieldEnd(); + $xfer += $input->readStructEnd(); + } finally { + $input->decrementRecursionDepth(); } - $xfer += $input->readStructEnd(); return $xfer; } @@ -380,36 +385,41 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set protected function writeStruct(string $class, array $spec, TProtocol $output): int { $xfer = 0; - $xfer += $output->writeStructBegin($class); - foreach ($spec as $fid => $fspec) { - $var = $fspec['var']; - if ($this->$var !== null) { - $ftype = $fspec['type']; - $xfer += $output->writeFieldBegin($var, $ftype, $fid); - if (isset(TBase::$tmethod[$ftype])) { - $func = 'write' . TBase::$tmethod[$ftype]; - $xfer += $output->$func($this->$var); - } else { - switch ($ftype) { - case TType::STRUCT: - $xfer += $this->$var->write($output); - break; - case TType::MAP: - $xfer += $this->writeMap($this->$var, $fspec, $output); - break; - case TType::LST: - $xfer += $this->writeList($this->$var, $fspec, $output, false); - break; - case TType::SET: - $xfer += $this->writeList($this->$var, $fspec, $output, true); - break; + $output->incrementRecursionDepth(); + try { + $xfer += $output->writeStructBegin($class); + foreach ($spec as $fid => $fspec) { + $var = $fspec['var']; + if ($this->$var !== null) { + $ftype = $fspec['type']; + $xfer += $output->writeFieldBegin($var, $ftype, $fid); + if (isset(TBase::$tmethod[$ftype])) { + $func = 'write' . TBase::$tmethod[$ftype]; + $xfer += $output->$func($this->$var); + } else { + switch ($ftype) { + case TType::STRUCT: + $xfer += $this->$var->write($output); + break; + case TType::MAP: + $xfer += $this->writeMap($this->$var, $fspec, $output); + break; + case TType::LST: + $xfer += $this->writeList($this->$var, $fspec, $output, false); + break; + case TType::SET: + $xfer += $this->writeList($this->$var, $fspec, $output, true); + break; + } } + $xfer += $output->writeFieldEnd(); } - $xfer += $output->writeFieldEnd(); } + $xfer += $output->writeFieldStop(); + $xfer += $output->writeStructEnd(); + } finally { + $output->decrementRecursionDepth(); } - $xfer += $output->writeFieldStop(); - $xfer += $output->writeStructEnd(); return $xfer; } diff --git a/lib/php/lib/Exception/TException.php b/lib/php/lib/Exception/TException.php index 9c188f2aa56..ffc43627861 100644 --- a/lib/php/lib/Exception/TException.php +++ b/lib/php/lib/Exception/TException.php @@ -215,47 +215,52 @@ protected function readStruct(string $class, array $spec, TProtocol $input): int $fname = null; $ftype = 0; $fid = 0; - $xfer += $input->readStructBegin($fname); - while (true) { - $xfer += $input->readFieldBegin($fname, $ftype, $fid); - if ($ftype == TType::STOP) { - break; - } - if (isset($spec[$fid])) { - $fspec = $spec[$fid]; - $var = $fspec['var']; - if ($ftype == $fspec['type']) { - $xfer = 0; - if (isset(TBase::$tmethod[$ftype])) { - $func = 'read' . TBase::$tmethod[$ftype]; - $xfer += $input->$func($this->$var); - } else { - switch ($ftype) { - case TType::STRUCT: - $class = $fspec['class']; - $this->$var = new $class(); - $xfer += $this->$var->read($input); - break; - case TType::MAP: - $xfer += $this->readMap($this->$var, $fspec, $input); - break; - case TType::LST: - $xfer += $this->readList($this->$var, $fspec, $input, false); - break; - case TType::SET: - $xfer += $this->readList($this->$var, $fspec, $input, true); - break; + $input->incrementRecursionDepth(); + try { + $xfer += $input->readStructBegin($fname); + while (true) { + $xfer += $input->readFieldBegin($fname, $ftype, $fid); + if ($ftype == TType::STOP) { + break; + } + if (isset($spec[$fid])) { + $fspec = $spec[$fid]; + $var = $fspec['var']; + if ($ftype == $fspec['type']) { + $xfer = 0; + if (isset(TBase::$tmethod[$ftype])) { + $func = 'read' . TBase::$tmethod[$ftype]; + $xfer += $input->$func($this->$var); + } else { + switch ($ftype) { + case TType::STRUCT: + $class = $fspec['class']; + $this->$var = new $class(); + $xfer += $this->$var->read($input); + break; + case TType::MAP: + $xfer += $this->readMap($this->$var, $fspec, $input); + break; + case TType::LST: + $xfer += $this->readList($this->$var, $fspec, $input, false); + break; + case TType::SET: + $xfer += $this->readList($this->$var, $fspec, $input, true); + break; + } } + } else { + $xfer += $input->skip($ftype); } } else { $xfer += $input->skip($ftype); } - } else { - $xfer += $input->skip($ftype); + $xfer += $input->readFieldEnd(); } - $xfer += $input->readFieldEnd(); + $xfer += $input->readStructEnd(); + } finally { + $input->decrementRecursionDepth(); } - $xfer += $input->readStructEnd(); return $xfer; } @@ -379,36 +384,41 @@ private function writeList(array $var, array $spec, TProtocol $output, bool $set protected function writeStruct(string $class, array $spec, TProtocol $output): int { $xfer = 0; - $xfer += $output->writeStructBegin($class); - foreach ($spec as $fid => $fspec) { - $var = $fspec['var']; - if ($this->$var !== null) { - $ftype = $fspec['type']; - $xfer += $output->writeFieldBegin($var, $ftype, $fid); - if (isset(TBase::$tmethod[$ftype])) { - $func = 'write' . TBase::$tmethod[$ftype]; - $xfer += $output->$func($this->$var); - } else { - switch ($ftype) { - case TType::STRUCT: - $xfer += $this->$var->write($output); - break; - case TType::MAP: - $xfer += $this->writeMap($this->$var, $fspec, $output); - break; - case TType::LST: - $xfer += $this->writeList($this->$var, $fspec, $output, false); - break; - case TType::SET: - $xfer += $this->writeList($this->$var, $fspec, $output, true); - break; + $output->incrementRecursionDepth(); + try { + $xfer += $output->writeStructBegin($class); + foreach ($spec as $fid => $fspec) { + $var = $fspec['var']; + if ($this->$var !== null) { + $ftype = $fspec['type']; + $xfer += $output->writeFieldBegin($var, $ftype, $fid); + if (isset(TBase::$tmethod[$ftype])) { + $func = 'write' . TBase::$tmethod[$ftype]; + $xfer += $output->$func($this->$var); + } else { + switch ($ftype) { + case TType::STRUCT: + $xfer += $this->$var->write($output); + break; + case TType::MAP: + $xfer += $this->writeMap($this->$var, $fspec, $output); + break; + case TType::LST: + $xfer += $this->writeList($this->$var, $fspec, $output, false); + break; + case TType::SET: + $xfer += $this->writeList($this->$var, $fspec, $output, true); + break; + } } + $xfer += $output->writeFieldEnd(); } - $xfer += $output->writeFieldEnd(); } + $xfer += $output->writeFieldStop(); + $xfer += $output->writeStructEnd(); + } finally { + $output->decrementRecursionDepth(); } - $xfer += $output->writeFieldStop(); - $xfer += $output->writeStructEnd(); return $xfer; } diff --git a/lib/php/lib/Protocol/TProtocol.php b/lib/php/lib/Protocol/TProtocol.php index 42eaf71dee6..3eafd93dbd4 100644 --- a/lib/php/lib/Protocol/TProtocol.php +++ b/lib/php/lib/Protocol/TProtocol.php @@ -35,6 +35,10 @@ */ abstract class TProtocol { + public const DEFAULT_RECURSION_DEPTH = 64; + + private int $recursionDepth = 0; + protected function __construct(protected TTransport $trans) { } @@ -44,6 +48,20 @@ public function getTransport(): TTransport return $this->trans; } + public function incrementRecursionDepth(): void + { + ++$this->recursionDepth; + if ($this->recursionDepth > self::DEFAULT_RECURSION_DEPTH) { + --$this->recursionDepth; + throw new TProtocolException('Maximum recursion depth exceeded', TProtocolException::DEPTH_LIMIT); + } + } + + public function decrementRecursionDepth(): void + { + --$this->recursionDepth; + } + abstract public function writeMessageBegin(string $name, int $type, int $seqid): int; abstract public function writeMessageEnd(): int; diff --git a/lib/php/test/Integration/Lib/Protocol/RecursionDepthTest.php b/lib/php/test/Integration/Lib/Protocol/RecursionDepthTest.php new file mode 100644 index 00000000000..b648deba790 --- /dev/null +++ b/lib/php/test/Integration/Lib/Protocol/RecursionDepthTest.php @@ -0,0 +1,178 @@ +, 1: class-string, 2: string}> + */ + public static function caseProvider(): array + { + $modes = [ + 'inline' => ['\PhpRec\RecTree', '\PhpRec\RecUnion', '\PhpRec\RecError'], + 'oop' => ['\PhpRecOop\RecTree', '\PhpRecOop\RecUnion', '\PhpRecOop\RecError'], + ]; + $protocols = [ + 'binary' => TBinaryProtocol::class, + 'compact' => TCompactProtocol::class, + 'json' => TJSONProtocol::class, + ]; + + $cases = []; + foreach ($modes as $mode => [$treeClass, $unionClass, $errorClass]) { + foreach ($protocols as $protocol => $protocolClass) { + $cases["$mode/$protocol/struct"] = [$protocolClass, $treeClass, 'item']; + $cases["$mode/$protocol/union"] = [$protocolClass, $unionClass, 'leaf']; + $cases["$mode/$protocol/exception"] = [$protocolClass, $errorClass, 'leaf']; + } + } + + return $cases; + } + + #[DataProvider('caseProvider')] + public function testRoundTripsAtTheDepthLimit(string $protocolClass, string $class, string $leafField): void + { + $original = $this->makeChain($class, $leafField, self::LIMIT); + + $buffer = new TMemoryBuffer(); + $original->write(new $protocolClass($buffer)); + + $decoded = new $class(); + $decoded->read(new $protocolClass(new TMemoryBuffer($buffer->getBuffer()))); + + $this->assertSame(self::LIMIT, $this->chainDepth($decoded)); + } + + #[DataProvider('caseProvider')] + public function testWritingPastTheDepthLimitThrows(string $protocolClass, string $class, string $leafField): void + { + $tooDeep = $this->makeChain($class, $leafField, self::LIMIT + 5); + + $error = $this->captureThrowable(function () use ($tooDeep, $protocolClass) { + $tooDeep->write(new $protocolClass(new TMemoryBuffer())); + }); + + $this->assertInstanceOf(TProtocolException::class, $error); + $this->assertSame(TProtocolException::DEPTH_LIMIT, $error->getCode()); + } + + #[DataProvider('caseProvider')] + public function testReadingPastTheDepthLimitThrows(string $protocolClass, string $class, string $leafField): void + { + // Craft the over-limit payload with raw protocol primitives, which are + // not depth-bounded, so the generated reader recurses through the + // guarded struct path (field id 1 / list / struct), not skip(). + $writeBuffer = new TMemoryBuffer(); + $this->writeDeepStruct(new $protocolClass($writeBuffer), self::LIMIT + 5); + + $readProtocol = new $protocolClass(new TMemoryBuffer($writeBuffer->getBuffer())); + $error = $this->captureThrowable(function () use ($readProtocol, $class) { + (new $class())->read($readProtocol); + }); + + $this->assertInstanceOf(TProtocolException::class, $error); + $this->assertSame(TProtocolException::DEPTH_LIMIT, $error->getCode()); + } + + private function makeChain(string $class, string $leafField, int $depth): object + { + $node = new $class([$leafField => 1]); + for ($i = 1; $i < $depth; $i++) { + $node = new $class(['children' => [$node]]); + } + + return $node; + } + + private function chainDepth(?object $node): int + { + $depth = 0; + while ($node !== null) { + $depth++; + $node = empty($node->children) ? null : $node->children[0]; + } + + return $depth; + } + + /** + * Emit "$levels" nested structs whose recursive field (id 1, list) + * holds exactly one child, so a generated reader recurses $levels deep + * through the guarded struct path. + */ + private function writeDeepStruct(TProtocol $output, int $levels): void + { + for ($i = 0; $i < $levels - 1; $i++) { + $output->writeStructBegin('Rec'); + $output->writeFieldBegin('children', TType::LST, 1); + $output->writeListBegin(TType::STRUCT, 1); + } + $output->writeStructBegin('Rec'); + $output->writeFieldStop(); + $output->writeStructEnd(); + for ($i = 0; $i < $levels - 1; $i++) { + $output->writeListEnd(); + $output->writeFieldEnd(); + $output->writeFieldStop(); + $output->writeStructEnd(); + } + } + + private function captureThrowable(callable $fn): ?\Throwable + { + try { + $fn(); + } catch (\Throwable $e) { + return $e; + } + + return null; + } +} diff --git a/lib/php/test/Makefile.am b/lib/php/test/Makefile.am index 674b07160cb..e7eea2882c7 100644 --- a/lib/php/test/Makefile.am +++ b/lib/php/test/Makefile.am @@ -19,19 +19,23 @@ PHPUNIT=php $(top_srcdir)/vendor/bin/phpunit -stubs: Resources/ThriftTest.thrift +stubs: Resources/ThriftTest.thrift Resources/RecursionDepth.thrift mkdir -p ./Resources/packages/php mkdir -p ./Resources/packages/phpi mkdir -p ./Resources/packages/phpv mkdir -p ./Resources/packages/phpvo mkdir -p ./Resources/packages/phpjs mkdir -p ./Resources/packages/phpcm + mkdir -p ./Resources/packages/phprec + mkdir -p ./Resources/packages/phprecoop $(THRIFT) --gen php:nsglobal="Basic" -r --out ./Resources/packages/php Resources/ThriftTest.thrift $(THRIFT) --gen php:inlined,nsglobal="BasicInline" -r --out ./Resources/packages/phpi Resources/ThriftTest.thrift $(THRIFT) --gen php:validate,nsglobal="Validate" -r --out ./Resources/packages/phpv Resources/ThriftTest.thrift $(THRIFT) --gen php:validate,oop,nsglobal="ValidateOop" -r --out ./Resources/packages/phpvo Resources/ThriftTest.thrift $(THRIFT) --gen php:json,nsglobal="Json" -r --out ./Resources/packages/phpjs Resources/ThriftTest.thrift $(THRIFT) --gen php:classmap,server,rest,nsglobal="Classmap" -r --out ./Resources/packages/phpcm Resources/ThriftTest.thrift + $(THRIFT) --gen php:nsglobal="PhpRec" -r --out ./Resources/packages/phprec Resources/RecursionDepth.thrift + $(THRIFT) --gen php:oop,nsglobal="PhpRecOop" -r --out ./Resources/packages/phprecoop Resources/RecursionDepth.thrift deps: $(top_srcdir)/composer.json composer install --working-dir=$(top_srcdir) diff --git a/lib/php/test/Resources/RecursionDepth.thrift b/lib/php/test/Resources/RecursionDepth.thrift new file mode 100644 index 00000000000..ac6482b799d --- /dev/null +++ b/lib/php/test/Resources/RecursionDepth.thrift @@ -0,0 +1,36 @@ +/* + * 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. + */ + +// Recursive types used to exercise the struct/union/exception read/write +// recursion-depth limit in the PHP library. + +struct RecTree { + 1: list children + 2: i16 item +} + +union RecUnion { + 1: list children + 2: i32 leaf +} + +exception RecError { + 1: list children + 2: i32 leaf +} diff --git a/lib/php/test/bootstrap.php b/lib/php/test/bootstrap.php index 5fcdadbc88a..51863a52830 100644 --- a/lib/php/test/bootstrap.php +++ b/lib/php/test/bootstrap.php @@ -12,6 +12,8 @@ $loader->registerNamespace('Validate', __DIR__ . '/Resources/packages/phpv'); $loader->registerNamespace('ValidateOop', __DIR__ . '/Resources/packages/phpvo'); $loader->registerNamespace('Json', __DIR__ . '/Resources/packages/phpjs'); +$loader->registerNamespace('PhpRec', __DIR__ . '/Resources/packages/phprec'); +$loader->registerNamespace('PhpRecOop', __DIR__ . '/Resources/packages/phprecoop'); #do not load this namespace here, it will be loaded in ClassLoaderTest //$loader->registerNamespace('Server', __DIR__ . '/Resources/packages/phpcm');