iemejia commented on code in PR #3863:
URL: https://github.com/apache/avro/pull/3863#discussion_r3568041562
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +279,74 @@ 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) {
+ // seek() can move past EOF, so a truncated/oversized block
would
+ // otherwise be "skipped" silently, hiding truncation. Reject a
+ // block size larger than the bytes actually remaining.
+ if ($blockSize > $decoder->bytesRemaining()) {
+ throw new AvroException('Array block size exceeds the
remaining input');
+ }
+ $decoder->skip($blockSize);
+ } else {
Review Comment:
Fixed — skipArray now also rejects a block size too small to hold blockCount
elements at their minimum on-wire size (`$blockCount > intdiv($blockSize,
$minBytes)` when minBytes > 0), before seeking.
##########
lang/php/lib/Datum/AvroIOBinaryDecoder.php:
##########
@@ -235,28 +279,74 @@ 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) {
+ // seek() can move past EOF, so a truncated/oversized block
would
+ // otherwise be "skipped" silently, hiding truncation. Reject a
+ // block size larger than the bytes actually remaining.
+ if ($blockSize > $decoder->bytesRemaining()) {
+ throw new AvroException('Array block size exceeds the
remaining input');
+ }
+ $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) {
+ // seek() can move past EOF; reject a block size larger than
the
+ // bytes remaining so a truncated block isn't silently skipped.
+ if ($blockSize > $decoder->bytesRemaining()) {
+ throw new AvroException('Map block size exceeds the
remaining input');
+ }
+ $decoder->skip($blockSize);
+ } else {
Review Comment:
Fixed — skipMap has the same block-size-vs-count check 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]
