AVRO-4286: [csharp] Enforce a maximum decompressed block size#3857
AVRO-4286: [csharp] Enforce a maximum decompressed block size#3857iemejia 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, 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
| /// Mirrors the Java SDK's decompression limit (AVRO-4247). Overridable with | ||
| /// the AVRO_MAX_DECOMPRESS_LENGTH environment variable. | ||
| /// </summary> | ||
| public const long DefaultMaxDecompressLength = 200L * 1024 * 1024; // 200 MiB |
There was a problem hiding this comment.
static readonly might be better than const here, so that the compiler won't copy the value to other assemblies that reference this one, and changing the default in the future won't require recompiling those assemblies.
There was a problem hiding this comment.
Good point — changed DefaultMaxDecompressLength to static readonly in 51bf9f8 so the value isn't inlined into referencing assemblies and a future default change won't require recompiling them.
| /// Name of the environment variable used to override the default maximum | ||
| /// decompressed size of a single block. | ||
| /// </summary> | ||
| public const string MaxDecompressLengthEnvVar = "AVRO_MAX_DECOMPRESS_LENGTH"; |
There was a problem hiding this comment.
This one, on the other hand, is less likely to be changed in the future, so const seems OK.
There was a problem hiding this comment.
Agreed — left MaxDecompressLengthEnvVar as const; it's a stable string identifier that referencing assemblies inlining is fine for.
| public static long GetMaxDecompressLength() | ||
| { | ||
| var value = Environment.GetEnvironmentVariable(MaxDecompressLengthEnvVar); | ||
| if (value != null && long.TryParse(value, out var parsed) && parsed > 0) |
There was a problem hiding this comment.
Could specify CultureInfo.InvariantCulture. I'm not sure it has any important effect here though, because the values are expected to be positive and thus NumberFormatInfo.NegativeSign shouldn't matter.
There was a problem hiding this comment.
Done in 51bf9f8 — the override is now parsed with NumberStyles.Integer, CultureInfo.InvariantCulture so it's culture-independent.
| throw new AvroRuntimeException( | ||
| $"Decompressed block size {length} exceeds the maximum allowed of {maxLength} bytes. " + | ||
| $"Set the {MaxDecompressLengthEnvVar} environment variable to raise the limit."); |
There was a problem hiding this comment.
When called from CopyBounded, length might not be the total length of the uncompressed data, so the user doesn't know what value in the environment variable would prevent the exception. But this seems the best that can be done here because DeflateStream.Length (and GZipStream.Length) would throw NotSupportedException and just reading to the end while discarding the data would still allow denial of service.
There was a problem hiding this comment.
Right. When called from CopyBounded the streaming codecs (deflate/gzip) can't know the total uncompressed length up front — DeflateStream.Length/GZipStream.Length throw NotSupportedException, and reading to the end to measure it would itself allow the DoS we're preventing. So the incremental cap (rejecting as soon as the running total exceeds the limit) is the best available behavior; the message reports the limit and the env var to raise it. Left as-is.
…t parse - Make DefaultMaxDecompressLength `static readonly` instead of `const` so changing the default later does not require recompiling assemblies that reference this one (and the value is not inlined into them). - Parse the AVRO_MAX_DECOMPRESS_LENGTH override with CultureInfo.InvariantCulture so the environment variable is interpreted consistently regardless of the current culture. Assisted-by: GitHub Copilot:claude-opus-4.8
| while ((read = source.Read(buffer, 0, buffer.Length)) > 0) | ||
| { | ||
| total += read; | ||
| CheckDecompressLength(total, maxLength); | ||
| destination.Write(buffer, 0, read); | ||
| } |
There was a problem hiding this comment.
Fixed — CopyBounded now checks read > maxLength - total before adding, so total can't overflow and wrap past the limit. total is always <= maxLength on entry and read > 0, so the subtraction is safe.
| throw new AvroRuntimeException( | ||
| $"Decompressed block size {length} exceeds the maximum allowed of {maxLength} bytes. " + | ||
| $"Set the {MaxDecompressLengthEnvVar} environment variable to raise the limit."); | ||
| } |
There was a problem hiding this comment.
Fixed — reworded to 'For data-file reads, the AVRO_MAX_DECOMPRESS_LENGTH environment variable raises the limit.', so it no longer implies the env var is the only way to change the maxLength passed to CopyBounded.
CheckDecompressLength takes an explicit maxLength (also used by CopyBounded with a caller-supplied max), but the message always told users to set the env var. Reword so it states the env var raises the limit for data-file reads, rather than implying it is the only knob. Assisted-by: GitHub Copilot:claude-opus-4.8
| MemoryStream outStream = new MemoryStream(); | ||
| using (var writer = DataFileWriter<GenericRecord>.OpenWriter( | ||
| new GenericWriter<GenericRecord>(schema), outStream, Codec.CreateCodec(Codec.Type.Null))) | ||
| { |
There was a problem hiding this comment.
MemoryStream.ToArray() is documented to work even after the stream is closed/disposed (it operates on the internal buffer, unlike most stream methods), so outStream.ToArray() after the using block does not throw ObjectDisposedException. The full File test suite passes (623/623) with this test, confirming it. Left as-is.
| throw new AvroRuntimeException( | ||
| $"Decompressed block size exceeds the maximum allowed of {maxLength} bytes. " + | ||
| $"Set the {MaxDecompressLengthEnvVar} environment variable to raise the limit."); | ||
| } |
There was a problem hiding this comment.
Already addressed (commits 968ac0c / 55d0e4f): the message now reads 'exceeds the maximum allowed of {maxLength} bytes (at least {total} bytes already decompressed). For data-file reads, the AVRO_MAX_DECOMPRESS_LENGTH environment variable raises the limit.' — so it includes observed size and is worded for the custom-maxLength case.
…n tests
- CopyBounded's over-limit exception now reports the amount already decompressed
("at least {total} bytes already decompressed"), matching the detail of the
non-streaming CheckDecompressLength message.
- The decompression-limit tests allocated 4 MiB / 2 MiB payloads against a 1 MiB
limit; use a 64 KiB limit with 128 KiB payloads instead, which exercises the
same behavior with far less memory/time.
Assisted-by: GitHub Copilot:claude-opus-4.8
| byte[] result = codec.Decompress(compressed, compressed.Length); | ||
| Assert.AreEqual(payload, result); | ||
| } |
There was a problem hiding this comment.
Fixed — changed to CollectionAssert.AreEqual(payload, result) for the byte-array comparison.
| byte[] result = codec.Decompress(compressed, compressed.Length); | ||
| Assert.AreEqual(payload, result); | ||
| } |
There was a problem hiding this comment.
Already addressed in commit 6bea53e — this line now uses CollectionAssert.AreEqual(payload, result).
…ray check Compare the decompressed byte[] with CollectionAssert.AreEqual, which does an element-wise comparison with clearer failure output than Assert.AreEqual. 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)