Copilot commented on code in PR #3846:
URL: https://github.com/apache/avro/pull/3846#discussion_r3564159225
##########
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:
In readMap(), the cumulative item-limit check uses count($items) as the
existing count. For maps, count($items) is the number of unique keys, so
repeated keys can significantly undercount how many key/value pairs were
actually decoded and allow hostile input to bypass the configured max across
blocks (DoS via large total pair_count). Track decoded pair count separately
and use that for the cumulative check.
##########
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:
checkCollectionItems() currently uses $existing + $blockCount >
$this->maxCollectionItems. If maxCollectionItems is configured near
PHP_INT_MAX, the addition can overflow (or become a float), potentially
weakening the guard. Use a subtraction-based comparison to avoid overflow, and
clarify in the doc that this helper expects a normalized (non-negative) block
count.
--
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]
