AVRO-4299: [perl] Validate available bytes before allocating for length-prefixed values by iemejia · Pull Request #3864 · apache/avro

iemejia · 2026-07-11T21:20:25Z

What is the purpose of the change

A bytes or string value is encoded as a length prefix followed by that many bytes of data, and an array or map block is encoded as an element count followed by that many items. A malicious or truncated input can declare a very large length or count while carrying little or no actual data, which causes a correspondingly large allocation before the shortfall is noticed.

This applies the equivalent of the Java SDK fix AVRO-4241 to the Perl SDK: when the source can report how many bytes remain, a declared length (or a collection block count) that exceeds the bytes actually available is rejected before allocating for it. The collection check uses the minimum on-wire size of the element schema, so a zero-byte element type (such as null) is never falsely rejected. Sources that cannot report their remaining size are unaffected.

_bytes_remaining reports the bytes still readable, found by seeking to the end and restoring the position; readers that cannot seek to the end (e.g. a streaming decompressor) yield undef and are simply skipped. decode_bytes rejects an over-large declared length above a threshold, and decode_array/decode_map reject a block whose element count could not be backed by the bytes remaining, using _min_bytes_per_element from the element schema.

This is a sub-task of AVRO-4292 and resolves AVRO-4299.

Verifying this change

This change added tests and can be verified as follows:

Extended lang/perl/t/03_bin_decode.t with over-limit bytes/array/map rejection and an array of nulls that must not be falsely rejected.
Run: cd lang/perl && prove -Ilib t/

Documentation

Does this pull request introduce a new feature? (no — hardening / robustness)
If yes, how is the feature documented? (not applicable)

…th-prefixed values and collections A bytes or string value is a length prefix followed by that many bytes, and an array or map block is an element count followed by that many items. A malicious or truncated input can declare a huge length or count with little or no data. - _bytes_remaining reports the bytes still readable, found by seeking to the end and restoring the position; readers that cannot seek to the end (e.g. a streaming decompressor) yield undef and are simply skipped. - decode_bytes rejects an over-large declared length above a threshold before allocating. - decode_array/decode_map reject a block whose element count could not be backed by the bytes remaining, using _min_bytes_per_element computed from the element schema so a zero-byte element type (e.g. null) is not falsely rejected. Mirrors the Java SDK's checks (AVRO-4241). Assisted-by: GitHub Copilot:claude-opus-4.8

The Lint CI step (perlcritic, default severity) flagged a Subroutines::ProhibitExplicitReturnUndef violation: 'return undef' in _bytes_remaining. Use a bare 'return', which yields undef in the scalar context the callers use, so a reader that cannot report its size still skips the check. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Hardens the Perl binary decoder against malicious/truncated inputs that advertise extremely large length/count prefixes by validating claimed sizes against the bytes actually remaining on seekable readers, preventing large allocations before short reads are detected.

Changes:

Add remaining-bytes checks for large bytes/string length-prefixed reads (seekable readers only).
Add remaining-bytes checks for array/map blocks using a computed minimum on-wire element size (with special handling to avoid rejecting zero-byte element types like null).
Extend Perl decoder tests to cover oversized/truncated bytes/array/map cases and a large array<null> that must still decode.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
lang/perl/lib/Avro/BinaryDecoder.pm	Adds seek-based “bytes remaining” logic and pre-allocation/pre-iteration validation for bytes/strings and array/map blocks.
lang/perl/t/03_bin_decode.t	Adds regression tests covering oversized length/count prefixes and ensuring array-of-nulls is not falsely rejected.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iemejia · 2026-07-11T22:29:05Z

    my $class = shift;
    my $reader = pop;
    my $size = decode_long($class, undef, undef, $reader);
+    _ensure_available($reader, $size);
    $reader->read(my $buf, $size);


Fixed in 92df248: decode_bytes now rejects a negative decoded length before reading.

iemejia · 2026-07-11T22:29:06Z

+sub _bytes_remaining {
+    my ($reader) = @_;
+    my $current = eval { $reader->tell };
+    return if !defined $current || $current < 0;
+    return eval {
+        $reader->seek(0, 2)         # SEEK_END
+            or die "seek to end failed\n";
+        my $end = $reader->tell;
+        $reader->seek($current, 0); # SEEK_SET, restore position
+        die "unknown end position\n" if !defined $end || $end < 0;
+        $end - $current;
+    };
+}


