Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564411173
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,72 @@ 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.
+ my $limit = $datafile->block_max_size;
+ $limit = _max_decompress_length() unless defined $limit;
+
Review Comment:
`block_max_size` is used here as the maximum *decompressed* block size, but
in `Avro::DataFileWriter` it controls when to flush based on `_current_size`
(which is the *compressed* block size for deflate/bzip2/zstandard). Reusing the
same attribute for different units/semantics between reader and writer is
likely to confuse callers and can cause surprising failures when a caller sets
`block_max_size` expecting writer-like behavior.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,72 @@ 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.
+ my $limit = $datafile->block_max_size;
+ $limit = _max_decompress_length() unless defined $limit;
+
## 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 = decompress(\$block);
+ _check_decompress_length(length($uncompressed), $limit);
+ do { open my $fh, '<', \$uncompressed; $fh };
Review Comment:
The zstandard path fully decompresses the block (`decompress(\$block)`)
before checking the size limit. That means a large/malicious zstandard block
can still force an unbounded allocation (OOM) even though
`_check_decompress_length` runs afterwards, so the intended decompression-bomb
protection is not actually effective for zstandard.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,72 @@ 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.
+ my $limit = $datafile->block_max_size;
+ $limit = _max_decompress_length() unless defined $limit;
+
## 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 = decompress(\$block);
+ _check_decompress_length(length($uncompressed), $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.
+sub _inflate_bounded {
+ my ($z, $limit) = @_;
+ my $uncompressed = '';
+ my $chunk;
+ my $status;
+ while (($status = $z->read($chunk, 65536)) > 0) {
+ $uncompressed .= $chunk;
+ _check_decompress_length(length($uncompressed), $limit);
+ }
+ if (!defined $status || $status < 0) {
+ croak "Error decompressing block";
+ }
Review Comment:
`_inflate_bounded` reads fixed 64KiB chunks and only checks the limit
*after* appending, so an over-limit block can still allocate up to ~64KiB
beyond `$limit` before being rejected. You can tighten the bound by sizing each
read based on the remaining budget (capped at 64KiB), and include the
decompressor error in the failure path to aid debugging.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -308,4 +362,7 @@ sub eof {
package Avro::DataFile::Error::UnsupportedCodec;
use parent 'Error::Simple';
+package Avro::DataFile::Error::DecompressionSize;
+use parent -norequire, 'Error::Simple';
Review Comment:
`Avro::DataFile::Error::DecompressionSize` uses `use parent -norequire,
'Error::Simple'` while the other error classes in this code
(`UnsupportedCodec`, `InvalidCodec`) use `use parent 'Error::Simple'`. Keeping
this consistent avoids surprising load-order differences and makes the error
hierarchy uniform.
##########
lang/perl/t/07_datafile_decompress_limit.t:
##########
@@ -0,0 +1,93 @@
+# 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 deflate_file {
+ my ($payload) = @_;
+ my $fh = File::Temp->new(UNLINK => 1);
+ my $writer = Avro::DataFileWriter->new(
+ fh => $fh,
+ writer_schema => $schema,
+ codec => 'deflate',
+ );
+ $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.
+{
+ my $big = "a" x (64 * 1024); # 64 KiB
+ my $fh = deflate_file($big);
+ my $reader = Avro::DataFileReader->new(
+ fh => $fh,
+ reader_schema => $schema,
+ block_max_size => 1024,
+ );
+ throws_ok { $reader->all }
+ 'Avro::DataFile::Error::DecompressionSize',
+ 'deflate block exceeding the limit is rejected';
+}
+
+## The AVRO_MAX_DECOMPRESS_LENGTH environment variable is honored too.
+{
+ my $big = "a" x (64 * 1024);
+ my $fh = deflate_file($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',
+ 'AVRO_MAX_DECOMPRESS_LENGTH is honored';
+}
+
+## A block within the limit still decodes correctly.
+{
+ my $payload = "hello world";
+ my $fh = deflate_file($payload);
+ my $reader = Avro::DataFileReader->new(
+ fh => $fh,
+ reader_schema => $schema,
+ block_max_size => 1024 * 1024,
+ );
+ my @all = $reader->all;
+ is_deeply \@all, [$payload], 'deflate block within the limit decodes';
+}
+
+done_testing;
Review Comment:
This test file only exercises the over-limit behavior for `deflate`, but the
PR also changes limit enforcement for `bzip2` and `zstandard`. Adding
equivalent over-limit tests for those codecs would protect against regressions
in the additional branches added to `DataFileReader`.
--
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]
