Copilot commented on code in PR #3864:
URL: https://github.com/apache/avro/pull/3864#discussion_r3567609675
##########
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);
Review Comment:
In skip_block()'s negative-count branch, the code trusts the declared
per-block byte-size and seeks by it. If a malformed input declares a block_size
that is too small for the declared element count (given the per-element
minimum), the skip will not advance far enough and subsequent decoding will be
misaligned. Add a lower-bound validation using $min_bytes (division-based, like
the other checks) before seeking.
##########
lang/perl/t/03_bin_decode.t:
##########
@@ -84,6 +84,68 @@ EOJ
is $dec, "a", "Binary_Encodings.Complex_Types.Unions-a";
}
+## union and enum index bounds
+{
+ my $union = Avro::Schema->parse(q(["string","null"]));
+ # Index 2 (zig-zag int 0x04) is out of range for a 2-branch union.
+ open my $reader, '<', \"\x04" or die "Can't open memory file: $!";
+ throws_ok {
+ Avro::BinaryDecoder->decode(
+ writer_schema => $union,
+ reader_schema => $union,
+ reader => $reader,
+ );
+ } 'Avro::Schema::Error::Parse', "union branch index out of range is
rejected";
+
+ # Index -1 (zig-zag int 0x01) must not wrap to a valid branch.
+ open $reader, '<', \"\x01" or die "Can't open memory file: $!";
+ throws_ok {
+ Avro::BinaryDecoder->decode(
+ writer_schema => $union,
+ reader_schema => $union,
+ reader => $reader,
+ );
+ } 'Avro::Schema::Error::Parse', "negative union branch index is rejected";
+
+ my $enum = Avro::Schema->parse(
+ q({ "type": "enum", "name": "e", "symbols": [ "a", "b" ] }));
+ # Index 9 (zig-zag int 0x12) is out of range for a 2-symbol enum.
+ open $reader, '<', \"\x12" or die "Can't open memory file: $!";
+ throws_ok {
+ Avro::BinaryDecoder->decode(
+ writer_schema => $enum,
+ reader_schema => $enum,
+ reader => $reader,
+ );
+ } 'Avro::Schema::Error::Parse', "enum symbol index out of range is
rejected";
+
+ # Index -1 (zig-zag int 0x01) must not wrap to a valid symbol.
+ open $reader, '<', \"\x01" or die "Can't open memory file: $!";
+ throws_ok {
+ Avro::BinaryDecoder->decode(
+ writer_schema => $enum,
+ reader_schema => $enum,
+ reader => $reader,
+ );
+ } 'Avro::Schema::Error::Parse', "negative enum symbol index is rejected";
+}
+
+## overlong varint
+{
+ # A 64-bit value uses at most 10 bytes; an 11th continuation byte is a
+ # malformed overlong varint and must be rejected.
Review Comment:
This comment says the 11th byte is a "continuation" byte, but the test data
uses 10 continuation bytes (0x80) followed by a terminating byte (0x01).
Tweaking the wording avoids confusion while keeping the test intent the same.
--
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]
