Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564842931
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +231,124 @@ sub read_block_header {
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";
+ }
+ $uncompressed .= $out;
+ _check_decompress_length(bytes::length($uncompressed), $limit);
+ }
Review Comment:
In the zstandard bounded decompression loop, the limit check happens after
concatenating `$out` into `$uncompressed`. If `$out` is very large for a single
`decompress($piece)` call, this can transiently allocate far beyond `$limit`
and can double peak memory due to string reallocation/copying. Check the
prospective length before concatenation and throw before growing
`$uncompressed`.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -207,6 +212,17 @@ sub read_block_header {
return if $codec eq 'null';
+ ## 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::DecompressionSize->throw(
+ "Compressed block size $datafile->{block_size} exceeds the
configured block_max_size of $block_max bytes"
+ );
Review Comment:
The new `block_max_size` pre-read guard throws
`Avro::DataFile::Error::DecompressionSize`, but this condition is about the
*compressed* block size, not the decompressed size cap. Reusing the same
exception class makes it hard for callers/tests to distinguish "compressed
block too large to buffer" vs "decompressed output too large" and can lead to
incorrect error handling.
##########
lang/perl/t/07_datafile_decompress_limit.t:
##########
@@ -0,0 +1,126 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+#!/usr/bin/env perl
+
+# A data-file block is decompressed according to the file's codec. A block with
+# a very high compression ratio can expand to far more memory than its
+# compressed size. These tests ensure that reading such a block is rejected
+# instead of allocating without bound.
+
+use strict;
+use warnings;
+use File::Temp;
+use Avro::Schema;
+use Avro::DataFileWriter;
+use Test::More;
+use Test::Exception;
+
+use_ok 'Avro::DataFileReader';
+
+my $schema = Avro::Schema->parse('"string"');
+
+sub codec_file {
+ my ($codec, $payload) = @_;
+ my $fh = File::Temp->new(UNLINK => 1);
+ my $writer = Avro::DataFileWriter->new(
+ fh => $fh,
+ writer_schema => $schema,
+ codec => $codec,
+ );
+ $writer->print($payload);
+ $writer->flush;
+ seek $fh, 0, 0;
+ return $fh;
+}
+
+## A large, highly compressible value compresses to a tiny block but would
+## decompress to far more than the configured limit; reading it must be
rejected
+## for every codec. The limit is set via AVRO_MAX_DECOMPRESS_LENGTH.
+sub assert_codec_rejects_oversized {
+ my ($codec) = @_;
+ my $big = "a" x (64 * 1024); # 64 KiB
+ my $fh = codec_file($codec, $big);
+ local $ENV{AVRO_MAX_DECOMPRESS_LENGTH} = 1024;
+ my $reader = Avro::DataFileReader->new(
+ fh => $fh,
+ reader_schema => $schema,
+ );
+ throws_ok { $reader->all }
+ 'Avro::DataFile::Error::DecompressionSize',
+ "$codec block exceeding the limit is rejected";
+}
+
+assert_codec_rejects_oversized('deflate');
+
+SKIP: {
+ eval { require IO::Compress::Bzip2; 1 }
+ or skip 'IO::Compress::Bzip2 not available', 1;
+ assert_codec_rejects_oversized('bzip2');
+}
+
+SKIP: {
+ eval { require Compress::Zstd::Decompressor; 1 }
+ or skip 'Compress::Zstd::Decompressor not available', 1;
+ assert_codec_rejects_oversized('zstandard');
+}
+
+## A block within the limit still decodes correctly.
+sub assert_codec_within_limit_decodes {
+ my ($codec) = @_;
+ my $payload = "hello world";
+ my $fh = codec_file($codec, $payload);
+ local $ENV{AVRO_MAX_DECOMPRESS_LENGTH} = 1024 * 1024;
+ my $reader = Avro::DataFileReader->new(
+ fh => $fh,
+ reader_schema => $schema,
+ );
+ my @all = $reader->all;
+ is_deeply \@all, [$payload], "$codec block within the limit decodes";
+}
+
+assert_codec_within_limit_decodes('deflate');
+
+SKIP: {
+ eval { require IO::Compress::Bzip2; 1 }
+ or skip 'IO::Compress::Bzip2 not available', 1;
+ assert_codec_within_limit_decodes('bzip2');
+}
+
+SKIP: {
+ eval { require Compress::Zstd::Decompressor; 1 }
+ or skip 'Compress::Zstd::Decompressor not available', 1;
+ assert_codec_within_limit_decodes('zstandard');
+}
+
+## When block_max_size is configured on the reader, a block whose declared
+## compressed size exceeds it is rejected before the compressed block is read
+## into memory (guarding against an attacker-controlled block_size allocation).
+{
+ my $payload = "a" x (32 * 1024); # 32 KiB, compresses to a few dozen bytes
+ my $fh = codec_file('deflate', $payload);
+ my $reader = Avro::DataFileReader->new(
+ fh => $fh,
+ reader_schema => $schema,
+ block_max_size => 8, # smaller than any real compressed block
+ );
+ throws_ok { $reader->all }
+ 'Avro::DataFile::Error::DecompressionSize',
+ 'compressed block exceeding block_max_size is rejected before reading';
+}
Review Comment:
The `block_max_size` test can pass for the wrong reason if a user/test
runner has `AVRO_MAX_DECOMPRESS_LENGTH` set low: it will still throw
`DecompressionSize`, but due to the decompression cap rather than the
compressed-size guard. Make the test deterministic by setting
`AVRO_MAX_DECOMPRESS_LENGTH` to a large value in this block and assert the
error message mentions the compressed block size / block_max_size check.
--
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]
