iemejia commented on code in PR #3846:
URL: https://github.com/apache/avro/pull/3846#discussion_r3564260625
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -320,6 +372,7 @@ public function readMap(
$decoder->readLong();
}
+ $this->checkCollectionItems(count($items), $pair_count);
for ($i = 0; $i < $pair_count; $i++) {
Review Comment:
Good catch — fixed in 7bd2e3a9c7. `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.
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -43,10 +43,61 @@
*/
class AvroIODatumReader
{
+ /**
+ * Default upper bound on the number of items in a single decoded array or
+ * map. The block count of an array or map is read from the (potentially
+ * untrusted or truncated) input and drives allocation of the resulting
+ * collection. Reading a collection that declares more items than this
limit
+ * 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
+
+ /**
+ * Name of the environment variable used to override the default maximum
+ * number of items permitted in a single decoded array or map.
+ */
+ public const MAX_COLLECTION_ITEMS_ENV = 'AVRO_MAX_COLLECTION_ITEMS';
+
+ private int $maxCollectionItems;
+
public function __construct(
private ?AvroSchema $writersSchema = null,
private ?AvroSchema $readersSchema = null
) {
+ $this->maxCollectionItems = self::defaultMaxCollectionItems();
+ }
+
+ /**
+ * Set the maximum number of items permitted in a single decoded array or
map.
+ */
+ public function setMaxCollectionItems(int $maxCollectionItems): void
+ {
+ $this->maxCollectionItems = $maxCollectionItems;
+ }
+
+ private static function defaultMaxCollectionItems(): int
+ {
+ $env = getenv(self::MAX_COLLECTION_ITEMS_ENV);
+ if (false !== $env && ctype_digit($env)) {
+ return (int) $env;
+ }
+
+ return self::DEFAULT_MAX_COLLECTION_ITEMS;
+ }
+
+ /**
+ * 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);
+ }
}
Review Comment:
Fixed in 7bd2e3a9c7. `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).
--
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]
