AVRO-4289: [php] Enforce a maximum decompressed block size#3856
AVRO-4289: [php] Enforce a maximum decompressed block size#3856iemejia wants to merge 9 commits into
Conversation
When reading a data file, each block is decompressed according to the file's codec. A block with a very high compression ratio (or a malformed block) could expand to far more memory than its compressed size. Enforce a configurable maximum decompressed size across the deflate, zstandard, snappy and bzip2 codecs, mirroring the Java SDK's decompression limit (AVRO-4247): deflate caps its output via gzinflate's max length so the allocation itself is bounded, and snappy rejects an over-large declared length up front. The limit defaults to 200 MiB and can be overridden with the AVRO_MAX_DECOMPRESS_LENGTH environment variable; exceeding it throws AvroDataIODecompressionSizeException. Assisted-by: GitHub Copilot:claude-opus-4.8
Reorder the new public test methods before the protected helper and drop spaces around string concatenation to match the lint rules. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
This PR hardens the PHP data-file reader against decompression bombs by enforcing a configurable maximum decompressed block size (default 200 MiB) and raising a dedicated exception when a block exceeds that limit.
Changes:
- Add
AVRO_MAX_DECOMPRESS_LENGTH(defaulting to 200 MiB) and enforce the limit during block decompression inAvroDataIOReader. - Introduce
AvroDataIODecompressionSizeExceptionto signal limit violations. - Add PHPUnit coverage for DEFLATE blocks that exceed / remain within the configured limit.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| lang/php/lib/DataFile/AvroDataIOReader.php | Adds max decompressed-size enforcement across codecs and Snappy declared-length parsing. |
| lang/php/lib/DataFile/AvroDataIODecompressionSizeException.php | New exception type for decompression-size limit violations. |
| lang/php/test/DataFileTest.php | New tests covering the decompression limit for DEFLATE blocks. |
| lang/php/lib/autoload.php | Registers the new exception file in the include-based loader. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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); |
There was a problem hiding this comment.
Fixed in fc78dc1 — gzUncompress now streams via inflate_add in chunks: genuine inflate errors (inflate_add === false) are reported as a gzip failure, while only an actual over-limit output raises AvroDataIODecompressionSizeException. No more misclassifying corrupt data.
| $value = getenv(self::MAX_DECOMPRESS_LENGTH_ENV); | ||
| if (false !== $value && ctype_digit($value) && (int) $value > 0) { | ||
| return (int) $value; | ||
| } |
There was a problem hiding this comment.
Fixed in fc78dc1 — switching to streaming inflate_add removes the $maxLength + 1 arithmetic entirely, so there is no float overflow near PHP_INT_MAX.
| $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)); |
There was a problem hiding this comment.
Fixed in fc78dc1 — the snappy declared-length parse (now ensureSnappyWithinLimit) caps the running value against the limit and treats any 32-bit wrap to negative as over-limit, so the guard holds on 32-bit builds.
| /** | ||
| * 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; | ||
| } |
There was a problem hiding this comment.
Fixed in fc78dc1 — see above: ensureSnappyWithinLimit parses with overflow protection and an early cap at the configured limit.
| /** | ||
| * 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 |
There was a problem hiding this comment.
Fixed in fc78dc1 — added snappy/zstandard/bzip2 decompression-limit tests (skipped when the extension is unavailable).
| include __DIR__.'/DataFile/AvroDataIO.php'; | ||
| include __DIR__.'/DataFile/AvroDataIODecompressionSizeException.php'; | ||
| include __DIR__.'/DataFile/AvroDataIOException.php'; | ||
| include __DIR__.'/DataFile/AvroDataIOReader.php'; |
There was a problem hiding this comment.
Fixed in fc78dc1 — autoload.php now includes AvroDataIOException before its subclass AvroDataIODecompressionSizeException, avoiding the fatal error under the include-based loader.
…nappy overflow - autoload.php: include AvroDataIOException before its subclass AvroDataIODecompressionSizeException (the previous order caused a fatal error under the include-based loader). - gzUncompress now streams via inflate_add in chunks: this bounds allocation incrementally and reports genuine inflate errors distinctly from an over-limit block (no more misclassifying corrupt data, and no maxLength+1 arithmetic that could overflow to a float near PHP_INT_MAX). - Replace snappyDeclaredLength with ensureSnappyWithinLimit, which caps the running varint against the limit and treats any 32-bit wrap as over-limit, so the guard holds on 32-bit builds. - Add snappy/zstandard/bzip2 decompression-limit tests (skipped when the extension is unavailable). Assisted-by: GitHub Copilot:claude-opus-4.8
| $shift += 7; | ||
| if ($shift > 28) { | ||
| return; // more than 5 bytes: malformed; the post-decompress check will catch it | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed. ensureSnappyWithinLimit() now throws AvroException('snappy uncompression failed - malformed length header.') when the varint exceeds five bytes, instead of silently returning; a uint32 Snappy length never needs more than five bytes. Pushed in 0d74370.
| $datum = ''; | ||
| $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.'); | ||
| } | ||
| $datum .= $out; | ||
| self::checkDecompressLength(strlen($datum), $maxLength); | ||
| } | ||
|
|
||
| $out = @inflate_add($context, '', ZLIB_FINISH); | ||
| if (false === $out) { | ||
| throw new AvroException('gzip uncompression failed.'); | ||
| } | ||
| $datum .= $out; | ||
| self::checkDecompressLength(strlen($datum), $maxLength); | ||
|
|
||
| return $datum; | ||
| } |
There was a problem hiding this comment.
Fixed. gzUncompress() now collects inflated chunks into an array and implode()s once at the end, tracking the running total separately for the per-chunk size check, avoiding repeated reallocation of a growing result string. Pushed in 0d74370.
…te append - ensureSnappyWithinLimit now rejects a Snappy length header longer than five varint bytes as malformed instead of silently deferring it; a uint32 length never needs more than five bytes. - gzUncompress collects inflated chunks into an array and joins once at the end instead of repeatedly appending to a growing string, avoiding quadratic reallocation on large blocks. The running total is tracked separately so the per-chunk size check is unchanged. Assisted-by: GitHub Copilot:claude-opus-4.8
| $maxLength = self::maxDecompressLength(); | ||
| // The Snappy block header declares the uncompressed length as a varint; | ||
| // reject an over-large block before allocating for it. Parsed with an | ||
| // early cap so it stays correct even if a 32-bit int would overflow. | ||
| self::ensureSnappyWithinLimit(substr((string) $compressed, 0, -4), $maxLength); | ||
| $crc32 = unpack('N', substr((string) $compressed, -4))[1]; | ||
| $datum = snappy_uncompress(substr((string) $compressed, 0, -4)); | ||
|
|
There was a problem hiding this comment.
Fixed. snappyUncompress() now rejects a block shorter than the 4-byte CRC trailer and checks unpack()'s result before indexing, instead of slicing with negative offsets and reading [1] off a false return. The payload slice is computed once and reused for the precheck and snappy_uncompress(). Pushed in 1fe9871.
| private static function ensureSnappyWithinLimit(string $data, int $maxLength): void | ||
| { | ||
| $result = 0; | ||
| $shift = 0; | ||
| $length = strlen($data); | ||
| for ($i = 0; $i < $length; $i++) { | ||
| $byte = ord($data[$i]); | ||
| $result += ($byte & 0x7F) << $shift; | ||
| if ($result < 0 || $result > $maxLength) { | ||
| throw new AvroDataIODecompressionSizeException($maxLength); | ||
| } | ||
| if (0 === ($byte & 0x80)) { | ||
| return; // declared length is within the limit | ||
| } | ||
| $shift += 7; | ||
| if ($shift > 28) { | ||
| // A Snappy uncompressed length is a uint32, encoded in at most | ||
| // five varint bytes; a longer encoding is malformed. | ||
| throw new AvroException('snappy uncompression failed - malformed length header.'); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed. ensureSnappyWithinLimit() now throws when the loop ends without a terminating byte (input ends with all continuation bits set), so a truncated/never-terminating length header is rejected as malformed rather than silently succeeding. Pushed in 1fe9871.
- snappyUncompress() now rejects a block shorter than the 4-byte CRC trailer and checks unpack()'s result, instead of slicing with negative offsets and reading [1] off a false return (which would raise a warning/fatal). The payload slice is computed once and reused for the precheck and snappy_uncompress(). - ensureSnappyWithinLimit() now rejects a length varint that never terminates (input ends with all continuation bits set) as malformed, rather than falling through and silently succeeding, so a truncated header cannot bypass the guard. Assisted-by: GitHub Copilot:claude-opus-4.8
| // Inflate in chunks and check the running length after each step so an | ||
| // over-large (or malicious) block is rejected without materializing the | ||
| // full output, 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. |
There was a problem hiding this comment.
Fixed. Reworded the comment so it no longer implies the full output is never materialized: each inflated chunk is materialized, but the running total is checked after every chunk so an over-large block is rejected before its whole output is accumulated. Pushed in e7bdb4a.
Reword the comment so it no longer implies the full output is never materialized: each inflated chunk is materialized, but the running total is checked after every chunk so an over-large block is rejected before its whole output is accumulated. Assisted-by: GitHub Copilot:claude-opus-4.8
| $dr = AvroDataIO::openFile($data_file); | ||
| $data = $dr->data(); | ||
| $dr->close(); | ||
| $this->assertSame([$payload], $data); |
There was a problem hiding this 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).
| $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)); |
There was a problem hiding this comment.
Fixed — try/finally around the reader close, failing explicitly if the expected exception isn't thrown.
| if (false === $context) { | ||
| throw new AvroException('gzip uncompression failed.'); | ||
| } |
There was a problem hiding this comment.
Fixed — reworded to 'deflate uncompression failed.' (this is the raw-deflate ZLIB_ENCODING_RAW path).
| $out = @inflate_add($context, $piece); | ||
| if (false === $out) { | ||
| throw new AvroException('gzip uncompression failed.'); | ||
| } |
There was a problem hiding this comment.
Fixed — 'deflate uncompression failed.'
| $out = @inflate_add($context, '', ZLIB_FINISH); | ||
| if (false === $out) { | ||
| throw new AvroException('gzip uncompression failed.'); | ||
| } |
There was a problem hiding this comment.
Fixed — 'deflate uncompression failed.'
| $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'); |
There was a problem hiding this comment.
Fixed — the deflate-limit test now closes the reader in a finally and fails explicitly via $this->fail().
…ed in tests Address review feedback: - The raw-deflate (ZLIB_ENCODING_RAW) decompression path reported "gzip uncompression failed"; reword to "deflate uncompression failed" to match the codec. - The decompression-limit tests only closed the reader on the happy path, so an unexpected exception from $dr->data() would leak the file handle. Wrap the reader usage in try/finally (closing in finally) and fail explicitly with $this->fail() when the expected exception is not thrown, instead of the $thrown flag. Assisted-by: GitHub Copilot:claude-opus-4.8
| $crc32 = $unpacked[1]; | ||
| // The Snappy block header declares the uncompressed length as a varint; | ||
| // reject an over-large block before allocating for it. Parsed with an | ||
| // early cap so it stays correct even if a 32-bit int would overflow. | ||
| self::ensureSnappyWithinLimit($payload, $maxLength); | ||
| $datum = snappy_uncompress($payload); | ||
|
|
||
| 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.'); | ||
| } |
There was a problem hiding this comment.
Fixed — the CRC32 values are now compared via sprintf('%u', ...) on both sides, so a float from unpack('N') on 32-bit PHP and a (possibly negative) int from crc32() normalize to the same unsigned representation instead of a false mismatch.
| 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('deflate uncompression failed.'); |
There was a problem hiding this comment.
Fixed — removed the @ from inflate_add(); the false-return check + exception is kept, so zlib warnings are no longer suppressed.
| $out = @inflate_add($context, '', ZLIB_FINISH); | ||
| if (false === $out) { | ||
| throw new AvroException('deflate uncompression failed.'); |
There was a problem hiding this comment.
Fixed — removed the @ from the ZLIB_FINISH inflate_add() call too.
Remove the @ operator from the two inflate_add() calls in the deflate path. The code already checks for a false return and throws, so suppression only hides zlib warnings that would help diagnose corrupt input, and it's inconsistent with the other codec helpers. Assisted-by: GitHub Copilot:claude-opus-4.8
| private function gzUncompress(string $compressed): string | ||
| { | ||
| $datum = gzinflate($compressed); | ||
| $maxLength = self::maxDecompressLength(); | ||
| $context = inflate_init(ZLIB_ENCODING_RAW); | ||
| if (false === $context) { | ||
| throw new AvroException('deflate uncompression failed.'); | ||
| } |
There was a problem hiding this comment.
Updated the PR description — it now says the deflate codec inflates in bounded chunks via inflate_init/inflate_add and rejects the block as soon as the accumulated output exceeds the limit, matching the implementation (no gzinflate).
What is the purpose of the change
The deflate codec inflates in bounded chunks (inflate_init/inflate_add) and rejects the block as soon as the accumulated output would exceed the limit, so the allocation is bounded incrementally; snappy rejects an over-large declared length up front; zstandard and bzip2 are checked after decompression. Exceeding the limit throws
AvroDataIODecompressionSizeException.When reading a data file, each block is decompressed according to the file's codec. A block with a very high compression ratio (or a malformed block) could expand to far more memory than its compressed size (a decompression bomb). This enforces a configurable maximum decompressed size while reading each block, mirroring the Java SDK's decompression limit (AVRO-4247). The limit defaults to 200 MiB and can be overridden with the
AVRO_MAX_DECOMPRESS_LENGTHenvironment variable.This is part of the umbrella issue AVRO-4283.
Verifying this change
This change added tests and can be verified as follows:
test/DataFileTest.php: a deflate block exceeding the limit is rejected and a within-limit block decodes.composer test(orphp vendor/bin/phpunit -c lang/php/phpunit.xml --filter DataFileTest)Documentation
AVRO_MAX_DECOMPRESS_LENGTHenvironment variable is documented in code comments)