iemejia commented on code in PR #3856:
URL: https://github.com/apache/avro/pull/3856#discussion_r3567373275
##########
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:
Fixed — reworded to 'deflate uncompression failed.' (this is the raw-deflate
ZLIB_ENCODING_RAW path).
##########
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:
Fixed — 'deflate uncompression failed.'
##########
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:
Fixed — 'deflate 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');
+ } 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:
Fixed — wrapped the reader in try/finally so `$dr->close()` always runs, and
use `$this->fail()` when no exception is thrown (dropped the $thrown flag).
##########
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:
Fixed — try/finally around the reader close, failing explicitly if the
expected exception isn't thrown.
##########
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:
Fixed — the deflate-limit test now closes the reader in a finally and fails
explicitly via `$this->fail()`.
--
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]
