Copilot commented on code in PR #3863:
URL: https://github.com/apache/avro/pull/3863#discussion_r3566498000
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +273,39 @@ public function skipRecord(AvroRecordSchema
$writersSchema, AvroIOBinaryDecoder
public function skipArray(AvroArraySchema $writersSchema,
AvroIOBinaryDecoder $decoder): void
{
+ $minBytes =
AvroIODatumReader::collectionElementMinBytes($writersSchema->items());
+ $skipped = 0;
$blockCount = $decoder->readLong();
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);
+ $skipped += $blockCount;
+ for ($i = 0; $i < $blockCount; $i++) {
+ AvroIODatumReader::skipData($writersSchema->items(),
$decoder);
+ }
}
$blockCount = $decoder->readLong();
}
}
public function skipMap(AvroMapSchema $writersSchema, AvroIOBinaryDecoder
$decoder): void
{
+ // Map entries always carry a >= 1 byte key, so the minimum is
positive.
+ $minBytes = 1 +
AvroIODatumReader::collectionElementMinBytes($writersSchema->values());
+ $skipped = 0;
$blockCount = $decoder->readLong();
while (0 !== $blockCount) {
Review Comment:
skipMap() uses a strict comparison in the loop condition. On 32-bit builds
with GMP enabled, readLong() can return numeric strings (e.g. '0'), so `0 !==
$blockCount` may never terminate the loop.
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -538,4 +594,118 @@ private function readDecimal(string $bytes, int $scale):
string
return (string) ($scale > 0 ? ($int / (10 ** $scale)) : $int);
}
+
+ /**
+ * 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. It returns 0 for any schema that can encode to
+ * zero bytes: the null primitive, but also a record with no fields or
whose
+ * fields all encode to zero bytes. A zero return disables the collection
+ * check for that element type (so, e.g., an array of nulls is not falsely
+ * rejected). Types that cannot be resolved cheaply default to 1.
+ *
+ * @param array<int, bool> $visited
+ */
+ private static function minBytesPerElement(mixed $schema, array $visited =
[]): int
+ {
+ $type = $schema instanceof AvroSchema ? $schema->type() : $schema;
+ // Named/complex field types may nest a schema object; unwrap one
level.
+ if ($type instanceof AvroSchema) {
+ return self::minBytesPerElement($type, $visited);
+ }
+ if (!is_string($type)) {
+ return 1;
+ }
+ switch ($type) {
+ case AvroSchema::NULL_TYPE:
+ return 0;
+ case AvroSchema::FLOAT_TYPE:
+ return 4;
+ case AvroSchema::DOUBLE_TYPE:
+ return 8;
+ case AvroSchema::FIXED_SCHEMA:
+ return $schema instanceof AvroFixedSchema ? $schema->size() :
1;
+ case AvroSchema::RECORD_SCHEMA:
+ case AvroSchema::ERROR_SCHEMA:
+ if (!$schema instanceof AvroRecordSchema) {
+ return 1;
+ }
+ $id = spl_object_id($schema);
+ if (isset($visited[$id])) {
+ return 1; // self-referencing schema: safe lower bound of
1 byte
+ }
+ $visited[$id] = true;
+ $total = 0;
+ foreach ($schema->fields() as $field) {
+ $total += self::minBytesPerElement($field, $visited);
+ }
Review Comment:
minBytesPerElement() iterates AvroRecordSchema fields as AvroField objects,
but then passes the AvroField itself back into minBytesPerElement(). That makes
every field contribute at least 1 byte, so records whose fields all encode to 0
bytes (e.g. a record with only `null` fields) are misclassified as
non-zero-byte and won’t be capped by the zero-byte collection limit.
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +273,39 @@ public function skipRecord(AvroRecordSchema
$writersSchema, AvroIOBinaryDecoder
public function skipArray(AvroArraySchema $writersSchema,
AvroIOBinaryDecoder $decoder): void
{
+ $minBytes =
AvroIODatumReader::collectionElementMinBytes($writersSchema->items());
+ $skipped = 0;
$blockCount = $decoder->readLong();
while (0 !== $blockCount) {
Review Comment:
skipArray() uses a strict comparison in the loop condition. On 32-bit builds
with GMP enabled, readLong() can return numeric strings (e.g. '0'), so `0 !==
$blockCount` will not terminate the loop correctly.
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -320,6 +346,8 @@ public function readMap(
$decoder->readLong();
}
+ // 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++) {
Review Comment:
readMap() uses count($items) as the "existing" element count for limit
enforcement, but $items is keyed by map keys so duplicate keys overwrite and
reduce count(). This can bypass the configured cumulative limit across blocks
(and the structural cap) by repeating the same key.
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -538,4 +594,118 @@ private function readDecimal(string $bytes, int $scale):
string
return (string) ($scale > 0 ? ($int / (10 ** $scale)) : $int);
}
+
+ /**
+ * 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. It returns 0 for any schema that can encode to
+ * zero bytes: the null primitive, but also a record with no fields or
whose
+ * fields all encode to zero bytes. A zero return disables the collection
+ * check for that element type (so, e.g., an array of nulls is not falsely
+ * rejected). Types that cannot be resolved cheaply default to 1.
+ *
+ * @param array<int, bool> $visited
+ */
+ private static function minBytesPerElement(mixed $schema, array $visited =
[]): int
+ {
+ $type = $schema instanceof AvroSchema ? $schema->type() : $schema;
+ // Named/complex field types may nest a schema object; unwrap one
level.
+ if ($type instanceof AvroSchema) {
+ return self::minBytesPerElement($type, $visited);
+ }
+ if (!is_string($type)) {
+ return 1;
+ }
+ switch ($type) {
+ case AvroSchema::NULL_TYPE:
+ return 0;
+ case AvroSchema::FLOAT_TYPE:
+ return 4;
+ case AvroSchema::DOUBLE_TYPE:
+ return 8;
+ case AvroSchema::FIXED_SCHEMA:
+ return $schema instanceof AvroFixedSchema ? $schema->size() :
1;
+ case AvroSchema::RECORD_SCHEMA:
+ case AvroSchema::ERROR_SCHEMA:
+ if (!$schema instanceof AvroRecordSchema) {
+ return 1;
+ }
+ $id = spl_object_id($schema);
+ if (isset($visited[$id])) {
+ return 1; // self-referencing schema: safe lower bound of
1 byte
+ }
+ $visited[$id] = true;
+ $total = 0;
+ foreach ($schema->fields() as $field) {
+ $total += self::minBytesPerElement($field, $visited);
+ }
+
+ return $total;
+ default:
+ // boolean, int, long, bytes, string, enum, union, array, map:
+ // all encode to at least one byte.
+ return 1;
+ }
+ }
+
+ /**
+ * Returns the configured collection limits as [zeroByteLimit,
structuralLimit].
+ * AVRO_MAX_COLLECTION_ITEMS, when a non-negative integer, caps both.
+ *
+ * @return array{int, int}
+ */
Review Comment:
The doc comment says AVRO_MAX_COLLECTION_ITEMS "caps" both limits, but
collectionLimits() actually sets both limits to the provided value (and tests
rely on increasing the limit). Please reword to match the behavior to avoid
misconfiguration.
##########
lang/php/test/DatumIOTest.php:
##########
@@ -261,6 +262,225 @@ public function
test_invalid_bytes_logical_type_out_of_range(): void
$writer->write("10000", $encoder);
}
+ // A bytes/string value declares a length prefix; a malicious or truncated
+ // input can declare far more bytes than actually exist. On a reader that
+ // can report its size, that is rejected before allocating for it.
+ public function test_read_bytes_rejects_length_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(100 * 1024 * 1024);
+ $io->seek(0);
+ $decoder = new AvroIOBinaryDecoder($io);
+
+ $this->expectException(AvroException::class);
+ $decoder->readBytes();
+ }
+
+ public function test_read_bytes_within_stream_still_reads(): void
+ {
+ $payload = str_repeat('x', 2 * 1024 * 1024);
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeBytes($payload);
+ $io->seek(0);
+ $decoder = new AvroIOBinaryDecoder($io);
+
+ $this->assertEquals($payload, $decoder->readBytes());
+ }
+
+ public function test_read_array_rejects_count_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(1000000); // 1,000,000
longs, no data
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"array","items":"long"}', $io);
+ }
+
+ public function test_read_map_rejects_count_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(1000000);
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"map","values":"long"}', $io);
+ }
+
+ public function test_read_array_of_null_not_falsely_rejected(): void
+ {
+ $count = 100000;
+ $io = new AvroStringIO();
+ $encoder = new AvroIOBinaryEncoder($io);
+ $encoder->writeLong($count); // one block of `count` nulls (zero bytes
each)
+ $encoder->writeLong(0); // end-of-array marker
+ $result = $this->decodeWith('{"type":"array","items":"null"}', $io);
+ $this->assertCount($count, $result);
+ }
+
+ public function test_read_array_within_stream_still_reads(): void
+ {
+ $schema = AvroSchema::parse('{"type":"array","items":"long"}');
+ $io = new AvroStringIO();
+ (new AvroIODatumWriter($schema))->write([1, 2, 3], new
AvroIOBinaryEncoder($io));
+ $this->assertEquals([1, 2, 3],
$this->decodeWith('{"type":"array","items":"long"}', $io));
+ }
+
+ public function test_array_of_null_exceeds_default_limit(): void
+ {
+ // The reported exploit: a ~6 byte payload declaring 200,000,000 nulls.
+ $this->expectException(AvroIOCollectionSizeException::class);
+ $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(200000000));
+ }
+
+ public function test_array_of_null_within_configured_limit_still_reads():
void
+ {
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $result = $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(1000));
+ $this->assertCount(1000, $result);
+ } finally {
+ putenv('AVRO_MAX_COLLECTION_ITEMS');
+ }
+ }
+
+ public function test_array_of_null_exceeds_configured_limit(): void
+ {
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(1001));
+ } finally {
+ putenv('AVRO_MAX_COLLECTION_ITEMS');
+ }
+ }
+
+ public function test_array_of_null_cumulative_across_blocks(): void
+ {
+ $io = new AvroStringIO();
+ $encoder = new AvroIOBinaryEncoder($io);
+ $encoder->writeLong(600);
+ $encoder->writeLong(600);
+ $encoder->writeLong(0);
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"array","items":"null"}', $io);
+ } finally {
+ putenv('AVRO_MAX_COLLECTION_ITEMS');
+ }
+ }
+
+ public function test_array_of_null_negative_block_count(): void
+ {
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $this->expectException(AvroException::class);
Review Comment:
This test case is checking that a negative block count for a zero-byte
element type is bounded by the collection-size limit, but it only asserts
AvroException. It should assert AvroIOCollectionSizeException to ensure the
limit guard is what triggers the rejection.
##########
lang/php/test/DatumIOTest.php:
##########
@@ -261,6 +262,225 @@ public function
test_invalid_bytes_logical_type_out_of_range(): void
$writer->write("10000", $encoder);
}
+ // A bytes/string value declares a length prefix; a malicious or truncated
+ // input can declare far more bytes than actually exist. On a reader that
+ // can report its size, that is rejected before allocating for it.
+ public function test_read_bytes_rejects_length_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(100 * 1024 * 1024);
+ $io->seek(0);
+ $decoder = new AvroIOBinaryDecoder($io);
+
+ $this->expectException(AvroException::class);
+ $decoder->readBytes();
+ }
+
+ public function test_read_bytes_within_stream_still_reads(): void
+ {
+ $payload = str_repeat('x', 2 * 1024 * 1024);
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeBytes($payload);
+ $io->seek(0);
+ $decoder = new AvroIOBinaryDecoder($io);
+
+ $this->assertEquals($payload, $decoder->readBytes());
+ }
+
+ public function test_read_array_rejects_count_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(1000000); // 1,000,000
longs, no data
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"array","items":"long"}', $io);
+ }
+
+ public function test_read_map_rejects_count_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(1000000);
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"map","values":"long"}', $io);
+ }
+
+ public function test_read_array_of_null_not_falsely_rejected(): void
+ {
+ $count = 100000;
+ $io = new AvroStringIO();
+ $encoder = new AvroIOBinaryEncoder($io);
+ $encoder->writeLong($count); // one block of `count` nulls (zero bytes
each)
+ $encoder->writeLong(0); // end-of-array marker
+ $result = $this->decodeWith('{"type":"array","items":"null"}', $io);
+ $this->assertCount($count, $result);
+ }
+
+ public function test_read_array_within_stream_still_reads(): void
+ {
+ $schema = AvroSchema::parse('{"type":"array","items":"long"}');
+ $io = new AvroStringIO();
+ (new AvroIODatumWriter($schema))->write([1, 2, 3], new
AvroIOBinaryEncoder($io));
+ $this->assertEquals([1, 2, 3],
$this->decodeWith('{"type":"array","items":"long"}', $io));
+ }
+
+ public function test_array_of_null_exceeds_default_limit(): void
+ {
+ // The reported exploit: a ~6 byte payload declaring 200,000,000 nulls.
+ $this->expectException(AvroIOCollectionSizeException::class);
+ $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(200000000));
+ }
+
+ public function test_array_of_null_within_configured_limit_still_reads():
void
+ {
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $result = $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(1000));
+ $this->assertCount(1000, $result);
+ } finally {
+ putenv('AVRO_MAX_COLLECTION_ITEMS');
+ }
+ }
+
+ public function test_array_of_null_exceeds_configured_limit(): void
+ {
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(1001));
+ } finally {
+ putenv('AVRO_MAX_COLLECTION_ITEMS');
+ }
+ }
+
+ public function test_array_of_null_cumulative_across_blocks(): void
+ {
+ $io = new AvroStringIO();
+ $encoder = new AvroIOBinaryEncoder($io);
+ $encoder->writeLong(600);
+ $encoder->writeLong(600);
+ $encoder->writeLong(0);
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $this->expectException(AvroException::class);
Review Comment:
This test is intended to verify the cumulative collection-size limit across
blocks, but it only asserts AvroException. Asserting
AvroIOCollectionSizeException specifically will better protect against
regressions where a different exception is thrown first.
##########
lang/php/test/DatumIOTest.php:
##########
@@ -261,6 +262,225 @@ public function
test_invalid_bytes_logical_type_out_of_range(): void
$writer->write("10000", $encoder);
}
+ // A bytes/string value declares a length prefix; a malicious or truncated
+ // input can declare far more bytes than actually exist. On a reader that
+ // can report its size, that is rejected before allocating for it.
+ public function test_read_bytes_rejects_length_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(100 * 1024 * 1024);
+ $io->seek(0);
+ $decoder = new AvroIOBinaryDecoder($io);
+
+ $this->expectException(AvroException::class);
+ $decoder->readBytes();
+ }
+
+ public function test_read_bytes_within_stream_still_reads(): void
+ {
+ $payload = str_repeat('x', 2 * 1024 * 1024);
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeBytes($payload);
+ $io->seek(0);
+ $decoder = new AvroIOBinaryDecoder($io);
+
+ $this->assertEquals($payload, $decoder->readBytes());
+ }
+
+ public function test_read_array_rejects_count_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(1000000); // 1,000,000
longs, no data
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"array","items":"long"}', $io);
+ }
+
+ public function test_read_map_rejects_count_beyond_stream(): void
+ {
+ $io = new AvroStringIO();
+ (new AvroIOBinaryEncoder($io))->writeLong(1000000);
+ $this->expectException(AvroException::class);
+ $this->decodeWith('{"type":"map","values":"long"}', $io);
+ }
+
+ public function test_read_array_of_null_not_falsely_rejected(): void
+ {
+ $count = 100000;
+ $io = new AvroStringIO();
+ $encoder = new AvroIOBinaryEncoder($io);
+ $encoder->writeLong($count); // one block of `count` nulls (zero bytes
each)
+ $encoder->writeLong(0); // end-of-array marker
+ $result = $this->decodeWith('{"type":"array","items":"null"}', $io);
+ $this->assertCount($count, $result);
+ }
+
+ public function test_read_array_within_stream_still_reads(): void
+ {
+ $schema = AvroSchema::parse('{"type":"array","items":"long"}');
+ $io = new AvroStringIO();
+ (new AvroIODatumWriter($schema))->write([1, 2, 3], new
AvroIOBinaryEncoder($io));
+ $this->assertEquals([1, 2, 3],
$this->decodeWith('{"type":"array","items":"long"}', $io));
+ }
+
+ public function test_array_of_null_exceeds_default_limit(): void
+ {
+ // The reported exploit: a ~6 byte payload declaring 200,000,000 nulls.
+ $this->expectException(AvroIOCollectionSizeException::class);
+ $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(200000000));
+ }
+
+ public function test_array_of_null_within_configured_limit_still_reads():
void
+ {
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $result = $this->decodeWith('{"type":"array","items":"null"}',
self::zeroByteBlock(1000));
+ $this->assertCount(1000, $result);
+ } finally {
+ putenv('AVRO_MAX_COLLECTION_ITEMS');
+ }
+ }
+
+ public function test_array_of_null_exceeds_configured_limit(): void
+ {
+ putenv('AVRO_MAX_COLLECTION_ITEMS=1000');
+
+ try {
+ $this->expectException(AvroException::class);
Review Comment:
This test is validating the collection-size limit path, but it only asserts
the base AvroException type. Tightening it to AvroIOCollectionSizeException
will ensure the dedicated limit exception continues to be thrown (and avoids
accidentally passing due to other AvroExceptions).
--
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]
