AVRO-4298: [php] Bound allocation when decoding length-prefixed values and collections by iemejia · Pull Request #3863 · apache/avro

iemejia · 2026-07-11T21:20:23Z

What is the purpose of the change

A bytes or string value is encoded as a length prefix followed by that many bytes of data, and an array or map block is encoded as an element count followed by that many items. A malicious or truncated input can declare a very large length or count while carrying little or no actual data, which causes a correspondingly large allocation before the shortfall is noticed.

This applies the equivalent of the Java SDK fix AVRO-4241 to the PHP SDK and extends it to collections. It has two complementary parts.

1. Validate available bytes before allocating

When the source can report how many bytes remain, a declared length (or a collection block count) that exceeds the bytes actually available is rejected before allocating for it. The collection check uses the minimum on-wire size of the element schema, so a zero-byte element type (such as null) is never falsely rejected. Sources that cannot report their remaining size are unaffected.

AvroIOBinaryDecoder::bytesRemaining() reports the bytes still readable (found by seeking to the end and restoring the position). read() rejects an over-large declared length above a threshold, and AvroIODatumReader::readArray/readMap reject a block whose element count could not be backed by the bytes remaining, using minBytesPerElement() from the element schema.

2. Cap collection allocation for zero-byte elements

Zero-byte elements (null, a zero-length fixed, or a record with only zero-byte fields) consume no input, so the available-bytes check cannot bound their count: a tiny payload such as {{"type":"array","items":"null"}} declaring a block count of 200,000,000 would otherwise drive an unbounded allocation. In addition to the available-bytes check, ensureCollectionAvailable caps the cumulative count of zero-byte elements (DEFAULT_MAX_COLLECTION_ITEMS = 10,000,000) and applies a structural cap to every collection (DEFAULT_MAX_COLLECTION_STRUCTURAL = Integer.MAX_VALUE - 8). The decoder's skipArray/skipMap are also bounded (element-aware). Rejections raise the dedicated AvroIOCollectionSizeException (registered in autoload.php). When set, the AVRO_MAX_COLLECTION_ITEMS environment variable caps both limits.

