You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "iemejia (via GitHub)" <gi...@apache.org> on 2026/07/11 13:04:12 UTC
[PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
iemejia opened a new pull request, #3855:
URL: https://github.com/apache/avro/pull/3855
## What is the purpose of the change
The deflate and bzip2 codecs are inflated in chunks and bounded before the full output is materialized. The limit uses the existing (previously unenforced) `block_max_size` attribute if set, otherwise the default/environment value. Exceeding the limit throws `Avro::DataFile::Error::DecompressionSize`.
When reading a data file, each block is decompressed according to the file's codec. A block with a very high compression ratio (or a malformed block) could expand to far more memory than its compressed size (a decompression bomb). This enforces a configurable maximum decompressed size while reading each block, mirroring the Java SDK's decompression limit (AVRO-4247). The limit defaults to 200 MiB and can be overridden with the `AVRO_MAX_DECOMPRESS_LENGTH` environment variable.
This is part of the umbrella issue AVRO-4283.
## Verifying this change
This change added tests and can be verified as follows:
- Added `t/07_datafile_decompress_limit.t` covering deflate over-limit rejection (via `block_max_size` and the environment variable) and within-limit decoding.
- 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; the new `AVRO_MAX_DECOMPRESS_LENGTH` environment variable is documented in code comments)
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567586551
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -203,36 +208,172 @@ sub read_block_header {
$datafile->{block_size} = Avro::BinaryDecoder->decode_long(
undef, undef, $fh,
);
+ ## Both are Avro long (zigzag) values, so a malformed/truncated file can
+ ## yield negatives. A negative block_size would flow into $want and a
+ ## negative-length read; a negative object_count is equally nonsensical.
+ ## Reject both before first use.
+ if ($datafile->{object_count} < 0) {
+ croak "Invalid negative object count: $datafile->{object_count}";
+ }
+ if ($datafile->{block_size} < 0) {
+ croak "Invalid negative block size: $datafile->{block_size}";
+ }
$datafile->{block_start} = tell $fh;
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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $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?)";
+ }
Review Comment:
Fixed — the block is now read in bounded 64 KiB chunks until $want or EOF (rather than one read that pre-extends the buffer to the declared block_size), then the exact byte count is verified, so a malformed huge block_size with little data fails after a bounded allocation.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564872958
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +231,128 @@ 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"
+ );
+ }
Review Comment:
Fixed both ways. Compress::Zstd::Decompressor (the streaming interface) first shipped in Compress::Zstd 0.10, so the prerequisite in Makefile.PL is now 'requires Compress::Zstd, 0.10'. As a belt-and-suspenders for portability, the zstandard case in t/04_datafile.t now SKIPs when Compress::Zstd::Decompressor is unavailable. Pushed in 31f054c.
##########
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::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
my $nread = read $fh, my $block, $datafile->{block_size} + MARKER_SIZE
or croak "Error reading from file: $!";
Review Comment:
Fixed. The block read now verifies the exact expected byte count (block_size + MARKER_SIZE): it croaks distinctly on a read error (undef) and on a short read (truncated/malformed file) with a clear message, instead of relying on 'or croak' which treated EOF/short reads as a generic error with $! often unset. Pushed in 31f054c.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567393550
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -207,32 +212,157 @@ 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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $block_size exceeds the configured block_max_size of $block_max bytes"
+ );
+ }
Review Comment:
`block_size` and `object_count` are decoded as Avro `long` values (zigzag), so a malformed/truncated file can yield negative numbers. A negative `block_size` can flow into `$want = $datafile->{block_size} + MARKER_SIZE` and later arithmetic/seek logic, leading to undefined behavior or errors from negative-length reads. Validate both values are >= 0 immediately after decoding (or before first use) and fail with a clear error.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564847567
##########
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:
Fixed. _zstd_decompress_bounded now checks the prospective total (bytes::length($uncompressed) + bytes::length($out)) against the limit before concatenating, so a single large decompressed chunk cannot transiently balloon memory past the cap or double peak memory via reallocation. Pushed in f917988.
##########
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:
Fixed. The pre-read guard now throws a distinct Avro::DataFile::Error::CompressedBlockSize (about the compressed block size), kept separate from DecompressionSize (the decompressed-size cap), so callers can distinguish the two conditions. Pushed in f917988.
##########
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:
Fixed. The test now sets AVRO_MAX_DECOMPRESS_LENGTH to a large value so the rejection can only come from the compressed-size guard, and asserts both the distinct class (Avro::DataFile::Error::CompressedBlockSize) and that the message mentions block_max_size. Pushed in f917988.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3568081533
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -203,36 +208,183 @@ sub read_block_header {
$datafile->{block_size} = Avro::BinaryDecoder->decode_long(
undef, undef, $fh,
);
+ ## Both are Avro long (zigzag) values, so a malformed/truncated file can
+ ## yield negatives. A negative block_size would flow into $want and a
+ ## negative-length read; a negative object_count is equally nonsensical.
+ ## Reject both before first use.
+ if ($datafile->{object_count} < 0) {
+ croak "Invalid negative object count: $datafile->{object_count}";
+ }
+ if ($datafile->{block_size} < 0) {
+ croak "Invalid negative block size: $datafile->{block_size}";
+ }
$datafile->{block_start} = tell $fh;
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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $block_size exceeds the configured block_max_size of $block_max bytes"
+ );
+ }
+
+ ## we need to read the entire block into memory, to inflate it. Read in
+ ## bounded chunks (rather than a single read of $want bytes, which would
+ ## pre-extend the buffer to the attacker-controlled block_size before a short
+ ## read is detected) and verify the exact byte count afterward: 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 $chunk_size = 64 * 1024;
+ while (length($block) < $want) {
+ my $need = $want - length($block);
+ $need = $chunk_size if $need > $chunk_size;
+ my $nread = read $fh, my $buf, $need;
+ if (!defined $nread) {
+ croak "Error reading from file: $!";
+ }
+ last if $nread == 0; # EOF
+ $block .= $buf;
+ }
+ if (length($block) != $want) {
+ croak "Short read: expected $want bytes for the block, got "
+ . length($block) . " (truncated file?)";
+ }
Review Comment:
Fixed — the bounded block-read loop and short-read check now use bytes::length, so a UTF-8-flagged scalar can't miscount bytes.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -203,36 +208,183 @@ sub read_block_header {
$datafile->{block_size} = Avro::BinaryDecoder->decode_long(
undef, undef, $fh,
);
+ ## Both are Avro long (zigzag) values, so a malformed/truncated file can
+ ## yield negatives. A negative block_size would flow into $want and a
+ ## negative-length read; a negative object_count is equally nonsensical.
+ ## Reject both before first use.
+ if ($datafile->{object_count} < 0) {
+ croak "Invalid negative object count: $datafile->{object_count}";
+ }
+ if ($datafile->{block_size} < 0) {
+ croak "Invalid negative block size: $datafile->{block_size}";
+ }
$datafile->{block_start} = tell $fh;
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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $block_size exceeds the configured block_max_size of $block_max bytes"
+ );
+ }
+
+ ## we need to read the entire block into memory, to inflate it. Read in
+ ## bounded chunks (rather than a single read of $want bytes, which would
+ ## pre-extend the buffer to the attacker-controlled block_size before a short
+ ## read is detected) and verify the exact byte count afterward: 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 $chunk_size = 64 * 1024;
+ while (length($block) < $want) {
+ my $need = $want - length($block);
+ $need = $chunk_size if $need > $chunk_size;
+ my $nread = read $fh, my $buf, $need;
+ if (!defined $nread) {
+ croak "Error reading from file: $!";
+ }
+ last if $nread == 0; # EOF
+ $block .= $buf;
+ }
+ if (length($block) != $want) {
+ croak "Short read: expected $want bytes for the block, got "
+ . length($block) . " (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; it is used above
+ ## as a compressed-size pre-read guard, but is not reused here for the
+ ## decompressed-size cap, 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;
Review Comment:
Mitigated — Compress::Zstd's streaming decompress() has no max-output parameter, so the smallest guarantee is one call's output. The input is now fed in 16 KiB pieces (down from 64 KiB) to bound each call's transient $out (~one internal block), and the running total is still checked before concatenating into $uncompressed, so peak stays at uncompressed + one bounded piece.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567634596
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -203,36 +208,183 @@ sub read_block_header {
$datafile->{block_size} = Avro::BinaryDecoder->decode_long(
undef, undef, $fh,
);
+ ## Both are Avro long (zigzag) values, so a malformed/truncated file can
+ ## yield negatives. A negative block_size would flow into $want and a
+ ## negative-length read; a negative object_count is equally nonsensical.
+ ## Reject both before first use.
+ if ($datafile->{object_count} < 0) {
+ croak "Invalid negative object count: $datafile->{object_count}";
+ }
+ if ($datafile->{block_size} < 0) {
+ croak "Invalid negative block size: $datafile->{block_size}";
+ }
$datafile->{block_start} = tell $fh;
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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $block_size exceeds the configured block_max_size of $block_max bytes"
+ );
+ }
+
+ ## we need to read the entire block into memory, to inflate it. Read in
+ ## bounded chunks (rather than a single read of $want bytes, which would
+ ## pre-extend the buffer to the attacker-controlled block_size before a short
+ ## read is detected) and verify the exact byte count afterward: 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 $chunk_size = 64 * 1024;
+ while (length($block) < $want) {
+ my $need = $want - length($block);
+ $need = $chunk_size if $need > $chunk_size;
+ my $nread = read $fh, my $buf, $need;
+ if (!defined $nread) {
+ croak "Error reading from file: $!";
+ }
+ last if $nread == 0; # EOF
+ $block .= $buf;
+ }
+ if (length($block) != $want) {
+ croak "Short read: expected $want bytes for the block, got "
+ . length($block) . " (truncated file?)";
+ }
Review Comment:
The bounded block read uses length() to track how many bytes have been accumulated. If the scalar ever becomes UTF-8 flagged (e.g., via an IO layer), length() counts characters rather than bytes, which can cause the loop to read the wrong amount and mis-handle the marker boundary. Since this code is explicitly byte-oriented and the file already depends on bytes::length elsewhere, use bytes::length consistently for the block-size accounting and error message.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -203,36 +208,183 @@ sub read_block_header {
$datafile->{block_size} = Avro::BinaryDecoder->decode_long(
undef, undef, $fh,
);
+ ## Both are Avro long (zigzag) values, so a malformed/truncated file can
+ ## yield negatives. A negative block_size would flow into $want and a
+ ## negative-length read; a negative object_count is equally nonsensical.
+ ## Reject both before first use.
+ if ($datafile->{object_count} < 0) {
+ croak "Invalid negative object count: $datafile->{object_count}";
+ }
+ if ($datafile->{block_size} < 0) {
+ croak "Invalid negative block size: $datafile->{block_size}";
+ }
$datafile->{block_start} = tell $fh;
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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $block_size exceeds the configured block_max_size of $block_max bytes"
+ );
+ }
+
+ ## we need to read the entire block into memory, to inflate it. Read in
+ ## bounded chunks (rather than a single read of $want bytes, which would
+ ## pre-extend the buffer to the attacker-controlled block_size before a short
+ ## read is detected) and verify the exact byte count afterward: 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 $chunk_size = 64 * 1024;
+ while (length($block) < $want) {
+ my $need = $want - length($block);
+ $need = $chunk_size if $need > $chunk_size;
+ my $nread = read $fh, my $buf, $need;
+ if (!defined $nread) {
+ croak "Error reading from file: $!";
+ }
+ last if $nread == 0; # EOF
+ $block .= $buf;
+ }
+ if (length($block) != $want) {
+ croak "Short read: expected $want bytes for the block, got "
+ . length($block) . " (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; it is used above
+ ## as a compressed-size pre-read guard, but is not reused here for the
+ ## decompressed-size cap, 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;
Review Comment:
In _zstd_decompress_bounded, the decompression-size cap is checked only after $decompressor->decompress($piece) has returned. Because decompress() materializes its output into the $out scalar before any size check can run, a highly-compressible/malicious frame could still force a very large allocation for $out even though the code then throws before concatenating into $uncompressed. This undermines the goal of rejecting over-limit blocks before large allocations are made; consider switching to (or adding a fallback for) a zstd API that supports pulling decompressed output in bounded chunks (read-style streaming) so the limit can be enforced before creating large intermediate buffers.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564793466
##########
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; 1 }
+ or skip 'Compress::Zstd not available', 1;
Review Comment:
The zstandard SKIP guard requires `Compress::Zstd`, but the reader implementation depends on `Compress::Zstd::Decompressor`. If the installed `Compress::Zstd` distribution does not provide the `Decompressor` submodule (or it is split), this test can try to run and then fail when `Avro::DataFileReader` is loaded/used. Require the exact module needed by the reader in the SKIP condition so the test is reliably skipped when that dependency is missing.
##########
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; 1 }
+ or skip 'Compress::Zstd 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; 1 }
+ or skip 'Compress::Zstd not available', 1;
Review Comment:
Same zstandard SKIP issue for the within-limit path: check for `Compress::Zstd::Decompressor` (used by `Avro::DataFileReader`) rather than only `Compress::Zstd`, otherwise the test can attempt to run even when the reader-side dependency is missing.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3568160186
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -203,36 +208,190 @@ sub read_block_header {
$datafile->{block_size} = Avro::BinaryDecoder->decode_long(
undef, undef, $fh,
);
+ ## Both are Avro long (zigzag) values, so a malformed/truncated file can
+ ## yield negatives. A negative block_size would flow into $want and a
+ ## negative-length read; a negative object_count is equally nonsensical.
+ ## Reject both before first use.
+ if ($datafile->{object_count} < 0) {
+ croak "Invalid negative object count: $datafile->{object_count}";
+ }
+ if ($datafile->{block_size} < 0) {
+ croak "Invalid negative block size: $datafile->{block_size}";
+ }
$datafile->{block_start} = tell $fh;
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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $block_size exceeds the configured block_max_size of $block_max bytes"
+ );
+ }
+
+ ## we need to read the entire block into memory, to inflate it. Read in
+ ## bounded chunks (rather than a single read of $want bytes, which would
+ ## pre-extend the buffer to the attacker-controlled block_size before a short
+ ## read is detected) and verify the exact byte count afterward: 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 $chunk_size = 64 * 1024;
+ while (bytes::length($block) < $want) {
+ my $need = $want - bytes::length($block);
+ $need = $chunk_size if $need > $chunk_size;
+ my $nread = read $fh, my $buf, $need;
+ if (!defined $nread) {
+ croak "Error reading from file: $!";
+ }
+ last if $nread == 0; # EOF
+ $block .= $buf;
+ }
+ if (bytes::length($block) != $want) {
+ croak "Short read: expected $want bytes for the block, got "
+ . bytes::length($block) . " (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; it is used above
+ ## as a compressed-size pre-read guard, but is not reused here for the
+ ## decompressed-size cap, 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:
Fixed — the require now runs under `local $@` and the error is captured into a lexical (`$require_error`) before building the message, so the caller's $@ is preserved.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567557706
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -203,36 +208,172 @@ sub read_block_header {
$datafile->{block_size} = Avro::BinaryDecoder->decode_long(
undef, undef, $fh,
);
+ ## Both are Avro long (zigzag) values, so a malformed/truncated file can
+ ## yield negatives. A negative block_size would flow into $want and a
+ ## negative-length read; a negative object_count is equally nonsensical.
+ ## Reject both before first use.
+ if ($datafile->{object_count} < 0) {
+ croak "Invalid negative object count: $datafile->{object_count}";
+ }
+ if ($datafile->{block_size} < 0) {
+ croak "Invalid negative block size: $datafile->{block_size}";
+ }
$datafile->{block_start} = tell $fh;
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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $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?)";
+ }
Review Comment:
`read $fh, $block, $want` can allocate based on the attacker-controlled `$want` (block_size + MARKER_SIZE) before you detect a short read. On malformed files with an enormous declared block_size but little/no remaining data, this can still trigger an OOM/DoS even though you later croak on `Short read`. Consider reading the compressed block in bounded chunks until `$want` bytes are accumulated (or EOF), so truncated files fail without preallocating `$want` bytes.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564864645
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +231,128 @@ 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"
+ );
+ }
Review Comment:
The zstandard path now requires `Compress::Zstd::Decompressor`, but the Perl distribution only declares `requires 'Compress::Zstd'` (no minimum version) and existing tests (e.g. `t/04_datafile.t`) exercise zstandard read/write unconditionally. In environments where `Compress::Zstd` is present but lacks the `Decompressor` submodule, this change will make zstandard decoding fail with `UnsupportedCodec` and can break the test suite/CPAN installs unexpectedly.
Consider updating the Perl prerequisites to require a `Compress::Zstd` version that provides `Compress::Zstd::Decompressor`, and/or adjusting existing zstandard tests to SKIP when `Compress::Zstd::Decompressor` is unavailable so the suite remains portable.
##########
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::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
my $nread = read $fh, my $block, $datafile->{block_size} + MARKER_SIZE
or croak "Error reading from file: $!";
Review Comment:
`read` is currently checked with `or croak`, which treats a short read / EOF as an “error” (often with `$!` unset) and can yield confusing downstream failures (bad marker / decompressor errors) on truncated or malformed files. It’s safer to verify the exact byte count and throw a clear error on short reads.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564814325
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -28,17 +28,23 @@ use Object::Tiny qw{
use constant MARKER_SIZE => 16;
-# TODO: refuse to read a block more than block_max_size, instead
-# do partial reads
+# A data-file block is decompressed according to the file's codec. A block with
+# a very high compression ratio (or a malformed block) can expand to far more
+# memory than its compressed size. To guard against unbounded allocation, the
+# decompressed size of a single block is capped. This mirrors the Java SDK's
+# decompression limit (AVRO-4247). The default can be overridden with the
+# AVRO_MAX_DECOMPRESS_LENGTH environment variable.
+use constant DEFAULT_MAX_DECOMPRESS_LENGTH => 200 * 1024 * 1024; # 200 MiB
use Avro::DataFile;
use Avro::BinaryDecoder;
use Avro::Schema;
use Carp;
-use Compress::Zstd;
-use IO::Uncompress::Bunzip2 qw(bunzip2);
+use Compress::Zstd::Decompressor;
+use IO::Uncompress::Bunzip2 ();
use IO::Uncompress::RawInflate ;
Review Comment:
`Avro::DataFileReader` now requires `Compress::Zstd::Decompressor` at compile time. The Perl distribution’s dependency list currently only declares `Compress::Zstd` (no version constraint), so environments with an older `Compress::Zstd` that lacks the `Decompressor` submodule will fail to load the reader entirely (even when not using the `zstandard` codec). Consider either (1) updating the Perl dependency spec to require a `Compress::Zstd` version that provides `Compress::Zstd::Decompressor`, or (2) lazy-loading `Compress::Zstd::Decompressor` only when the `zstandard` codec is encountered and throwing `Avro::DataFile::Error::UnsupportedCodec` when it’s unavailable.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567377769
##########
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"
Review Comment:
The error message interpolates `$datafile->{block_size}` inside a double-quoted string, which will stringify `$datafile` (e.g. `HASH(0x...)`) rather than the block size value. This makes the exception message misleading for callers trying to diagnose why the block was rejected.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567451770
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -207,32 +212,157 @@ 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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $block_size exceeds the configured block_max_size of $block_max bytes"
+ );
+ }
Review Comment:
Fixed — object_count and block_size are now validated as >= 0 immediately after decoding, so a negative block_size can't flow into $want / a negative-length read.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -207,32 +212,157 @@ 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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $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.)
Review Comment:
Fixed — reworded to clarify that block_max_size IS used above as a compressed-size pre-read guard, but is not reused here for the decompressed-size cap (to avoid conflating the two units).
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3568125921
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -203,36 +208,190 @@ sub read_block_header {
$datafile->{block_size} = Avro::BinaryDecoder->decode_long(
undef, undef, $fh,
);
+ ## Both are Avro long (zigzag) values, so a malformed/truncated file can
+ ## yield negatives. A negative block_size would flow into $want and a
+ ## negative-length read; a negative object_count is equally nonsensical.
+ ## Reject both before first use.
+ if ($datafile->{object_count} < 0) {
+ croak "Invalid negative object count: $datafile->{object_count}";
+ }
+ if ($datafile->{block_size} < 0) {
+ croak "Invalid negative block size: $datafile->{block_size}";
+ }
$datafile->{block_start} = tell $fh;
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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $block_size exceeds the configured block_max_size of $block_max bytes"
+ );
+ }
+
+ ## we need to read the entire block into memory, to inflate it. Read in
+ ## bounded chunks (rather than a single read of $want bytes, which would
+ ## pre-extend the buffer to the attacker-controlled block_size before a short
+ ## read is detected) and verify the exact byte count afterward: 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 $chunk_size = 64 * 1024;
+ while (bytes::length($block) < $want) {
+ my $need = $want - bytes::length($block);
+ $need = $chunk_size if $need > $chunk_size;
+ my $nread = read $fh, my $buf, $need;
+ if (!defined $nread) {
+ croak "Error reading from file: $!";
+ }
+ last if $nread == 0; # EOF
+ $block .= $buf;
+ }
+ if (bytes::length($block) != $want) {
+ croak "Short read: expected $want bytes for the block, got "
+ . bytes::length($block) . " (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; it is used above
+ ## as a compressed-size pre-read guard, but is not reused here for the
+ ## decompressed-size cap, 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:
The lazy `require` for Compress::Zstd::Decompressor uses `eval { ... }` without localizing `$@`, which can clobber the caller’s `$@` (it will be cleared on success). Localize `$@` and capture the require error in a lexical before building the UnsupportedCodec message.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567427864
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -207,32 +212,157 @@ 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};
+ my $block_size = $datafile->{block_size};
+ if (defined $block_max && $block_size > $block_max) {
+ Avro::DataFile::Error::CompressedBlockSize->throw(
+ "Compressed block size $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.)
Review Comment:
The note in this decompression-limit comment says `block_max_size` “is not reused here”, but `block_max_size` is now reused earlier in the same method as a *compressed-size* pre-read guard (lines 215–225). Updating the wording to clarify that `block_max_size` is not reused for the *decompressed-size cap* (but is used for compressed-size guarding) will prevent reader/writer semantics confusion.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564613410
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -28,14 +28,19 @@ use Object::Tiny qw{
use constant MARKER_SIZE => 16;
-# TODO: refuse to read a block more than block_max_size, instead
-# do partial reads
+# A data-file block is decompressed according to the file's codec. A block with
+# a very high compression ratio (or a malformed block) can expand to far more
+# memory than its compressed size. To guard against unbounded allocation, the
+# decompressed size of a single block is capped. This mirrors the Java SDK's
+# decompression limit (AVRO-4247). The default can be overridden with the
+# AVRO_MAX_DECOMPRESS_LENGTH environment variable.
+use constant DEFAULT_MAX_DECOMPRESS_LENGTH => 200 * 1024 * 1024; # 200 MiB
use Avro::DataFile;
use Avro::BinaryDecoder;
use Avro::Schema;
use Carp;
-use Compress::Zstd;
+use Compress::Zstd::Decompressor;
use IO::Uncompress::Bunzip2 qw(bunzip2);
Review Comment:
`IO::Uncompress::Bunzip2` is now used via the class method (`IO::Uncompress::Bunzip2->new`), but `bunzip2` is still imported and never referenced. This unused import adds noise and may trigger linting/Perl::Critic warnings.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +220,98 @@ 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();
+
Review Comment:
The PR description says the decompression limit uses the existing `block_max_size` attribute when set, but the implementation now always uses `AVRO_MAX_DECOMPRESS_LENGTH` / the default and explicitly does *not* reuse `block_max_size` (see the note here). Please update the PR description to match the current behavior so users aren’t misled about how to configure the limit.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564764695
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,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 };
Review Comment:
`open my $fh, '<', \$uncompressed` isn’t checked for failure; if it fails, `$datafile->{reader}` becomes undef and later reads will fail with less clear errors. Check `open` and surface `$!` via `croak`.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,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();
Review Comment:
The new decompressed-size cap doesn’t prevent an OOM from the *compressed* block read: `read $fh, my $block, $datafile->{block_size} + MARKER_SIZE` can still try to allocate an attacker-controlled `$block_size` before any decompression limit is applied. Consider enforcing a compressed-size bound before reading the block (at least when `block_max_size` is configured on the reader), or switching to partial reads.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,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 };
Review Comment:
`open my $fh, '<', \$uncompressed` isn’t checked for failure; if it fails, `$datafile->{reader}` becomes undef and later reads will fail with less clear errors. Check `open` and surface `$!` via `croak`.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,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 };
Review Comment:
`open my $fh, '<', \$uncompressed` isn’t checked for failure; if it fails, `$datafile->{reader}` becomes undef and later reads will fail with less clear errors. Check `open` and surface `$!` via `croak`.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564797836
##########
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; 1 }
+ or skip 'Compress::Zstd not available', 1;
Review Comment:
Fixed. The SKIP guard now requires Compress::Zstd::Decompressor (the exact module the reader uses) instead of Compress::Zstd, so the test is reliably skipped when that dependency is missing. Pushed in ccab12c.
##########
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; 1 }
+ or skip 'Compress::Zstd 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; 1 }
+ or skip 'Compress::Zstd not available', 1;
Review Comment:
Fixed the same way for the within-limit path: it now checks for Compress::Zstd::Decompressor. Pushed in ccab12c.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564664396
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -28,14 +28,19 @@ use Object::Tiny qw{
use constant MARKER_SIZE => 16;
-# TODO: refuse to read a block more than block_max_size, instead
-# do partial reads
+# A data-file block is decompressed according to the file's codec. A block with
+# a very high compression ratio (or a malformed block) can expand to far more
+# memory than its compressed size. To guard against unbounded allocation, the
+# decompressed size of a single block is capped. This mirrors the Java SDK's
+# decompression limit (AVRO-4247). The default can be overridden with the
+# AVRO_MAX_DECOMPRESS_LENGTH environment variable.
+use constant DEFAULT_MAX_DECOMPRESS_LENGTH => 200 * 1024 * 1024; # 200 MiB
use Avro::DataFile;
use Avro::BinaryDecoder;
use Avro::Schema;
use Carp;
-use Compress::Zstd;
+use Compress::Zstd::Decompressor;
use IO::Uncompress::Bunzip2 qw(bunzip2);
Review Comment:
Fixed. Only the OO `IO::Uncompress::Bunzip2->new` interface is used, so the module is now imported without the `bunzip2` function (`use IO::Uncompress::Bunzip2 ();`). Pushed in 985cabc.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +220,98 @@ 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();
+
Review Comment:
Updated the PR description: the limit is driven by `AVRO_MAX_DECOMPRESS_LENGTH` (default 200 MiB) and is independent of `block_max_size` (which bounds buffered compressed input, not decompressed size). The description now also reflects that deflate, bzip2, and zstandard are all covered.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564748378
##########
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:
Fixed. _inflate_bounded now uses bytes::length() for both the read budget and the limit check, so the cap is enforced on bytes regardless of any UTF-8 flag, matching Avro::DataFileWriter's block-size accounting. Pushed in f3d866e.
##########
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:
Fixed. _zstd_decompress_bounded also uses bytes::length() when enforcing the limit (and for iterating the compressed block). Pushed in f3d866e.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564713910
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +220,98 @@ 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);
+ next unless defined $out;
+ $uncompressed .= $out;
+ _check_decompress_length(length($uncompressed), $limit);
+ }
+ return $uncompressed;
Review Comment:
Addressed. Note this module's Compress::Zstd::Decompressor has no error or flush method: decompress() croaks on a corrupt frame and otherwise emits all output while consuming its input. I changed _zstd_decompress_bounded to fail closed on an undefined return (croak) instead of silently skipping it, and documented the drain-on-consume behavior. Pushed in d924bc2.
##########
lang/perl/t/07_datafile_decompress_limit.t:
##########
@@ -0,0 +1,95 @@
+# 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; 1 }
+ or skip 'Compress::Zstd not available', 1;
+ assert_codec_rejects_oversized('zstandard');
+}
+
+## A block within the limit still decodes correctly.
+{
+ my $payload = "hello world";
+ my $fh = codec_file('deflate', $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], 'deflate block within the limit decodes';
+}
+
+done_testing;
Review Comment:
Fixed. The within-limit decode test is now a helper exercised for bzip2 and zstandard as well as deflate (skipped when the codec modules are unavailable), guarding the bounded streaming paths against regressions. Pushed in d924bc2.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567388063
##########
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"
Review Comment:
Perl does interpolate a hash-element deref like `$datafile->{block_size}` inside a double-quoted string (verified: it prints the value, e.g. `1234`, not `HASH(0x...)`), so the message was already correct. I've extracted a `$block_size` local and used it in both the comparison and the message for clarity.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564681034
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +220,98 @@ 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);
+ next unless defined $out;
+ $uncompressed .= $out;
+ _check_decompress_length(length($uncompressed), $limit);
+ }
+ return $uncompressed;
Review Comment:
`_zstd_decompress_bounded` ignores `undef` returns from `Compress::Zstd::Decompressor->decompress`, which the Compress::Zstd API uses to signal either an error or that more input is needed. As written, a decompression error can be silently skipped, and the function also never calls `flush`, so valid streams that buffer output may be truncated. Handle the `undef` case by checking `$decompressor->error` and, after feeding all input, call `flush` and include its output (and enforce the size limit on it too).
##########
lang/perl/t/07_datafile_decompress_limit.t:
##########
@@ -0,0 +1,95 @@
+# 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; 1 }
+ or skip 'Compress::Zstd not available', 1;
+ assert_codec_rejects_oversized('zstandard');
+}
+
+## A block within the limit still decodes correctly.
+{
+ my $payload = "hello world";
+ my $fh = codec_file('deflate', $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], 'deflate block within the limit decodes';
+}
+
+done_testing;
Review Comment:
The within-limit decode test only covers `deflate`, but this PR introduces new bounded decompression paths for `bzip2` and `zstandard` as well. Adding within-limit decoding assertions for those codecs (skipped when the codec modules are unavailable) would better protect against regressions like missing flush/error handling in the streaming paths.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564777942
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,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();
Review Comment:
Addressed. When the reader is configured with block_max_size, a block whose declared compressed size exceeds it is now rejected before the compressed block is read into memory, guarding against an attacker-controlled block_size allocation. (Without block_max_size configured, the read still relies on the file's actual length; the decompressed-size cap remains the guard against expansion.) Added a test. Pushed in cbba52a.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,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 };
Review Comment:
Fixed. The three in-memory open sites now go through _open_decompressed(), which croaks with $! on failure instead of leaving reader undef. Pushed in cbba52a.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,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 };
Review Comment:
Fixed via the shared _open_decompressed() helper, which checks open and croaks with $!. Pushed in cbba52a.
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -215,24 +221,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 };
Review Comment:
Fixed via the shared _open_decompressed() helper, which checks open and croaks with $!. Pushed in cbba52a.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3567354834
##########
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:
Fixed — the 'not available' error now appends `$@` so a require failure for another reason (missing shared lib, compile error) is diagnosable.
##########
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:
Fixed — the DecompressionSize message now includes the actual `$length` that exceeded the limit.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564494822
##########
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:
Fixed in aa239b8c80 — the reader no longer reuses block_max_size (a writer-side compressed-block flush threshold) for the decompressed-size cap; the limit is the AVRO_MAX_DECOMPRESS_LENGTH env var / DEFAULT_MAX_DECOMPRESS_LENGTH.
##########
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:
Fixed in aa239b8c80 — zstandard now decompresses via Compress::Zstd::Decompressor in chunks, rejecting an over-large block before its full form is materialized.
##########
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:
Fixed in aa239b8c80 — _inflate_bounded sizes each read to the remaining budget (capped at 64 KiB), so the buffer overshoots the limit by at most one byte, and the croak now includes the decompressor's $z->error.
##########
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:
Fixed in aa239b8c80 — the DecompressionSize error class now uses `use parent 'Error::Simple'`, consistent with the other error classes.
##########
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:
Fixed in aa239b8c80 — added over-limit tests for bzip2 and zstandard (skipped when the codec module is unavailable).
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4288: [perl] Enforce a maximum decompressed block size [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3855:
URL: https://github.com/apache/avro/pull/3855#discussion_r3564818348
##########
lang/perl/lib/Avro/DataFileReader.pm:
##########
@@ -28,17 +28,23 @@ use Object::Tiny qw{
use constant MARKER_SIZE => 16;
-# TODO: refuse to read a block more than block_max_size, instead
-# do partial reads
+# A data-file block is decompressed according to the file's codec. A block with
+# a very high compression ratio (or a malformed block) can expand to far more
+# memory than its compressed size. To guard against unbounded allocation, the
+# decompressed size of a single block is capped. This mirrors the Java SDK's
+# decompression limit (AVRO-4247). The default can be overridden with the
+# AVRO_MAX_DECOMPRESS_LENGTH environment variable.
+use constant DEFAULT_MAX_DECOMPRESS_LENGTH => 200 * 1024 * 1024; # 200 MiB
use Avro::DataFile;
use Avro::BinaryDecoder;
use Avro::Schema;
use Carp;
-use Compress::Zstd;
-use IO::Uncompress::Bunzip2 qw(bunzip2);
+use Compress::Zstd::Decompressor;
+use IO::Uncompress::Bunzip2 ();
use IO::Uncompress::RawInflate ;
Review Comment:
Fixed via option (2): Compress::Zstd::Decompressor is now loaded lazily inside the zstandard decompression path rather than with a compile-time `use`. The reader loads and works for the other codecs even when the Decompressor submodule is unavailable, and reading a zstandard block in that case throws Avro::DataFile::Error::UnsupportedCodec with a clear message. Pushed in 5b1ba03.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org