Copilot commented on code in PR #3863:
URL: https://github.com/apache/avro/pull/3863#discussion_r3567395276
##########
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:
In the negative-block form, skipArray() skips the declared block byte-size
via seek() without verifying that many bytes are actually available. Since
AvroFile::seek() and AvroStringIO::seek() allow seeking past EOF, a truncated
or malicious block can be “skipped” silently, leaving the decoder positioned
beyond the input and potentially allowing a decode/projection to proceed
without detecting truncation. Consider validating blockSize against
bytesRemaining() before skipping.
##########
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:
In the negative-block form, skipMap() skips the declared block byte-size via
seek() without checking it against available bytes. Because seek() can move
past EOF, a truncated block can be skipped without raising, which can hide
truncation during projection. Validate the block size against bytesRemaining()
before calling skip().
--
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]
