Copilot commented on code in PR #3856:
URL: https://github.com/apache/avro/pull/3856#discussion_r3567335264
##########
lang/php/test/DataFileTest.php:
##########
@@ -411,4 +497,38 @@ protected static function remove_data_file($data_file):
void
unlink($data_file);
}
}
+
+ /**
+ * Write a single, highly compressible block with the given codec, then
read
+ * it back with a small decompression limit and assert it is rejected.
+ */
+ private function assertCodecRejectsOversizedBlock(string $codec): void
+ {
+ $data_file =
$this->add_data_file(sprintf('data-decompress-limit-%s.avr', $codec));
+ $dw = AvroDataIO::openFile($data_file, 'w', '"string"', $codec);
+ $dw->append(str_repeat('a', 64 * 1024)); // 64 KiB, compresses tiny
+ $dw->close();
+
+ $previous = getenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV);
+ putenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV.'=1024');
+
+ try {
+ $dr = AvroDataIO::openFile($data_file);
+ $thrown = false;
+
+ try {
+ $dr->data();
+ } catch (AvroDataIODecompressionSizeException $e) {
+ $thrown = true;
+ }
+ $dr->close();
+ $this->assertTrue($thrown, sprintf('expected a decompression size
exception for %s', $codec));
Review Comment:
As in the deflate-specific test, $dr->close() isn’t guaranteed to run if
$dr->data() throws an unexpected exception. A try/finally around the reader
close avoids leaking file handles and lets you avoid the $thrown flag by
failing explicitly when no exception is thrown.
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -220,18 +234,69 @@ private function readBlockHeader(): string|int
return $this->decoder->readLong();
}
+ /**
+ * The maximum number of bytes a single block is allowed to decompress to.
+ */
+ private static function maxDecompressLength(): int
+ {
+ $value = getenv(self::MAX_DECOMPRESS_LENGTH_ENV);
+ if (false !== $value && ctype_digit($value) && (int) $value > 0) {
+ return (int) $value;
+ }
+
+ return self::DEFAULT_MAX_DECOMPRESS_LENGTH;
+ }
+
+ /**
+ * @throws AvroDataIODecompressionSizeException if the length exceeds the
limit
+ */
+ private static function checkDecompressLength(int $length, int
$maxLength): void
+ {
+ if ($length > $maxLength) {
+ throw new AvroDataIODecompressionSizeException($maxLength);
+ }
+ }
+
/**
* @throws AvroException
*/
private function gzUncompress(string $compressed): string
{
- $datum = gzinflate($compressed);
+ $maxLength = self::maxDecompressLength();
+ $context = inflate_init(ZLIB_ENCODING_RAW);
+ if (false === $context) {
+ throw new AvroException('gzip uncompression failed.');
+ }
- if (false === $datum) {
+ // Inflate in chunks and check the running total after each step so an
+ // over-large (or malicious) block is rejected before its whole output
is
+ // accumulated (each inflated chunk is still materialized), while
genuine
+ // decompression errors (inflate_add === false) are reported
distinctly.
+ // Pieces are collected and joined once at the end to avoid repeatedly
+ // reallocating a growing result string.
+ $pieces = [];
+ $total = 0;
+ $length = strlen($compressed);
+ for ($offset = 0; $offset < $length; $offset +=
self::INFLATE_CHUNK_SIZE) {
+ $piece = substr($compressed, $offset, self::INFLATE_CHUNK_SIZE);
+ $out = @inflate_add($context, $piece);
+ if (false === $out) {
+ throw new AvroException('gzip uncompression failed.');
+ }
Review Comment:
This error message says “gzip uncompression failed” but the operation is
DEFLATE (raw) decompression. Aligning the wording with the codec name makes
failures easier to interpret.
##########
lang/php/test/DataFileTest.php:
##########
@@ -386,6 +388,90 @@ public function
test_differing_schemas_with_complex_objects(): void
}
}
+ /**
+ * A block with a very high compression ratio can expand to far more memory
+ * than its compressed size; reading such a block must be rejected once its
+ * decompressed size would exceed the configured maximum.
+ */
+ public function test_deflate_block_decompression_limit(): void
+ {
+ $data_file = $this->add_data_file('data-decompress-limit-deflate.avr');
+ $dw = AvroDataIO::openFile($data_file, 'w', '"string"',
AvroDataIO::DEFLATE_CODEC);
+ $dw->append(str_repeat('a', 64 * 1024)); // 64 KiB, compresses tiny
+ $dw->close();
+
+ $previous = getenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV);
+ putenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV.'=1024');
+
+ try {
+ $dr = AvroDataIO::openFile($data_file);
+ $thrown = false;
+
+ try {
+ $dr->data();
+ } catch (AvroDataIODecompressionSizeException $e) {
+ $thrown = true;
+ }
+ $dr->close();
+ $this->assertTrue($thrown, 'expected a decompression size
exception');
+ } finally {
+ if (false === $previous) {
+ putenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV);
+ } else {
+
putenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV.'='.$previous);
+ }
+ }
+ }
+
+ public function test_deflate_block_within_decompression_limit(): void
+ {
+ $data_file = $this->add_data_file('data-decompress-within-limit.avr');
+ $payload = 'hello world';
+ $dw = AvroDataIO::openFile($data_file, 'w', '"string"',
AvroDataIO::DEFLATE_CODEC);
+ $dw->append($payload);
+ $dw->close();
+
+ $previous = getenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV);
+ putenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV.'=1048576');
+
+ try {
+ $dr = AvroDataIO::openFile($data_file);
+ $data = $dr->data();
+ $dr->close();
+ $this->assertSame([$payload], $data);
Review Comment:
If $dr->data() throws, $dr->close() won’t run. Using a try/finally ensures
the reader is always closed even on failure paths.
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -220,18 +234,69 @@ private function readBlockHeader(): string|int
return $this->decoder->readLong();
}
+ /**
+ * The maximum number of bytes a single block is allowed to decompress to.
+ */
+ private static function maxDecompressLength(): int
+ {
+ $value = getenv(self::MAX_DECOMPRESS_LENGTH_ENV);
+ if (false !== $value && ctype_digit($value) && (int) $value > 0) {
+ return (int) $value;
+ }
+
+ return self::DEFAULT_MAX_DECOMPRESS_LENGTH;
+ }
+
+ /**
+ * @throws AvroDataIODecompressionSizeException if the length exceeds the
limit
+ */
+ private static function checkDecompressLength(int $length, int
$maxLength): void
+ {
+ if ($length > $maxLength) {
+ throw new AvroDataIODecompressionSizeException($maxLength);
+ }
+ }
+
/**
* @throws AvroException
*/
private function gzUncompress(string $compressed): string
{
- $datum = gzinflate($compressed);
+ $maxLength = self::maxDecompressLength();
+ $context = inflate_init(ZLIB_ENCODING_RAW);
+ if (false === $context) {
+ throw new AvroException('gzip uncompression failed.');
+ }
- if (false === $datum) {
+ // Inflate in chunks and check the running total after each step so an
+ // over-large (or malicious) block is rejected before its whole output
is
+ // accumulated (each inflated chunk is still materialized), while
genuine
+ // decompression errors (inflate_add === false) are reported
distinctly.
+ // Pieces are collected and joined once at the end to avoid repeatedly
+ // reallocating a growing result string.
+ $pieces = [];
+ $total = 0;
+ $length = strlen($compressed);
+ for ($offset = 0; $offset < $length; $offset +=
self::INFLATE_CHUNK_SIZE) {
+ $piece = substr($compressed, $offset, self::INFLATE_CHUNK_SIZE);
+ $out = @inflate_add($context, $piece);
+ if (false === $out) {
+ throw new AvroException('gzip uncompression failed.');
+ }
+ $pieces[] = $out;
+ $total += strlen($out);
+ self::checkDecompressLength($total, $maxLength);
+ }
+
+ $out = @inflate_add($context, '', ZLIB_FINISH);
+ if (false === $out) {
throw new AvroException('gzip uncompression failed.');
}
Review Comment:
As above, this is the DEFLATE codec path; the error message should say
“deflate uncompression failed” rather than “gzip uncompression failed”.
##########
lang/php/test/DataFileTest.php:
##########
@@ -386,6 +388,90 @@ public function
test_differing_schemas_with_complex_objects(): void
}
}
+ /**
+ * A block with a very high compression ratio can expand to far more memory
+ * than its compressed size; reading such a block must be rejected once its
+ * decompressed size would exceed the configured maximum.
+ */
+ public function test_deflate_block_decompression_limit(): void
+ {
+ $data_file = $this->add_data_file('data-decompress-limit-deflate.avr');
+ $dw = AvroDataIO::openFile($data_file, 'w', '"string"',
AvroDataIO::DEFLATE_CODEC);
+ $dw->append(str_repeat('a', 64 * 1024)); // 64 KiB, compresses tiny
+ $dw->close();
+
+ $previous = getenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV);
+ putenv(AvroDataIOReader::MAX_DECOMPRESS_LENGTH_ENV.'=1024');
+
+ try {
+ $dr = AvroDataIO::openFile($data_file);
+ $thrown = false;
+
+ try {
+ $dr->data();
+ } catch (AvroDataIODecompressionSizeException $e) {
+ $thrown = true;
+ }
+ $dr->close();
+ $this->assertTrue($thrown, 'expected a decompression size
exception');
Review Comment:
$dr->close() won’t run if $dr->data() throws an unexpected exception (i.e.,
not AvroDataIODecompressionSizeException), which can leak file handles and make
later tests flaky. Wrapping the reader usage in a try/finally also lets you
avoid the $thrown flag by failing explicitly when no exception is thrown.
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -220,18 +234,69 @@ private function readBlockHeader(): string|int
return $this->decoder->readLong();
}
+ /**
+ * The maximum number of bytes a single block is allowed to decompress to.
+ */
+ private static function maxDecompressLength(): int
+ {
+ $value = getenv(self::MAX_DECOMPRESS_LENGTH_ENV);
+ if (false !== $value && ctype_digit($value) && (int) $value > 0) {
+ return (int) $value;
+ }
+
+ return self::DEFAULT_MAX_DECOMPRESS_LENGTH;
+ }
+
+ /**
+ * @throws AvroDataIODecompressionSizeException if the length exceeds the
limit
+ */
+ private static function checkDecompressLength(int $length, int
$maxLength): void
+ {
+ if ($length > $maxLength) {
+ throw new AvroDataIODecompressionSizeException($maxLength);
+ }
+ }
+
/**
* @throws AvroException
*/
private function gzUncompress(string $compressed): string
{
- $datum = gzinflate($compressed);
+ $maxLength = self::maxDecompressLength();
+ $context = inflate_init(ZLIB_ENCODING_RAW);
+ if (false === $context) {
+ throw new AvroException('gzip uncompression failed.');
+ }
Review Comment:
This code path is used for the data-file DEFLATE codec (raw deflate via
ZLIB_ENCODING_RAW), but the error message says “gzip uncompression failed”.
Using “deflate” here would be more accurate for users debugging codec issues.
--
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]
