AVRO-4280: [perl] Bound collection size when decoding arrays and maps#3847
AVRO-4280: [perl] Bound collection size when decoding arrays and maps#3847iemejia wants to merge 4 commits into
Conversation
The block count of an array or map is read from the input and drives allocation of the resulting collection. A very large or malformed block count (for example from truncated input) could request an unbounded allocation. Validate the block count per block and cumulatively against a configurable maximum ($Avro::BinaryDecoder::MAX_COLLECTION_ITEMS, overridable via AVRO_MAX_COLLECTION_ITEMS), mirroring the Java SDK's collection item limit, and throw Avro::BinaryDecoder::Error::CollectionSize when exceeded. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
This PR hardens the Perl Avro binary decoder against unbounded allocations when decoding arrays and maps by enforcing a configurable maximum collection size, aligned with similar limits in other language SDKs.
Changes:
- Introduces
$Avro::BinaryDecoder::MAX_COLLECTION_ITEMS(default(2 ** 31) - 8) with optional override viaAVRO_MAX_COLLECTION_ITEMS. - Adds collection-size checks to
decode_arrayanddecode_map, raisingAvro::BinaryDecoder::Error::CollectionSizeon limit violations. - Adds a new Perl test file covering limit enforcement scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lang/perl/lib/Avro/BinaryDecoder.pm | Adds max-collection-item configuration, enforcement checks in array/map decoding, and a new error class. |
| lang/perl/t/06_bin_decode_limits.t | Adds tests validating array/map decoding limits and default restoration behavior. |
Comments suppressed due to low confidence (1)
lang/perl/lib/Avro/BinaryDecoder.pm:336
- decode_map uses scalar(keys %hash) as the running total for _check_collection_items, but that counts unique keys rather than the number of key/value pairs decoded. With repeated keys across blocks, the cumulative limit can be bypassed (unbounded pairs parsed, CPU/time and transient allocations) while staying under the cap. Track the decoded-pair count separately and validate against that.
_check_collection_items(scalar(keys %hash), $block_count);
for (1..$block_count) {
my $key = decode_string($class, @_);
unless (defined $key && length $key) {
throw Avro::Schema::Error::Parse("key of map is invalid");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## Ensure that decoding the next block of $block_count items would not grow the | ||
| ## collection beyond $MAX_COLLECTION_ITEMS. Throws on a negative block count or | ||
| ## when the running total would exceed the limit. | ||
| sub _check_collection_items { | ||
| my ($existing, $block_count) = @_; |
There was a problem hiding this comment.
Fixed in 9499118. Reworded the comment to clarify that callers pass the normalized (non-negative) count — in the Avro encoding a negative count merely signals a following block-size long whose absolute value is the count — and that the negative check here is a defensive guard against malformed input.
| use Test::More; | ||
| use Test::Exception; | ||
|
|
||
| use_ok 'Avro::BinaryDecoder'; |
There was a problem hiding this comment.
Fixed in 9499118. The test now clears AVRO_MAX_COLLECTION_ITEMS in a BEGIN block before any Avro module is loaded, so the default-limit assertion is independent of the runner environment. Verified by running the suite with AVRO_MAX_COLLECTION_ITEMS=5 set.
- Track decoded pair count in decode_map instead of scalar(keys %hash); repeated keys collapse in the hash and could otherwise bypass the limit. - Clarify the _check_collection_items comment: callers pass a normalized (non-negative) count and the negative check is a defensive guard. - Clear AVRO_MAX_COLLECTION_ITEMS before loading the module in the test so the default-limit assertion is independent of the runner environment. - Add a test covering repeated map keys across blocks. Assisted-by: GitHub Copilot:claude-opus-4.8
| # Two blocks of 6 items (zigzag(6) = 0x0c) exceed the limit cumulatively. | ||
| throws_ok { decode_bytes($array_schema, "\x0c\x0c") } $err, | ||
| "array cumulative block count above limit is rejected"; |
There was a problem hiding this comment.
Fixed in d28dfd9 — the cumulative-array input is now well-formed (\x0c\x0c\x00: two blocks of six null items plus the terminating 0 block count).
| # Repeated map keys collapse in the hash; the cumulative check must still | ||
| # count every decoded pair. Two blocks of 6 pairs all keyed "a" | ||
| # (zigzag(6)=0x0c, key string = 0x02 0x61, null value = no bytes) exceed 10. | ||
| my $repeated_key_map = "\x0c" . ("\x02\x61" x 6) . "\x0c"; | ||
| throws_ok { decode_bytes($map_schema, $repeated_key_map) } $err, |
There was a problem hiding this comment.
Fixed in d28dfd9 — $repeated_key_map now includes the second block's key/value bytes and the terminating 0 block count, so it is well-formed Avro map data.
| # By default the limit is generous enough not to affect ordinary decoding. | ||
| is $Avro::BinaryDecoder::MAX_COLLECTION_ITEMS, (2 ** 31) - 8, | ||
| "default collection item limit restored outside local scope"; |
There was a problem hiding this comment.
Fixed in d28dfd9 — the assertion now references $Avro::BinaryDecoder::DEFAULT_MAX_COLLECTION_ITEMS instead of duplicating the literal.
- Make the cumulative-array and repeated-key-map test inputs well-formed Avro encodings (include the second block's items and the terminating 0 block count) so they test the limit check rather than coupling to when the decoder throws. - Reference $Avro::BinaryDecoder::DEFAULT_MAX_COLLECTION_ITEMS in the default assertion instead of duplicating the literal. Assisted-by: GitHub Copilot:claude-opus-4.8
| # Negative count: unsigned varint 0x15 decodes (zigzag) to -11, whose | ||
| # absolute value (11) is used; a block size long (0x00) follows. | ||
| throws_ok { decode_bytes($array_schema, "\x15\x00") } $err, | ||
| "negative array block count is bounded by its absolute value"; |
There was a problem hiding this comment.
Fixed in 3d206c5 — the negative-block-count array input now includes the terminating 0 block count (\x15\x00\x00), keeping the test data well-formed Avro.
Include the terminating 0 block count in the negative-block-count array test so the input is well-formed Avro, and silence the harmless 'used only once' warning for the DEFAULT_MAX_COLLECTION_ITEMS package global. Assisted-by: GitHub Copilot:claude-opus-4.8
What is the purpose of the change
When decoding an array or map, the block count is read from the input and drives
allocation of the resulting collection. A very large or malformed block count
(for example from truncated or hostile input) could request an unbounded
allocation, since items such as
nulloccupy no bytes on the wire.This validates the block count — per block and cumulatively — against a
configurable maximum before allocating, mirroring the Java SDK's collection item
limit (
org.apache.avro.limits.collectionItems.maxLength, AVRO-3819). The limitdefaults to
2^31 - 8items and can be overridden with theAVRO_MAX_COLLECTION_ITEMSenvironment variable, or by setting$Avro::BinaryDecoder::MAX_COLLECTION_ITEMS. Exceeding it throwsAvro::BinaryDecoder::Error::CollectionSize.This is part of the umbrella issue AVRO-4277.
Verifying this change
This change added tests and can be verified as follows:
lang/perl/t/06_bin_decode_limits.tcovering: a single block count abovethe limit (array and map), a cumulative limit across blocks, a negative block
count bounded by its absolute value, and a within-limit collection still
decoding correctly.
cd lang/perl && prove -Ilib t/Documentation
AVRO_MAX_COLLECTION_ITEMSenvironment variable is documented in code comments)