iemejia commented on code in PR #3864:
URL: https://github.com/apache/avro/pull/3864#discussion_r3568024894
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -215,27 +387,78 @@ sub decode_enum {
}
sub skip_block {
- my $class = shift;
- my ($reader, $block_content) = @_;
- my $block_count = decode_long($class, undef, undef, $reader);
+ # Called as a plain function by skip_array/skip_map as
+ # skip_block($reader, $min_bytes, $coderef); it is not a method, so there
is
+ # no leading class argument. decode_long ignores its (shifted) first
argument
+ # and reads from the last, so pass __PACKAGE__ for it and $reader last.
+ my ($reader, $min_bytes, $block_content) = @_;
+ # Zero-byte elements loop with no per-item input, so bound them tightly;
+ # other elements consume bytes (bounded by EOF) so only the structural cap
+ # applies. Every collection is capped by the structural limit, so for
+ # zero-byte elements use whichever of the two limits is tighter.
+ my $limit = $MAX_COLLECTION_STRUCTURAL;
+ if ($min_bytes <= 0 && $MAX_COLLECTION_ITEMS < $limit) {
+ $limit = $MAX_COLLECTION_ITEMS;
+ }
+ my $block_count = decode_long(__PACKAGE__, undef, undef, $reader);
+ my $skipped = 0;
while ($block_count) {
if ($block_count < 0) {
- $reader->seek($block_count, 0);
- next;
+ # A byte-sized (negative-count) block still declares an element
+ # count; bound it cumulatively too, so a huge declared count with a
+ # small or zero block size cannot bypass the cap (e.g. zero-byte
+ # elements whose block size is 0).
+ my $count = -$block_count;
+ if ($skipped + $count > $limit) {
+ throw Avro::BinaryDecoder::Error::CollectionSize(
+ "Cannot skip a collection of more than $limit "
+ . "elements (declared @{[ $skipped + $count ]})");
+ }
+ $skipped += $count;
+ # A negative count is followed by a long block size in bytes, which
+ # lets the whole block be skipped without decoding each item. Skip
+ # forward by that many bytes relative to the current position
+ # (SEEK_CUR); whence 0 (SEEK_SET) would seek to an absolute (here
+ # nonsensical) offset. A failed seek is treated as fatal.
+ my $block_size = decode_long(__PACKAGE__, undef, undef, $reader);
+ if ($block_size < 0) {
+ # A negative block size would seek the reader backwards,
+ # risking an infinite loop or mis-decoding of a corrupt input.
+ throw Avro::Schema::Error::Parse(
+ "Invalid negative block size: $block_size");
+ }
+ # Reject a block size that exceeds the bytes actually remaining
+ # before skipping, so a truncated input fails instead of seeking
+ # past EOF.
+ _ensure_available($reader, $block_size, 1);
+ unless ($reader->seek($block_size, 1)) {
+ throw Avro::Schema::Error::Parse(
+ "Failed to skip block of $block_size bytes");
+ }
}
else {
+ # Bound the cumulative element count: skipping a huge block of
+ # zero-byte elements would otherwise loop unboundedly (a CPU
+ # exhaustion) even though it allocates nothing.
+ if ($skipped + $block_count > $limit) {
+ throw Avro::BinaryDecoder::Error::CollectionSize(
+ "Cannot skip a collection of more than $limit "
+ . "elements (declared @{[ $skipped + $block_count ]})");
+ }
+ $skipped += $block_count;
for (1..$block_count) {
$block_content->();
}
Review Comment:
This is a Perl foreach-range optimization: `for (1..$n)` in a foreach loop
is special-cased and iterated lazily — it does NOT build the list. Verified:
`for (1..200_000_000) {}` uses 0 extra RSS (4636 kB before and after). So
there's no materialization here; and the block count is already bounded by the
cumulative caps before this loop runs.
##########
lang/perl/lib/Avro/BinaryDecoder.pm:
##########
@@ -296,13 +526,27 @@ sub decode_map {
my $block_count = decode_long($class, @_);
my $writer_values = $writer_schema->values;
my $reader_values = $reader_schema->values;
+ # A map key is a non-empty string (decode_map rejects empty keys below), so
+ # its minimum on-wire size is 2 bytes: a 1-byte length prefix plus at least
+ # 1 byte of key data, in addition to the value.
+ my $min_bytes = 2 + _min_bytes_per_element($writer_values);
+ # Track the number of decoded entries separately: scalar(keys %hash)
+ # undercounts when duplicate keys overwrite earlier entries, which would
let
+ # a multi-block map exceed the cumulative caps without being rejected.
+ my $decoded = 0;
while ($block_count) {
my $block_size;
if ($block_count < 0) {
$block_count = -$block_count;
$block_size = decode_long($class, @_);
+ if ($block_size < 0) {
+ throw Avro::Schema::Error::Parse(
+ "Invalid negative map block size: $block_size");
+ }
## XXX we can skip with $reader_schema?
}
+ _ensure_collection_available($reader, $decoded, $block_count,
$min_bytes);
+ $decoded += $block_count;
for (1..$block_count) {
my $key = decode_string($class, @_);
unless (defined $key && length $key) {
Review Comment:
Same — `for (1..$block_count)` in decode_map is the foreach-range form,
which Perl iterates lazily without materializing the list (confirmed: no RSS
growth for 200M). The count is also bounded by ensure_collection_available
before the loop.
--
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]
