AVRO-4286: [csharp] Enforce a maximum decompressed block size#3857
AVRO-4286: [csharp] Enforce a maximum decompressed block size#3857iemejia wants to merge 5 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, mirroring the Java SDK's decompression limit (AVRO-4247): the built-in deflate codec is inflated in chunks and bounded before the full output is materialized, and DataFileReader additionally checks the decompressed size of every codec's output as a safeguard. The limit defaults to 200 MiB and can be overridden with the AVRO_MAX_DECOMPRESS_LENGTH environment variable; exceeding it throws AvroRuntimeException. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
This PR hardens the C# Avro data-file reader against decompression bombs by enforcing a configurable maximum decompressed block size (default 200 MiB), aligned with similar protections in other SDKs.
Changes:
- Add a global maximum decompressed block size (
AVRO_MAX_DECOMPRESS_LENGTH) and bounded stream-copy helper inCodec. - Enforce the limit during deflate decompression and add a post-decompression safeguard in
DataFileReader. - Add NUnit coverage for both exceeding-limit and within-limit deflate blocks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lang/csharp/src/apache/test/File/FileTests.cs | Adds tests validating deflate blocks are rejected/accepted based on the configured decompression limit. |
| lang/csharp/src/apache/main/File/DeflateCodec.cs | Switches deflate decompression to a bounded copy to fail early when output would exceed the max size. |
| lang/csharp/src/apache/main/File/DataFileReader.cs | Adds a post-decompression size check as a safeguard for codecs that return fully materialized buffers. |
| lang/csharp/src/apache/main/File/Codec.cs | Introduces default limit/env override + helpers for checking and bounded copying. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new AvroRuntimeException( | ||
| $"Decompressed block size exceeds the maximum allowed of {maxLength} bytes"); | ||
| } |
There was a problem hiding this comment.
Fixed in 15737e5 — the exception now reports the observed decompressed size and names the AVRO_MAX_DECOMPRESS_LENGTH environment variable to raise the limit.
| public static void CopyBounded(Stream source, Stream destination, long maxLength) | ||
| { | ||
| byte[] buffer = new byte[81920]; | ||
| long total = 0; | ||
| int read; | ||
| while ((read = source.Read(buffer, 0, buffer.Length)) > 0) |
There was a problem hiding this comment.
Fixed in 15737e5 — CopyBounded now validates its arguments (ArgumentNullException for null streams, ArgumentOutOfRangeException for a negative maxLength), with a test.
…Bounded args - The decompression-limit exception now reports the observed decompressed size and names the AVRO_MAX_DECOMPRESS_LENGTH environment variable to raise it. - CopyBounded (public API) validates its arguments, throwing ArgumentNullException for null streams and ArgumentOutOfRangeException for a negative maxLength instead of a NullReferenceException / unexpected behavior. Add a test. Assisted-by: GitHub Copilot:claude-opus-4.8
| _currentBlock.Data = _codec.Decompress(_currentBlock.Data, (int)_blockSize); | ||
| // Guard against a block that decompresses to more than the | ||
| // allowed maximum (a decompression bomb). The built-in deflate | ||
| // codec is already bounded during decompression; this covers | ||
| // any codec that returns a fully decompressed buffer. | ||
| Codec.CheckDecompressLength(_currentBlock.Data.Length, Codec.GetMaxDecompressLength()); |
There was a problem hiding this comment.
Fixed. HasNext() now passes the original exception as the inner exception (and uses e.Message in the wrapper text), so the underlying cause and stack trace are preserved for callers. Pushed in f8c8e1d.
HasNext() wrapped any block-fetch failure in an AvroRuntimeException but only folded the original into the message string, dropping the stack trace and exception chain. Pass the original as the inner exception and use its Message in the text so callers can inspect and log the underlying cause. Assisted-by: GitHub Copilot:claude-opus-4.8
| catch (Exception e) | ||
| { | ||
| throw new AvroRuntimeException(string.Format(CultureInfo.InvariantCulture, | ||
| "Error fetching meta data for key: {0}", key), e); | ||
| "Error fetching next object from block: {0}", e.Message), e); | ||
| } |
There was a problem hiding this comment.
Fixed. My earlier change had accidentally edited GetMetaString's catch. Restored its metadata-specific message ("Error fetching meta data for key: {key}") with the inner exception preserved. Pushed in 7a05ac1.
| _currentBlock.Data = _codec.Decompress(_currentBlock.Data, (int)_blockSize); | ||
| // Guard against a block that decompresses to more than the | ||
| // allowed maximum (a decompression bomb). The built-in deflate | ||
| // codec is already bounded during decompression; this covers | ||
| // any codec that returns a fully decompressed buffer. | ||
| Codec.CheckDecompressLength(_currentBlock.Data.Length, Codec.GetMaxDecompressLength()); |
There was a problem hiding this comment.
Fixed. The inner-exception preservation is now applied to HasNext() (the intended target): it passes the original exception as InnerException and uses e.Message in the wrapper text. Pushed in 7a05ac1.
| // Guard against a block that decompresses to more than the | ||
| // allowed maximum (a decompression bomb). The built-in deflate | ||
| // codec is already bounded during decompression; this covers | ||
| // any codec that returns a fully decompressed buffer. | ||
| Codec.CheckDecompressLength(_currentBlock.Data.Length, Codec.GetMaxDecompressLength()); |
There was a problem hiding this comment.
Fixed. Added TestReaderRejectsOversizedBlock, which writes a Null-codec block larger than the limit and asserts the DataFileReader read path (HasNext -> CheckDecompressLength) rejects it, covering codecs that return a fully materialized buffer. Pushed in 7a05ac1.
…test reader path - The previous change accidentally edited GetMetaString's catch instead of HasNext. Restore GetMetaString's metadata-specific message (with the key) and apply the inner-exception preservation to HasNext, where a block-fetch failure is wrapped: it now passes the original exception as InnerException and uses its Message in the text. - Add TestReaderRejectsOversizedBlock, which writes a Null-codec block larger than the limit and asserts the DataFileReader read path (not just a direct codec call) rejects it, covering CheckDecompressLength for codecs that return a fully materialized buffer. Assisted-by: GitHub Copilot:claude-opus-4.8
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The oversized-block test had an empty foreach body with an unused loop variable, which CodeQL flagged as a useless assignment and an empty loop body. Assert.NotNull(rec) inside the loop reads the variable and gives the body a meaningful statement while still forcing the block to be read and decompressed. Assisted-by: GitHub Copilot:claude-opus-4.8
What is the purpose of the change
The built-in deflate codec is inflated in chunks and bounded before the full output is materialized, and
DataFileReaderadditionally checks the decompressed size of every codec's output as a safeguard. Exceeding the limit throwsAvroRuntimeException.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.This is part of the umbrella issue AVRO-4283.
Verifying this change
This change added tests and can be verified as follows:
test/File/FileTests.cs: a deflate block exceeding the limit is rejected and a within-limit block decodes.cd lang/csharp && dotnet test src/apache/test/Avro.test.csprojDocumentation
AVRO_MAX_DECOMPRESS_LENGTHenvironment variable is documented in code comments)