From 2210a460a373f0fbf0c93cf5775a9bc682f61c53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sat, 11 Jul 2026 22:27:36 +0200 Subject: [PATCH 01/28] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values and collections A bytes or string value is a length prefix followed by that many bytes, and an array or map block is an element count followed by that many items. A malicious or truncated input can declare a huge length or count with little or no data. - BinaryDecoder.RemainingBytes() reports the bytes still readable for a seekable stream (or -1). ReadBytes and both ReadString implementations reject an over-large declared length before allocating. - DefaultReader.ReadArray/ReadMap reject a block whose element count could not be backed by the bytes remaining, using MinBytesPerElement() computed from the element schema so a zero-byte element type (e.g. null) is not falsely rejected. The count is checked on the raw long before the int cast, which also avoids the cast overflowing into a bogus pre-allocation. Mirrors the Java SDK's checks (AVRO-4241). Non-seekable streams and non-binary decoders are unaffected. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/Generic/GenericReader.cs | 79 ++++++++++- .../src/apache/main/IO/BinaryDecoder.cs | 37 +++++ .../main/IO/BinaryDecoder.netstandard2.0.cs | 2 + .../IO/BinaryDecoder.notnetstandard2.0.cs | 2 + .../src/apache/test/IO/BinaryCodecTests.cs | 128 +++++++++++++++++- 5 files changed, 244 insertions(+), 4 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index 0b945b9ff5e..cf7d50c8313 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -404,8 +404,14 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem ArraySchema rs = (ArraySchema)readerSchema; object result = CreateArray(reuse, rs); int i = 0; - for (int n = (int)d.ReadArrayStart(); n != 0; n = (int)d.ReadArrayNext()) + int minBytes = MinBytesPerElement(writerSchema.ItemSchema); + for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext()) { + // Reject a block whose element count could not be backed by the + // bytes remaining before allocating for it (checked on the raw + // long, which also avoids the int cast overflowing). + EnsureCollectionAvailable(d, nl, minBytes); + int n = (int)nl; if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n); for (int j = 0; j < n; j++, i++) { @@ -490,8 +496,12 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re { MapSchema rs = (MapSchema)readerSchema; object result = CreateMap(reuse, rs); - for (int n = (int)d.ReadMapStart(); n != 0; n = (int)d.ReadMapNext()) + // Map keys are strings (>= 1 byte length prefix) plus the value. + int minBytes = 1 + MinBytesPerElement(writerSchema.ValueSchema); + for (long nl = d.ReadMapStart(); nl != 0; nl = d.ReadMapNext()) { + EnsureCollectionAvailable(d, nl, minBytes); + int n = (int)nl; for (int j = 0; j < n; j++) { string k = d.ReadString(); @@ -501,6 +511,71 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re return result; } + /// + /// Minimum number of bytes a single value of the given schema can occupy + /// on the wire. Used to reject an array/map block count that could not be + /// backed by the bytes remaining. A type that can encode to zero bytes + /// (null) returns 0, which disables the collection check for it (so an + /// array of nulls is not falsely rejected). A depth limit breaks + /// self-referencing schemas. + /// + private static int MinBytesPerElement(Schema schema, int depth = 0) + { + if (schema == null || depth > 64) + { + return 0; + } + + switch (schema.Tag) + { + case Schema.Type.Null: + return 0; + case Schema.Type.Float: + return 4; + case Schema.Type.Double: + return 8; + case Schema.Type.Fixed: + return ((FixedSchema)schema).Size; + case Schema.Type.Record: + case Schema.Type.Error: + int total = 0; + foreach (Field f in (RecordSchema)schema) + { + total += MinBytesPerElement(f.Schema, depth + 1); + } + + return total; + default: + // boolean, int, long, bytes, string, enum, union, array, map: + // all encode to at least one byte. + return 1; + } + } + + /// + /// Rejects a collection (array or map) block whose declared element count + /// could not be backed by the bytes actually remaining, before + /// allocating. Skipped when the per-element minimum is zero, or when the + /// decoder cannot report how many bytes remain. + /// + private static void EnsureCollectionAvailable(Decoder d, long count, int minBytesPerElement) + { + if (count <= 0 || minBytesPerElement <= 0) + { + return; + } + + if (d is BinaryDecoder bd) + { + long remaining = bd.RemainingBytes(); + if (remaining >= 0 && count > remaining / minBytesPerElement) + { + throw new AvroException( + $"Collection claims {count} elements with at least {minBytesPerElement} bytes each, but only {remaining} bytes are available"); + } + } + } + /// /// Used by the default implementation of ReadMap() to create a fresh map object. The default /// implementation of this method returns a IDictionary<string, map>. diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index 56aaa6e1815..1bbaf83853e 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -264,11 +264,48 @@ public void SkipFixed(int len) // Read p bytes into a new byte buffer private byte[] read(long p) { + EnsureAvailableBytes(p); byte[] buffer = new byte[p]; Read(buffer, 0, buffer.Length); return buffer; } + /// + /// When the underlying stream can report its length, verifies that at + /// least bytes remain before the caller + /// allocates a buffer of that size. This guards against an + /// out-of-memory attack from a malicious or truncated input that + /// declares a huge length prefix but carries little actual data. The + /// check is skipped for non-seekable streams, whose remaining length is + /// unknown. + /// + /// Number of bytes about to be read. + internal void EnsureAvailableBytes(long length) + { + if (length > 0) + { + long remaining = RemainingBytes(); + if (remaining >= 0 && length > remaining) + { + throw new AvroException( + $"Cannot read {length} bytes, only {remaining} bytes remaining in the stream"); + } + } + } + + /// + /// Returns the number of bytes still available to read from the + /// underlying stream when it is seekable, or -1 when that count is not + /// known (a non-seekable stream). Used to reject a declared length or a + /// collection block count that exceeds the data actually available + /// before allocating for it. + /// + /// The number of bytes remaining, or -1 if unknown. + public long RemainingBytes() + { + return stream.CanSeek ? stream.Length - stream.Position : -1; + } + private byte read() { int n = stream.ReadByte(); diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs index a37d6fa6c84..90ac362b13a 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs @@ -85,6 +85,8 @@ public string ReadString() throw new AvroException("Can not deserialize a string with negative length!"); } + EnsureAvailableBytes(length); + if (length > MaxDotNetArrayLength) { throw new AvroException("String length is not supported!"); diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs index c4a0dfaaf31..22cea838647 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs @@ -73,6 +73,8 @@ public string ReadString() throw new AvroException("Can not deserialize a string with negative length!"); } + EnsureAvailableBytes(length); + if (length <= MaxFastReadLength) { byte[] bufferArray = null; diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index a638b73fea2..8fb96aa6937 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -22,6 +22,7 @@ using System.Linq; using System.Text; using Avro.IO; +using Avro.Generic; namespace Avro.Test { @@ -278,10 +279,12 @@ public void TestInvalidInputWithMaxIntAsStringLength() iostr.Position = 0; Decoder d = new BinaryDecoder(iostr); + // The declared length far exceeds the bytes remaining in the + // (seekable) stream, so it is rejected before any allocation. var exception = Assert.Throws(() => d.ReadString()); Assert.NotNull(exception); - Assert.AreEqual("String length is not supported!", exception.Message); + StringAssert.Contains("bytes remaining in the stream", exception.Message); iostr.Close(); } } @@ -306,10 +309,12 @@ public void TestInvalidInputWithMaxArrayLengthAsStringLength() iostr.Position = 0; Decoder d = new BinaryDecoder(iostr); + // The declared length far exceeds the bytes remaining in the + // (seekable) stream, so it is rejected before any allocation. var exception = Assert.Throws(() => d.ReadString()); Assert.NotNull(exception); - Assert.AreEqual("Could not read as many bytes from stream as expected!", exception.Message); + StringAssert.Contains("bytes remaining in the stream", exception.Message); iostr.Close(); } } @@ -430,5 +435,124 @@ public void TestFixed(int size) TestSkip(b, (Decoder d) => d.SkipFixed(size), (Encoder e, byte[] t) => e.WriteFixed(t), size); } + + // A bytes/string value is a length prefix followed by that many bytes. + // A malicious or truncated input can declare a huge length with little + // actual data; on a seekable stream the reader must reject it before + // allocating, rather than attempting a huge allocation. + [Test] + public void TestReadBytesRejectsLengthBeyondStream() + { + MemoryStream ms = new MemoryStream(); + Encoder e = new BinaryEncoder(ms); + e.WriteLong(1_000_000); // declares 1,000,000 bytes... + ms.Position = 0; // ...but no data follows + Decoder d = new BinaryDecoder(ms); + Assert.Throws(() => d.ReadBytes()); + } + + [Test] + public void TestReadStringRejectsLengthBeyondStream() + { + MemoryStream ms = new MemoryStream(); + Encoder e = new BinaryEncoder(ms); + e.WriteLong(1_000_000); // declares 1,000,000 bytes... + ms.Position = 0; // ...but no data follows + Decoder d = new BinaryDecoder(ms); + Assert.Throws(() => d.ReadString()); + } + + // A well-formed value whose declared length fits the stream still reads. + [Test] + public void TestReadBytesWithinStreamStillReads() + { + byte[] payload = Encoding.UTF8.GetBytes("hello"); + MemoryStream ms = new MemoryStream(); + Encoder e = new BinaryEncoder(ms); + e.WriteBytes(payload); + ms.Position = 0; + Decoder d = new BinaryDecoder(ms); + Assert.AreEqual(payload, d.ReadBytes()); + } + + // On a non-seekable stream the remaining length is unknown, so the + // pre-check is skipped and a valid value still decodes. + [Test] + public void TestReadBytesNonSeekableStreamStillReads() + { + byte[] payload = Encoding.UTF8.GetBytes("hello"); + MemoryStream backing = new MemoryStream(); + Encoder e = new BinaryEncoder(backing); + e.WriteBytes(payload); + byte[] encoded = backing.ToArray(); + + using (var ns = new NonSeekableStream(new MemoryStream(encoded))) + { + Decoder d = new BinaryDecoder(ns); + Assert.AreEqual(payload, d.ReadBytes()); + } + } + + // An array/map block declares an element count; a malicious or truncated + // input can declare far more elements than the remaining bytes could + // hold. The count is validated against the bytes remaining before + // allocating, using the minimum on-wire size of the element schema (so + // 0-byte elements like null are not falsely rejected). + [Test] + public void TestReadArrayRejectsCountBeyondStream() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"long\"}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(1000000); // 1,000,000 longs, no data + ms.Position = 0; + var reader = new GenericReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + + [Test] + public void TestReadMapRejectsCountBeyondStream() + { + var schema = Avro.Schema.Parse("{\"type\":\"map\",\"values\":\"long\"}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(1000000); + ms.Position = 0; + var reader = new GenericReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + + [Test] + public void TestReadArrayOfNullNotFalselyRejected() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); + var ms = new MemoryStream(); + var enc = new BinaryEncoder(ms); + enc.WriteLong(100000); // one block of 100,000 nulls (zero bytes each) + enc.WriteLong(0); // end-of-array marker + ms.Position = 0; + var reader = new GenericReader(schema, schema); + var result = (Array)reader.Read(null, new BinaryDecoder(ms)); + Assert.AreEqual(100000, result.Length); + } + + // Minimal read-only, forward-only stream wrapper reporting CanSeek=false. + private sealed class NonSeekableStream : Stream + { + private readonly Stream inner; + public NonSeekableStream(Stream inner) { this.inner = inner; } + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + public override void Flush() { } + public override int Read(byte[] buffer, int offset, int count) => inner.Read(buffer, offset, count); + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } } } From 04308d0338897f54e1743986aa0f82be7f1d2ec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 00:21:09 +0200 Subject: [PATCH 02/28] AVRO-4295: [csharp] Reject negative lengths and out-of-range counts; harden min-bytes sum Review feedback: - read(long) now rejects a negative length explicitly instead of letting it flow into a negative array allocation. - EnsureCollectionAvailable rejects a block count above int.MaxValue before the callers cast it to int, independently of the per-element size (so it also covers null-element arrays where the byte check is skipped). - MinBytesPerElement accumulates record field minima in a long and clamps to int.MaxValue, so deep nesting cannot overflow int into a value <= 0 that would disable the check; the depth guard now returns 1 (not 0) for the same reason. - The NonSeekableStream test helper disposes its wrapped inner stream. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/Generic/GenericReader.cs | 38 +++++++++++++++++-- .../src/apache/main/IO/BinaryDecoder.cs | 5 +++ .../src/apache/test/IO/BinaryCodecTests.cs | 36 ++++++++++++++++++ 3 files changed, 75 insertions(+), 4 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index cf7d50c8313..5a08c8d1e6e 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -521,11 +521,19 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re /// private static int MinBytesPerElement(Schema schema, int depth = 0) { - if (schema == null || depth > 64) + if (schema == null) { return 0; } + if (depth > 64) + { + // A cyclic or pathologically deep schema. Return 1 (not 0) so the + // collection check stays enabled rather than being silently + // bypassed; a valid recursive value always encodes to >= 1 byte. + return 1; + } + switch (schema.Tag) { case Schema.Type.Null: @@ -538,13 +546,20 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) return ((FixedSchema)schema).Size; case Schema.Type.Record: case Schema.Type.Error: - int total = 0; + // Accumulate in a long and clamp so a deeply nested schema + // cannot overflow int into a value <= 0, which would disable + // the collection check. + long total = 0; foreach (Field f in (RecordSchema)schema) { total += MinBytesPerElement(f.Schema, depth + 1); + if (total >= int.MaxValue) + { + return int.MaxValue; + } } - return total; + return (int)total; default: // boolean, int, long, bytes, string, enum, union, array, map: // all encode to at least one byte. @@ -560,7 +575,22 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) /// private static void EnsureCollectionAvailable(Decoder d, long count, int minBytesPerElement) { - if (count <= 0 || minBytesPerElement <= 0) + if (count <= 0) + { + return; + } + + // A .NET collection cannot hold more than int.MaxValue elements and + // the callers cast the block count to int; reject an out-of-range + // count independently of the per-element size (which is 0 for e.g. + // arrays of null, where the byte check below is skipped). + if (count > int.MaxValue) + { + throw new AvroException( + $"Collection block count {count} exceeds the maximum supported size"); + } + + if (minBytesPerElement <= 0) { return; } diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index 1bbaf83853e..978d3bbffc0 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -264,6 +264,11 @@ public void SkipFixed(int len) // Read p bytes into a new byte buffer private byte[] read(long p) { + if (p < 0) + { + throw new AvroException($"Can not read a negative number of bytes: {p}"); + } + EnsureAvailableBytes(p); byte[] buffer = new byte[p]; Read(buffer, 0, buffer.Length); diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 8fb96aa6937..48b3de7990d 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -534,6 +534,32 @@ public void TestReadArrayOfNullNotFalselyRejected() Assert.AreEqual(100000, result.Length); } + // A negative bytes length must be rejected explicitly rather than + // flowing into a negative array allocation. + [Test] + public void TestReadBytesRejectsNegativeLength() + { + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(-5); + ms.Position = 0; + var d = new BinaryDecoder(ms); + Assert.Throws(() => d.ReadBytes()); + } + + // A block count larger than int.MaxValue must be rejected before the + // int cast, even for a null-element array where the byte check is + // skipped. + [Test] + public void TestReadArrayRejectsCountAboveIntMax() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong((long)int.MaxValue + 1); + ms.Position = 0; + var reader = new GenericReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + // Minimal read-only, forward-only stream wrapper reporting CanSeek=false. private sealed class NonSeekableStream : Stream { @@ -553,6 +579,16 @@ public override void Flush() { } public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); public override void SetLength(long value) => throw new NotSupportedException(); public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + + protected override void Dispose(bool disposing) + { + if (disposing) + { + inner.Dispose(); + } + + base.Dispose(disposing); + } } } } From 7376905bbb6c2b6e7e3958299aaa07b41a037e98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 01:00:12 +0200 Subject: [PATCH 03/28] AVRO-4295: [csharp] Use long for collection min-bytes to avoid int overflow Review feedback: the map's minBytes was computed as 1 + MinBytesPerElement(...). Since MinBytesPerElement clamps to int.MaxValue, adding 1 in int arithmetic overflows to a negative value, which makes minBytesPerElement <= 0 and silently disables the remaining-bytes validation for maps. Compute both the array and map minima as long (1L + ...) and take a long in EnsureCollectionAvailable. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/csharp/src/apache/main/Generic/GenericReader.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index 5a08c8d1e6e..b89c0422582 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -404,7 +404,7 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem ArraySchema rs = (ArraySchema)readerSchema; object result = CreateArray(reuse, rs); int i = 0; - int minBytes = MinBytesPerElement(writerSchema.ItemSchema); + long minBytes = MinBytesPerElement(writerSchema.ItemSchema); for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext()) { // Reject a block whose element count could not be backed by the @@ -497,7 +497,7 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re MapSchema rs = (MapSchema)readerSchema; object result = CreateMap(reuse, rs); // Map keys are strings (>= 1 byte length prefix) plus the value. - int minBytes = 1 + MinBytesPerElement(writerSchema.ValueSchema); + long minBytes = 1L + MinBytesPerElement(writerSchema.ValueSchema); for (long nl = d.ReadMapStart(); nl != 0; nl = d.ReadMapNext()) { EnsureCollectionAvailable(d, nl, minBytes); @@ -573,7 +573,7 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) /// allocating. Skipped when the per-element minimum is zero, or when the /// decoder cannot report how many bytes remain. /// - private static void EnsureCollectionAvailable(Decoder d, long count, int minBytesPerElement) + private static void EnsureCollectionAvailable(Decoder d, long count, long minBytesPerElement) { if (count <= 0) { From 4232c99984568e7cca8ba5c539dc174d9092ce97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 01:23:46 +0200 Subject: [PATCH 04/28] AVRO-4295: [csharp] Reject bytes length above the max .NET array length Review feedback: ReadBytes() -> read(long) allocates new byte[p]. A seekable stream can declare (and hold) more than the maximum .NET array length, so new byte[p] would throw an OverflowException/OutOfMemoryException instead of a consistent AvroException. Reject a length above MaxDotNetArrayLength first, as ReadString() already does. Added a test using a non-seekable stream. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/csharp/src/apache/main/IO/BinaryDecoder.cs | 8 ++++++++ .../src/apache/test/IO/BinaryCodecTests.cs | 17 +++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index 978d3bbffc0..11cd4080bb7 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -269,6 +269,14 @@ private byte[] read(long p) throw new AvroException($"Can not read a negative number of bytes: {p}"); } + if (p > MaxDotNetArrayLength) + { + // A .NET array cannot be larger than this; reject with a + // consistent AvroException rather than letting new byte[p] throw + // an OverflowException/OutOfMemoryException. Matches ReadString(). + throw new AvroException($"Length {p} exceeds the maximum supported array length"); + } + EnsureAvailableBytes(p); byte[] buffer = new byte[p]; Read(buffer, 0, buffer.Length); diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 48b3de7990d..92c1f951ed3 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -560,6 +560,23 @@ public void TestReadArrayRejectsCountAboveIntMax() Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); } + // A bytes length above the maximum .NET array length must be rejected + // with a consistent AvroException rather than letting new byte[p] throw. + // A non-seekable stream is used so the length reaches the array-length + // check without needing gigabytes of backing data. + [Test] + public void TestReadBytesRejectsLengthAboveMaxArrayLength() + { + var backing = new MemoryStream(); + new BinaryEncoder(backing).WriteLong(3_000_000_000L); // > any MaxDotNetArrayLength + byte[] encoded = backing.ToArray(); + using (var ns = new NonSeekableStream(new MemoryStream(encoded))) + { + var d = new BinaryDecoder(ns); + Assert.Throws(() => d.ReadBytes()); + } + } + // Minimal read-only, forward-only stream wrapper reporting CanSeek=false. private sealed class NonSeekableStream : Stream { From 601f0ab0aee98c91f7ef5650bcef149cee89597a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 07:16:35 +0200 Subject: [PATCH 05/28] AVRO-4295: [csharp] Clamp RemainingBytes; reject negative counts; deep null returns 0 Review feedback: - RemainingBytes() clamps a negative result (Position past Length on a truncated or externally seeked stream) to 0, so callers only ever see -1 (unknown) or a non-negative count and the check is not skipped. - EnsureCollectionAvailable rejects a negative block count explicitly (it can arise from long.MinValue overflow when negating a negative count) instead of returning early and letting the caller cast it to int. - MinBytesPerElement applies the depth guard only in the record case, so a zero-byte leaf type (null) nested under deep records still returns 0. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/Generic/GenericReader.cs | 28 +++++++++++++------ .../src/apache/main/IO/BinaryDecoder.cs | 11 +++++++- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index b89c0422582..95ce3dbcf80 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -526,14 +526,6 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) return 0; } - if (depth > 64) - { - // A cyclic or pathologically deep schema. Return 1 (not 0) so the - // collection check stays enabled rather than being silently - // bypassed; a valid recursive value always encodes to >= 1 byte. - return 1; - } - switch (schema.Tag) { case Schema.Type.Null: @@ -546,6 +538,16 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) return ((FixedSchema)schema).Size; case Schema.Type.Record: case Schema.Type.Error: + if (depth > 64) + { + // A cyclic or pathologically deep record. Return 1 (not + // 0) so the collection check stays enabled; a valid + // recursive value always encodes to >= 1 byte. The depth + // guard is applied only here, so zero-byte leaf types + // such as null still return 0 regardless of depth. + return 1; + } + // Accumulate in a long and clamp so a deeply nested schema // cannot overflow int into a value <= 0, which would disable // the collection check. @@ -575,11 +577,19 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) /// private static void EnsureCollectionAvailable(Decoder d, long count, long minBytesPerElement) { - if (count <= 0) + if (count == 0) { return; } + // A negative count is corrupt/malicious data (it can also arise from + // long.MinValue overflow when negating a negative block count), and + // the callers cast the block count to int; reject it explicitly. + if (count < 0) + { + throw new AvroException($"Invalid negative collection block count: {count}"); + } + // A .NET collection cannot hold more than int.MaxValue elements and // the callers cast the block count to int; reject an out-of-range // count independently of the per-element size (which is 0 for e.g. diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index 11cd4080bb7..d2b0e29a27b 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -316,7 +316,16 @@ internal void EnsureAvailableBytes(long length) /// The number of bytes remaining, or -1 if unknown. public long RemainingBytes() { - return stream.CanSeek ? stream.Length - stream.Position : -1; + if (!stream.CanSeek) + { + return -1; + } + + // Clamp to 0: if the stream was externally seeked past its end (or + // truncated), Position can exceed Length. Callers should only ever + // see -1 (unknown) or a non-negative count. + long remaining = stream.Length - stream.Position; + return remaining < 0 ? 0 : remaining; } private byte read() From d200b5f073738188d66738b5a42cc31389682321 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 16:42:12 +0200 Subject: [PATCH 06/28] AVRO-4295: [csharp] Cap zero-byte collection element allocation Completes the available-bytes protection for collections. Elements whose schema encodes to zero bytes (null, a zero-length fixed, or a record with only zero-byte fields) consume no input, so the bytes-remaining check cannot bound their count. A tiny payload declaring a huge array block count of such elements (e.g. {"type":"array","items":"null"} with a count of 200,000,000) therefore drove an unbounded ResizeArray and exhausted memory. EnsureCollectionAvailable now tracks the cumulative count across blocks and enforces, per block: a structural cap on all collections (MaxCollectionStructural = Integer.MAX_VALUE - 8, also covering non-seekable decoders and keeping the total within the int range the callers cast to); a zero-byte item cap (MaxCollectionItems = 10,000,000) when the per-element minimum is zero; and the existing bytes-remaining check otherwise. AVRO_MAX_COLLECTION_ITEMS, when set, caps both. The reader array/map loops and the schema-resolution Skip path for arrays and maps are all bounded the same way, so skipping a huge zero-byte block cannot loop unboundedly. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/Generic/GenericReader.cs | 76 +++++++++++++------ .../src/apache/test/IO/BinaryCodecTests.cs | 34 +++++++++ 2 files changed, 88 insertions(+), 22 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index 95ce3dbcf80..6ee2f7ca4fb 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -405,12 +405,14 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem object result = CreateArray(reuse, rs); int i = 0; long minBytes = MinBytesPerElement(writerSchema.ItemSchema); + long total = 0; for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext()) { // Reject a block whose element count could not be backed by the - // bytes remaining before allocating for it (checked on the raw - // long, which also avoids the int cast overflowing). - EnsureCollectionAvailable(d, nl, minBytes); + // bytes remaining (or, for zero-byte elements, that exceeds the + // item cap) before allocating for it. Checked on the raw long, + // which also avoids the int cast below overflowing. + total = EnsureCollectionAvailable(d, total, nl, minBytes); int n = (int)nl; if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n); for (int j = 0; j < n; j++, i++) @@ -498,9 +500,10 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re object result = CreateMap(reuse, rs); // Map keys are strings (>= 1 byte length prefix) plus the value. long minBytes = 1L + MinBytesPerElement(writerSchema.ValueSchema); + long total = 0; for (long nl = d.ReadMapStart(); nl != 0; nl = d.ReadMapNext()) { - EnsureCollectionAvailable(d, nl, minBytes); + total = EnsureCollectionAvailable(d, total, nl, minBytes); int n = (int)nl; for (int j = 0; j < n; j++) { @@ -569,19 +572,34 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) } } - /// - /// Rejects a collection (array or map) block whose declared element count - /// could not be backed by the bytes actually remaining, before - /// allocating. Skipped when the per-element minimum is zero, or when the - /// decoder cannot report how many bytes remain. - /// - private static void EnsureCollectionAvailable(Decoder d, long count, long minBytesPerElement) + // Collection allocation limits, guarding against a block-count DoS. Both + // default to the same values as the other Avro SDKs and can be overridden + // (to a single value capping both) via the AVRO_MAX_COLLECTION_ITEMS + // environment variable. + private static readonly long MaxCollectionItems = ReadCollectionLimit(10_000_000L); + private static readonly long MaxCollectionStructural = ReadCollectionLimit(2147483639L); + + private static long ReadCollectionLimit(long defaultValue) { - if (count == 0) + string env = Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS"); + if (!string.IsNullOrEmpty(env) && long.TryParse(env, out long value) && value >= 0) { - return; + return value; } + return defaultValue; + } + + /// + /// Rejects a collection (array or map) block that could drive an unbounded + /// allocation, before allocating for it. A block whose declared element + /// count could not be backed by the bytes actually remaining is rejected; + /// zero-byte element blocks (where the bytes-remaining check does not + /// apply) are bounded by a cumulative item cap; and every collection is + /// bounded by a structural cap. Returns the running total across blocks. + /// + private static long EnsureCollectionAvailable(Decoder d, long total, long count, long minBytesPerElement) + { // A negative count is corrupt/malicious data (it can also arise from // long.MinValue overflow when negating a negative block count), and // the callers cast the block count to int; reject it explicitly. @@ -590,22 +608,28 @@ private static void EnsureCollectionAvailable(Decoder d, long count, long minByt throw new AvroException($"Invalid negative collection block count: {count}"); } - // A .NET collection cannot hold more than int.MaxValue elements and - // the callers cast the block count to int; reject an out-of-range - // count independently of the per-element size (which is 0 for e.g. - // arrays of null, where the byte check below is skipped). - if (count > int.MaxValue) + total += count; + + // A structural cap covers all collections, including non-seekable + // decoders that cannot report their remaining bytes, and keeps the + // running total within the int range the callers cast to. + if (total > MaxCollectionStructural) { throw new AvroException( - $"Collection block count {count} exceeds the maximum supported size"); + $"Collection size {total} exceeds the maximum allowed size of {MaxCollectionStructural}"); } if (minBytesPerElement <= 0) { - return; + // Zero-byte elements (e.g. null) consume no input, so the + // bytes-remaining check cannot bound them; cap by item count. + if (total > MaxCollectionItems) + { + throw new AvroException( + $"Collection of zero-byte elements ({total}) exceeds the maximum allowed size of {MaxCollectionItems}"); + } } - - if (d is BinaryDecoder bd) + else if (d is BinaryDecoder bd) { long remaining = bd.RemainingBytes(); if (remaining >= 0 && count > remaining / minBytesPerElement) @@ -614,6 +638,8 @@ private static void EnsureCollectionAvailable(Decoder d, long count, long minByt $"Collection claims {count} elements with at least {minBytesPerElement} bytes each, but only {remaining} bytes are available"); } } + + return total; } /// @@ -776,8 +802,11 @@ protected virtual void Skip(Schema writerSchema, Decoder d) case Schema.Type.Array: { Schema s = (writerSchema as ArraySchema).ItemSchema; + long minBytes = MinBytesPerElement(s); + long total = 0; for (long n = d.ReadArrayStart(); n != 0; n = d.ReadArrayNext()) { + total = EnsureCollectionAvailable(d, total, n, minBytes); for (long i = 0; i < n; i++) Skip(s, d); } } @@ -785,8 +814,11 @@ protected virtual void Skip(Schema writerSchema, Decoder d) case Schema.Type.Map: { Schema s = (writerSchema as MapSchema).ValueSchema; + long minBytes = 1L + MinBytesPerElement(s); + long total = 0; for (long n = d.ReadMapStart(); n != 0; n = d.ReadMapNext()) { + total = EnsureCollectionAvailable(d, total, n, minBytes); for (long i = 0; i < n; i++) { d.SkipString(); Skip(s, d); } } } diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 92c1f951ed3..090d4cc44bb 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -577,6 +577,40 @@ public void TestReadBytesRejectsLengthAboveMaxArrayLength() } } + // A zero-byte element type (null) consumes no input, so the + // bytes-remaining check cannot bound its block count; a tiny payload + // declaring a huge count must instead be rejected by the item cap + // before building the array. + [Test] + public void TestReadArrayOfNullRejectsHugeCount() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(200_000_000); // ~4 byte payload + ms.Position = 0; + var reader = new GenericReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + + // The skip path (a writer field absent from the reader schema) must be + // bounded too, so skipping a huge zero-byte block cannot loop endlessly. + [Test] + public void TestSkipArrayOfNullRejectsHugeCount() + { + var writer = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"Foo\",\"fields\":[" + + "{\"name\":\"arr\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}," + + "{\"name\":\"val\",\"type\":\"int\"}]}"); + var reader = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"Foo\",\"fields\":[" + + "{\"name\":\"val\",\"type\":\"int\"}]}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(200_000_000); // arr block count; skipped + ms.Position = 0; + var r = new GenericReader(writer, reader); + Assert.Throws(() => r.Read(null, new BinaryDecoder(ms))); + } + // Minimal read-only, forward-only stream wrapper reporting CanSeek=false. private sealed class NonSeekableStream : Stream { From 99cefe0435b805c41d10ad2cf169b70420ad60c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 18:16:37 +0200 Subject: [PATCH 07/28] AVRO-4295: [csharp] Reject long.MinValue array/map block count A negative block count is normalized by negating it (result = -result), but long.MinValue cannot be negated: under unchecked arithmetic it wraps back to a negative value, and under checked arithmetic it throws OverflowException. The zig-zag encoding of long.MinValue is a valid 10-byte varint, so this is reachable from malformed input. Reject long.MinValue explicitly in doReadItemCount() with a clear AvroException, consistent with the C and C++ bindings. Adds tests for a long.MinValue block count on ReadArrayStart() and ReadMapStart(). Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/IO/BinaryDecoder.cs | 8 ++++++++ .../src/apache/test/IO/BinaryCodecTests.cs | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index d2b0e29a27b..a3d9d45c1d6 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -340,6 +340,14 @@ private long doReadItemCount() long result = ReadLong(); if (result < 0) { + // long.MinValue cannot be negated (it would overflow); reject it + // explicitly rather than propagating a wrapped negative or, under + // checked arithmetic, throwing an OverflowException. + if (result == long.MinValue) + { + throw new AvroException("Invalid negative block count: " + result); + } + ReadLong(); // Consume byte-count if present result = -result; } diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 090d4cc44bb..51115d26bd6 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -611,6 +611,25 @@ public void TestSkipArrayOfNullRejectsHugeCount() Assert.Throws(() => r.Read(null, new BinaryDecoder(ms))); } + // A block count of long.MinValue cannot be negated; it must be rejected + // rather than wrapping back to a negative (or huge) count. + [Test] + public void TestReadArrayRejectsInt64MinBlockCount() + { + // long.MinValue zig-zag encodes as the 10-byte varint below. + var data = new byte[] { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01 }; + var d = new BinaryDecoder(new MemoryStream(data)); + Assert.Throws(() => d.ReadArrayStart()); + } + + [Test] + public void TestReadMapRejectsInt64MinBlockCount() + { + var data = new byte[] { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01 }; + var d = new BinaryDecoder(new MemoryStream(data)); + Assert.Throws(() => d.ReadMapStart()); + } + // Minimal read-only, forward-only stream wrapper reporting CanSeek=false. private sealed class NonSeekableStream : Stream { From 3671989ce5e5ae9ab0852fa947a21e54f48066a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 19:01:38 +0200 Subject: [PATCH 08/28] AVRO-4295: [csharp] Cast bounded length to int; clamp structural cap Addresses review feedback: - read(long p) now allocates new byte[(int)p]; p is already bounded to <= MaxDotNetArrayLength above, so the cast required for array allocation cannot overflow. - MaxCollectionStructural is clamped to int.MaxValue. The callers cast the cumulative block count to int to size .NET collections, so a structural limit above int.MaxValue (e.g. from a large AVRO_MAX_COLLECTION_ITEMS override) would otherwise reintroduce an int-overflow on that cast. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/csharp/src/apache/main/Generic/GenericReader.cs | 7 ++++++- lang/csharp/src/apache/main/IO/BinaryDecoder.cs | 4 +++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index 6ee2f7ca4fb..3e5eb93bcd5 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -577,7 +577,12 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) // (to a single value capping both) via the AVRO_MAX_COLLECTION_ITEMS // environment variable. private static readonly long MaxCollectionItems = ReadCollectionLimit(10_000_000L); - private static readonly long MaxCollectionStructural = ReadCollectionLimit(2147483639L); + // The structural cap is additionally clamped to int.MaxValue: the callers + // cast the (cumulative) block count to int to size .NET collections, so a + // structural limit above int.MaxValue (e.g. from a large env override) + // would reintroduce an int-overflow on that cast. + private static readonly long MaxCollectionStructural = + Math.Min(ReadCollectionLimit(2147483639L), int.MaxValue); private static long ReadCollectionLimit(long defaultValue) { diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index a3d9d45c1d6..7e3ed03f1a7 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -278,7 +278,9 @@ private byte[] read(long p) } EnsureAvailableBytes(p); - byte[] buffer = new byte[p]; + // p has been bounded to <= MaxDotNetArrayLength above, so the cast to + // int (required for array allocation) cannot overflow. + byte[] buffer = new byte[(int)p]; Read(buffer, 0, buffer.Length); return buffer; } From 1af6c0a84fa0a6ae5b36b4b00b296b79d93e828a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 20:26:01 +0200 Subject: [PATCH 09/28] AVRO-4295: [csharp] Clarify two doc comments Addresses review feedback (documentation only): - read(long): the max-length guard "mirrors" ReadString() but the thrown message differs; reword so it no longer claims an exact match. - MinBytesPerElement summary: a zero-byte minimum is returned not only for null but also for composites that encode to zero bytes (e.g. a record whose fields are all zero-byte), which disables the bytes-remaining check (those are bounded by the zero-byte item cap instead). Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/csharp/src/apache/main/Generic/GenericReader.cs | 10 ++++++---- lang/csharp/src/apache/main/IO/BinaryDecoder.cs | 3 ++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index 3e5eb93bcd5..83c9a61bfe0 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -517,10 +517,12 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re /// /// Minimum number of bytes a single value of the given schema can occupy /// on the wire. Used to reject an array/map block count that could not be - /// backed by the bytes remaining. A type that can encode to zero bytes - /// (null) returns 0, which disables the collection check for it (so an - /// array of nulls is not falsely rejected). A depth limit breaks - /// self-referencing schemas. + /// backed by the bytes remaining. A type that encodes to zero bytes + /// returns 0 (not only null, but also composites that encode to + /// nothing, e.g. a record whose fields are all zero-byte), which disables + /// the bytes-remaining check for it (so an array of such elements is not + /// falsely rejected; they are instead bounded by the zero-byte item cap). + /// A depth limit breaks self-referencing schemas. /// private static int MinBytesPerElement(Schema schema, int depth = 0) { diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index 7e3ed03f1a7..9f2b7e6fbfd 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -273,7 +273,8 @@ private byte[] read(long p) { // A .NET array cannot be larger than this; reject with a // consistent AvroException rather than letting new byte[p] throw - // an OverflowException/OutOfMemoryException. Matches ReadString(). + // an OverflowException/OutOfMemoryException, mirroring the + // maximum-length guard in ReadString() (the message differs). throw new AvroException($"Length {p} exceeds the maximum supported array length"); } From d3089624304f8e692768873f8dca908463ab98c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 21:00:14 +0200 Subject: [PATCH 10/28] AVRO-4295: [csharp] Read string length as long to avoid int overflow Addresses review feedback: ReadString read the length prefix via ReadInt(), which casts the Avro long to int. A declared length above int.MaxValue overflowed to a negative int, bypassing EnsureAvailableBytes and throwing a misleading "negative length" error. Read the length as a long, apply the remaining-bytes guard and the max-array-length check, then cast to int. Applies to both the netstandard2.0 and non-netstandard2.0 decoders. Adds a regression test (via a non-seekable stream so the length check is reached). Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../main/IO/BinaryDecoder.netstandard2.0.cs | 12 ++++++-- .../IO/BinaryDecoder.notnetstandard2.0.cs | 30 +++++++++++-------- .../src/apache/test/IO/BinaryCodecTests.cs | 18 +++++++++++ 3 files changed, 45 insertions(+), 15 deletions(-) diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs index 90ac362b13a..2cb25cf5f37 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs @@ -78,7 +78,11 @@ public double ReadDouble() /// String read from the stream. public string ReadString() { - int length = ReadInt(); + // Read the length as a long: the prefix is an Avro long, so a value + // above int.MaxValue would overflow ReadInt() to a negative int, + // bypass EnsureAvailableBytes and throw a misleading "negative length" + // error. Validate the bounds before casting to int. + long length = ReadLong(); if (length < 0) { @@ -92,11 +96,13 @@ public string ReadString() throw new AvroException("String length is not supported!"); } + int intLength = (int)length; + using (var binaryReader = new BinaryReader(stream, Encoding.UTF8, true)) { - var bytes = binaryReader.ReadBytes(length); + var bytes = binaryReader.ReadBytes(intLength); - if (bytes.Length != length) + if (bytes.Length != intLength) { throw new AvroException("Could not read as many bytes from stream as expected!"); } diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs index 22cea838647..740de780c16 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs @@ -66,7 +66,11 @@ public double ReadDouble() /// String read from the stream. public string ReadString() { - int length = ReadInt(); + // Read the length as a long: the prefix is an Avro long, so a value + // above int.MaxValue would overflow ReadInt() to a negative int, + // bypass EnsureAvailableBytes and throw a misleading "negative length" + // error. Validate the bounds before casting to int. + long length = ReadLong(); if (length < 0) { @@ -75,15 +79,22 @@ public string ReadString() EnsureAvailableBytes(length); - if (length <= MaxFastReadLength) + if (length > MaxDotNetArrayLength) + { + throw new AvroException("String length is not supported!"); + } + + int intLength = (int)length; + + if (intLength <= MaxFastReadLength) { byte[] bufferArray = null; try { - Span buffer = length <= StackallocThreshold ? - stackalloc byte[length] : - (bufferArray = ArrayPool.Shared.Rent(length)).AsSpan(0, length); + Span buffer = intLength <= StackallocThreshold ? + stackalloc byte[intLength] : + (bufferArray = ArrayPool.Shared.Rent(intLength)).AsSpan(0, intLength); Read(buffer); @@ -99,16 +110,11 @@ public string ReadString() } else { - if (length > MaxDotNetArrayLength) - { - throw new AvroException("String length is not supported!"); - } - using (var binaryReader = new BinaryReader(stream, Encoding.UTF8, true)) { - var bytes = binaryReader.ReadBytes(length); + var bytes = binaryReader.ReadBytes(intLength); - if (bytes.Length != length) + if (bytes.Length != intLength) { throw new AvroException("Could not read as many bytes from stream as expected!"); } diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 51115d26bd6..1a764e4bf11 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -630,6 +630,24 @@ public void TestReadMapRejectsInt64MinBlockCount() Assert.Throws(() => d.ReadMapStart()); } + // A string length prefix above int.MaxValue must be rejected as an + // unsupported length, not overflow the int cast into a negative length. + // A non-seekable stream is used so the check is reached without the + // remaining-bytes guard firing first. + [Test] + public void TestReadStringRejectsLengthAboveInt32() + { + var backing = new MemoryStream(); + new BinaryEncoder(backing).WriteLong((long)int.MaxValue + 1); + byte[] encoded = backing.ToArray(); + using (var ns = new NonSeekableStream(new MemoryStream(encoded))) + { + var d = new BinaryDecoder(ns); + var ex = Assert.Throws(() => d.ReadString()); + StringAssert.Contains("not supported", ex.Message); + } + } + // Minimal read-only, forward-only stream wrapper reporting CanSeek=false. private sealed class NonSeekableStream : Stream { From cf8a68752d2e8e3ee9a494724367f78a4885be90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 21:34:51 +0200 Subject: [PATCH 11/28] AVRO-4295: [csharp] Clamp structural cap to runtime max array length Addresses review feedback: MaxCollectionStructural was clamped to int.MaxValue, but the default reader grows its backing array via Array.Resize, whose maximum supported length is smaller (0x7FFFFFC7 on modern .NET, 0x3FFFFFFF on netstandard2.0). A collection size passing EnsureCollectionAvailable could still throw OutOfMemoryException/OverflowException inside ResizeArray instead of a deterministic AvroException. Clamp the structural limit to the runtime's max array length, mirroring BinaryDecoder.MaxDotNetArrayLength. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/Generic/GenericReader.cs | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index 83c9a61bfe0..a001f39089a 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -579,12 +579,25 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) // (to a single value capping both) via the AVRO_MAX_COLLECTION_ITEMS // environment variable. private static readonly long MaxCollectionItems = ReadCollectionLimit(10_000_000L); - // The structural cap is additionally clamped to int.MaxValue: the callers - // cast the (cumulative) block count to int to size .NET collections, so a - // structural limit above int.MaxValue (e.g. from a large env override) - // would reintroduce an int-overflow on that cast. + + // The largest array the runtime can allocate. Mirrors + // BinaryDecoder.MaxDotNetArrayLength: the default reader grows its backing + // array via Array.Resize, which throws (OutOfMemoryException/OverflowException) + // above this length rather than a deterministic AvroException. +#if NETSTANDARD2_0 + private const int MaxDotNetArrayLength = 0x3FFFFFFF; +#else + private const int MaxDotNetArrayLength = 0x7FFFFFC7; +#endif + + // The structural cap is additionally clamped to the runtime's maximum + // array length: the callers cast the (cumulative) block count to int to + // size .NET collections, and a limit above the max array length (e.g. from + // a large env override, or int.MaxValue itself) would let a collection + // that passes EnsureCollectionAvailable still fault inside Array.Resize + // instead of failing deterministically. private static readonly long MaxCollectionStructural = - Math.Min(ReadCollectionLimit(2147483639L), int.MaxValue); + Math.Min(ReadCollectionLimit(2147483639L), MaxDotNetArrayLength); private static long ReadCollectionLimit(long defaultValue) { From 20fb487ce19dfe138ac06e93f42099a6ee2f0e31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 22:42:04 +0200 Subject: [PATCH 12/28] AVRO-4295: [csharp] Grow array on demand instead of preallocating count ReadArray resized the backing array to the full declared block count up front (ResizeArray(ref result, i + n)) before reading any element. On a non-seekable stream EnsureCollectionAvailable cannot bound the count against the remaining bytes, so a large declared count could drive a multi-gigabyte Array.Resize before the truncated input is detected. Preallocate only a bounded amount (MaxCollectionPrealloc) up front and grow the array geometrically on demand as elements are read. Blocks no larger than the cap keep the original single-resize fast path; a hostile count now fails with a bounded AvroException after a small allocation. Adds a non-seekable-stream regression test. (ReadMap already grows via Dictionary.Add, so it is unaffected.) Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/Generic/GenericReader.cs | 31 ++++++++++++++++++- .../src/apache/test/IO/BinaryCodecTests.cs | 20 ++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index a001f39089a..6664efa1b52 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -414,9 +414,28 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem // which also avoids the int cast below overflowing. total = EnsureCollectionAvailable(d, total, nl, minBytes); int n = (int)nl; - if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n); + // Preallocate only a bounded amount up front, then grow on demand + // below. On a non-seekable stream EnsureCollectionAvailable cannot + // bound the count, so resizing straight to i+n could allocate a + // huge array before any element is read; a truncated stream instead + // fails within Read() after a bounded growth. Blocks no larger than + // the cap keep the original single-resize fast path. + int prealloc = i + Math.Min(n, MaxCollectionPrealloc); + if (GetArraySize(result) < prealloc) ResizeArray(ref result, prealloc); for (int j = 0; j < n; j++, i++) { + if (GetArraySize(result) <= i) + { + int current = GetArraySize(result); + int grown = current + (current >> 1) + 1; + if (grown <= i) + { + grown = i + 1; + } + + ResizeArray(ref result, grown); + } + SetArrayElement(result, i, Read(GetArrayElement(result, i), writerSchema.ItemSchema, rs.ItemSchema, d)); } } @@ -599,6 +618,16 @@ private static int MinBytesPerElement(Schema schema, int depth = 0) private static readonly long MaxCollectionStructural = Math.Min(ReadCollectionLimit(2147483639L), MaxDotNetArrayLength); + // Upper bound on how many elements the backing array is grown by in a + // single step while decoding. The array still grows to hold every element + // actually read; this only avoids resizing to the full (possibly + // attacker-declared) block count up front, before any element is read. + // That matters most for non-seekable streams, where the bytes-available + // check cannot bound the declared count, so a single Array.Resize to the + // block count could allocate a huge array before the truncated stream is + // detected. + private const int MaxCollectionPrealloc = 1024; + private static long ReadCollectionLimit(long defaultValue) { string env = Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS"); diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 1a764e4bf11..da644f64004 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -592,6 +592,26 @@ public void TestReadArrayOfNullRejectsHugeCount() Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); } + // A non-zero-byte array on a non-seekable stream cannot have its block + // count bounded by the bytes remaining (the length is unknown). The + // backing array must therefore be grown on demand rather than + // preallocated to the declared count, so a huge count with truncated data + // fails with a bounded AvroException instead of attempting a multi- + // gigabyte allocation. + [Test] + public void TestReadArrayHugeCountOnStreamClampsPreallocation() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"long\"}"); + var backing = new MemoryStream(); + new BinaryEncoder(backing).WriteLong(200_000_000); // block count; no element data + byte[] encoded = backing.ToArray(); + using (var ns = new NonSeekableStream(new MemoryStream(encoded))) + { + var reader = new GenericReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ns))); + } + } + // The skip path (a writer field absent from the reader schema) must be // bounded too, so skipping a huge zero-byte block cannot loop endlessly. [Test] From 6350b1ef5b21ffbe1b8e35a5a7644939205734d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 23:22:17 +0200 Subject: [PATCH 13/28] AVRO-4295: [csharp] Explicitly bounds-check union and enum indices UnionSchema's indexer performed no range check (an out-of-range or negative branch index surfaced as an ArgumentOutOfRangeException from the IList indexer), and EnumSchema's indexer checked only the upper bound (a negative symbol index fell through to Symbols[index] and threw ArgumentOutOfRangeException). Validate the index against [0, count) in both indexers and throw a clear AvroException, so a malformed union/enum index on read fails deterministically with an Avro-specific error. Adds tests for negative and too-large union and enum indices via GenericReader. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/Schema/EnumSchema.cs | 4 +-- .../src/apache/main/Schema/UnionSchema.cs | 6 ++++ .../src/apache/test/IO/BinaryCodecTests.cs | 28 +++++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/lang/csharp/src/apache/main/Schema/EnumSchema.cs b/lang/csharp/src/apache/main/Schema/EnumSchema.cs index 225780310a6..6b3db1a16f9 100644 --- a/lang/csharp/src/apache/main/Schema/EnumSchema.cs +++ b/lang/csharp/src/apache/main/Schema/EnumSchema.cs @@ -231,8 +231,8 @@ public string this[int index] { get { - if (index < Symbols.Count) return Symbols[index]; - throw new AvroException("Enumeration out of range. Must be less than " + Symbols.Count + ", but is " + index); + if (index >= 0 && index < Symbols.Count) return Symbols[index]; + throw new AvroException("Enumeration out of range. Must be in [0, " + Symbols.Count + "), but is " + index); } } diff --git a/lang/csharp/src/apache/main/Schema/UnionSchema.cs b/lang/csharp/src/apache/main/Schema/UnionSchema.cs index af9ba758363..3f48d5c88ab 100644 --- a/lang/csharp/src/apache/main/Schema/UnionSchema.cs +++ b/lang/csharp/src/apache/main/Schema/UnionSchema.cs @@ -100,6 +100,12 @@ public Schema this[int index] { get { + if (index < 0 || index >= Schemas.Count) + { + throw new AvroException( + "Union branch index out of range. Must be in [0, " + Schemas.Count + "), but is " + index); + } + return Schemas[index]; } } diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index da644f64004..23c1c23a463 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -642,6 +642,34 @@ public void TestReadArrayRejectsInt64MinBlockCount() Assert.Throws(() => d.ReadArrayStart()); } + // A union branch index outside [0, branch count) must be rejected with a + // clear AvroException rather than an ArgumentOutOfRangeException. + [Test] + public void TestReadUnionRejectsOutOfRangeIndex() + { + var schema = Avro.Schema.Parse("[\"null\",\"long\"]"); + // Branch index 5 (zig-zag long 0x0a) and -1 (0x01); only 2 branches exist. + foreach (var data in new[] { new byte[] { 0x0a }, new byte[] { 0x01 } }) + { + var reader = new GenericReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(new MemoryStream(data)))); + } + } + + // An enum symbol index outside [0, symbol count) must be rejected with a + // clear AvroException. + [Test] + public void TestReadEnumRejectsOutOfRangeIndex() + { + var schema = Avro.Schema.Parse("{\"type\":\"enum\",\"name\":\"E\",\"symbols\":[\"A\",\"B\"]}"); + // Symbol index 9 (zig-zag int 0x12) and -1 (0x01); only 2 symbols exist. + foreach (var data in new[] { new byte[] { 0x12 }, new byte[] { 0x01 } }) + { + var reader = new GenericReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(new MemoryStream(data)))); + } + } + [Test] public void TestReadMapRejectsInt64MinBlockCount() { From 91609bd5fe0ac71395642c6ff9e3f312f70765dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sun, 12 Jul 2026 23:49:40 +0200 Subject: [PATCH 14/28] AVRO-4295: [csharp] Reject overlong varints in ReadLong ReadLong looped over continuation bytes with no cap, so an overlong varint was accepted (with the shift wrapping mod 64) as a silently wrong value instead of being rejected. A 64-bit value uses at most 10 bytes; reject an 11th continuation byte with AvroException, matching the Java, C and C++ SDKs. Adds a regression test. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/csharp/src/apache/main/IO/BinaryDecoder.cs | 7 +++++++ lang/csharp/src/apache/test/IO/BinaryCodecTests.cs | 13 +++++++++++++ 2 files changed, 20 insertions(+) diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index 9f2b7e6fbfd..3d56ab9219b 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -76,6 +76,13 @@ public long ReadLong() int shift = 7; while ((b & 0x80) != 0) { + // A 64-bit value uses at most 10 bytes (shifts 0..63); reject an + // overlong varint rather than silently wrapping to a wrong value. + if (shift >= 70) + { + throw new AvroException("Varint is too long"); + } + b = read(); n |= (b & 0x7FUL) << shift; shift += 7; diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 23c1c23a463..ed4aaabc2ac 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -678,6 +678,19 @@ public void TestReadMapRejectsInt64MinBlockCount() Assert.Throws(() => d.ReadMapStart()); } + // A 64-bit value uses at most 10 bytes; an 11th continuation byte is a + // malformed (overlong) varint and must be rejected rather than silently + // wrapping to a wrong value. + [Test] + public void TestReadLongRejectsOverlongVarint() + { + var data = new byte[11]; + for (int i = 0; i < 10; i++) data[i] = 0x80; // 10 continuation bytes + data[10] = 0x01; + var d = new BinaryDecoder(new MemoryStream(data)); + Assert.Throws(() => d.ReadLong()); + } + // A string length prefix above int.MaxValue must be rejected as an // unsupported length, not overflow the int cast into a negative length. // A non-seekable stream is used so the check is reached without the From 345a803a86f172745d5e6fd59ff02f6a55cb1c89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 01:13:23 +0200 Subject: [PATCH 15/28] AVRO-4295: [csharp] Reject block-count overflow before adding to the total EnsureCollectionAvailable did `total += count` before the cap checks. With a collection split across blocks (e.g. block 1 count=1, block 2 count=long.MaxValue) the addition could overflow, wrapping `total` negative and bypassing both the structural cap and the zero-byte item cap, letting an oversized count reach the later (int) cast. Check `count > MaxCollectionStructural - total` before adding (total is always <= MaxCollectionStructural on entry and count >= 0, so the subtraction cannot underflow/overflow), rejecting the overflow deterministically. Adds a non-seekable-stream regression test. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/Generic/GenericReader.cs | 16 +++++++------- .../src/apache/test/IO/BinaryCodecTests.cs | 21 +++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index 6664efa1b52..bc5e9c4571d 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -657,17 +657,19 @@ private static long EnsureCollectionAvailable(Decoder d, long total, long count, throw new AvroException($"Invalid negative collection block count: {count}"); } - total += count; - - // A structural cap covers all collections, including non-seekable - // decoders that cannot report their remaining bytes, and keeps the - // running total within the int range the callers cast to. - if (total > MaxCollectionStructural) + // Reject before adding so an oversized block count cannot overflow + // `total` (wrapping it negative and bypassing the caps below). The + // running total is always <= MaxCollectionStructural on entry (the + // invariant this method maintains) and count >= 0, so the subtraction + // cannot underflow or overflow. + if (count > MaxCollectionStructural - total) { throw new AvroException( - $"Collection size {total} exceeds the maximum allowed size of {MaxCollectionStructural}"); + $"Collection block count {count} exceeds the maximum allowed size of {MaxCollectionStructural}"); } + total += count; + if (minBytesPerElement <= 0) { // Zero-byte elements (e.g. null) consume no input, so the diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index ed4aaabc2ac..a66aa80e761 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -631,6 +631,27 @@ public void TestSkipArrayOfNullRejectsHugeCount() Assert.Throws(() => r.Read(null, new BinaryDecoder(ms))); } + // A collection split across blocks must not let the running total + // overflow: block 1 count=1 then block 2 count=long.MaxValue would wrap + // the total negative and bypass the caps. A non-seekable stream disables + // the bytes-remaining check, so the structural pre-add check must reject it. + [Test] + public void TestReadArrayRejectsBlockCountOverflow() + { + var backing = new MemoryStream(); + var enc = new BinaryEncoder(backing); + enc.WriteLong(1); // block 1: one element + enc.WriteLong(42); // the element + enc.WriteLong(long.MaxValue); // block 2: huge count -> would overflow total + byte[] encoded = backing.ToArray(); + using (var ns = new NonSeekableStream(new MemoryStream(encoded))) + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"long\"}"); + var reader = new GenericReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ns))); + } + } + // A block count of long.MinValue cannot be negated; it must be rejected // rather than wrapping back to a negative (or huge) count. [Test] From ffecc24c52e2a3b9f74934ed9f59bf94f03daaac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 01:34:39 +0200 Subject: [PATCH 16/28] AVRO-4295: [csharp] Clamp array growth to the structural cap The on-demand growth `current + (current >> 1) + 1` was computed in int (risking overflow to a negative size for very large arrays) and could overshoot the runtime's max array length / the structural cap, making Array.Resize throw a runtime exception even when the declared count was within limits. Compute the growth in long and clamp it to MaxCollectionStructural (which is <= the max .NET array length); the validated element count never exceeds that cap, so the clamp cannot starve a legitimate collection. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/main/Generic/GenericReader.cs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index bc5e9c4571d..fb1fac08283 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -427,13 +427,24 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem if (GetArraySize(result) <= i) { int current = GetArraySize(result); - int grown = current + (current >> 1) + 1; - if (grown <= i) + // Grow ~1.5x, computed in long to avoid int overflow, and + // clamp to the structural cap (which is <= the runtime's + // max array length). The validated element count never + // exceeds that cap, so clamping cannot starve a legitimate + // collection while it keeps Array.Resize from being handed + // an over-large (or overflowed/negative) size. + long grown = (long)current + (current >> 1) + 1; + if (grown < i + 1) { grown = i + 1; } - ResizeArray(ref result, grown); + if (grown > MaxCollectionStructural) + { + grown = MaxCollectionStructural; + } + + ResizeArray(ref result, (int)grown); } SetArrayElement(result, i, Read(GetArrayElement(result, i), writerSchema.ItemSchema, rs.ItemSchema, d)); From 8361138aeb708a1635d34d1f04d49473926f3396 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 01:58:55 +0200 Subject: [PATCH 17/28] AVRO-4295: [csharp] Reject malformed 10-byte varints in ReadLong The 10th continuation byte is shifted by 63, so any payload bit above bit 0 (b & 0x7E) would be silently dropped, decoding a malformed varint to a wrong value. A valid 64-bit encoding has those bits clear; reject the input with "Invalid long encoding" when they are set. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/csharp/src/apache/main/IO/BinaryDecoder.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index 3d56ab9219b..b876f2ce45e 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -84,6 +84,14 @@ public long ReadLong() } b = read(); + // The 10th byte (shift == 63) contributes only bit 63; any higher + // payload bit (b & 0x7E) would be silently dropped by << 63, so a + // valid encoding must have them clear. Reject otherwise. + if (shift == 63 && (b & 0x7E) != 0) + { + throw new AvroException("Invalid long encoding"); + } + n |= (b & 0x7FUL) << shift; shift += 7; } From 98f8da0eb100d8be9aa6550f53f7e99fb4908765 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 02:19:33 +0200 Subject: [PATCH 18/28] AVRO-4295: [csharp] Compute prealloc in long; clarify structural-cap message - prealloc was computed with int arithmetic (i + Math.Min(n, cap)), which could overflow when i is near the structural cap. Compute in long, clamp to MaxCollectionStructural, then cast to int. - The structural-cap exception said the "block count" exceeded the limit, but the guard fires when total + count would; reword to "{total} + {count} exceeds ...". Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/csharp/src/apache/main/Generic/GenericReader.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index fb1fac08283..13b3e90fdd4 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -419,8 +419,11 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem // bound the count, so resizing straight to i+n could allocate a // huge array before any element is read; a truncated stream instead // fails within Read() after a bounded growth. Blocks no larger than - // the cap keep the original single-resize fast path. - int prealloc = i + Math.Min(n, MaxCollectionPrealloc); + // the cap keep the original single-resize fast path. Compute in + // long and clamp so a large i near the structural cap cannot + // overflow the int sum. + long preallocLong = Math.Min((long)i + Math.Min(n, MaxCollectionPrealloc), MaxCollectionStructural); + int prealloc = (int)preallocLong; if (GetArraySize(result) < prealloc) ResizeArray(ref result, prealloc); for (int j = 0; j < n; j++, i++) { @@ -676,7 +679,7 @@ private static long EnsureCollectionAvailable(Decoder d, long total, long count, if (count > MaxCollectionStructural - total) { throw new AvroException( - $"Collection block count {count} exceeds the maximum allowed size of {MaxCollectionStructural}"); + $"Collection size {total} + {count} exceeds the maximum allowed size of {MaxCollectionStructural}"); } total += count; From b5d6c6935faec3e2ef982a2bab3db23d73848df9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 03:18:39 +0200 Subject: [PATCH 19/28] AVRO-4295: [csharp] Isolate collection tests from AVRO_MAX_COLLECTION_ITEMS The collection-limit tests assume the default caps, but AVRO_MAX_COLLECTION_ITEMS overrides them, so a value set in a shell/CI could make them flaky (fail, or reject a legitimate array). Add [SetUp]/[TearDown] that clear the env var for each test and restore it afterward, so they run deterministically against the defaults. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/test/IO/BinaryCodecTests.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index a66aa80e761..d2dc820f37d 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -39,6 +39,24 @@ namespace Avro.Test [TestFixture] public class BinaryCodecTests { + private string _previousMaxCollectionItems; + + // The collection-limit tests assume the default caps. AVRO_MAX_COLLECTION_ITEMS + // overrides them, so a value set in the shell/CI could make these tests + // flaky (or reject a legitimate array). Clear it for each test and restore + // it afterward so the tests are deterministic against the defaults. + [SetUp] + public void ClearCollectionItemsEnv() + { + _previousMaxCollectionItems = Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS"); + Environment.SetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS", null); + } + + [TearDown] + public void RestoreCollectionItemsEnv() + { + Environment.SetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS", _previousMaxCollectionItems); + } /// /// Writes an avro type T with value t into a stream using the encode method e From 22b8caca4d0668e4d007655cf832b31e775d433e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 03:37:13 +0200 Subject: [PATCH 20/28] AVRO-4295: [csharp] Revert ineffective env [SetUp]; document static caps GenericReader captures AVRO_MAX_COLLECTION_ITEMS into static readonly fields at class load, so clearing the env var at runtime in [SetUp] had no effect on the already-computed limits. Remove the misleading [SetUp]/[TearDown] and document that the collection-limit tests assume the process was not started with a custom AVRO_MAX_COLLECTION_ITEMS. Assisted-by: GitHub Copilot:claude-opus-4.8 --- .../src/apache/test/IO/BinaryCodecTests.cs | 24 +++++-------------- 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index d2dc820f37d..5095b2047e5 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -39,24 +39,12 @@ namespace Avro.Test [TestFixture] public class BinaryCodecTests { - private string _previousMaxCollectionItems; - - // The collection-limit tests assume the default caps. AVRO_MAX_COLLECTION_ITEMS - // overrides them, so a value set in the shell/CI could make these tests - // flaky (or reject a legitimate array). Clear it for each test and restore - // it afterward so the tests are deterministic against the defaults. - [SetUp] - public void ClearCollectionItemsEnv() - { - _previousMaxCollectionItems = Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS"); - Environment.SetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS", null); - } - - [TearDown] - public void RestoreCollectionItemsEnv() - { - Environment.SetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS", _previousMaxCollectionItems); - } + // NOTE: the collection-limit tests assume the default caps. GenericReader + // captures AVRO_MAX_COLLECTION_ITEMS into static readonly fields at class + // load, so the value is fixed for the process; these tests therefore + // assume the test process was not started with a custom + // AVRO_MAX_COLLECTION_ITEMS (the normal case). Clearing it at runtime + // would have no effect on the already-captured limits. /// /// Writes an avro type T with value t into a stream using the encode method e From 6a9cb20e09668bf8247057d13afc4263744711f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Mon, 13 Jul 2026 06:31:41 +0200 Subject: [PATCH 21/28] AVRO-4295: [csharp] Bound huge-count reject tests to just over the item cap The array decode and skip tests declared 200,000,000 elements. Since a zero-byte element consumes no input, a future regression of the item-cap guard could let those loops run toward that count and hang/OOM the test process. Use 10,000,001 (just over the default 10,000,000 zero-byte item cap) so the rejection path is still exercised but the failure mode stays bounded. Assisted-by: GitHub Copilot:claude-opus-4.8 --- lang/csharp/src/apache/test/IO/BinaryCodecTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 5095b2047e5..2b210fc4789 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -592,7 +592,7 @@ public void TestReadArrayOfNullRejectsHugeCount() { var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); var ms = new MemoryStream(); - new BinaryEncoder(ms).WriteLong(200_000_000); // ~4 byte payload + new BinaryEncoder(ms).WriteLong(10_000_001); // just over the zero-byte item cap; bounded if the guard regresses ms.Position = 0; var reader = new GenericReader(schema, schema); Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); @@ -631,7 +631,7 @@ public void TestSkipArrayOfNullRejectsHugeCount() "{\"type\":\"record\",\"name\":\"Foo\",\"fields\":[" + "{\"name\":\"val\",\"type\":\"int\"}]}"); var ms = new MemoryStream(); - new BinaryEncoder(ms).WriteLong(200_000_000); // arr block count; skipped + new BinaryEncoder(ms).WriteLong(10_000_001); // just over the zero-byte item cap; bounded if the guard regresses ms.Position = 0; var r = new GenericReader(writer, reader); Assert.Throws(() => r.Read(null, new BinaryDecoder(ms))); From ed310ad6b5bea861e715ea8898733d55af9bca0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Thu, 6 Aug 2026 22:25:30 +0200 Subject: [PATCH 22/28] AVRO-4295: [csharp] Bound zero-byte collection elements per datum, not per collection The zero-byte-element item cap (array-style elements that consume no input, which the bytes-remaining check cannot bound) was enforced per collection: ReadArray/ReadMap and Skip each started their running total from zero. Because a container file carries its own schema, an attacker can declare a record with many such collection fields, each block individually under the limit but jointly unbounded, so a tiny payload still drives a huge aggregate allocation. Track the cumulative zero-byte element count on the DefaultReader instance (zeroByteItemsRead), reset at the start of each top-level Read, and check it in EnsureCollectionAvailable (now an instance method). The structural and bytes-remaining checks stay per collection. Adds regression tests for a multi-field record rejected cumulatively and a within-limit record that still decodes (resetting between datums). --- .../src/apache/main/Generic/GenericReader.cs | 25 ++++++++-- .../src/apache/test/IO/BinaryCodecTests.cs | 50 ++++++++++++++++++- 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index 13b3e90fdd4..503ea82e828 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -110,6 +110,15 @@ public class DefaultReader /// public Schema WriterSchema { get; private set; } + // Cumulative number of zero-byte-encoded collection elements (e.g. an + // array of nulls) seen while decoding the current datum. Reset per + // top-level Read. Such elements consume no input, so the bytes-remaining + // check cannot bound them, and a per-collection cap is not enough either: + // a record's schema can declare many collection fields, each block under + // the limit but jointly unbounded. The cap is therefore applied across + // the whole datum. See EnsureCollectionAvailable. + private long zeroByteItemsRead; + /// /// Constructs the default reader for the given schemas using the DefaultReader. If the @@ -137,6 +146,10 @@ public DefaultReader(Schema writerSchema, Schema readerSchema) /// Object read from the decoder. public T Read(T reuse, Decoder decoder) { + // Start a fresh zero-byte-element budget for this datum. The cap is + // cumulative across every collection decoded in this datum (see + // EnsureCollectionAvailable), not per collection. + zeroByteItemsRead = 0; return (T)Read(reuse, WriterSchema, ReaderSchema, decoder); } @@ -661,7 +674,7 @@ private static long ReadCollectionLimit(long defaultValue) /// apply) are bounded by a cumulative item cap; and every collection is /// bounded by a structural cap. Returns the running total across blocks. /// - private static long EnsureCollectionAvailable(Decoder d, long total, long count, long minBytesPerElement) + private long EnsureCollectionAvailable(Decoder d, long total, long count, long minBytesPerElement) { // A negative count is corrupt/malicious data (it can also arise from // long.MinValue overflow when negating a negative block count), and @@ -687,11 +700,15 @@ private static long EnsureCollectionAvailable(Decoder d, long total, long count, if (minBytesPerElement <= 0) { // Zero-byte elements (e.g. null) consume no input, so the - // bytes-remaining check cannot bound them; cap by item count. - if (total > MaxCollectionItems) + // bytes-remaining check cannot bound them. Cap the cumulative + // count across the whole datum, not just this collection: a + // record's schema can declare many zero-byte collection fields, + // each block under the limit but jointly unbounded. + zeroByteItemsRead += count; + if (zeroByteItemsRead > MaxCollectionItems) { throw new AvroException( - $"Collection of zero-byte elements ({total}) exceeds the maximum allowed size of {MaxCollectionItems}"); + $"Collection of zero-byte elements ({zeroByteItemsRead}) exceeds the maximum allowed size of {MaxCollectionItems}"); } } else if (d is BinaryDecoder bd) diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 2b210fc4789..e8e86929ab4 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -598,7 +598,55 @@ public void TestReadArrayOfNullRejectsHugeCount() Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); } - // A non-zero-byte array on a non-seekable stream cannot have its block + // The zero-byte item cap is cumulative across a decoded datum, not per + // collection. A container file carries its own schema, so an attacker can + // declare a record with many array fields, each block individually + // under the limit but jointly unbounded. The first field (1 null) plus a + // second field declaring the full cap (10,000,000) exceeds the cap in + // aggregate and must be rejected before allocating the second field. + [Test] + public void TestRecordOfNullArrayFieldsRejectedCumulativelyAcrossDatum() + { + var schema = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"R\",\"fields\":[" + + "{\"name\":\"a\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}," + + "{\"name\":\"b\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}]}"); + var ms = new MemoryStream(); + var enc = new BinaryEncoder(ms); + enc.WriteLong(1); // field a: one null + enc.WriteLong(0); // end of a + enc.WriteLong(10_000_000); // field b: cumulative 10,000,001 > cap; rejected before allocating + enc.WriteLong(0); // end of b (not reached) + ms.Position = 0; + var reader = new GenericReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + + // The complement: a record whose array fields are jointly under the + // cap decodes normally, and the per-datum budget resets between datums so + // reusing the reader does not accumulate across records. + [Test] + public void TestRecordOfNullArrayFieldsWithinCumulativeLimitReads() + { + var schema = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"R\",\"fields\":[" + + "{\"name\":\"a\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}," + + "{\"name\":\"b\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}]}"); + var ms = new MemoryStream(); + var enc = new BinaryEncoder(ms); + enc.WriteLong(3); enc.WriteLong(0); // field a: 3 nulls + enc.WriteLong(3); enc.WriteLong(0); // field b: 3 nulls + var reader = new GenericReader(schema, schema); + // Decode twice on the same reader: the per-datum budget must reset. + for (int i = 0; i < 2; i++) + { + ms.Position = 0; + var rec = (GenericRecord)reader.Read(null, new BinaryDecoder(ms)); + Assert.AreEqual(3, ((System.Collections.IList)rec["a"]).Count); + Assert.AreEqual(3, ((System.Collections.IList)rec["b"]).Count); + } + } + // count bounded by the bytes remaining (the length is unknown). The // backing array must therefore be grown on demand rather than // preallocated to the declared count, so a huge count with truncated data From 1ed2417604a362e7102f68d43f69247d97fe4191 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Fri, 7 Aug 2026 18:34:14 +0200 Subject: [PATCH 23/28] AVRO-4295: [csharp] Apply collection allocation caps to all readers Commit ed310ad6b hardened only DefaultReader (used by GenericReader). The other public readers -- GenericDatumReader/SpecificDatumReader (via PreresolvingDatumReader), SpecificDefaultReader, and ReflectDefaultReader -- read array/map blocks without any bound, so a tiny payload could still decode a huge collection (e.g. 100,000,000 zero-byte elements from a few bytes) or preallocate an oversized array. Extract the shared guards (min-bytes-per-element, structural/item caps, and EnsureCollectionAvailable) into an internal CollectionBounds helper so the caps cannot drift between the reader implementations, and wire every reader's ReadArray/ReadMap/skip paths through it. The block count is read as a long and validated before the int cast, and PreresolvingDatumReader grows its backing array in bounded, geometric chunks so a huge count on a non-seekable stream cannot preallocate the whole block up front. The per-datum zero-byte-element budget is tracked in a thread-static nesting scope (mirroring the Java fix) rather than on the reader instance, preserving PreresolvingDatumReader's documented thread-sharing contract; nested scopes accumulate into the enclosing datum and only the outermost resets. Adds regression tests for the Generic, Specific, and Reflect reader paths, including a concurrency test for a shared reader. --- .../apache/main/Generic/CollectionBounds.cs | 259 ++++++++++++++++++ .../src/apache/main/Generic/GenericReader.cs | 205 ++------------ .../main/Generic/PreresolvingDatumReader.cs | 83 +++++- .../main/Reflect/ReflectDefaultReader.cs | 17 +- .../apache/main/Specific/SpecificReader.cs | 16 +- .../src/apache/test/IO/BinaryCodecTests.cs | 193 +++++++++++++ .../src/apache/test/Reflect/TestArray.cs | 14 + .../src/apache/test/Specific/SpecificTests.cs | 25 ++ 8 files changed, 610 insertions(+), 202 deletions(-) create mode 100644 lang/csharp/src/apache/main/Generic/CollectionBounds.cs diff --git a/lang/csharp/src/apache/main/Generic/CollectionBounds.cs b/lang/csharp/src/apache/main/Generic/CollectionBounds.cs new file mode 100644 index 00000000000..d9566816fa7 --- /dev/null +++ b/lang/csharp/src/apache/main/Generic/CollectionBounds.cs @@ -0,0 +1,259 @@ +/* + * 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. + */ +using System; +using Avro.IO; + +namespace Avro.Generic +{ + /// + /// Shared allocation guards for decoding Avro collections (arrays and maps), + /// used by every generic/specific reader. Avro encodes a collection as one or + /// more blocks, each prefixed with an element count; a malicious or truncated + /// input can declare far more elements than the stream could ever hold, driving + /// an unbounded allocation from a tiny payload. These helpers reject such counts + /// before anything is allocated. The same logic backs both reader + /// implementations ( and + /// ) so the caps cannot drift apart. + /// + internal static class CollectionBounds + { + // Collection allocation limits, guarding against a block-count DoS. Both + // default to the same values as the other Avro SDKs and can be overridden + // (to a single value capping both) via the AVRO_MAX_COLLECTION_ITEMS + // environment variable. + internal static readonly long MaxCollectionItems = ReadCollectionLimit(10_000_000L); + + // The largest array the runtime can allocate. Mirrors + // BinaryDecoder.MaxDotNetArrayLength: the readers size .NET arrays from the + // (cumulative) block count, which throws (OutOfMemoryException/ + // OverflowException) above this length rather than a deterministic + // AvroException. +#if NETSTANDARD2_0 + private const int MaxDotNetArrayLength = 0x3FFFFFFF; +#else + private const int MaxDotNetArrayLength = 0x7FFFFFC7; +#endif + + // The structural cap is additionally clamped to the runtime's maximum + // array length: the callers cast the (cumulative) block count to int to + // size .NET collections, and a limit above the max array length (e.g. from + // a large env override, or int.MaxValue itself) would let a collection + // that passes EnsureCollectionAvailable still fault inside Array.Resize + // instead of failing deterministically. + internal static readonly long MaxCollectionStructural = + Math.Min(ReadCollectionLimit(2147483639L), MaxDotNetArrayLength); + + // Upper bound on how many elements the backing array is grown by in a + // single step while decoding. The array still grows to hold every element + // actually read; this only avoids resizing to the full (possibly + // attacker-declared) block count up front, before any element is read. + // That matters most for non-seekable streams, where the bytes-available + // check cannot bound the declared count, so a single resize to the block + // count could allocate a huge array before the truncated stream is + // detected. + internal const int MaxCollectionPrealloc = 1024; + + private static long ReadCollectionLimit(long defaultValue) + { + string env = Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS"); + if (!string.IsNullOrEmpty(env) && long.TryParse(env, out long value) && value >= 0) + { + return value; + } + + return defaultValue; + } + + // Per-thread, per-datum cumulative count of zero-byte-encoded collection + // elements (e.g. an array of nulls). Such elements consume no input, so + // the bytes-remaining check cannot bound them, and a per-collection cap is + // not enough either: a record's schema can declare many zero-byte + // collection fields, each block under the limit but jointly unbounded. The + // budget is therefore cumulative across a whole datum. It is thread-static, + // not reader instance state, because a resolved reader may be reused or + // shared among threads (see PreresolvingDatumReader), which such an + // instance field would make unsafe. + [ThreadStatic] private static long zeroByteItemsRead; + + // Nesting depth of the active decode scope on this thread. A delegated + // reader or a skipped writer field decodes within the enclosing datum's + // scope and accumulates into its budget; only the outermost scope resets + // the running total. + [ThreadStatic] private static int scopeDepth; + + /// + /// Opens a decode scope bounding the cumulative zero-byte-element + /// allocation for the current datum. Scopes nest: a nested scope (a + /// delegated reader or a skipped field) accumulates into the enclosing + /// datum's budget, and only the outermost scope resets the running total, + /// so the cap applies across the whole datum rather than per collection. + /// Dispose the returned scope (via using) once the datum is decoded; + /// the budget is thread-static, so the scope must be closed on the same + /// thread, and it is always closed so state cannot leak into later decodes. + /// + internal static Scope EnterScope() + { + if (scopeDepth == 0) + { + zeroByteItemsRead = 0; + } + + scopeDepth++; + return default; + } + + /// + /// The disposable returned by . A stateless struct + /// so using incurs no allocation; closing the outermost scope resets + /// the per-datum budget. + /// + internal readonly struct Scope : IDisposable + { + /// + public void Dispose() + { + if (--scopeDepth == 0) + { + zeroByteItemsRead = 0; + } + } + } + + /// + /// Minimum number of bytes a single value of the given schema can occupy + /// on the wire. Used to reject an array/map block count that could not be + /// backed by the bytes remaining. A type that encodes to zero bytes + /// returns 0 (not only null, but also composites that encode to + /// nothing, e.g. a record whose fields are all zero-byte), which disables + /// the bytes-remaining check for it (so an array of such elements is not + /// falsely rejected; they are instead bounded by the zero-byte item cap). + /// A depth limit breaks self-referencing schemas. + /// + internal static int MinBytesPerElement(Schema schema, int depth = 0) + { + if (schema == null) + { + return 0; + } + + switch (schema.Tag) + { + case Schema.Type.Null: + return 0; + case Schema.Type.Float: + return 4; + case Schema.Type.Double: + return 8; + case Schema.Type.Fixed: + return ((FixedSchema)schema).Size; + case Schema.Type.Record: + case Schema.Type.Error: + if (depth > 64) + { + // A cyclic or pathologically deep record. Return 1 (not + // 0) so the collection check stays enabled; a valid + // recursive value always encodes to >= 1 byte. The depth + // guard is applied only here, so zero-byte leaf types + // such as null still return 0 regardless of depth. + return 1; + } + + // Accumulate in a long and clamp so a deeply nested schema + // cannot overflow int into a value <= 0, which would disable + // the collection check. + long total = 0; + foreach (Field f in (RecordSchema)schema) + { + total += MinBytesPerElement(f.Schema, depth + 1); + if (total >= int.MaxValue) + { + return int.MaxValue; + } + } + + return (int)total; + default: + // boolean, int, long, bytes, string, enum, union, array, map: + // all encode to at least one byte. + return 1; + } + } + + /// + /// Rejects a collection (array or map) block that could drive an unbounded + /// allocation, before allocating for it. A block whose declared element + /// count could not be backed by the bytes actually remaining is rejected; + /// zero-byte element blocks (where the bytes-remaining check does not + /// apply) are bounded by a cumulative item cap; and every collection is + /// bounded by a structural cap. Returns the running total across blocks. + /// + /// Decoder the collection is being read from. + /// Running element total across the blocks decoded so far for this collection. + /// Element count declared by the current block. + /// Minimum on-wire size of one element (see ). + internal static long EnsureCollectionAvailable(Decoder d, long total, long count, long minBytesPerElement) + { + // A negative count is corrupt/malicious data (it can also arise from + // long.MinValue overflow when negating a negative block count), and + // the callers cast the block count to int; reject it explicitly. + if (count < 0) + { + throw new AvroException($"Invalid negative collection block count: {count}"); + } + + // Reject before adding so an oversized block count cannot overflow + // `total` (wrapping it negative and bypassing the caps below). The + // running total is always <= MaxCollectionStructural on entry (the + // invariant this method maintains) and count >= 0, so the subtraction + // cannot underflow or overflow. + if (count > MaxCollectionStructural - total) + { + throw new AvroException( + $"Collection size {total} + {count} exceeds the maximum allowed size of {MaxCollectionStructural}"); + } + + total += count; + + if (minBytesPerElement <= 0) + { + // Zero-byte elements (e.g. null) consume no input, so the + // bytes-remaining check cannot bound them. Cap the cumulative + // count across the whole datum, not just this collection: a + // record's schema can declare many zero-byte collection fields, + // each block under the limit but jointly unbounded. + zeroByteItemsRead += count; + if (zeroByteItemsRead > MaxCollectionItems) + { + throw new AvroException( + $"Collection of zero-byte elements ({zeroByteItemsRead}) exceeds the maximum allowed size of {MaxCollectionItems}"); + } + } + else if (d is BinaryDecoder bd) + { + long remaining = bd.RemainingBytes(); + if (remaining >= 0 && count > remaining / minBytesPerElement) + { + throw new AvroException( + $"Collection claims {count} elements with at least {minBytesPerElement} bytes each, but only {remaining} bytes are available"); + } + } + + return total; + } + } +} diff --git a/lang/csharp/src/apache/main/Generic/GenericReader.cs b/lang/csharp/src/apache/main/Generic/GenericReader.cs index 503ea82e828..8163c430755 100644 --- a/lang/csharp/src/apache/main/Generic/GenericReader.cs +++ b/lang/csharp/src/apache/main/Generic/GenericReader.cs @@ -110,15 +110,6 @@ public class DefaultReader /// public Schema WriterSchema { get; private set; } - // Cumulative number of zero-byte-encoded collection elements (e.g. an - // array of nulls) seen while decoding the current datum. Reset per - // top-level Read. Such elements consume no input, so the bytes-remaining - // check cannot bound them, and a per-collection cap is not enough either: - // a record's schema can declare many collection fields, each block under - // the limit but jointly unbounded. The cap is therefore applied across - // the whole datum. See EnsureCollectionAvailable. - private long zeroByteItemsRead; - /// /// Constructs the default reader for the given schemas using the DefaultReader. If the @@ -146,11 +137,13 @@ public DefaultReader(Schema writerSchema, Schema readerSchema) /// Object read from the decoder. public T Read(T reuse, Decoder decoder) { - // Start a fresh zero-byte-element budget for this datum. The cap is + // Open a fresh zero-byte-element budget for this datum. The cap is // cumulative across every collection decoded in this datum (see - // EnsureCollectionAvailable), not per collection. - zeroByteItemsRead = 0; - return (T)Read(reuse, WriterSchema, ReaderSchema, decoder); + // CollectionBounds.EnsureCollectionAvailable), not per collection. + using (CollectionBounds.EnterScope()) + { + return (T)Read(reuse, WriterSchema, ReaderSchema, decoder); + } } /// @@ -417,7 +410,7 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem ArraySchema rs = (ArraySchema)readerSchema; object result = CreateArray(reuse, rs); int i = 0; - long minBytes = MinBytesPerElement(writerSchema.ItemSchema); + long minBytes = CollectionBounds.MinBytesPerElement(writerSchema.ItemSchema); long total = 0; for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext()) { @@ -425,7 +418,7 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem // bytes remaining (or, for zero-byte elements, that exceeds the // item cap) before allocating for it. Checked on the raw long, // which also avoids the int cast below overflowing. - total = EnsureCollectionAvailable(d, total, nl, minBytes); + total = CollectionBounds.EnsureCollectionAvailable(d, total, nl, minBytes); int n = (int)nl; // Preallocate only a bounded amount up front, then grow on demand // below. On a non-seekable stream EnsureCollectionAvailable cannot @@ -435,7 +428,7 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem // the cap keep the original single-resize fast path. Compute in // long and clamp so a large i near the structural cap cannot // overflow the int sum. - long preallocLong = Math.Min((long)i + Math.Min(n, MaxCollectionPrealloc), MaxCollectionStructural); + long preallocLong = Math.Min((long)i + Math.Min(n, CollectionBounds.MaxCollectionPrealloc), CollectionBounds.MaxCollectionStructural); int prealloc = (int)preallocLong; if (GetArraySize(result) < prealloc) ResizeArray(ref result, prealloc); for (int j = 0; j < n; j++, i++) @@ -455,9 +448,9 @@ protected virtual object ReadArray(object reuse, ArraySchema writerSchema, Schem grown = i + 1; } - if (grown > MaxCollectionStructural) + if (grown > CollectionBounds.MaxCollectionStructural) { - grown = MaxCollectionStructural; + grown = CollectionBounds.MaxCollectionStructural; } ResizeArray(ref result, (int)grown); @@ -545,11 +538,11 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re MapSchema rs = (MapSchema)readerSchema; object result = CreateMap(reuse, rs); // Map keys are strings (>= 1 byte length prefix) plus the value. - long minBytes = 1L + MinBytesPerElement(writerSchema.ValueSchema); + long minBytes = 1L + CollectionBounds.MinBytesPerElement(writerSchema.ValueSchema); long total = 0; for (long nl = d.ReadMapStart(); nl != 0; nl = d.ReadMapNext()) { - total = EnsureCollectionAvailable(d, total, nl, minBytes); + total = CollectionBounds.EnsureCollectionAvailable(d, total, nl, minBytes); int n = (int)nl; for (int j = 0; j < n; j++) { @@ -560,170 +553,6 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re return result; } - /// - /// Minimum number of bytes a single value of the given schema can occupy - /// on the wire. Used to reject an array/map block count that could not be - /// backed by the bytes remaining. A type that encodes to zero bytes - /// returns 0 (not only null, but also composites that encode to - /// nothing, e.g. a record whose fields are all zero-byte), which disables - /// the bytes-remaining check for it (so an array of such elements is not - /// falsely rejected; they are instead bounded by the zero-byte item cap). - /// A depth limit breaks self-referencing schemas. - /// - private static int MinBytesPerElement(Schema schema, int depth = 0) - { - if (schema == null) - { - return 0; - } - - switch (schema.Tag) - { - case Schema.Type.Null: - return 0; - case Schema.Type.Float: - return 4; - case Schema.Type.Double: - return 8; - case Schema.Type.Fixed: - return ((FixedSchema)schema).Size; - case Schema.Type.Record: - case Schema.Type.Error: - if (depth > 64) - { - // A cyclic or pathologically deep record. Return 1 (not - // 0) so the collection check stays enabled; a valid - // recursive value always encodes to >= 1 byte. The depth - // guard is applied only here, so zero-byte leaf types - // such as null still return 0 regardless of depth. - return 1; - } - - // Accumulate in a long and clamp so a deeply nested schema - // cannot overflow int into a value <= 0, which would disable - // the collection check. - long total = 0; - foreach (Field f in (RecordSchema)schema) - { - total += MinBytesPerElement(f.Schema, depth + 1); - if (total >= int.MaxValue) - { - return int.MaxValue; - } - } - - return (int)total; - default: - // boolean, int, long, bytes, string, enum, union, array, map: - // all encode to at least one byte. - return 1; - } - } - - // Collection allocation limits, guarding against a block-count DoS. Both - // default to the same values as the other Avro SDKs and can be overridden - // (to a single value capping both) via the AVRO_MAX_COLLECTION_ITEMS - // environment variable. - private static readonly long MaxCollectionItems = ReadCollectionLimit(10_000_000L); - - // The largest array the runtime can allocate. Mirrors - // BinaryDecoder.MaxDotNetArrayLength: the default reader grows its backing - // array via Array.Resize, which throws (OutOfMemoryException/OverflowException) - // above this length rather than a deterministic AvroException. -#if NETSTANDARD2_0 - private const int MaxDotNetArrayLength = 0x3FFFFFFF; -#else - private const int MaxDotNetArrayLength = 0x7FFFFFC7; -#endif - - // The structural cap is additionally clamped to the runtime's maximum - // array length: the callers cast the (cumulative) block count to int to - // size .NET collections, and a limit above the max array length (e.g. from - // a large env override, or int.MaxValue itself) would let a collection - // that passes EnsureCollectionAvailable still fault inside Array.Resize - // instead of failing deterministically. - private static readonly long MaxCollectionStructural = - Math.Min(ReadCollectionLimit(2147483639L), MaxDotNetArrayLength); - - // Upper bound on how many elements the backing array is grown by in a - // single step while decoding. The array still grows to hold every element - // actually read; this only avoids resizing to the full (possibly - // attacker-declared) block count up front, before any element is read. - // That matters most for non-seekable streams, where the bytes-available - // check cannot bound the declared count, so a single Array.Resize to the - // block count could allocate a huge array before the truncated stream is - // detected. - private const int MaxCollectionPrealloc = 1024; - - private static long ReadCollectionLimit(long defaultValue) - { - string env = Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS"); - if (!string.IsNullOrEmpty(env) && long.TryParse(env, out long value) && value >= 0) - { - return value; - } - - return defaultValue; - } - - /// - /// Rejects a collection (array or map) block that could drive an unbounded - /// allocation, before allocating for it. A block whose declared element - /// count could not be backed by the bytes actually remaining is rejected; - /// zero-byte element blocks (where the bytes-remaining check does not - /// apply) are bounded by a cumulative item cap; and every collection is - /// bounded by a structural cap. Returns the running total across blocks. - /// - private long EnsureCollectionAvailable(Decoder d, long total, long count, long minBytesPerElement) - { - // A negative count is corrupt/malicious data (it can also arise from - // long.MinValue overflow when negating a negative block count), and - // the callers cast the block count to int; reject it explicitly. - if (count < 0) - { - throw new AvroException($"Invalid negative collection block count: {count}"); - } - - // Reject before adding so an oversized block count cannot overflow - // `total` (wrapping it negative and bypassing the caps below). The - // running total is always <= MaxCollectionStructural on entry (the - // invariant this method maintains) and count >= 0, so the subtraction - // cannot underflow or overflow. - if (count > MaxCollectionStructural - total) - { - throw new AvroException( - $"Collection size {total} + {count} exceeds the maximum allowed size of {MaxCollectionStructural}"); - } - - total += count; - - if (minBytesPerElement <= 0) - { - // Zero-byte elements (e.g. null) consume no input, so the - // bytes-remaining check cannot bound them. Cap the cumulative - // count across the whole datum, not just this collection: a - // record's schema can declare many zero-byte collection fields, - // each block under the limit but jointly unbounded. - zeroByteItemsRead += count; - if (zeroByteItemsRead > MaxCollectionItems) - { - throw new AvroException( - $"Collection of zero-byte elements ({zeroByteItemsRead}) exceeds the maximum allowed size of {MaxCollectionItems}"); - } - } - else if (d is BinaryDecoder bd) - { - long remaining = bd.RemainingBytes(); - if (remaining >= 0 && count > remaining / minBytesPerElement) - { - throw new AvroException( - $"Collection claims {count} elements with at least {minBytesPerElement} bytes each, but only {remaining} bytes are available"); - } - } - - return total; - } - /// /// Used by the default implementation of ReadMap() to create a fresh map object. The default /// implementation of this method returns a IDictionary<string, map>. @@ -884,11 +713,11 @@ protected virtual void Skip(Schema writerSchema, Decoder d) case Schema.Type.Array: { Schema s = (writerSchema as ArraySchema).ItemSchema; - long minBytes = MinBytesPerElement(s); + long minBytes = CollectionBounds.MinBytesPerElement(s); long total = 0; for (long n = d.ReadArrayStart(); n != 0; n = d.ReadArrayNext()) { - total = EnsureCollectionAvailable(d, total, n, minBytes); + total = CollectionBounds.EnsureCollectionAvailable(d, total, n, minBytes); for (long i = 0; i < n; i++) Skip(s, d); } } @@ -896,11 +725,11 @@ protected virtual void Skip(Schema writerSchema, Decoder d) case Schema.Type.Map: { Schema s = (writerSchema as MapSchema).ValueSchema; - long minBytes = 1L + MinBytesPerElement(s); + long minBytes = 1L + CollectionBounds.MinBytesPerElement(s); long total = 0; for (long n = d.ReadMapStart(); n != 0; n = d.ReadMapNext()) { - total = EnsureCollectionAvailable(d, total, n, minBytes); + total = CollectionBounds.EnsureCollectionAvailable(d, total, n, minBytes); for (long i = 0; i < n; i++) { d.SkipString(); Skip(s, d); } } } diff --git a/lang/csharp/src/apache/main/Generic/PreresolvingDatumReader.cs b/lang/csharp/src/apache/main/Generic/PreresolvingDatumReader.cs index 53270faecdb..73133f382f2 100644 --- a/lang/csharp/src/apache/main/Generic/PreresolvingDatumReader.cs +++ b/lang/csharp/src/apache/main/Generic/PreresolvingDatumReader.cs @@ -15,6 +15,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +using System; using System.Collections.Generic; using System.IO; using Avro.IO; @@ -69,7 +70,15 @@ protected PreresolvingDatumReader(Schema writerSchema, Schema readerSchema) /// public T Read(T reuse, Decoder decoder) { - return (T)_reader(reuse, decoder); + // Open a fresh zero-byte-element budget for this datum. The cap is + // cumulative across every collection decoded in this datum (see + // CollectionBounds.EnsureCollectionAvailable), not per collection. The + // budget is thread-static, so this reader stays safe to share among + // threads as documented. + using (CollectionBounds.EnterScope()) + { + return (T)_reader(reuse, decoder); + } } /// @@ -364,15 +373,24 @@ private ReadItem ResolveMap(MapSchema writerSchema, MapSchema readerSchema) var reader = ResolveReader(ws, rs); var mapAccess = GetMapAccess(readerSchema); - return (r,d) => ReadMap(r, d, mapAccess, reader); + // Map keys are strings (>= 1 byte length prefix) plus the value. + long valueMinBytes = 1L + CollectionBounds.MinBytesPerElement(ws); + return (r,d) => ReadMap(r, d, mapAccess, reader, valueMinBytes); } - private object ReadMap(object reuse, Decoder decoder, MapAccess mapAccess, ReadItem valueReader) + private object ReadMap(object reuse, Decoder decoder, MapAccess mapAccess, ReadItem valueReader, long valueMinBytes) { object map = mapAccess.Create(reuse); - for (int n = (int)decoder.ReadMapStart(); n != 0; n = (int)decoder.ReadMapNext()) + long total = 0; + for (long nl = decoder.ReadMapStart(); nl != 0; nl = decoder.ReadMapNext()) { + // Reject a block whose element count could not be backed by the + // bytes remaining (or, for zero-byte elements, that exceeds the + // item cap) before allocating for it. Checked on the raw long, + // which also avoids the int cast below overflowing. + total = CollectionBounds.EnsureCollectionAvailable(decoder, total, nl, valueMinBytes); + int n = (int)nl; mapAccess.AddElements(map, n, valueReader, decoder, false); } return map; @@ -383,18 +401,57 @@ private ReadItem ResolveArray(ArraySchema writerSchema, ArraySchema readerSchema var itemReader = ResolveReader(writerSchema.ItemSchema, readerSchema.ItemSchema); var arrayAccess = GetArrayAccess(readerSchema); - return (r, d) => ReadArray(r, d, arrayAccess, itemReader, IsReusable(readerSchema.ItemSchema.Tag)); + long itemMinBytes = CollectionBounds.MinBytesPerElement(writerSchema.ItemSchema); + return (r, d) => ReadArray(r, d, arrayAccess, itemReader, IsReusable(readerSchema.ItemSchema.Tag), itemMinBytes); } - private object ReadArray(object reuse, Decoder decoder, ArrayAccess arrayAccess, ReadItem itemReader, bool itemReusable) + private object ReadArray(object reuse, Decoder decoder, ArrayAccess arrayAccess, ReadItem itemReader, bool itemReusable, long itemMinBytes) { object array = arrayAccess.Create(reuse); int i = 0; - for (int n = (int)decoder.ReadArrayStart(); n != 0; n = (int)decoder.ReadArrayNext()) + // Capacity we have requested from arrayAccess.EnsureSize so far. The + // block is read in bounded chunks that grow this geometrically, so a + // huge declared count on a non-seekable stream (where the + // bytes-remaining check cannot bound it) does not preallocate the + // whole block before any element is read; a truncated stream instead + // faults after a bounded growth. + int capacity = 0; + long total = 0; + for (long nl = decoder.ReadArrayStart(); nl != 0; nl = decoder.ReadArrayNext()) { - arrayAccess.EnsureSize(ref array, i + n); - arrayAccess.AddElements(array, n, i, itemReader, decoder, itemReusable); - i += n; + total = CollectionBounds.EnsureCollectionAvailable(decoder, total, nl, itemMinBytes); + int n = (int)nl; + int remaining = n; + while (remaining > 0) + { + int chunk = Math.Min(remaining, CollectionBounds.MaxCollectionPrealloc); + int needed = i + chunk; + if (capacity < needed) + { + // Grow ~1.5x (amortized O(n), so a legitimate large array + // is not resized on every chunk) plus one chunk, then + // clamp to the structural cap (which is <= the runtime's + // max array length). The validated element count never + // exceeds that cap. + long grown = (long)capacity + (capacity >> 1) + CollectionBounds.MaxCollectionPrealloc; + if (grown < needed) + { + grown = needed; + } + + if (grown > CollectionBounds.MaxCollectionStructural) + { + grown = CollectionBounds.MaxCollectionStructural; + } + + capacity = (int)grown; + arrayAccess.EnsureSize(ref array, capacity); + } + + arrayAccess.AddElements(array, chunk, i, itemReader, decoder, itemReusable); + i += chunk; + remaining -= chunk; + } } arrayAccess.Resize(ref array, i); return array; @@ -486,20 +543,26 @@ private DecoderSkip GetSkip(Schema writerSchema) return d => d.SkipFixed(size); case Schema.Type.Array: var itemSkip = GetSkip(((ArraySchema)writerSchema).ItemSchema); + var arrayItemMinBytes = CollectionBounds.MinBytesPerElement(((ArraySchema)writerSchema).ItemSchema); return d => { + long total = 0; for (long n = d.ReadArrayStart(); n != 0; n = d.ReadArrayNext()) { + total = CollectionBounds.EnsureCollectionAvailable(d, total, n, arrayItemMinBytes); for (long i = 0; i < n; i++) itemSkip(d); } }; case Schema.Type.Map: { var valueSkip = GetSkip(((MapSchema)writerSchema).ValueSchema); + var mapValueMinBytes = 1L + CollectionBounds.MinBytesPerElement(((MapSchema)writerSchema).ValueSchema); return d => { + long total = 0; for (long n = d.ReadMapStart(); n != 0; n = d.ReadMapNext()) { + total = CollectionBounds.EnsureCollectionAvailable(d, total, n, mapValueMinBytes); for (long i = 0; i < n; i++) { d.SkipString(); valueSkip(d); } } }; diff --git a/lang/csharp/src/apache/main/Reflect/ReflectDefaultReader.cs b/lang/csharp/src/apache/main/Reflect/ReflectDefaultReader.cs index 034cb89f88e..2a4a4006652 100644 --- a/lang/csharp/src/apache/main/Reflect/ReflectDefaultReader.cs +++ b/lang/csharp/src/apache/main/Reflect/ReflectDefaultReader.cs @@ -20,6 +20,7 @@ using System.Collections; using System.Collections.Generic; using System.Globalization; +using Avro.Generic; using Avro.IO; using Avro.Specific; using Newtonsoft.Json.Linq; @@ -496,8 +497,16 @@ protected override object ReadArray(object reuse, ArraySchema writerSchema, Sche } int i = 0; - for (int n = (int)dec.ReadArrayStart(); n != 0; n = (int)dec.ReadArrayNext()) + long minBytes = CollectionBounds.MinBytesPerElement(writerSchema.ItemSchema); + long total = 0; + for (long nl = dec.ReadArrayStart(); nl != 0; nl = dec.ReadArrayNext()) { + // Reject a block whose element count could not be backed by the + // bytes remaining (or, for zero-byte elements, that exceeds the + // cumulative item cap) before allocating for it. Checked on the + // raw long, which also avoids the int cast below overflowing. + total = CollectionBounds.EnsureCollectionAvailable(dec, total, nl, minBytes); + int n = (int)nl; for (int j = 0; j < n; j++, i++) { arrayHelper.Add(Read(null, writerSchema.ItemSchema, rs.ItemSchema, dec)); @@ -532,8 +541,12 @@ protected override object ReadMap(object reuse, MapSchema writerSchema, Schema r map = (System.Collections.IDictionary)Activator.CreateInstance(GetTypeFromSchema(rs, false)); } - for (int n = (int)d.ReadMapStart(); n != 0; n = (int)d.ReadMapNext()) + long minBytes = 1L + CollectionBounds.MinBytesPerElement(writerSchema.ValueSchema); + long total = 0; + for (long nl = d.ReadMapStart(); nl != 0; nl = d.ReadMapNext()) { + total = CollectionBounds.EnsureCollectionAvailable(d, total, nl, minBytes); + int n = (int)nl; for (int j = 0; j < n; j++) { string k = d.ReadString(); diff --git a/lang/csharp/src/apache/main/Specific/SpecificReader.cs b/lang/csharp/src/apache/main/Specific/SpecificReader.cs index 1019fa36ced..d6223f00a2e 100644 --- a/lang/csharp/src/apache/main/Specific/SpecificReader.cs +++ b/lang/csharp/src/apache/main/Specific/SpecificReader.cs @@ -213,8 +213,16 @@ protected override object ReadArray(object reuse, ArraySchema writerSchema, Sche array = ObjectCreator.Instance.New(getTargetType(readerSchema), Schema.Type.Array) as System.Collections.IList; int i = 0; - for (int n = (int)dec.ReadArrayStart(); n != 0; n = (int)dec.ReadArrayNext()) + long minBytes = CollectionBounds.MinBytesPerElement(writerSchema.ItemSchema); + long total = 0; + for (long nl = dec.ReadArrayStart(); nl != 0; nl = dec.ReadArrayNext()) { + // Reject a block whose element count could not be backed by the + // bytes remaining (or, for zero-byte elements, that exceeds the + // cumulative item cap) before allocating for it. Checked on the + // raw long, which also avoids the int cast below overflowing. + total = CollectionBounds.EnsureCollectionAvailable(dec, total, nl, minBytes); + int n = (int)nl; for (int j = 0; j < n; j++, i++) array.Add(Read(null, writerSchema.ItemSchema, rs.ItemSchema, dec)); } @@ -245,8 +253,12 @@ protected override object ReadMap(object reuse, MapSchema writerSchema, Schema r else map = ObjectCreator.Instance.New(getTargetType(readerSchema), Schema.Type.Map) as System.Collections.IDictionary; - for (int n = (int)d.ReadMapStart(); n != 0; n = (int)d.ReadMapNext()) + long minBytes = 1L + CollectionBounds.MinBytesPerElement(writerSchema.ValueSchema); + long total = 0; + for (long nl = d.ReadMapStart(); nl != 0; nl = d.ReadMapNext()) { + total = CollectionBounds.EnsureCollectionAvailable(d, total, nl, minBytes); + int n = (int)nl; for (int j = 0; j < n; j++) { string k = d.ReadString(); diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index e8e86929ab4..4c5e0acb30e 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -784,6 +784,199 @@ public void TestReadStringRejectsLengthAboveInt32() } } + // C# has a second reader implementation, GenericDatumReader (based on + // PreresolvingDatumReader), which is public and used directly by + // callers. It must enforce the same collection-allocation caps as the + // DefaultReader used above; the following tests exercise it independently. + + // A zero-byte element type (null) consumes no input, so the + // bytes-remaining check cannot bound its block count: a tiny payload + // declaring a huge count must be rejected by the item cap before building + // the array, rather than decoding e.g. 100,000,000 elements from 5 bytes. + [Test] + public void TestDatumReaderReadArrayOfNullRejectsHugeCount() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(100_000_000); // well over the zero-byte item cap + ms.Position = 0; + var reader = new GenericDatumReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + + // A non-zero-byte element block whose declared count could not be backed + // by the bytes remaining must be rejected before allocating. + [Test] + public void TestDatumReaderReadArrayRejectsCountBeyondStream() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"long\"}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(1000000); // 1,000,000 longs, no data + ms.Position = 0; + var reader = new GenericDatumReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + + // The complement: a legitimate array of nulls under the cap still decodes. + [Test] + public void TestDatumReaderReadArrayOfNullNotFalselyRejected() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); + var ms = new MemoryStream(); + var enc = new BinaryEncoder(ms); + enc.WriteLong(100000); // one block of 100,000 nulls (zero bytes each) + enc.WriteLong(0); // end-of-array marker + ms.Position = 0; + var reader = new GenericDatumReader(schema, schema); + var result = (object[])reader.Read(null, new BinaryDecoder(ms)); + Assert.AreEqual(100000, result.Length); + } + + [Test] + public void TestDatumReaderReadMapRejectsCountBeyondStream() + { + var schema = Avro.Schema.Parse("{\"type\":\"map\",\"values\":\"long\"}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(1000000); + ms.Position = 0; + var reader = new GenericDatumReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + + // The zero-byte item cap is cumulative across a decoded datum, not per + // collection: a record declaring many array fields, each block under + // the limit but jointly over it, must be rejected before allocating the + // offending field. + [Test] + public void TestDatumReaderRecordOfNullArrayFieldsRejectedCumulatively() + { + var schema = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"R\",\"fields\":[" + + "{\"name\":\"a\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}," + + "{\"name\":\"b\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}]}"); + var ms = new MemoryStream(); + var enc = new BinaryEncoder(ms); + enc.WriteLong(1); // field a: one null + enc.WriteLong(0); // end of a + enc.WriteLong(10_000_000); // field b: cumulative 10,000,001 > cap; rejected before allocating + enc.WriteLong(0); // end of b (not reached) + ms.Position = 0; + var reader = new GenericDatumReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + + // The complement: jointly-under-the-cap fields decode, and the per-datum + // budget resets between datums so reusing the reader does not accumulate. + [Test] + public void TestDatumReaderRecordOfNullArrayFieldsWithinLimitReads() + { + var schema = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"R\",\"fields\":[" + + "{\"name\":\"a\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}," + + "{\"name\":\"b\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}]}"); + var ms = new MemoryStream(); + var enc = new BinaryEncoder(ms); + enc.WriteLong(3); enc.WriteLong(0); // field a: 3 nulls + enc.WriteLong(3); enc.WriteLong(0); // field b: 3 nulls + var reader = new GenericDatumReader(schema, schema); + for (int i = 0; i < 2; i++) + { + ms.Position = 0; + var rec = (GenericRecord)reader.Read(null, new BinaryDecoder(ms)); + Assert.AreEqual(3, ((object[])rec["a"]).Length); + Assert.AreEqual(3, ((object[])rec["b"]).Length); + } + } + + // The skip path (a writer field absent from the reader schema) must be + // bounded too, so skipping a huge zero-byte block cannot loop endlessly. + [Test] + public void TestDatumReaderSkipArrayOfNullRejectsHugeCount() + { + var writer = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"Foo\",\"fields\":[" + + "{\"name\":\"arr\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}," + + "{\"name\":\"val\",\"type\":\"int\"}]}"); + var reader = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"Foo\",\"fields\":[" + + "{\"name\":\"val\",\"type\":\"int\"}]}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(100_000_000); // well over the zero-byte item cap + ms.Position = 0; + var r = new GenericDatumReader(writer, reader); + Assert.Throws(() => r.Read(null, new BinaryDecoder(ms))); + } + + // A block count larger than int.MaxValue must be rejected before the int + // cast, even for a null-element array where the byte check is skipped. + [Test] + public void TestDatumReaderReadArrayRejectsCountAboveIntMax() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong((long)int.MaxValue + 1); + ms.Position = 0; + var reader = new GenericDatumReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ms))); + } + + // A huge non-zero-byte count on a non-seekable stream (where the + // bytes-remaining check cannot bound it) must not preallocate the whole + // block: the backing store grows on demand, so a truncated stream fails + // with a bounded AvroException instead of a multi-gigabyte allocation. + [Test] + public void TestDatumReaderReadArrayHugeCountOnStreamClampsPreallocation() + { + var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"long\"}"); + var backing = new MemoryStream(); + new BinaryEncoder(backing).WriteLong(200_000_000); // block count; no element data + byte[] encoded = backing.ToArray(); + using (var ns = new NonSeekableStream(new MemoryStream(encoded))) + { + var reader = new GenericDatumReader(schema, schema); + Assert.Throws(() => reader.Read(null, new BinaryDecoder(ns))); + } + } + + // A resolved PreresolvingDatumReader is documented as safe to share among + // threads. The per-datum zero-byte-element budget must therefore not be + // reader instance state (a shared counter would let concurrent decodes + // reset and increment each other); it is thread-static. Many threads + // decoding within-cap data through one shared reader must all succeed. + [Test] + public void TestDatumReaderSharedAcrossThreadsIsThreadSafe() + { + var schema = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"R\",\"fields\":[" + + "{\"name\":\"a\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}," + + "{\"name\":\"b\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}]}"); + var ms = new MemoryStream(); + var enc = new BinaryEncoder(ms); + enc.WriteLong(3); enc.WriteLong(0); // field a: 3 nulls + enc.WriteLong(3); enc.WriteLong(0); // field b: 3 nulls + byte[] encoded = ms.ToArray(); + + var reader = new GenericDatumReader(schema, schema); + var errors = new System.Collections.Concurrent.ConcurrentQueue(); + System.Threading.Tasks.Parallel.For(0, 32, _ => + { + try + { + for (int i = 0; i < 500; i++) + { + var rec = (GenericRecord)reader.Read(null, new BinaryDecoder(new MemoryStream(encoded))); + Assert.AreEqual(3, ((object[])rec["a"]).Length); + Assert.AreEqual(3, ((object[])rec["b"]).Length); + } + } + catch (Exception ex) + { + errors.Enqueue(ex); + } + }); + Assert.IsEmpty(errors); + } + // Minimal read-only, forward-only stream wrapper reporting CanSeek=false. private sealed class NonSeekableStream : Stream { diff --git a/lang/csharp/src/apache/test/Reflect/TestArray.cs b/lang/csharp/src/apache/test/Reflect/TestArray.cs index ede5af33594..21dd2da9537 100644 --- a/lang/csharp/src/apache/test/Reflect/TestArray.cs +++ b/lang/csharp/src/apache/test/Reflect/TestArray.cs @@ -82,6 +82,20 @@ public void ListTest() } } + // A malicious array block declaring far more elements than the stream + // could hold must be rejected before allocating, matching the Generic and + // Specific readers. Exercises ReflectDefaultReader.ReadArray. + [TestCase] + public void ListRejectsCountBeyondStream() + { + var schema = Schema.Parse(_simpleList); // array + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(1000000); // 1,000,000 strings, no data + ms.Seek(0, SeekOrigin.Begin); + var reader = new ReflectReader>(schema, schema); + Assert.Throws(() => reader.Read(new BinaryDecoder(ms))); + } + [TestCase] public void ListRecTest() { diff --git a/lang/csharp/src/apache/test/Specific/SpecificTests.cs b/lang/csharp/src/apache/test/Specific/SpecificTests.cs index 1aa3c3a03ae..1fecddf300d 100644 --- a/lang/csharp/src/apache/test/Specific/SpecificTests.cs +++ b/lang/csharp/src/apache/test/Specific/SpecificTests.cs @@ -511,6 +511,31 @@ public void DeserializeToLogicalTypeWithDefault() } + // A malicious array block declaring far more elements than the stream + // could hold must be rejected before allocating, matching the Generic + // reader. Exercises SpecificDefaultReader.ReadArray (via SpecificReader) + // and SpecificDatumReader (via PreresolvingDatumReader). + [TestCase] + public void TestSpecificReaderRejectsArrayCountBeyondStream() + { + var schema = EmbeddedGenericsRecord._SCHEMA; + + byte[] Malicious() + { + var ms = new MemoryStream(); + var enc = new BinaryEncoder(ms); + enc.WriteUnionIndex(0); // OptionalInt: union branch 0 (null) + enc.WriteLong(1000000); // OptionalIntList: 1,000,000 items, no data + return ms.ToArray(); + } + + var r1 = new SpecificReader(schema, schema); + Assert.Throws(() => r1.Read(null, new BinaryDecoder(new MemoryStream(Malicious())))); + + var r2 = new SpecificDatumReader(schema, schema); + Assert.Throws(() => r2.Read(null, new BinaryDecoder(new MemoryStream(Malicious())))); + } + private static S deserialize(Stream ms, Schema ws, Schema rs) where S : class, ISpecificRecord { long initialPos = ms.Position; From c5e4306b880b635de9c9f127d4e8abe0f1700350 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Fri, 7 Aug 2026 18:51:36 +0200 Subject: [PATCH 24/28] AVRO-4295: [csharp] Fix stale test comments flagged by review Address two Copilot review notes: the collection-limit test NOTE now refers to CollectionBounds (which captures AVRO_MAX_COLLECTION_ITEMS) instead of the outdated GenericReader, and the fragmented comment on TestReadArrayHugeCountOnStreamClampsPreallocation is rewritten as a complete sentence that names the non-seekable-stream case. --- .../src/apache/test/IO/BinaryCodecTests.cs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 4c5e0acb30e..2805cb4208c 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -39,12 +39,12 @@ namespace Avro.Test [TestFixture] public class BinaryCodecTests { - // NOTE: the collection-limit tests assume the default caps. GenericReader - // captures AVRO_MAX_COLLECTION_ITEMS into static readonly fields at class - // load, so the value is fixed for the process; these tests therefore - // assume the test process was not started with a custom - // AVRO_MAX_COLLECTION_ITEMS (the normal case). Clearing it at runtime - // would have no effect on the already-captured limits. + // NOTE: the collection-limit tests assume the default caps. CollectionBounds + // (shared by every reader) captures AVRO_MAX_COLLECTION_ITEMS into static + // readonly fields at class load, so the value is fixed for the process; + // these tests therefore assume the test process was not started with a + // custom AVRO_MAX_COLLECTION_ITEMS (the normal case). Clearing it at + // runtime would have no effect on the already-captured limits. /// /// Writes an avro type T with value t into a stream using the encode method e @@ -647,11 +647,11 @@ public void TestRecordOfNullArrayFieldsWithinCumulativeLimitReads() } } - // count bounded by the bytes remaining (the length is unknown). The - // backing array must therefore be grown on demand rather than - // preallocated to the declared count, so a huge count with truncated data - // fails with a bounded AvroException instead of attempting a multi- - // gigabyte allocation. + // On a non-seekable stream the declared element count cannot be bounded by + // the bytes remaining (the length is unknown). The backing array must + // therefore be grown on demand rather than preallocated to the declared + // count, so a huge count with truncated data fails with a bounded + // AvroException instead of attempting a multi-gigabyte allocation. [Test] public void TestReadArrayHugeCountOnStreamClampsPreallocation() { From 57aae1f5a27b2077a71467ae0d8ead693b994cbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Fri, 7 Aug 2026 19:03:17 +0200 Subject: [PATCH 25/28] AVRO-4295: [csharp] Grow decoded array by the current chunk, not the prealloc bound In PreresolvingDatumReader.ReadArray the geometric growth added the fixed MaxCollectionPrealloc (1024) instead of the actual chunk size, so a small array (e.g. 3 items) was resized to 1024 up front and then shrunk again at the end. Adding the current chunk keeps the bounded ~1.5x growth for large arrays (where chunk == MaxCollectionPrealloc) while avoiding the needless over-allocation and copy for small arrays. --- .../apache/main/Generic/PreresolvingDatumReader.cs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lang/csharp/src/apache/main/Generic/PreresolvingDatumReader.cs b/lang/csharp/src/apache/main/Generic/PreresolvingDatumReader.cs index 73133f382f2..8826930f3d8 100644 --- a/lang/csharp/src/apache/main/Generic/PreresolvingDatumReader.cs +++ b/lang/csharp/src/apache/main/Generic/PreresolvingDatumReader.cs @@ -429,11 +429,13 @@ private object ReadArray(object reuse, Decoder decoder, ArrayAccess arrayAccess, if (capacity < needed) { // Grow ~1.5x (amortized O(n), so a legitimate large array - // is not resized on every chunk) plus one chunk, then - // clamp to the structural cap (which is <= the runtime's - // max array length). The validated element count never - // exceeds that cap. - long grown = (long)capacity + (capacity >> 1) + CollectionBounds.MaxCollectionPrealloc; + // is not resized on every chunk) plus the current chunk, + // then clamp to the structural cap (which is <= the + // runtime's max array length). Adding `chunk` (not the full + // prealloc bound) avoids over-allocating a small array to + // MaxCollectionPrealloc only to shrink it again at the end. + // The validated element count never exceeds the cap. + long grown = (long)capacity + (capacity >> 1) + chunk; if (grown < needed) { grown = needed; From 43b262d72a75f5d2f3548e20387046869119c29d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Fri, 7 Aug 2026 19:10:43 +0200 Subject: [PATCH 26/28] AVRO-4295: [csharp] Drop generic catch in concurrency test (CodeQL) CodeQL flagged the generic `catch (Exception)` in the shared-reader concurrency test. Parallel.For already aggregates any exception thrown in its body (a spurious AvroException from a thread-safety regression, or a failed assertion) into an AggregateException and rethrows it, so the try/catch and the error queue were redundant. Remove them; the test still fails if any worker throws. --- .../src/apache/test/IO/BinaryCodecTests.cs | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 2805cb4208c..4754c1d6967 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -957,24 +957,20 @@ public void TestDatumReaderSharedAcrossThreadsIsThreadSafe() byte[] encoded = ms.ToArray(); var reader = new GenericDatumReader(schema, schema); - var errors = new System.Collections.Concurrent.ConcurrentQueue(); + // Parallel.For aggregates any exception thrown in the body (a spurious + // AvroException from a thread-safety bug, or a failed assertion) into + // an AggregateException and rethrows it, failing the test. A shared + // per-instance zero-byte budget would surface here; the thread-static + // budget must not. System.Threading.Tasks.Parallel.For(0, 32, _ => { - try + for (int i = 0; i < 500; i++) { - for (int i = 0; i < 500; i++) - { - var rec = (GenericRecord)reader.Read(null, new BinaryDecoder(new MemoryStream(encoded))); - Assert.AreEqual(3, ((object[])rec["a"]).Length); - Assert.AreEqual(3, ((object[])rec["b"]).Length); - } - } - catch (Exception ex) - { - errors.Enqueue(ex); + var rec = (GenericRecord)reader.Read(null, new BinaryDecoder(new MemoryStream(encoded))); + Assert.AreEqual(3, ((object[])rec["a"]).Length); + Assert.AreEqual(3, ((object[])rec["b"]).Length); } }); - Assert.IsEmpty(errors); } // Minimal read-only, forward-only stream wrapper reporting CanSeek=false. From 824bc73aa5cc42a871703779dde748939bb6d4c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Sat, 22 Aug 2026 14:09:24 +0200 Subject: [PATCH 27/28] AVRO-4295: [csharp] Bound the skip path too (reject negative/oversized skips) The read path was hardened against negative and oversized length prefixes, but the skip path (used when resolving away a writer-only field) still called stream.Seek() unchecked. A negative bytes/string length would seek backwards and re-read already-consumed data; a huge length would seek past the end. Route Skip(int)/Skip(long) through a single guard that rejects negative skips with an AvroException and enforces EnsureAvailableBytes(), mirroring read(long). Adds tests: SkipBytes/SkipString reject a negative length, SkipBytes rejects a length beyond the remaining data, and resolving away a negative-length bytes field throws instead of corrupting following fields. --- .../src/apache/main/IO/BinaryDecoder.cs | 15 ++++- .../src/apache/test/IO/BinaryCodecTests.cs | 55 +++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs index b876f2ce45e..bd7ee7263f8 100644 --- a/lang/csharp/src/apache/main/IO/BinaryDecoder.cs +++ b/lang/csharp/src/apache/main/IO/BinaryDecoder.cs @@ -374,11 +374,24 @@ private long doReadItemCount() private void Skip(int p) { - stream.Seek(p, SeekOrigin.Current); + Skip((long)p); } private void Skip(long p) { + if (p < 0) + { + // A negative length would seek backwards, re-reading data already + // consumed (or before the start of the stream). Reject it with a + // consistent AvroException, mirroring the guard in read(long). + throw new AvroException($"Can not skip a negative number of bytes: {p}"); + } + + // Do not skip past the end of the data: a malicious or truncated + // input can declare a huge length prefix on a field that is being + // skipped during schema resolution. This mirrors the guard applied + // on the read path. + EnsureAvailableBytes(p); stream.Seek(p, SeekOrigin.Current); } } diff --git a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs index 4754c1d6967..8dddfb7ffc7 100644 --- a/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs +++ b/lang/csharp/src/apache/test/IO/BinaryCodecTests.cs @@ -552,6 +552,61 @@ public void TestReadBytesRejectsNegativeLength() Assert.Throws(() => d.ReadBytes()); } + // The skip path (used when resolving away a writer-only field) must also + // reject a negative length: seeking backwards would re-read data already + // consumed rather than skipping forward. + [Test] + public void TestSkipBytesRejectsNegativeLength() + { + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(-5); + ms.Position = 0; + var d = new BinaryDecoder(ms); + Assert.Throws(() => d.SkipBytes()); + } + + [Test] + public void TestSkipStringRejectsNegativeLength() + { + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(-5); + ms.Position = 0; + var d = new BinaryDecoder(ms); + Assert.Throws(() => d.SkipString()); + } + + // Skipping past the end of the data (a huge length prefix on a skipped + // field) must be rejected rather than silently seeking beyond the end. + [Test] + public void TestSkipBytesRejectsLengthBeyondRemaining() + { + var ms = new MemoryStream(); + new BinaryEncoder(ms).WriteLong(1000); // claims 1000 bytes, but none follow + ms.Position = 0; + var d = new BinaryDecoder(ms); + Assert.Throws(() => d.SkipBytes()); + } + + // Resolving away a writer-only bytes field whose length is negative must + // fail cleanly instead of corrupting the decoding of following fields. + [Test] + public void TestSkipNegativeBytesDuringResolution() + { + var writer = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"S\",\"fields\":[" + + "{\"name\":\"b\",\"type\":\"bytes\"},{\"name\":\"a\",\"type\":\"long\"}]}"); + var reader = Avro.Schema.Parse( + "{\"type\":\"record\",\"name\":\"S\",\"fields\":[" + + "{\"name\":\"a\",\"type\":\"long\"}]}"); + var ms = new MemoryStream(); + var enc = new BinaryEncoder(ms); + enc.WriteLong(-1); // b: negative bytes length (malformed) + enc.WriteLong(5); // a + ms.Position = 0; + var r = new GenericReader(writer, reader); + Assert.Throws(() => r.Read(null, new BinaryDecoder(ms))); + } + // A block count larger than int.MaxValue must be rejected before the // int cast, even for a null-element array where the byte check is // skipped. From 331dc22f62923e8949479c93225c53a27c26b79f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Tue, 25 Aug 2026 15:10:15 +0200 Subject: [PATCH 28/28] AVRO-4295: [csharp] Add PreresolvingDatumReader collection-bounds tests The collection-allocation guards are shared by both reader paths, but the dedicated tests only exercised the specific reader for the huge-array-count case. Add tests that drive the PreresolvingDatumReader path directly via GenericDatumReader for the remaining scenarios (AVRO-4306): - a map block declaring far more entries than the stream can hold is rejected before allocating (asserts the bounds-check message, not a later EOF); - an array of zero-byte elements (null) exceeding the cumulative item cap is rejected; - a legitimate small array of null is still read (no false rejection); - a valid collection on a non-seekable stream (RemainingBytes() == -1) is still read correctly. Verified the two rejection tests fail with "End of stream reached" when the EnsureCollectionAvailable guard is removed, confirming they exercise the bounds check rather than an incidental end-of-stream fault. --- .../PreresolvingDatumReaderBoundsTests.cs | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 lang/csharp/src/apache/test/Generic/PreresolvingDatumReaderBoundsTests.cs diff --git a/lang/csharp/src/apache/test/Generic/PreresolvingDatumReaderBoundsTests.cs b/lang/csharp/src/apache/test/Generic/PreresolvingDatumReaderBoundsTests.cs new file mode 100644 index 00000000000..60b33e37647 --- /dev/null +++ b/lang/csharp/src/apache/test/Generic/PreresolvingDatumReaderBoundsTests.cs @@ -0,0 +1,153 @@ +/* + * 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. + */ +using System; +using System.IO; +using Avro.Generic; +using Avro.IO; +using NUnit.Framework; + +namespace Avro.Test.Generic +{ + /// + /// Collection-allocation bounds (AVRO-4295 / AVRO-4306) must also apply to the + /// path (the base of + /// and + /// ), not only to the + /// DefaultReader/GenericReader path. These tests exercise that path directly + /// via for the map, zero-byte-element and + /// non-seekable-stream cases; the huge-array-count case on the specific reader + /// is covered by SpecificTests.TestSpecificReaderRejectsArrayCountBeyondStream. + /// + [TestFixture] + class PreresolvingDatumReaderBoundsTests + { + private static byte[] Encode(Action write) + { + var ms = new MemoryStream(); + write(new BinaryEncoder(ms)); + return ms.ToArray(); + } + + // A map block declaring far more entries than the stream could hold must be + // rejected before allocating, on the PreresolvingDatumReader path. Asserting + // the guard's message ensures the rejection comes from the bounds check + // (before any allocation), not from a later end-of-stream fault. + [TestCase] + public void MapCountBeyondStreamRejected() + { + var schema = Schema.Parse("{\"type\":\"map\",\"values\":\"long\"}"); + // One block declaring 1,000,000 entries, followed by no data. + byte[] malicious = Encode(enc => enc.WriteLong(1000000)); + + var reader = new GenericDatumReader(schema, schema); + var ex = Assert.Throws( + () => reader.Read(null, new BinaryDecoder(new MemoryStream(malicious)))); + Assert.That(ex.Message, Does.Contain("bytes are available")); + } + + // Zero-byte elements (array of null) consume no input, so the + // bytes-remaining check cannot bound them; the cumulative item cap must. + [TestCase] + public void ArrayOfNullHugeCountRejected() + { + var schema = Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); + // 20,000,000 null items (zero bytes each) exceeds the 10,000,000 item cap. + byte[] malicious = Encode(enc => enc.WriteLong(20000000)); + + var reader = new GenericDatumReader(schema, schema); + var ex = Assert.Throws( + () => reader.Read(null, new BinaryDecoder(new MemoryStream(malicious)))); + Assert.That(ex.Message, Does.Contain("zero-byte elements")); + } + + // A legitimate small array of null must still be read, not falsely rejected. + [TestCase] + public void ArrayOfNullSmallCountReads() + { + var schema = Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); + byte[] data = Encode(enc => + { + enc.WriteLong(3); // block of 3 null items + enc.WriteLong(0); // terminator + }); + + var reader = new GenericDatumReader(schema, schema); + var result = (object[])reader.Read(null, new BinaryDecoder(new MemoryStream(data))); + Assert.AreEqual(3, result.Length); + Assert.IsNull(result[0]); + Assert.IsNull(result[2]); + } + + // A valid collection on a non-seekable stream (RemainingBytes() == -1, so + // the bytes-available check is skipped) must still be read correctly. + [TestCase] + public void NonSeekableStreamStillReads() + { + var schema = Schema.Parse("{\"type\":\"array\",\"items\":\"long\"}"); + byte[] data = Encode(enc => + { + enc.WriteLong(3); // block of 3 items + enc.WriteLong(1); + enc.WriteLong(2); + enc.WriteLong(3); + enc.WriteLong(0); // terminator + }); + + var reader = new GenericDatumReader(schema, schema); + using (var ns = new NonSeekableStream(new MemoryStream(data))) + { + var result = (object[])reader.Read(null, new BinaryDecoder(ns)); + Assert.AreEqual(3, result.Length); + Assert.AreEqual(1L, result[0]); + Assert.AreEqual(2L, result[1]); + Assert.AreEqual(3L, result[2]); + } + } + + // Minimal read-only, forward-only stream wrapper reporting CanSeek=false. + private sealed class NonSeekableStream : Stream + { + private readonly Stream inner; + public NonSeekableStream(Stream inner) { this.inner = inner; } + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + public override void Flush() { } + public override int Read(byte[] buffer, int offset, int count) => inner.Read(buffer, offset, count); + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + + protected override void Dispose(bool disposing) + { + if (disposing) + { + inner.Dispose(); + } + + base.Dispose(disposing); + } + } + } +}