sveneld commented on code in PR #3552:
URL: https://github.com/apache/thrift/pull/3552#discussion_r3320371107
##########
lib/php/lib/Base/TBase.php:
##########
@@ -216,6 +216,8 @@ protected function readStruct(string $class, array $spec,
TProtocol $input): int
$fname = null;
$ftype = 0;
$fid = 0;
+ $input->incrementRecursionDepth();
+ try {
$xfer += $input->readStructBegin($fname);
Review Comment:
Style: the `try { ... }` block body isn't indented relative to the
surrounding code, so the body sits at the same column as `try {` itself. PSR-12
expects the body to be one level deeper. The same shape is in `writeStruct`
below. CI is green only because phpcs doesn't enforce
`Generic.WhiteSpace.ScopeIndent` for this rule, but visually it reads as if the
`try` had no body. Suggest:
```suggestion
$input->incrementRecursionDepth();
try {
$xfer += $input->readStructBegin($fname);
```
…and reindent the matched closing brace + every line in between.
##########
lib/php/lib/Base/TBase.php:
##########
@@ -380,6 +385,8 @@ private function writeList(array $var, array $spec,
TProtocol $output, bool $set
protected function writeStruct(string $class, array $spec, TProtocol
$output): int
{
$xfer = 0;
+ $output->incrementRecursionDepth();
+ try {
$xfer += $output->writeStructBegin($class);
Review Comment:
Same indent issue as in `readStruct` — body of `try {` is one level
shallower than it should be.
##########
lib/php/test/Unit/Lib/Protocol/TProtocolTest.php:
##########
@@ -264,6 +264,40 @@ public function testSkipBinaryThrowsForUnknownType(): void
TProtocol::skipBinary(new TMemoryBuffer(), 999);
}
+ public function testIncrementRecursionDepthAllowsUpToLimit(): void
+ {
+ $transport = new TMemoryBuffer();
+ $protocol = new TBinaryProtocol($transport);
+ for ($i = 0; $i < TProtocol::DEFAULT_RECURSION_DEPTH; $i++) {
+ $protocol->incrementRecursionDepth();
+ }
+ $this->assertTrue(true); // no exception thrown
+ }
+
+ public function testIncrementRecursionDepthThrowsAtLimit(): void
+ {
Review Comment:
Nit: PHPUnit canonical way to express “the code under test must not throw
and has no value to assert on” is `$this->expectNotToPerformAssertions();` at
the top of the test, rather than a trailing `assertTrue(true)`. Same for
`testDecrementRecursionDepthRestoresCapacity` below.
##########
lib/php/test/Unit/Lib/Protocol/TProtocolTest.php:
##########
@@ -264,6 +264,40 @@ public function testSkipBinaryThrowsForUnknownType(): void
TProtocol::skipBinary(new TMemoryBuffer(), 999);
}
+ public function testIncrementRecursionDepthAllowsUpToLimit(): void
+ {
+ $transport = new TMemoryBuffer();
+ $protocol = new TBinaryProtocol($transport);
+ for ($i = 0; $i < TProtocol::DEFAULT_RECURSION_DEPTH; $i++) {
+ $protocol->incrementRecursionDepth();
+ }
+ $this->assertTrue(true); // no exception thrown
+ }
+
+ public function testIncrementRecursionDepthThrowsAtLimit(): void
+ {
+ $this->expectException(TProtocolException::class);
+ $this->expectExceptionCode(TProtocolException::DEPTH_LIMIT);
+
+ $transport = new TMemoryBuffer();
+ $protocol = new TBinaryProtocol($transport);
+ for ($i = 0; $i <= TProtocol::DEFAULT_RECURSION_DEPTH; $i++) {
+ $protocol->incrementRecursionDepth();
+ }
+ }
+
+ public function testDecrementRecursionDepthRestoresCapacity(): void
+ {
+ $transport = new TMemoryBuffer();
+ $protocol = new TBinaryProtocol($transport);
+ for ($i = 0; $i < TProtocol::DEFAULT_RECURSION_DEPTH; $i++) {
+ $protocol->incrementRecursionDepth();
+ }
+ $protocol->decrementRecursionDepth();
+ $protocol->incrementRecursionDepth(); // should not throw
+ $this->assertTrue(true);
+ }
+
private function buildBinaryBuffer(callable $writer): string
{
Review Comment:
Same nit — prefer `$this->expectNotToPerformAssertions();` over
`assertTrue(true)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
