Copilot commented on code in PR #3863:
URL: https://github.com/apache/avro/pull/3863#discussion_r3565164457
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -65,9 +73,37 @@ public function readBoolean(): bool
*/
public function read(int $len): string
{
+ if ($len < 0) {
+ // AvroStringIO::read() accepts a negative length and moves the
+ // pointer backwards; reject it before delegating.
+ throw new AvroException("Cannot read a negative number of bytes:
{$len}");
+ }
+ if ($len > self::MAX_UNCHECKED_READ) {
+ $remaining = $this->bytesRemaining();
+ if ($len > $remaining) {
+ throw new AvroException("Cannot read {$len} bytes, only
{$remaining} remaining.");
+ }
+ }
+
return $this->io->read($len);
}
+ /**
+ * Number of bytes still available to read, determined by seeking to the
end
+ * and restoring the position (which both the string and file IO
+ * implementations support). Used to reject a declared length or collection
+ * block count that exceeds the data actually available before allocating.
+ */
+ public function bytesRemaining(): int
+ {
+ $current = $this->io->tell();
+ $this->io->seek(0, AvroIO::SEEK_END);
+ $end = $this->io->tell();
+ $this->io->seek($current, AvroIO::SEEK_SET);
+
+ return $end - $current;
+ }
Review Comment:
bytesRemaining() can return a negative value if the underlying AvroIO has
been seeked past EOF (AvroStringIO::seek() allows this). That can produce
confusing error messages like "only -5 bytes remaining" and makes the
remaining-bytes checks harder to reason about. Clamp the value to 0 so it
always represents a non-negative number of readable bytes.
##########
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
+ }
Review Comment:
minBytesPerElement() should only return 0 for schemas that truly encode to
zero bytes (null). Returning 0 to break recursion underestimates the minimum
size of self-referential record schemas and can disable the
collection-availability guard for arrays/maps of such records. Use a safe lower
bound (1) when a recursion cycle is detected so the check remains conservative.
--
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]