This folds in and supersedes the standalone collection-limit change (AVRO-4281, #3846), so this PR is the single complete fix for collection/length-prefixed allocation DoS in the PHP SDK.

This is a sub-task of AVRO-4292 and resolves AVRO-4298.

Verifying this change

This change added tests and can be verified as follows:

Extended lang/php/test/DatumIOTest.php with over-limit bytes/array/map rejection, an array<null> with a huge block count that must be rejected, and skip bounding.
Run: ./build.sh test (PHP), or vendor/bin/phpunit -c lang/php/phpunit.xml

Documentation

Does this pull request introduce a new feature? (no — hardening / robustness)
If yes, how is the feature documented? (not applicable)

…h-prefixed values and collections A bytes or string value is a length prefix followed by that many bytes, and an array or map block is an element count followed by that many items. A malicious or truncated input can declare a huge length or count with little or no data. - AvroIOBinaryDecoder::bytesRemaining() reports the bytes still readable (found by seeking to the end and restoring the position). read() uses it to reject an over-large declared length above a threshold before allocating. - AvroIODatumReader::readArray/readMap reject a block whose element count could not be backed by the bytes remaining, using minBytesPerElement() computed from the element schema so a zero-byte element type (e.g. null) is not falsely rejected. Mirrors the Java SDK's checks (AVRO-4241). Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Hardens the PHP Avro binary decoder/reader against malicious or truncated inputs that declare very large length-prefixed bytes/string values or oversized array/map blocks, by validating declared sizes/counts against bytes remaining before large allocations/iterations.

Changes:

Added AvroIOBinaryDecoder::bytesRemaining() and a large-read guard in read() to fail fast when a declared length exceeds remaining data.
Added collection block validation in AvroIODatumReader::readArray() / readMap() using a computed minimum on-wire size per element.
Added PHPUnit coverage for over-limit bytes/array/map and a “array of nulls” non-false-positive case.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
lang/php/lib/Datum/AvroIOBinaryDecoder.php	Adds bytes-remaining detection and a size-vs-remaining guard for large reads.
lang/php/lib/Datum/AvroIODatumReader.php	Adds pre-iteration validation for array/map block counts using a minimum-bytes-per-element estimate.
lang/php/test/DatumIOTest.php	Adds tests asserting early rejection of oversized declared lengths/counts and a null-array pass case.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iemejia · 2026-07-11T22:29:01Z

    public function read(int $len): string
    {
+        if ($len > self::MAX_UNCHECKED_READ) {
+            $remaining = $this->bytesRemaining();
+            if ($len > $remaining) {
+                throw new AvroException("Cannot read {$len} bytes, only {$remaining} remaining.");
+            }
+        }
+


Fixed in 5e997eb: read(int) now rejects a negative length before delegating to AvroIO::read().

iemejia · 2026-07-11T22:29:02Z

+        $remaining = $decoder->bytesRemaining();
+        if ($count * $minBytesPerElement > $remaining) {


Fixed in 5e997eb: ensureCollectionAvailable now compares count against intdiv(remaining, minBytes) instead of multiplying, avoiding overflow/float coercion for large counts.

iemejia · 2026-07-11T22:29:03Z

            }
+            self::ensureCollectionAvailable($decoder, $blockCount, self::minBytesPerElement($writersSchema->items()));
            for ($i = 0; $i < $blockCount; $i++) {


Fixed in 5e997eb: readArray now uses a non-strict comparison (0 != blockCount), matching readMap, so a GMP numeric-string block count of '0' terminates the loop on 32-bit builds.

…ock loop Review feedback: - read(int) now rejects a negative length. AvroStringIO::read() accepts a negative value and moves the pointer backwards, leading to incorrect decoding. - ensureCollectionAvailable compares count against intdiv(remaining, minBytes) rather than multiplying, so a large count cannot overflow (to a float or wrap) and slip past the guard. - readArray uses a non-strict comparison (0 != blockCount) to match readMap; on 32-bit builds with GMP, readLong returns numeric strings, so a strict compare against 0 would not terminate the loop. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

iemejia · 2026-07-11T23:06:51Z

+        $current = $this->io->tell();
+        $this->io->seek(0, AvroIO::SEEK_END);
+        $end = $this->io->tell();
+        $this->io->seek($current, AvroIO::SEEK_SET);
+
+        return $end - $current;
+    }


Fixed in f3a4764: bytesRemaining() clamps to 0 via max(0, ...), so a position past EOF no longer yields a negative remaining count.

iemejia · 2026-07-11T23:06:52Z

+                $id = spl_object_id($schema);
+                if (isset($visited[$id])) {
+                    return 0; // break recursion for self-referencing schemas
+                }


Fixed in f3a4764: minBytesPerElement() returns 1 (not 0) on a self-referencing cycle, keeping the guard conservative for arrays/maps of recursive records.

Review feedback: - bytesRemaining() clamps to 0 with max(0, ...), since AvroStringIO::seek() allows seeking past EOF, which would otherwise produce a confusing negative remaining count. - minBytesPerElement() returns 1 (not 0) when a self-referencing record cycle is detected, keeping the collection-availability guard conservative rather than disabling it for arrays/maps of recursive records. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-11T23:30:45Z

+     * Minimum number of bytes a single value of the given schema can occupy on
+     * the wire. Used to reject an array/map block count that could not be backed
+     * by the bytes remaining. A type that can encode to zero bytes (null)
+     * returns 0, which disables the collection check for it (so an array of
+     * nulls is not falsely rejected). Types that cannot be resolved cheaply


Fixed in bd6bb27: the doc comment now notes that minBytesPerElement() also returns 0 for a record with no fields (or whose fields all encode to zero bytes), not only null.

Review feedback: the doc comment claimed only null can encode to zero bytes, but minBytesPerElement() also returns 0 for a record with no fields or whose fields all encode to zero bytes. Update the comment so a 0 return (which disables the collection guard for that element type) is not misunderstood in future changes. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Completes the available-bytes protection for collections and supersedes the separate collection-limit change. Elements whose schema encodes to zero bytes (null, a zero-length fixed, or a record with only zero-byte fields) consume no input, so the bytes-remaining check cannot bound their count. A tiny payload declaring a huge array block count of such elements (e.g. {"type":"array","items":"null"} with a count of 200,000,000) therefore drove an unbounded allocation and exhausted memory. ensureCollectionAvailable now enforces the bytes-remaining check plus a zero-byte cap (DEFAULT_MAX_COLLECTION_ITEMS = 10,000,000) and a structural cap on all collections (DEFAULT_MAX_COLLECTION_STRUCTURAL = Integer.MAX_VALUE - 8). AVRO_MAX_COLLECTION_ITEMS, when set, caps both. Rejections raise the dedicated AvroIOCollectionSizeException. The decoder's skipArray/skipMap are also bounded (element-aware) so a huge block cannot loop unboundedly. Assisted-by: GitHub Copilot:claude-opus-4.8

iemejia · 2026-07-12T13:06:01Z

This PR now also includes the collection block-count cap for [php], so it is the single complete fix for collection allocation DoS in this SDK. In addition to validating available bytes before allocating length-prefixed values, it bounds the number of array/map items per block:

a heap-aware cap for zero-byte-element collections (e.g. array<null>), which otherwise bypass the available-bytes check because each element reads 0 bytes;
a structural cap for all collections;
bounded skip paths so projection/skip cannot loop unboundedly.

With this, the standalone collection-limit change for [php] (AVRO-4281, #3846) is redundant and is being closed as superseded by this PR.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

iemejia · 2026-07-12T17:32:28Z

+        $minBytes = AvroIODatumReader::collectionElementMinBytes($writersSchema->items());
+        $skipped = 0;
        $blockCount = $decoder->readLong();
        while (0 !== $blockCount) {


Fixed in 23f15e0: skipArray uses a non-strict comparison (0 != $blockCount), so a GMP numeric-string '0' terminates the loop on 32-bit builds.

iemejia · 2026-07-12T17:32:29Z

+        $minBytes = 1 + AvroIODatumReader::collectionElementMinBytes($writersSchema->values());
+        $skipped = 0;
        $blockCount = $decoder->readLong();
        while (0 !== $blockCount) {


Fixed in 23f15e0: skipMap got the same non-strict comparison.

iemejia · 2026-07-12T17:32:31Z

+                foreach ($schema->fields() as $field) {
+                    $total += self::minBytesPerElement($field, $visited);
+                }


Made explicit in 23f15e0 (now traverses via $field->type()). Note this was already correct at runtime: AvroField extends AvroSchema, so minBytesPerElement(field) used $field->type() internally and a record of null fields returned 0 (verified) — but $field->type() is clearer.

iemejia · 2026-07-12T17:32:32Z

+            // Map keys are strings (>= 1 byte length prefix) plus the value.
+            self::ensureCollectionAvailable($decoder, count($items), $pair_count, $minBytes);
            for ($i = 0; $i < $pair_count; $i++) {


Fixed in 23f15e0: readMap now bounds against a separate cumulative counter rather than count($items), so repeating a key can't shrink the count and slip past the cap. Added a regression test.

iemejia · 2026-07-12T17:32:33Z

+    /**
+     * Returns the configured collection limits as [zeroByteLimit, structuralLimit].
+     * AVRO_MAX_COLLECTION_ITEMS, when a non-negative integer, caps both.
+     *
+     * @return array{int, int}
+     */


Fixed in 23f15e0: reworded — the env var overrides both limits to the given value (which may raise or lower them), not only cap them.

iemejia · 2026-07-12T17:32:34Z

+        putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+        try {
+            $this->expectException(AvroException::class);


Fixed in 23f15e0: now asserts AvroIOCollectionSizeException.

iemejia · 2026-07-12T17:32:36Z

+        putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+        try {
+            $this->expectException(AvroException::class);


Fixed in 23f15e0: now asserts AvroIOCollectionSizeException.

iemejia · 2026-07-12T17:32:37Z

+        putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+        try {
+            $this->expectException(AvroException::class);


Fixed in 23f15e0: now asserts AvroIOCollectionSizeException.

A negative block count is normalized by negating it, but PHP_INT_MIN cannot be negated: -PHP_INT_MIN silently promotes to a float. The float count then hits the int type hint of ensureCollectionAvailable() and raises a raw TypeError rather than a proper Avro exception. The zig-zag encoding of PHP_INT_MIN is a valid 10-byte varint, so this is reachable from malformed input. Reject PHP_INT_MIN explicitly in readArray/readMap before negating, matching the C, C++ and C# bindings. The skip path already byte-skips negative blocks without negating, so it is unaffected. Adds a test for a PHP_INT_MIN array block count. Assisted-by: GitHub Copilot:claude-opus-4.8

Addresses review feedback: - readMap now bounds against a separate cumulative pairs-read counter instead of count($items). Keyed by map key, count($items) shrinks when a stream repeats the same key, which could slip past the cumulative/structural cap. Adds a regression test. - skipArray/skipMap loop conditions use a non-strict comparison (0 != $count) like readArray/readMap, so a GMP numeric-string block count of '0' terminates the loop on 32-bit builds. - minBytesPerElement traverses record fields via $field->type() (the field's value schema) explicitly rather than relying on AvroField extending AvroSchema. - Tighten the collection-limit tests to assert AvroIOCollectionSizeException, and reword the collectionLimits() doc comment (the env var overrides both limits to the given value rather than only capping them). Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

iemejia · 2026-07-12T18:44:18Z

            if ($blockCount < 0) {
+                // PHP_INT_MIN cannot be negated: -PHP_INT_MIN promotes to a
+                // float, so reject it rather than propagating a non-int count.
+                if (PHP_INT_MIN === $blockCount) {


Fixed in 063699d: the PHP_INT_MIN guard uses a non-strict comparison (==), so on 32-bit GMP builds where readLong() returns numeric strings the minimum block count is still rejected.

iemejia · 2026-07-12T18:44:19Z

            if ($pair_count < 0) {
+                // PHP_INT_MIN cannot be negated: -PHP_INT_MIN promotes to a
+                // float, so reject it rather than propagating a non-int count.
+                if (PHP_INT_MIN === $pair_count) {


Fixed in 063699d: same non-strict comparison applied to the map path.

iemejia · 2026-07-12T18:44:20Z

+        while (0 != $blockCount) {
            if ($blockCount < 0) {
                $decoder->skip($this->readLong());
-            }
-            for ($i = 0; $i < $blockCount; $i++) {
-                AvroIODatumReader::skipData($writersSchema->items(), $decoder);
+            } else {
+                AvroIODatumReader::checkSkipCollectionCount($skipped, $blockCount, $minBytes);


Fixed in 063699d: skipArray now applies checkSkipCollectionCount() on the negative (byte-sized) path too (normalizing the count, rejecting PHP_INT_MIN and a negative block size), and reads the block byte-size from the passed $decoder rather than $this. Added a regression test.

iemejia · 2026-07-12T18:44:21Z

+        while (0 != $blockCount) {
            if ($blockCount < 0) {
                $decoder->skip($this->readLong());
-            }
-            for ($i = 0; $i < $blockCount; $i++) {
-                $decoder->skipString();
-                AvroIODatumReader::skipData($writersSchema->values(), $decoder);
+            } else {
+                AvroIODatumReader::checkSkipCollectionCount($skipped, $blockCount, $minBytes);


Fixed in 063699d: skipMap got the same treatment (bound negative blocks, reject negative size, read from $decoder).

…ompare Addresses review feedback: - skipArray/skipMap applied checkSkipCollectionCount only for non-negative blocks, so the negative (byte-sized) block form could skip an unbounded collection and bypass the limit; and the block byte-size was read from $this rather than the passed $decoder. Both paths now normalize the count, apply the skip cap, reject PHP_INT_MIN and a negative block size, and read the block size from $decoder. Adds a regression test. - readArray/readMap PHP_INT_MIN guards use a non-strict comparison, so on 32-bit builds with GMP (where readLong returns numeric strings) the minimum block count is still rejected consistently. Assisted-by: GitHub Copilot:claude-opus-4.8

skipArray/skipMap skipped a negative-block's declared byte-size via seek() without checking it. AvroFile/AvroStringIO seek() can move past EOF, so a truncated or oversized block was silently skipped, leaving the decoder positioned beyond the input and hiding truncation during projection. Reject a block size larger than bytesRemaining() before skipping. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-13T01:24:50Z

+            case AvroSchema::FIXED_SCHEMA:
+                return $schema instanceof AvroFixedSchema ? $schema->size() : 1;


Fixed — minBytesPerElement now returns max(0, $schema->size()) for fixed, so a malformed negative fixed size can't make the minimum negative and cause the element to be mis-treated as zero-byte.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

minBytesPerElement returned AvroFixedSchema::size() verbatim. Since schema parsing does not reject a negative "size", a malformed fixed schema could make this negative, causing ensureCollectionAvailable to treat the element as zero-byte (minBytesPerElement > 0 false) and skip the bytes-remaining check. Clamp to max(0, size()). Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

iemejia · 2026-07-13T04:27:52Z

+                if ($blockSize > $decoder->bytesRemaining()) {
+                    throw new AvroException('Array block size exceeds the remaining input');
+                }
+                $decoder->skip($blockSize);
+            } else {


Fixed — skipArray now also rejects a block size too small to hold blockCount elements at their minimum on-wire size ($blockCount > intdiv($blockSize, $minBytes) when minBytes > 0), before seeking.

iemejia · 2026-07-13T04:27:54Z

+                if ($blockSize > $decoder->bytesRemaining()) {
+                    throw new AvroException('Map block size exceeds the remaining input');
+                }
+                $decoder->skip($blockSize);
+            } else {


Fixed — skipMap has the same block-size-vs-count check before skipping.

…t count skipArray/skipMap validated the block byte-size against bytesRemaining but not against the declared element count. Reject a block size too small to contain blockCount elements at their minimum on-wire size (when minBytes > 0), so a malformed size can't leave the decoder mid-element and misalign later decoding. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

github-actions Bot added the PHP label Jul 11, 2026

iemejia requested a review from Copilot July 11, 2026 21:25

Copilot started reviewing on behalf of iemejia July 11, 2026 21:25 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 11, 2026 22:35

Copilot started reviewing on behalf of iemejia July 11, 2026 22:36 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 11, 2026 23:08

Copilot started reviewing on behalf of iemejia July 11, 2026 23:09 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 05:05

Copilot started reviewing on behalf of iemejia July 12, 2026 05:05 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia mentioned this pull request Jul 12, 2026

AVRO-4281: [php] Bound collection size when decoding arrays and maps #3846

Closed

iemejia changed the title ~~AVRO-4298: [php] Validate available bytes before allocating for length-prefixed values~~ AVRO-4298: [php] Bound allocation when decoding length-prefixed values and collections Jul 12, 2026

iemejia requested a review from Copilot July 12, 2026 14:33

Copilot started reviewing on behalf of iemejia July 12, 2026 14:33 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia added 2 commits July 12, 2026 18:24

iemejia requested a review from Copilot July 12, 2026 17:37

Copilot started reviewing on behalf of iemejia July 12, 2026 17:37 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 13, 2026 00:29

Copilot started reviewing on behalf of iemejia July 13, 2026 00:29 View session