Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567332840
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -207,32 +212,156 @@ sub read_block_header {
return if $codec eq 'null';
- ## we need to read the entire block into memory, to inflate it
- my $nread = read $fh, my $block, $datafile->{block_size} + MARKER_SIZE
- or croak "Error reading from file: $!";
+ ## Guard against an attacker-controlled block_size triggering a huge
+ ## allocation for the compressed block itself, before any decompression
+ ## happens. When the reader is configured with block_max_size, reject a
+ ## block whose declared compressed size exceeds that bound up front.
+ my $block_max = $datafile->{block_max_size};
+ if (defined $block_max && $datafile->{block_size} > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $datafile->{block_size} exceeds the
configured block_max_size of $block_max bytes"
+ );
+ }
+
+ ## we need to read the entire block into memory, to inflate it. Verify the
+ ## exact byte count: a short read (truncated/malformed file) would
otherwise
+ ## slip through and surface later as a confusing marker/decompressor error.
+ my $want = $datafile->{block_size} + MARKER_SIZE;
+ my $block;
+ my $nread = read $fh, $block, $want;
+ if (!defined $nread) {
+ croak "Error reading from file: $!";
+ }
+ if ($nread != $want) {
+ croak "Short read: expected $want bytes for the block, got $nread
(truncated file?)";
+ }
## remove the marker
my $marker = substr $block, -(MARKER_SIZE), MARKER_SIZE, '';
$datafile->{block_marker} = $marker;
+ ## The decompressed size of a block is capped to guard against a block with
+ ## a very high compression ratio expanding to far more memory than its
+ ## compressed size. The limit is the AVRO_MAX_DECOMPRESS_LENGTH environment
+ ## variable, or DEFAULT_MAX_DECOMPRESS_LENGTH. (Note: block_max_size is a
+ ## writer-side flush threshold measured in compressed bytes and is not
reused
+ ## here to avoid conflating the two units.)
+ my $limit = _max_decompress_length();
+
## this is our new reader
$datafile->{reader} = do {
if ($codec eq 'deflate') {
- IO::Uncompress::RawInflate->new(\$block);
+ my $z = IO::Uncompress::RawInflate->new(\$block)
+ or croak "Error inflating block:
$IO::Uncompress::RawInflate::RawInflateError";
+ my $uncompressed = _inflate_bounded($z, $limit);
+ _open_decompressed(\$uncompressed);
}
elsif ($codec eq 'bzip2') {
- my $uncompressed;
- bunzip2 \$block => \$uncompressed;
- do { open $fh, '<', \$uncompressed; $fh };
+ my $z = IO::Uncompress::Bunzip2->new(\$block)
+ or croak "Error decompressing bzip2 block:
$IO::Uncompress::Bunzip2::Bunzip2Error";
+ my $uncompressed = _inflate_bounded($z, $limit);
+ _open_decompressed(\$uncompressed);
}
elsif ($codec eq 'zstandard') {
- do { open $fh, '<', \(decompress(\$block)); $fh };
+ my $uncompressed = _zstd_decompress_bounded(\$block, $limit);
+ _open_decompressed(\$uncompressed);
}
};
return;
}
+## Open an in-memory read handle over the decompressed block, surfacing any
+## failure via croak rather than leaving $datafile->{reader} undefined (which
+## would fail later with a less clear error). The handle keeps a reference to
+## the scalar, so the caller's buffer stays alive for the lifetime of the read.
+sub _open_decompressed {
+ my ($uncompressed_ref) = @_;
+ open my $fh, '<', $uncompressed_ref
+ or croak "Error opening decompressed block for reading: $!";
+ return $fh;
+}
+
+## Read from a streaming decompressor in chunks, rejecting the block as soon as
+## its decompressed size would exceed $limit so an over-large (or malicious)
+## block is not fully materialized in memory. Each read is sized to the
+## remaining budget (capped at 64 KiB) so the buffer overshoots $limit by at
+## most one byte before the check fires.
+sub _inflate_bounded {
+ my ($z, $limit) = @_;
+ my $uncompressed = '';
+ my $chunk;
+ my $status;
+ while (1) {
+ my $budget = $limit - bytes::length($uncompressed) + 1;
+ my $to_read = $budget < 65536 ? $budget : 65536;
+ $status = $z->read($chunk, $to_read);
+ last unless defined $status && $status > 0;
+ $uncompressed .= $chunk;
+ _check_decompress_length(bytes::length($uncompressed), $limit);
+ }
+ if (!defined $status || $status < 0) {
+ croak "Error decompressing block: " . $z->error;
+ }
+ return $uncompressed;
+}
+
+## Streaming zstandard decompression, bounded the same way as _inflate_bounded
so
+## a high-ratio block is rejected before its full form is materialized.
+sub _zstd_decompress_bounded {
+ my ($block_ref, $limit) = @_;
+ # Load the zstandard decompressor lazily so the reader still loads and
works
+ # for other codecs when Compress::Zstd::Decompressor is unavailable (e.g.
an
+ # older Compress::Zstd distribution that lacks the Decompressor submodule).
+ unless (eval { require Compress::Zstd::Decompressor; 1 }) {
+ Avro::DataFile::Error::UnsupportedCodec->throw(
+ "Cannot read zstandard-compressed block:
Compress::Zstd::Decompressor is not available"
+ );
+ }
+ my $decompressor = Compress::Zstd::Decompressor->new;
+ my $uncompressed = '';
+ my $length = bytes::length($$block_ref);
+ my $offset = 0;
+ while ($offset < $length) {
+ my $piece = substr($$block_ref, $offset, 65536);
+ $offset += 65536;
+ my $out = $decompressor->decompress($piece);
+ # The streaming decompressor croaks on a corrupt frame and otherwise
+ # emits all output produced while consuming the input it is given
(there
+ # is no separate flush step in this API). Treat an undefined return as
a
+ # failure and fail closed, rather than silently skipping it, so a
+ # malformed block cannot masquerade as a short, within-limit result.
+ unless (defined $out) {
+ croak "Error decompressing zstandard block";
+ }
+ # Check the prospective total before growing $uncompressed so a single
+ # large decompressed chunk cannot transiently balloon memory past the
+ # limit (or double peak memory from string reallocation).
+ _check_decompress_length(
+ bytes::length($uncompressed) + bytes::length($out), $limit);
+ $uncompressed .= $out;
+ }
+ return $uncompressed;
+}
+
+sub _check_decompress_length {
+ my ($length, $limit) = @_;
+ if ($length > $limit) {
+ Avro::DataFile::Error::DecompressionSize->throw(
+ "Decompressed block size exceeds the maximum allowed of $limit
bytes"
+ );
+ }
Review Comment:
The DecompressionSize exception message doesn’t include the actual
decompressed size that triggered the limit, which makes debugging difficult
(especially when the limit comes from an env var). Including `$length` makes
the error more informative without changing behavior.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -207,32 +212,156 @@ sub read_block_header {
return if $codec eq 'null';
- ## we need to read the entire block into memory, to inflate it
- my $nread = read $fh, my $block, $datafile->{block_size} + MARKER_SIZE
- or croak "Error reading from file: $!";
+ ## Guard against an attacker-controlled block_size triggering a huge
+ ## allocation for the compressed block itself, before any decompression
+ ## happens. When the reader is configured with block_max_size, reject a
+ ## block whose declared compressed size exceeds that bound up front.
+ my $block_max = $datafile->{block_max_size};
+ if (defined $block_max && $datafile->{block_size} > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $datafile->{block_size} exceeds the
configured block_max_size of $block_max bytes"
+ );
+ }
+
+ ## we need to read the entire block into memory, to inflate it. Verify the
+ ## exact byte count: a short read (truncated/malformed file) would
otherwise
+ ## slip through and surface later as a confusing marker/decompressor error.
+ my $want = $datafile->{block_size} + MARKER_SIZE;
+ my $block;
+ my $nread = read $fh, $block, $want;
+ if (!defined $nread) {
+ croak "Error reading from file: $!";
+ }
+ if ($nread != $want) {
+ croak "Short read: expected $want bytes for the block, got $nread
(truncated file?)";
+ }
## remove the marker
my $marker = substr $block, -(MARKER_SIZE), MARKER_SIZE, '';
$datafile->{block_marker} = $marker;
+ ## The decompressed size of a block is capped to guard against a block with
+ ## a very high compression ratio expanding to far more memory than its
+ ## compressed size. The limit is the AVRO_MAX_DECOMPRESS_LENGTH environment
+ ## variable, or DEFAULT_MAX_DECOMPRESS_LENGTH. (Note: block_max_size is a
+ ## writer-side flush threshold measured in compressed bytes and is not
reused
+ ## here to avoid conflating the two units.)
+ my $limit = _max_decompress_length();
+
## this is our new reader
$datafile->{reader} = do {
if ($codec eq 'deflate') {
- IO::Uncompress::RawInflate->new(\$block);
+ my $z = IO::Uncompress::RawInflate->new(\$block)
+ or croak "Error inflating block:
$IO::Uncompress::RawInflate::RawInflateError";
+ my $uncompressed = _inflate_bounded($z, $limit);
+ _open_decompressed(\$uncompressed);
}
elsif ($codec eq 'bzip2') {
- my $uncompressed;
- bunzip2 \$block => \$uncompressed;
- do { open $fh, '<', \$uncompressed; $fh };
+ my $z = IO::Uncompress::Bunzip2->new(\$block)
+ or croak "Error decompressing bzip2 block:
$IO::Uncompress::Bunzip2::Bunzip2Error";
+ my $uncompressed = _inflate_bounded($z, $limit);
+ _open_decompressed(\$uncompressed);
}
elsif ($codec eq 'zstandard') {
- do { open $fh, '<', \(decompress(\$block)); $fh };
+ my $uncompressed = _zstd_decompress_bounded(\$block, $limit);
+ _open_decompressed(\$uncompressed);
}
};
return;
}
+## Open an in-memory read handle over the decompressed block, surfacing any
+## failure via croak rather than leaving $datafile->{reader} undefined (which
+## would fail later with a less clear error). The handle keeps a reference to
+## the scalar, so the caller's buffer stays alive for the lifetime of the read.
+sub _open_decompressed {
+ my ($uncompressed_ref) = @_;
+ open my $fh, '<', $uncompressed_ref
+ or croak "Error opening decompressed block for reading: $!";
+ return $fh;
+}
+
+## Read from a streaming decompressor in chunks, rejecting the block as soon as
+## its decompressed size would exceed $limit so an over-large (or malicious)
+## block is not fully materialized in memory. Each read is sized to the
+## remaining budget (capped at 64 KiB) so the buffer overshoots $limit by at
+## most one byte before the check fires.
+sub _inflate_bounded {
+ my ($z, $limit) = @_;
+ my $uncompressed = '';
+ my $chunk;
+ my $status;
+ while (1) {
+ my $budget = $limit - bytes::length($uncompressed) + 1;
+ my $to_read = $budget < 65536 ? $budget : 65536;
+ $status = $z->read($chunk, $to_read);
+ last unless defined $status && $status > 0;
+ $uncompressed .= $chunk;
+ _check_decompress_length(bytes::length($uncompressed), $limit);
+ }
+ if (!defined $status || $status < 0) {
+ croak "Error decompressing block: " . $z->error;
+ }
+ return $uncompressed;
+}
+
+## Streaming zstandard decompression, bounded the same way as _inflate_bounded
so
+## a high-ratio block is rejected before its full form is materialized.
+sub _zstd_decompress_bounded {
+ my ($block_ref, $limit) = @_;
+ # Load the zstandard decompressor lazily so the reader still loads and
works
+ # for other codecs when Compress::Zstd::Decompressor is unavailable (e.g.
an
+ # older Compress::Zstd distribution that lacks the Decompressor submodule).
+ unless (eval { require Compress::Zstd::Decompressor; 1 }) {
+ Avro::DataFile::Error::UnsupportedCodec->throw(
+ "Cannot read zstandard-compressed block:
Compress::Zstd::Decompressor is not available"
+ );
+ }
Review Comment:
When lazy-loading Compress::Zstd::Decompressor fails, the error is always
reported as “not available”. In practice, `require` can fail for other reasons
(e.g., missing shared library, compilation error). Including the underlying
`$@` makes the exception actionable for users diagnosing zstandard support.
--
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]