Fixed in 92df248: _bytes_remaining now always restores the original position after seeking to the end (even if reading the end offset fails) and treats a restore failure as fatal, so the reader is never silently left at EOF.

iemejia · 2026-07-11T22:29:07Z

+    # Map keys are strings (>= 1 byte length prefix) plus the value.
+    my $min_bytes = 1 + _min_bytes_per_element($writer_values);


Fixed in 92df248: decode_map now uses 2 bytes as the minimum key size (1-byte length prefix + at least 1 byte of data), since empty keys are rejected, making the pre-check slightly tighter.

… tighter map key min Review feedback: - decode_bytes rejects a negative decoded length before reading, since Avro bytes/string lengths must be non-negative. - _bytes_remaining now always restores the original position after seeking to the end (even if reading the end offset fails), and treats a restore failure as fatal, so the reader is never left at EOF and silently used for subsequent decoding. - decode_map keys are non-empty strings (empty keys are rejected), so the minimum per-entry size uses 2 bytes for the key (1-byte length prefix plus at least 1 byte of data) instead of 1, making the pre-check slightly tighter. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-11T23:06:53Z

+    return if $count <= 0 || $min_bytes <= 0;
+    my $remaining = _bytes_remaining($reader);
+    return unless defined $remaining;
+    if ($count * $min_bytes > $remaining) {


Fixed in 98aaeb6: _ensure_collection_available compares count against int(remaining / min_bytes) instead of multiplying, avoiding overflow/wrap on 32-bit builds.

Review feedback: $count * $min_bytes could overflow/wrap on some Perl builds (notably 32-bit), defeating the pre-check for very large declared counts. Compare $count against int($remaining / $min_bytes) instead. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-11T23:30:46Z

+        throw Avro::Schema::Error::Parse(
+            "Invalid negative bytes/string length: $size");
+    }
+    _ensure_available($reader, $size);
    $reader->read(my $buf, $size);


Fixed in fa8332b: decode_bytes now verifies read() returned exactly the declared number of bytes (rejecting truncated input), and skip_bytes (used by skip_string during resolution) rejects a negative declared length.

Review feedback: - decode_bytes now verifies that read() returned exactly the declared number of bytes; a short read (truncated input) is rejected instead of silently returning an under-sized buffer that would misalign the decoder. - skip_bytes (used by skip_string during schema resolution) rejects a negative declared length, consistent with decode_bytes. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-12T05:26:20Z

+        throw Avro::Schema::Error::Parse(
+            "Invalid negative bytes/string length: $size");
+    }
    $reader->seek($size, 0);


Fixed in 9a4cf0a: skip_bytes() now uses whence 1 (SEEK_CUR) to skip forward relative to the current position, instead of whence 0 (SEEK_SET) which seeks to the absolute offset.

Review feedback: skip_bytes() used seek($size, 0), i.e. SEEK_SET, which seeks to the absolute offset $size instead of skipping $size bytes forward from the current position. Use whence 1 (SEEK_CUR) for a relative skip. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-12T06:05:21Z

+    # Skip forward by $size bytes relative to the current position
+    # (SEEK_CUR); whence 0 (SEEK_SET) would incorrectly seek to the
+    # absolute offset $size.
+    $reader->seek($size, 1);
    return;


Fixed in 25ab8ab: skip_bytes now calls _ensure_available before skipping (mirroring decode_bytes), so a declared length that exceeds the remaining bytes is rejected on a size-reporting reader instead of silently seeking past EOF, and a failed seek is treated as fatal.

While here I fixed the same absolute-vs-relative seek bug in two related paths that shared it: skip_fixed used seek(size, 0) (SEEK_SET) — now SEEK_CUR — and skip_block's negative-count branch ignored the block-size long, did an absolute seek of a negative offset, and re-looped on the same count (an infinite loop); it now reads the block size and skips it relatively. Added tests for skipping fixed/bytes writer fields absent from the reader schema and for rejecting an oversized skipped bytes length. Full suite (386 tests) and perlcritic pass.

