AVRO-4288: [perl] Enforce a maximum decompressed block size#3855
AVRO-4288: [perl] Enforce a maximum decompressed block size#3855iemejia wants to merge 10 commits into
Conversation
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. Enforce a configurable maximum decompressed size across the deflate, bzip2 and zstandard codecs, mirroring the Java SDK's decompression limit (AVRO-4247): deflate and bzip2 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 defaults to 200 MiB and can be overridden with the AVRO_MAX_DECOMPRESS_LENGTH environment variable; exceeding it throws Avro::DataFile::Error::DecompressionSize. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
This PR hardens the Perl Avro DataFileReader against decompression bombs by enforcing a configurable maximum decompressed block size during block decoding, using either the existing block_max_size attribute or the AVRO_MAX_DECOMPRESS_LENGTH environment variable (default 200 MiB).
Changes:
- Add bounded, chunked decompression for
deflateandbzip2blocks, with a newAvro::DataFile::Error::DecompressionSizeexception on limit exceed. - Enforce a decompressed-size cap for
zstandardblocks (currently checked afterdecompress). - Add a new test file validating over-limit rejection (deflate) and within-limit decoding.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lang/perl/lib/Avro/DataFileReader.pm | Adds decompression-size limit enforcement and a new DecompressionSize error while reading compressed blocks. |
| lang/perl/t/07_datafile_decompress_limit.t | Introduces tests for decompression limit behavior (currently focused on deflate). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## 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; | ||
|
|
There was a problem hiding this comment.
Fixed in aa239b8 — 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.
| 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 }; |
There was a problem hiding this comment.
Fixed in aa239b8 — zstandard now decompresses via Compress::Zstd::Decompressor in chunks, rejecting an over-large block before its full form is materialized.
| while (($status = $z->read($chunk, 65536)) > 0) { | ||
| $uncompressed .= $chunk; | ||
| _check_decompress_length(length($uncompressed), $limit); | ||
| } | ||
| if (!defined $status || $status < 0) { | ||
| croak "Error decompressing block"; | ||
| } |
There was a problem hiding this comment.
Fixed in aa239b8 — _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.
| package Avro::DataFile::Error::DecompressionSize; | ||
| use parent -norequire, 'Error::Simple'; |
There was a problem hiding this comment.
Fixed in aa239b8 — the DecompressionSize error class now uses use parent 'Error::Simple', consistent with the other error classes.
| ## 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; |
There was a problem hiding this comment.
Fixed in aa239b8 — added over-limit tests for bzip2 and zstandard (skipped when the codec module is unavailable).
…; tests - Stop reusing block_max_size (a writer-side compressed-block flush threshold) for the reader's decompressed-size cap; the limit is now the AVRO_MAX_DECOMPRESS_LENGTH environment variable / DEFAULT_MAX_DECOMPRESS_LENGTH. - Decompress zstandard via Compress::Zstd::Decompressor in chunks so an over-large block is rejected before its full form is materialized. - Tighten _inflate_bounded: size each read to the remaining budget (capped at 64 KiB) so the buffer overshoots the limit by at most one byte, and include the decompressor error in the croak message. - Make the DecompressionSize error class consistent (use parent 'Error::Simple'). - Add over-limit tests for bzip2 and zstandard (skipped when unavailable). Assisted-by: GitHub Copilot:claude-opus-4.8
| use Carp; | ||
| use Compress::Zstd; | ||
| use Compress::Zstd::Decompressor; | ||
| use IO::Uncompress::Bunzip2 qw(bunzip2); |
There was a problem hiding this 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.
| ## 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(); | ||
|
|
There was a problem hiding this 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.
Only the OO IO::Uncompress::Bunzip2->new interface is used; import the module without the bunzip2 function to avoid an unused import. Assisted-by: GitHub Copilot:claude-opus-4.8
| 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; |
There was a problem hiding this 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.
| ## 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; |
There was a problem hiding this 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.
…zstd round-trips - _zstd_decompress_bounded now treats an undefined decompress() return as a failure and croaks, instead of silently skipping it, so a malformed block cannot masquerade as a short, within-limit result. (The streaming decompressor emits all output while consuming its input and has no separate flush step, noted in a comment.) - 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. Assisted-by: GitHub Copilot:claude-opus-4.8
| 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); |
There was a problem hiding this 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.
| $uncompressed .= $out; | ||
| _check_decompress_length(length($uncompressed), $limit); |
There was a problem hiding this comment.
Fixed. _zstd_decompress_bounded also uses bytes::length() when enforcing the limit (and for iterating the compressed block). Pushed in f3d866e.
_inflate_bounded and _zstd_decompress_bounded counted the running output with length(), which returns character length. If the scalar were ever UTF-8 upgraded, that could undercount bytes and let a block exceed the configured byte cap. Use bytes::length() for byte-accurate accounting, matching how Avro::DataFileWriter tracks block sizes. Assisted-by: GitHub Copilot:claude-opus-4.8
| ## 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(); |
There was a problem hiding this 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.
| 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 }; |
There was a problem hiding this 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.
| 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 }; |
There was a problem hiding this comment.
Fixed via the shared _open_decompressed() helper, which checks open and croaks with $!. Pushed in cbba52a.
| elsif ($codec eq 'zstandard') { | ||
| do { open $fh, '<', \(decompress(\$block)); $fh }; | ||
| my $uncompressed = _zstd_decompress_bounded(\$block, $limit); | ||
| do { open my $fh, '<', \$uncompressed; $fh }; |
There was a problem hiding this comment.
Fixed via the shared _open_decompressed() helper, which checks open and croaks with $!. Pushed in cbba52a.
- Guard against an attacker-controlled block_size causing a huge allocation for the compressed block itself: when the reader is configured with block_max_size, reject a block whose declared compressed size exceeds it before reading the block into memory. Added a test. - The in-memory read handles over the decompressed block were opened without checking for failure, leaving reader undef and surfacing unclear errors later. Factor the three open sites into _open_decompressed(), which croaks with $! on failure. Assisted-by: GitHub Copilot:claude-opus-4.8
| eval { require Compress::Zstd; 1 } | ||
| or skip 'Compress::Zstd not available', 1; |
There was a problem hiding this 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.
| eval { require Compress::Zstd; 1 } | ||
| or skip 'Compress::Zstd not available', 1; |
There was a problem hiding this comment.
Fixed the same way for the within-limit path: it now checks for Compress::Zstd::Decompressor. Pushed in ccab12c.
The zstandard SKIP guards checked for Compress::Zstd, but the reader depends on Compress::Zstd::Decompressor. Require that submodule in the SKIP conditions so the tests are reliably skipped when the reader-side dependency is missing rather than attempting to run and failing later. Assisted-by: GitHub Copilot:claude-opus-4.8
| use Compress::Zstd::Decompressor; | ||
| use IO::Uncompress::Bunzip2 (); | ||
| use IO::Uncompress::RawInflate ; |
There was a problem hiding this 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.
The reader hard-required Compress::Zstd::Decompressor at compile time, so an environment with an older Compress::Zstd lacking the Decompressor submodule would fail to load the reader entirely, even when only other codecs are used. Load Compress::Zstd::Decompressor lazily in the zstandard path and throw Avro::DataFile::Error::UnsupportedCodec with a clear message when it is unavailable, so the reader keeps working for the other codecs. Assisted-by: GitHub Copilot:claude-opus-4.8
| $uncompressed .= $out; | ||
| _check_decompress_length(bytes::length($uncompressed), $limit); | ||
| } |
There was a problem hiding this 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.
| 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" | ||
| ); |
There was a problem hiding this 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.
| { | ||
| 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'; | ||
| } |
There was a problem hiding this 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.
…size error - _zstd_decompress_bounded now checks the prospective total length before concatenating each decompressed chunk into the buffer, so a single large chunk cannot transiently balloon memory past the limit (or double peak memory via reallocation). - The block_max_size pre-read guard now throws a distinct Avro::DataFile::Error::CompressedBlockSize (compressed-size condition) instead of DecompressionSize (decompressed-size cap), so callers can tell the two apart. - The block_max_size test sets a large AVRO_MAX_DECOMPRESS_LENGTH so the rejection can only come from the compressed-size guard, and asserts the distinct class and message. Assisted-by: GitHub Copilot:claude-opus-4.8
| # 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" | ||
| ); | ||
| } |
There was a problem hiding this 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.
| ## 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: $!"; |
There was a problem hiding this 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.
- The reader needs Compress::Zstd::Decompressor (the streaming interface first shipped in Compress::Zstd 0.10), so bump the prerequisite to require >= 0.10 and skip the zstandard case in t/04_datafile.t when the Decompressor submodule is unavailable, keeping the suite portable. - The block read used 'or croak', which treats a short read / EOF (with $! often unset) as a generic error and lets a partial read slip through. Verify the exact expected byte count and croak with a clear 'short read' message on a truncated/malformed file. Assisted-by: GitHub Copilot:claude-opus-4.8
What is the purpose of the change
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_LENGTHenvironment variable.The deflate and bzip2 codecs are inflated in chunks and bounded before the full output is materialized, so an over-limit block is rejected without first allocating its entire decompressed size. The zstandard codec is likewise bounded. Exceeding the limit throws
Avro::DataFile::Error::DecompressionSize.The limit is independent of the reader's
block_max_sizeattribute, which controls how much compressed input is buffered per block, not how large a block may decompress to.This is part of the umbrella issue AVRO-4283.
Verifying this change
This change added tests and can be verified as follows:
t/07_datafile_decompress_limit.tcovering over-limit rejection for deflate, bzip2, and zstandard, plus within-limit decoding.cd lang/perl && prove -Ilib t/Documentation
AVRO_MAX_DECOMPRESS_LENGTHenvironment variable is documented in code comments)