Copilot commented on code in PR #3863:
URL: https://github.com/apache/avro/pull/3863#discussion_r3565069267
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -65,9 +73,32 @@ public function readBoolean(): bool
*/
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.");
+ }
+ }
+
Review Comment:
read(int $len) should reject negative lengths before delegating to
AvroIO::read(). AvroStringIO::read() accepts negative values and will move the
internal pointer backwards, which can lead to incorrect decoding and potential
out-of-bounds behavior when reading malformed data.
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -538,4 +541,80 @@ 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. 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
+ * default to 1, which is safe because the only zero-byte primitive is
null.
+ *
+ * @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 0; // break recursion for self-referencing schemas
+ }
+ $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;
+ }
+ }
+
+ /**
+ * Rejects a collection (array or map) block whose declared element count
+ * could not be backed by the bytes actually remaining, before iterating.
+ * Skipped when the per-element minimum is zero (e.g. an array of nulls).
+ *
+ * @throws AvroException if the declared count exceeds the bytes remaining
+ */
+ private static function ensureCollectionAvailable(
+ AvroIOBinaryDecoder $decoder,
+ int $count,
+ int $minBytesPerElement
+ ): void {
+ if ($count <= 0 || $minBytesPerElement <= 0) {
+ return;
+ }
+ $remaining = $decoder->bytesRemaining();
+ if ($count * $minBytesPerElement > $remaining) {
Review Comment:
The overflow check in ensureCollectionAvailable() multiplies $count *
$minBytesPerElement, which can overflow and become a float (or wrap) for large
counts, potentially letting an invalid block pass the guard. Compare using
division/intdiv to avoid overflow.
##########
lang/php/lib/Datum/AvroIODatumReader.php:
##########
@@ -290,6 +290,7 @@ public function readArray(
$blockCount = -$blockCount;
$decoder->readLong(); // Read (and ignore) block size
}
+ self::ensureCollectionAvailable($decoder, $blockCount,
self::minBytesPerElement($writersSchema->items()));
for ($i = 0; $i < $blockCount; $i++) {
Review Comment:
readArray() uses a strict comparison (0 !== $blockCount). On 32-bit
platforms with GMP enabled, AvroIOBinaryDecoder::readLong() returns numeric
strings, so a block count of "0" would not terminate the loop. Using a
non-strict numeric comparison avoids incorrect behavior on GMP paths.
--
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]