Review feedback: skip_bytes could silently succeed when the declared length exceeded the bytes actually available (a seek past EOF is allowed on many handles), causing subsequent decoding to read EOF as zeros instead of failing on truncated input. - skip_bytes now calls _ensure_available before skipping (mirroring decode_bytes) and treats a failed seek as fatal. - skip_fixed used seek(size, 0) (SEEK_SET, absolute); use SEEK_CUR for a relative skip and throw on a failed seek. - skip_block's negative-count branch ignored the block-size long, did an absolute seek of a negative offset, and re-looped on the same count (an infinite loop). Read the block size and skip it relatively (SEEK_CUR), throwing on a failed seek. Added tests covering skipping fixed/bytes writer fields absent from the reader schema and rejecting an oversized skipped bytes length. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-12T07:00:59Z

+            my $block_size = decode_long($class, undef, undef, $reader);
+            unless ($reader->seek($block_size, 1)) {
+                throw Avro::Schema::Error::Parse(
+                    "Failed to skip block of $block_size bytes");
+            }


Fixed in 8af98be: the negative-count path now rejects a negative block size (throwing Invalid negative block size) before seeking, so a corrupt input cannot seek the reader backwards into an infinite loop or mis-decode. It also validates the block size against the bytes remaining via _ensure_available before the relative seek, consistent with skip_bytes.

Review feedback: in skip_block's negative-count path the decoded block-size long was not validated. A corrupted input with a negative block size would make seek($block_size, SEEK_CUR) move the reader backwards, risking an infinite loop or mis-decoding. Reject a negative block size, and validate it against the bytes remaining (via _ensure_available) before seeking. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

iemejia · 2026-07-12T07:37:06Z

    my $block_count = decode_long($class, undef, undef, $reader);
    while ($block_count) {
        if ($block_count < 0) {
-            $reader->seek($block_count, 0);
-            next;
+            # A negative count is followed by a long block size in bytes, which
+            # lets the whole block be skipped without decoding each item. Skip


Great catch — this was a genuine pre-existing bug. Fixed in fce9c6a: skip_block no longer shifts an implicit $class; it takes ($reader, $block_content) to match the plain-function call style used by skip_array/skip_map, and passes PACKAGE to decode_long (which ignores its first argument). Previously $class captured the reader handle and $reader captured the coderef, so decode_long would call $coderef->read and die whenever an array/map field was skipped during projection. Added a test that skips writer array and map fields absent from the reader schema (it would have crashed on the old code).

Review feedback: skip_block used a method-style signature (my $class = shift) but its only callers, skip_array and skip_map, invoke it as a plain function skip_block($reader, $coderef). That made $class capture the reader handle and $reader capture the coderef, so decode_long would try $coderef->read and die whenever an array or map field was skipped during schema projection. Drop the implicit $class parameter so the plain-function call style works, and pass __PACKAGE__ to decode_long (which ignores it). Added a test that skips writer array and map fields absent from the reader schema. Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

github-actions Bot added the Perl label Jul 11, 2026

iemejia requested a review from Copilot July 11, 2026 21:57

Copilot started reviewing on behalf of iemejia July 11, 2026 21:58 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 11, 2026 22:36

Copilot started reviewing on behalf of iemejia July 11, 2026 22:36 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 11, 2026 23:12

Copilot started reviewing on behalf of iemejia July 11, 2026 23:12 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 05:05

Copilot started reviewing on behalf of iemejia July 12, 2026 05:05 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 05:34

Copilot started reviewing on behalf of iemejia July 12, 2026 05:34 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 06:46

Copilot started reviewing on behalf of iemejia July 12, 2026 06:47 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 07:21

Copilot started reviewing on behalf of iemejia July 12, 2026 07:21 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

iemejia requested a review from Copilot July 12, 2026 07:48

Copilot started reviewing on behalf of iemejia July 12, 2026 07:49 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

		# Map keys are strings (>= 1 byte length prefix) plus the value.
		my $min_bytes = 1 + _min_bytes_per_element($writer_values);

Uh oh!

Conversation

iemejia commented Jul 11, 2026

What is the purpose of the change

Verifying this change

Documentation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants