Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564738375
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +220,105 @@ 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);
+ do { open my $fh, '<', \$uncompressed; $fh };
}
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);
+ do { open my $fh, '<', \$uncompressed; $fh };
}
elsif ($codec eq 'zstandard') {
- do { open $fh, '<', \(decompress(\$block)); $fh };
+ my $uncompressed = _zstd_decompress_bounded(\$block, $limit);
+ do { open my $fh, '<', \$uncompressed; $fh };
}
};
return;
}
+## 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 - 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(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) = @_;
+ my $decompressor = Compress::Zstd::Decompressor->new;
+ my $uncompressed = '';
+ my $length = 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(length($uncompressed), $limit);
Review Comment:
Similar to `_inflate_bounded`, this zstandard path uses
`length($uncompressed)` when enforcing the maximum decompressed size. To ensure
the cap is applied to bytes (not characters), use `bytes::length` when calling
`_check_decompress_length`.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +220,105 @@ 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);
+ do { open my $fh, '<', \$uncompressed; $fh };
}
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);
+ do { open my $fh, '<', \$uncompressed; $fh };
}
elsif ($codec eq 'zstandard') {
- do { open $fh, '<', \(decompress(\$block)); $fh };
+ my $uncompressed = _zstd_decompress_bounded(\$block, $limit);
+ do { open my $fh, '<', \$uncompressed; $fh };
}
};
return;
}
+## 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 - 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(length($uncompressed), $limit);
Review Comment:
The decompression limit is intended to be in bytes, but `_inflate_bounded`
uses `length($uncompressed)` (character length). If the scalar were ever
upgraded to UTF-8, `length` could undercount bytes and allow a block to exceed
the configured byte cap. Use `bytes::length` for byte-accurate accounting (this
matches how `Avro::DataFileWriter` tracks block sizes).
--
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]
