Copilot commented on code in PR #3857:
URL: https://github.com/apache/avro/pull/3857#discussion_r3567457315
##########
lang/csharp/src/apache/main/File/Codec.cs:
##########
@@ -29,6 +30,97 @@ namespace Avro.File
/// </summary>
public abstract class Codec
{
+ /// <summary>
+ /// Default upper bound, in bytes, on the size a single data-file
block may
+ /// decompress to. A block with a very high compression ratio (or a
malformed
+ /// block) can otherwise expand to far more memory than its compressed
size.
+ /// Mirrors the Java SDK's decompression limit (AVRO-4247).
Overridable with
+ /// the AVRO_MAX_DECOMPRESS_LENGTH environment variable.
+ /// </summary>
+ public static readonly long DefaultMaxDecompressLength = 200L * 1024 *
1024; // 200 MiB
+
+ /// <summary>
+ /// 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";
+
+ /// <summary>
+ /// The maximum number of bytes a single block is allowed to
decompress to.
+ /// </summary>
+ /// <returns>The configured limit, honoring the environment
override.</returns>
+ public static long GetMaxDecompressLength()
+ {
+ var value =
Environment.GetEnvironmentVariable(MaxDecompressLengthEnvVar);
+ if (value != null && long.TryParse(value, NumberStyles.Integer,
CultureInfo.InvariantCulture, out var parsed) && parsed > 0)
+ {
+ return parsed;
+ }
+
+ return DefaultMaxDecompressLength;
+ }
+
+ /// <summary>
+ /// Throws if the given decompressed length exceeds the maximum
allowed.
+ /// </summary>
+ /// <param name="length">The number of decompressed bytes.</param>
+ /// <param name="maxLength">The maximum number of decompressed bytes
allowed.</param>
+ public static void CheckDecompressLength(long length, long maxLength)
+ {
+ if (length > maxLength)
+ {
+ throw new AvroRuntimeException(
+ $"Decompressed block size {length} exceeds the maximum
allowed of {maxLength} bytes. " +
+ $"For data-file reads, the {MaxDecompressLengthEnvVar}
environment variable raises the limit.");
+ }
+ }
+
+ /// <summary>
+ /// Copies a decompression stream to the destination, rejecting the
block as
+ /// soon as its decompressed size would exceed <paramref
name="maxLength"/> so
+ /// an over-large (or malicious) block is not fully materialized in
memory.
+ /// </summary>
+ /// <param name="source">The decompression stream to read from.</param>
+ /// <param name="destination">The stream to write the decompressed
data to.</param>
+ /// <param name="maxLength">The maximum number of decompressed bytes
allowed.</param>
+ public static void CopyBounded(Stream source, Stream destination, long
maxLength)
+ {
+ if (source == null)
+ {
+ throw new ArgumentNullException(nameof(source));
+ }
+
+ if (destination == null)
+ {
+ throw new ArgumentNullException(nameof(destination));
+ }
+
+ if (maxLength < 0)
+ {
+ throw new ArgumentOutOfRangeException(nameof(maxLength),
"maxLength must not be negative.");
+ }
+
+ byte[] buffer = new byte[81920];
+ long total = 0;
+ int read;
+ while ((read = source.Read(buffer, 0, buffer.Length)) > 0)
+ {
+ // Pre-add bound check: total is always <= maxLength here and
+ // read > 0, so maxLength - total >= 0 and this cannot
overflow.
+ // Rejecting before adding stops total from overflowing and
+ // wrapping past the limit for a very large maxLength.
+ if (read > maxLength - total)
+ {
+ throw new AvroRuntimeException(
+ $"Decompressed block size exceeds the maximum allowed
of {maxLength} bytes. " +
+ $"Set the {MaxDecompressLengthEnvVar} environment
variable to raise the limit.");
+ }
Review Comment:
`CopyBounded` throws an `AvroRuntimeException` whose message always points
to `AVRO_MAX_DECOMPRESS_LENGTH`, even though callers may be passing a custom
`maxLength` unrelated to data-file reads. The message also omits any observed
size information (unlike `CheckDecompressLength`). Consider rewording to be
generic and include how far decoding got (e.g., copied bytes + next chunk size)
while still mentioning the env var only as the data-file-read override.
##########
lang/csharp/src/apache/test/File/FileTests.cs:
##########
@@ -425,6 +425,112 @@ public void OpenAppendWriter_IncorrectOutStream_Throws()
Assert.Throws(typeof(AvroRuntimeException), action);
}
+ /// <summary>
+ /// A block with a very high compression ratio can expand to far more
memory
+ /// than its compressed size; decompressing such a block must be
rejected once
+ /// its decompressed size would exceed the configured maximum.
+ /// </summary>
+ [Test]
+ public void TestDeflateDecompressionLimit()
+ {
+ var codec = new DeflateCodec();
+ byte[] big = new byte[4 * 1024 * 1024]; // 4 MiB of zeros,
compresses tiny
+ byte[] compressed = codec.Compress(big);
+
+ var previous =
Environment.GetEnvironmentVariable(Codec.MaxDecompressLengthEnvVar);
+
Environment.SetEnvironmentVariable(Codec.MaxDecompressLengthEnvVar, "1048576");
// 1 MiB
+ try
+ {
+ Assert.Throws<AvroRuntimeException>(
+ () => codec.Decompress(compressed, compressed.Length));
+ }
+ finally
+ {
+
Environment.SetEnvironmentVariable(Codec.MaxDecompressLengthEnvVar, previous);
+ }
+ }
+
+ [Test]
+ public void TestDeflateWithinDecompressionLimit()
+ {
+ var codec = new DeflateCodec();
+ byte[] payload = System.Text.Encoding.UTF8.GetBytes("hello world");
+ byte[] compressed = codec.Compress(payload);
+
+ var previous =
Environment.GetEnvironmentVariable(Codec.MaxDecompressLengthEnvVar);
+
Environment.SetEnvironmentVariable(Codec.MaxDecompressLengthEnvVar, "1048576");
// 1 MiB
+ try
+ {
+ byte[] result = codec.Decompress(compressed,
compressed.Length);
+ Assert.AreEqual(payload, result);
+ }
+ finally
+ {
+
Environment.SetEnvironmentVariable(Codec.MaxDecompressLengthEnvVar, previous);
+ }
+ }
+
+ [Test]
+ public void TestCopyBoundedValidatesArguments()
+ {
+ using (var stream = new MemoryStream())
+ {
+ Assert.Throws<ArgumentNullException>(() =>
Codec.CopyBounded(null, stream, 10));
+ Assert.Throws<ArgumentNullException>(() =>
Codec.CopyBounded(stream, null, 10));
+ Assert.Throws<ArgumentOutOfRangeException>(() =>
Codec.CopyBounded(stream, stream, -1));
+ }
+ }
+
+ /// <summary>
+ /// The DataFileReader itself must reject a block whose decompressed
size
+ /// exceeds the configured maximum. This covers the safeguard applied
to
+ /// every codec that returns a fully materialized buffer (here the Null
+ /// codec, which performs no internally bounded decompression), not
just a
+ /// direct call to a codec's Decompress method.
+ /// </summary>
+ [Test]
+ public void TestReaderRejectsOversizedBlock()
+ {
+ const string schemaStr =
+
"{\"type\":\"record\",\"name\":\"n\",\"fields\":[{\"name\":\"f1\",\"type\":\"string\"}]}";
+ Schema schema = Schema.Parse(schemaStr);
+ var recordSchema = schema as RecordSchema;
+
+ // A single record whose string field is larger than the limit
below.
+ string big = new string('a', 2 * 1024 * 1024); // 2 MiB
+
+ MemoryStream outStream = new MemoryStream();
+ using (var writer = DataFileWriter<GenericRecord>.OpenWriter(
+ new GenericWriter<GenericRecord>(schema), outStream,
Codec.CreateCodec(Codec.Type.Null)))
+ {
Review Comment:
`DataFileWriter<...>.OpenWriter(writer, outStream, codec)` defaults to
`leaveOpen=false`, so disposing `writer` closes/disposes `outStream`. This
makes `outStream.ToArray()` after the `using` block throw
`ObjectDisposedException`, causing this test to fail. Use the `leaveOpen`
overload (or move `ToArray()` before disposing the writer).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
