iemejia commented on code in PR #3863:
URL: https://github.com/apache/avro/pull/3863#discussion_r3567443483


##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +279,63 @@ 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) {
+        while (0 != $blockCount) {
+            $blockSize = null;
             if ($blockCount < 0) {
-                $decoder->skip($this->readLong());
+                if (PHP_INT_MIN == $blockCount) {
+                    throw new AvroException('Invalid array block count');
+                }
+                $blockCount = -$blockCount;
+                $blockSize = $decoder->readLong();
+                if ($blockSize < 0) {
+                    throw new AvroException('Invalid negative array block 
size');
+                }
             }
-            for ($i = 0; $i < $blockCount; $i++) {
-                AvroIODatumReader::skipData($writersSchema->items(), $decoder);
+            // Bound the (normalized) count on both the sized and unsized 
paths so
+            // a negative block count cannot bypass the skip limit.
+            AvroIODatumReader::checkSkipCollectionCount($skipped, $blockCount, 
$minBytes);
+            $skipped += $blockCount;
+            if (null !== $blockSize) {
+                $decoder->skip($blockSize);
+            } else {

Review Comment:
   Fixed — skipArray now rejects a block byte-size larger than 
`bytesRemaining()` before calling seek(), so a truncated/oversized block can't 
be silently skipped past EOF.



##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +279,63 @@ 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) {
+        while (0 != $blockCount) {
+            $blockSize = null;
             if ($blockCount < 0) {
-                $decoder->skip($this->readLong());
+                if (PHP_INT_MIN == $blockCount) {
+                    throw new AvroException('Invalid array block count');
+                }
+                $blockCount = -$blockCount;
+                $blockSize = $decoder->readLong();
+                if ($blockSize < 0) {
+                    throw new AvroException('Invalid negative array block 
size');
+                }
             }
-            for ($i = 0; $i < $blockCount; $i++) {
-                AvroIODatumReader::skipData($writersSchema->items(), $decoder);
+            // Bound the (normalized) count on both the sized and unsized 
paths so
+            // a negative block count cannot bypass the skip limit.
+            AvroIODatumReader::checkSkipCollectionCount($skipped, $blockCount, 
$minBytes);
+            $skipped += $blockCount;
+            if (null !== $blockSize) {
+                $decoder->skip($blockSize);
+            } else {
+                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) {
+        while (0 != $blockCount) {
+            $blockSize = null;
             if ($blockCount < 0) {
-                $decoder->skip($this->readLong());
+                if (PHP_INT_MIN == $blockCount) {
+                    throw new AvroException('Invalid map block count');
+                }
+                $blockCount = -$blockCount;
+                $blockSize = $decoder->readLong();
+                if ($blockSize < 0) {
+                    throw new AvroException('Invalid negative map block size');
+                }
             }
-            for ($i = 0; $i < $blockCount; $i++) {
-                $decoder->skipString();
-                AvroIODatumReader::skipData($writersSchema->values(), 
$decoder);
+            AvroIODatumReader::checkSkipCollectionCount($skipped, $blockCount, 
$minBytes);
+            $skipped += $blockCount;
+            if (null !== $blockSize) {
+                $decoder->skip($blockSize);
+            } else {

Review Comment:
   Fixed — skipMap likewise validates the block byte-size against 
`bytesRemaining()` before skipping.



-- 
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]

Reply via email to