Skip to content

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

Closed
iemejia wants to merge 5 commits into
apache:mainfrom
iemejia:AVRO-4281-php-collection-limit
Closed

AVRO-4281: [php] Bound collection size when decoding arrays and maps#3846
iemejia wants to merge 5 commits into
apache:mainfrom
iemejia:AVRO-4281-php-collection-limit

Conversation

@iemejia

@iemejia iemejia commented Jul 11, 2026

Copy link
Copy Markdown
Member

What is the purpose of the change

When decoding an array or map, the block count is read from the input and drives
allocation of the resulting collection. A very large or malformed block count
(for example from truncated or hostile input) could request an unbounded
allocation, since items such as null occupy no bytes on the wire.

This validates the block count — per block and cumulatively — against a
configurable maximum before allocating, mirroring the Java SDK's collection item
limit (org.apache.avro.limits.collectionItems.maxLength, AVRO-3819). The limit
defaults to 2^31 - 8 items and can be overridden with the
AVRO_MAX_COLLECTION_ITEMS environment variable, or per reader via
AvroIODatumReader::setMaxCollectionItems(). Exceeding it throws the new
Apache\Avro\Datum\AvroIOCollectionSizeException.

This is part of the umbrella issue AVRO-4277.

Verifying this change

This change added tests and can be verified as follows:

  • Added tests in lang/php/test/IODatumReaderTest.php covering: a single block
    count above the limit (array and map), a cumulative limit across blocks, a
    negative block count bounded by its absolute value, and a within-limit
    collection still decoding correctly.
  • Run: composer test (or php vendor/bin/phpunit -c lang/php/phpunit.xml --filter IODatumReaderTest)

Documentation

  • Does this pull request introduce a new feature? (no — hardening/robustness)
  • If yes, how is the feature documented? (not applicable; the new
    AVRO_MAX_COLLECTION_ITEMS environment variable is documented in code comments)

The block count of an array or map is read from the input and drives
allocation of the resulting collection. A very large or malformed block
count (for example from truncated input) could request an unbounded
allocation. Validate the block count per block and cumulatively against a
configurable maximum (AVRO_MAX_COLLECTION_ITEMS), mirroring the Java SDK's
collection item limit, and throw AvroIOCollectionSizeException when exceeded.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the PHP Avro decoder against unbounded allocations (and related DoS vectors) by enforcing a configurable maximum number of decoded items for arrays and maps, similar to the Java SDK’s collection item limit.

Changes:

  • Add a default/max configurable collection item limit (env var + per-reader setter) and enforce it during array/map decoding.
  • Introduce AvroIOCollectionSizeException for limit violations.
  • Add PHPUnit coverage for per-block, cumulative-across-blocks, negative-count, and within-limit scenarios.

Reviewed changes

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

File Description
lang/php/lib/Datum/AvroIODatumReader.php Adds max-collection-items configuration and enforces bounds while decoding arrays/maps.
lang/php/lib/Datum/AvroIOCollectionSizeException.php Adds a dedicated exception for collection-size limit violations.
lang/php/lib/autoload.php Registers the new exception in the PHP autoloader.
lang/php/test/IODatumReaderTest.php Adds tests validating collection-size enforcement behavior.

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

Comment on lines 372 to 376
$decoder->readLong();
}

$this->checkCollectionItems(count($items), $pair_count);
for ($i = 0; $i < $pair_count; $i++) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 7bd2e3a. readMap() now tracks a separate $itemsRead pair counter instead of count($items), so repeated keys can no longer collapse in the result and bypass the cumulative limit. Added test_map_cumulative_limit_with_repeated_keys covering repeated keys across blocks.

Comment on lines 89 to 101
/**
* Guard against unbounded allocation when decoding an array or map.
*
* @throws AvroIOCollectionSizeException if the block count is negative or
* the running total would exceed the
* configured maximum.
*/
private function checkCollectionItems(int $existing, int $blockCount): void
{
if ($blockCount < 0 || $existing + $blockCount > $this->maxCollectionItems) {
throw new AvroIOCollectionSizeException($this->maxCollectionItems);
}
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7bd2e3a. checkCollectionItems() now compares $blockCount > $this->maxCollectionItems - $existing (subtraction-based) so it can't overflow near PHP_INT_MAX, and the docblock now states that the block count passed in is already normalized (non-negative).

iemejia added 2 commits July 11, 2026 15:09
Move the new private helper methods below the public methods to match the
ordered_class_elements rule enforced by the lint step.

Assisted-by: GitHub Copilot:claude-opus-4.8
- Track decoded pair count in readMap instead of count($items); repeated
  keys collapse in the result and could otherwise bypass the cumulative limit.
- Use a subtraction-based comparison in checkCollectionItems so it cannot
  overflow when the limit is configured near PHP_INT_MAX, and document that
  the block count passed in is already normalized.
- Add a test covering repeated keys across blocks.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

* throws an {@see AvroIOCollectionSizeException} instead of attempting a
* potentially huge allocation. Mirrors the Java SDK's collection item limit.
*/
public const DEFAULT_MAX_COLLECTION_ITEMS = 2147483639; // 2^31 - 8

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 175b42f. The value 2147483639 is intentional (it matches Java's Integer.MAX_VALUE - 8); corrected the misleading 2^31 - 8 comment accordingly.

Comment on lines 317 to 321
$blockCount = -$blockCount;
$decoder->readLong(); // Read (and ignore) block size
}
$this->checkCollectionItems(count($items), $blockCount);
for ($i = 0; $i < $blockCount; $i++) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 175b42f. readArray now uses a loose comparison (0 != $blockCount) so the loop terminates when readLong() returns a numeric string under GMP on 32-bit, matching readMap.

- readArray now uses a loose comparison against 0 so the loop terminates when
  readLong() returns a numeric string (GMP on 32-bit builds), matching readMap.
- Correct the default-limit comment: 2147483639 is Integer.MAX_VALUE - 8
  (matching the Java SDK), not 2^31 - 8.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment on lines +71 to 77
/**
* Set the maximum number of items permitted in a single decoded array or map.
*/
public function setMaxCollectionItems(int $maxCollectionItems): void
{
$this->maxCollectionItems = $maxCollectionItems;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3062721 — setMaxCollectionItems now throws InvalidArgumentException for a negative limit, with a test.

setMaxCollectionItems now throws InvalidArgumentException for a negative
limit, which would otherwise make the guard nonsensical (non-empty
collections always throw while empty ones succeed). Add a test.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@iemejia

iemejia commented Jul 12, 2026

Copy link
Copy Markdown
Member Author

Superseded by #3863 (AVRO-4298), which now contains this collection block-count cap for [php] together with the available-bytes validation, forming a single complete fix for collection allocation DoS in this SDK. The collection-limit work here has been folded into that PR, so closing this as superseded. Follow-up review and discussion should continue on #3863.

@iemejia iemejia closed this Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants