Copilot commented on code in PR #3856:
URL: https://github.com/apache/avro/pull/3856#discussion_r3564411646
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -276,17 +321,51 @@ private function snappyUncompress(string $compressed):
string
if (!extension_loaded('snappy')) {
throw new AvroException('Please install ext-snappy to use snappy
compression.');
}
+ $maxLength = self::maxDecompressLength();
+ // The Snappy block header declares the uncompressed length as a
varint;
+ // reject an over-large block before allocating for it.
+ $declared = self::snappyDeclaredLength(substr((string) $compressed, 0,
-4));
Review Comment:
The declared-length precheck relies on `snappyDeclaredLength()` returning a
correct integer. On 32-bit PHP (supported via GMP elsewhere in this codebase),
the current varint bit-shifting can overflow and wrap, potentially bypassing
the decompression-size guard. Passing `$maxLength` into the parser enables safe
early-capping before overflow.
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -220,17 +231,47 @@ 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();
+ // gzinflate caps its output at the given length: a block that would
+ // decompress to more than the limit yields false here without
+ // materializing the full (potentially huge) output. The '@' suppresses
+ // the "insufficient memory" notice zlib emits when the cap is hit.
+ $datum = @gzinflate($compressed, $maxLength + 1);
Review Comment:
`gzinflate()` returning `false` indicates a decompression failure (e.g.,
corrupt data), not that the output exceeded the cap. Currently this maps all
inflate failures to `AvroDataIODecompressionSizeException` and the comment
claims the cap yields `false`, which is misleading and changes the error type
for non-size failures.
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -220,17 +231,47 @@ 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;
+ }
Review Comment:
`maxDecompressLength()` can return `PHP_INT_MAX`, which makes `$maxLength +
1` overflow to a float in `gzUncompress()` (and can trigger a TypeError when
passed to `gzinflate`). Clamping the env override to `PHP_INT_MAX - 1` keeps
the inflate cap arithmetic safe.
##########
lang/php/lib/DataFile/AvroDataIOReader.php:
##########
@@ -276,17 +321,51 @@ private function snappyUncompress(string $compressed):
string
if (!extension_loaded('snappy')) {
throw new AvroException('Please install ext-snappy to use snappy
compression.');
}
+ $maxLength = self::maxDecompressLength();
+ // The Snappy block header declares the uncompressed length as a
varint;
+ // reject an over-large block before allocating for it.
+ $declared = self::snappyDeclaredLength(substr((string) $compressed, 0,
-4));
+ if (null !== $declared) {
+ self::checkDecompressLength($declared, $maxLength);
+ }
$crc32 = unpack('N', substr((string) $compressed, -4))[1];
$datum = snappy_uncompress(substr((string) $compressed, 0, -4));
if (false === $datum) {
throw new AvroException('snappy uncompression failed.');
}
+ self::checkDecompressLength(strlen($datum), $maxLength);
+
if ($crc32 !== crc32($datum)) {
throw new AvroException('snappy uncompression failed - crc32
mismatch.');
}
return $datum;
}
+
+ /**
+ * Return the uncompressed length declared in a raw Snappy block header,
+ * which prefixes the data as a little-endian base-128 varint. Returns null
+ * if the header cannot be parsed.
+ */
+ private static function snappyDeclaredLength(string $data): ?int
+ {
+ $result = 0;
+ $shift = 0;
+ $length = strlen($data);
+ for ($i = 0; $i < $length; $i++) {
+ $byte = ord($data[$i]);
+ $result |= ($byte & 0x7F) << $shift;
+ if (0 === ($byte & 0x80)) {
+ return $result;
+ }
+ $shift += 7;
+ if ($shift > 63) {
+ break;
+ }
+ }
+
+ return null;
+ }
Review Comment:
`snappyDeclaredLength()` uses unbounded bit-shifts into a PHP int. On 32-bit
builds this can overflow before the caller compares against `$maxLength`, which
can undermine the decompression size guard. Consider parsing with overflow
protection and an early cap at `$maxLength + 1`.
##########
lang/php/test/DataFileTest.php:
##########
@@ -386,6 +388,66 @@ 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
Review Comment:
Only DEFLATE is exercised by the new decompression-limit tests, but this PR
also changes the behavior for Snappy (declared-length precheck) and adds
post-decompression checks for Zstandard and Bzip2. Consider adding analogous
tests for these codecs (skipping when the relevant extension isn’t available)
so regressions in the size checks are caught.
##########
lang/php/lib/autoload.php:
##########
@@ -27,6 +27,7 @@
include __DIR__.'/AvroUtil.php';
include __DIR__.'/DataFile/AvroDataIO.php';
+include __DIR__.'/DataFile/AvroDataIODecompressionSizeException.php';
include __DIR__.'/DataFile/AvroDataIOException.php';
include __DIR__.'/DataFile/AvroDataIOReader.php';
Review Comment:
`autoload.php` includes `AvroDataIODecompressionSizeException` before
`AvroDataIOException`, but the new exception extends `AvroDataIOException`.
With this include-based loader, that ordering will cause a fatal error when
`autoload.php` is required. Include the parent class first.
--
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]
