You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "iemejia (via GitHub)" <gi...@apache.org> on 2026/07/11 21:20:22 UTC
[PR] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values [avro]
iemejia opened a new pull request, #3860:
URL: https://github.com/apache/avro/pull/3860
## What is the purpose of the change
A `bytes` or `string` value is encoded as a length prefix followed by that many bytes of data, and an `array` or `map` block is encoded as an element count followed by that many items. A malicious or truncated input can declare a very large length or count while carrying little or no actual data, which causes a correspondingly large allocation before the shortfall is noticed.
This applies the equivalent of the Java SDK fix [AVRO-4241](https://issues.apache.org/jira/browse/AVRO-4241) to the C# SDK: when the source can report how many bytes remain, a declared length (or a collection block count) that exceeds the bytes actually available is rejected before allocating for it. The collection check uses the minimum on-wire size of the element schema, so a zero-byte element type (such as `null`) is never falsely rejected. Sources that cannot report their remaining size are unaffected.
`BinaryDecoder.RemainingBytes()` reports the bytes still readable for a seekable stream (or -1). `ReadBytes`/`ReadString` reject an over-large declared length, and `DefaultReader.ReadArray`/`ReadMap` reject a block whose element count could not be backed by the bytes remaining, computing `MinBytesPerElement()` from the element schema. The count is checked on the raw `long` before the `int` cast, which also avoids the cast overflowing into a bogus pre-allocation.
This is a sub-task of [AVRO-4292](https://issues.apache.org/jira/browse/AVRO-4292) and resolves [AVRO-4295](https://issues.apache.org/jira/browse/AVRO-4295).
## Verifying this change
This change added tests and can be verified as follows:
- Extended `lang/csharp/src/apache/test/IO/BinaryCodecTests.cs` with over-limit `bytes`/`string`/`array`/`map` rejection, a non-seekable fallback, and an array of nulls that must not be falsely rejected.
- Run: `cd lang/csharp && dotnet test`
## Documentation
- Does this pull request introduce a new feature? (no — hardening / robustness)
- If yes, how is the feature documented? (not applicable)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565206500
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,53 @@ 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);
Review Comment:
ReadBytes() ultimately allocates via read(long p). Even with EnsureAvailableBytes(p), a seekable stream can still declare (and contain) more than the maximum supported .NET array length; `new byte[p]` will then throw a runtime exception (e.g., OOM/overflow) rather than a consistent AvroException. Consider rejecting lengths above MaxDotNetArrayLength before allocating, similar to ReadString().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567512174
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ Assert.AreEqual(100000, result.Length);
+ }
Review Comment:
These tests implicitly assume the default collection caps, but the implementation allows overriding them via the AVRO_MAX_COLLECTION_ITEMS environment variable. If that env var is set (e.g., in a developer shell or CI), this test can fail unexpectedly (or, for low values, reject the 100,000-element array). Consider making the test hermetic by saving/restoring AVRO_MAX_COLLECTION_ITEMS and forcing it unset for the duration of the test.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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]
+ 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<object>(writer, reader);
+ Assert.Throws<AvroException>(() => r.Read(null, new BinaryDecoder(ms)));
+ }
Review Comment:
Like TestReadArrayOfNullRejectsHugeCount, this uses a very large declared count and depends on the default MaxCollectionItems behavior. If AVRO_MAX_COLLECTION_ITEMS is set higher, the skip loop could run for a very long time; if the guard regresses, it could also trigger very large allocations. Consider unsetting AVRO_MAX_COLLECTION_ITEMS within the test and reducing the count to just above the default cap to keep the failure mode bounded.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(ms)));
+ }
Review Comment:
This test relies on the default MaxCollectionItems limit, but AVRO_MAX_COLLECTION_ITEMS can raise that cap; in that case, the decode might proceed and attempt an enormous allocation/loop. Also, using a very large count (200,000,000) makes the test potentially catastrophic if the guard regresses (OOM / long hang). Recommend (1) forcing AVRO_MAX_COLLECTION_ITEMS unset inside the test, and (2) using a count just over the default cap (10,000,001) to keep the failure mode bounded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565239771
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,53 @@ 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);
Review Comment:
Fixed in 4232c99: read(long) now rejects a length above MaxDotNetArrayLength (per TFM) with a consistent AvroException before allocating, matching ReadString(). Added a test that uses a non-seekable stream so the check is reached without gigabytes of data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3566906340
##########
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);
Review Comment:
ReadString() uses ReadInt() for the length prefix, so a declared length > int.MaxValue can overflow to a negative int, skip the EnsureAvailableBytes check, and fail with a misleading “negative length” exception. Consider reading the length as a long and validating it (including MaxDotNetArrayLength) before casting to int.
##########
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!");
Review Comment:
ReadString() reads the length prefix via ReadInt(), which casts a long to int. A declared string length > int.MaxValue will overflow to a negative int, bypass EnsureAvailableBytes, and throw the misleading “negative length” AvroException. Read the length as a long, validate bounds, then cast to int after checks so oversized lengths are rejected deterministically and the remaining-bytes guard always applies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3737487463
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +441,571 @@
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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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(10_000_001); // just over the zero-byte item cap; bounded if the guard regresses
+ ms.Position = 0;
+ var reader = new GenericReader<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(ms)));
+ }
+
+ // 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<null> 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(ms)));
+ }
+
+ // The complement: a record whose array<null> 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<object>(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
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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]
+ 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(10_000_001); // just over the zero-byte item cap; bounded if the guard regresses
+ ms.Position = 0;
+ var r = new GenericReader<object>(writer, reader);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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]
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(new MemoryStream(data))));
+ }
+ }
+
+ [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<AvroException>(() => 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<AvroException>(() => 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
+ // 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<AvroException>(() => d.ReadString());
+ StringAssert.Contains("not supported", ex.Message);
+ }
+ }
+
+ // C# has a second reader implementation, GenericDatumReader<T> (based on
+ // PreresolvingDatumReader<T>), 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<null> 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(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<object>(writer, reader);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var errors = new System.Collections.Concurrent.ConcurrentQueue<Exception>();
+ 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);
+ }
Review Comment:
Fixed in `43b262d72a`. Removed the generic `catch (Exception)` and the error queue: `Parallel.For` already aggregates any exception thrown in its body (including a spurious `AvroException` from a thread-safety regression or a failed NUnit assertion) into an `AggregateException` and rethrows it, so the test still fails if any worker throws. The CodeQL alert should now be resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565728544
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,61 @@ 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}");
+ }
+
+ 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);
return buffer;
}
+ /// <summary>
+ /// When the underlying stream can report its length, verifies that at
+ /// least <paramref name="length"/> 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.
+ /// </summary>
+ /// <param name="length">Number of bytes about to be read.</param>
+ 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");
+ }
+ }
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ /// <returns>The number of bytes remaining, or -1 if unknown.</returns>
+ public long RemainingBytes()
+ {
+ return stream.CanSeek ? stream.Length - stream.Position : -1;
+ }
Review Comment:
Fixed in 601f0ab0ae: RemainingBytes() now 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.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +511,101 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
+ private static int MinBytesPerElement(Schema schema, int depth = 0)
+ {
+ 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:
+ 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:
+ // 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;
+ }
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ private static void EnsureCollectionAvailable(Decoder d, long count, long minBytesPerElement)
+ {
+ if (count <= 0)
+ {
+ return;
+ }
+
Review Comment:
Fixed in 601f0ab0ae: EnsureCollectionAvailable now rejects a negative count explicitly (it can arise from long.MinValue overflow when negating a negative count) instead of returning early. The depth guard in MinBytesPerElement is also applied only in the record case now, so a zero-byte leaf type (null) still returns 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565150220
##########
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;
}
+ /// <summary>
+ /// When the underlying stream can report its length, verifies that at
+ /// least <paramref name="length"/> 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.
+ /// </summary>
+ /// <param name="length">Number of bytes about to be read.</param>
+ 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");
+ }
+ }
+ }
Review Comment:
Fixed in 04308d0: read(long) now rejects a negative length explicitly before allocating.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +511,71 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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;
+ }
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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");
+ }
+ }
+ }
Review Comment:
Fixed in 04308d0: EnsureCollectionAvailable now rejects a block count above int.MaxValue before the callers cast it to int, independently of minBytesPerElement (so it also covers null-element arrays). Added a test.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +511,71 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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:
Review Comment:
Fixed in 04308d0: 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 also returns 1 (not 0) now.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(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();
Review Comment:
Fixed in 04308d0: NonSeekableStream now overrides Dispose(bool) to dispose the wrapped inner stream.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3566581819
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,70 @@ 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}");
+ }
+
+ 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);
Review Comment:
`read(long p)` allocates `new byte[p]`, but array lengths in C# must be `int`. This won’t compile (and even if it did, it would be unsafe). Since the method already rejects `p > MaxDotNetArrayLength`, it’s safe to cast to int for the allocation.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +514,134 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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);
+ private static readonly long MaxCollectionStructural = ReadCollectionLimit(2147483639L);
+
+ 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;
+ }
Review Comment:
`AVRO_MAX_COLLECTION_ITEMS` is described as a cap, but `ReadCollectionLimit()` currently replaces the default with any non-negative value. If a higher value is set, `MaxCollectionStructural` can exceed `int.MaxValue`, which reintroduces overflow risk when block counts are cast to int (and contradicts the “cap” behavior). Consider treating the env var as an upper bound (min with the default).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3566951580
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +514,141 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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 <c>null</c>, 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.
+ /// </summary>
+ 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 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);
+
Review Comment:
MaxCollectionStructural is clamped to int.MaxValue, but the default reader allocates object[] via Array.Resize, whose maximum supported length is smaller than int.MaxValue on some TFMs (e.g., 0x7FFFFFC7 on modern .NET, 0x3FFFFFFF on older runtimes). As a result, a collection size that passes EnsureCollectionAvailable can still trigger a runtime exception (OutOfMemoryException/Overflow) during ResizeArray rather than a deterministic AvroException. Consider clamping the structural limit to the runtime’s max array length as well, mirroring BinaryDecoder’s MaxDotNetArrayLength behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567131541
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +533,164 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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 <c>null</c>, 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.
+ /// </summary>
+ 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;
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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.
+ if (count < 0)
+ {
+ throw new AvroException($"Invalid negative collection block count: {count}");
+ }
+
+ total += count;
Review Comment:
`total += count` can overflow `long` in unchecked arithmetic when a collection is split across blocks (e.g., first block count=1, next block count=long.MaxValue). That overflow can wrap `total` negative and bypass both the structural cap and the zero-byte item cap, allowing an oversized block count to reach the later `(int)` cast and potentially corrupt decoding / bypass allocation limits. Use checked addition or a pre-add bound check to guarantee overflow is rejected deterministically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567435614
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,49 @@ 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())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n);
+ // 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 = 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
+ // 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);
Review Comment:
Fixed — prealloc is now computed in long and clamped to MaxCollectionStructural before the int cast, so a large i near the cap can't overflow the sum.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +544,166 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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 <c>null</c>, 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.
+ /// </summary>
+ 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;
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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.
+ 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 block count {count} exceeds the maximum allowed size of {MaxCollectionStructural}");
Review Comment:
Fixed — the message now reads 'Collection size {total} + {count} exceeds ...' to reflect that the guard fires on the cumulative total, not the block count alone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565707447
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,61 @@ 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}");
+ }
+
+ 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);
return buffer;
}
+ /// <summary>
+ /// When the underlying stream can report its length, verifies that at
+ /// least <paramref name="length"/> 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.
+ /// </summary>
+ /// <param name="length">Number of bytes about to be read.</param>
+ 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");
+ }
+ }
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ /// <returns>The number of bytes remaining, or -1 if unknown.</returns>
+ public long RemainingBytes()
+ {
+ return stream.CanSeek ? stream.Length - stream.Position : -1;
+ }
Review Comment:
RemainingBytes() can return a negative value when stream.CanSeek is true but Position > Length (e.g., the stream was externally seeked past end or truncated). That violates the method’s own contract (“bytes remaining” vs -1) and also causes EnsureAvailableBytes() to skip validation (it only checks remaining >= 0), reintroducing the large-allocation risk on such streams. Clamp negative remaining to 0 so callers only ever see -1 (unknown) or a non-negative remaining count.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +511,101 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
+ private static int MinBytesPerElement(Schema schema, int depth = 0)
+ {
+ 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:
+ 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:
+ // 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;
+ }
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ private static void EnsureCollectionAvailable(Decoder d, long count, long minBytesPerElement)
+ {
+ if (count <= 0)
+ {
+ return;
+ }
+
Review Comment:
EnsureCollectionAvailable() silently returns for count < 0. Decoder.ReadArrayStart/ReadMapStart are documented to return 0 for end and >0 for a chunk size, so a negative count indicates corrupt/malicious data (and can occur via long.MinValue overflow when negating a negative block count). Returning early lets the caller cast the negative long to int, which can lead to incorrect sizing/loops. Reject negative counts explicitly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567503802
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,52 @@ 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())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n);
+ // Reject a block whose element count could not be backed by the
Review Comment:
Good catch — this is a real gap specific to C#'s architecture: `PreresolvingDatumReader<T>` (base of SpecificDatumReader<T>/GenericDatumReader<T>) is a separate reader implementation from the DefaultReader/GenericReader path hardened in this PR, so it doesn't inherit these checks (unlike Java, where SpecificDatumReader extends the hardened GenericDatumReader). Hardening it means sharing the limit logic, threading element min-bytes into ReadArray/ReadMap, clamping EnsureSize preallocation, and adding tests on the specific-reader path — a distinct change with its own regression risk on a widely-used path. Filed as AVRO-4306 (https://issues.apache.org/jira/browse/AVRO-4306) to do it properly with dedicated tests rather than bolt it onto this PR late in review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567462453
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,52 @@ 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())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n);
+ // Reject a block whose element count could not be backed by the
Review Comment:
The new collection hardening here (MinBytesPerElement/EnsureCollectionAvailable + bounded preallocation/growth) only applies to DefaultReader/GenericReader. The main datum-reader path used by GenericDatumReader<T>/SpecificDatumReader<T> (via PreresolvingDatumReader<T>) still reads array/map blocks with `(int)decoder.ReadArrayStart()` / `(int)decoder.ReadMapStart()` and then preallocates/loops based on that count without any RemainingBytes/MaxCollection* checks (see PreresolvingDatumReader.cs:374-399 and its skip lambdas at 487-505). That leaves the original allocation/CPU DoS vector (especially for zero-byte elements like `null`) when callers use those readers, and it also means the new tests in BinaryCodecTests (which use GenericReader) won’t cover the common GenericDatumReader/SpecificDatumReader code path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567638028
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +441,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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
Review Comment:
Using a declared count of 200,000,000 makes this test potentially catastrophic (very long run / OOM) if the collection-size guard regresses in the future, since an array<null> consumes no input and the loop could attempt to allocate/grow toward that size. Prefer a count just above the default zero-byte item cap (10,000,000) to keep the failure mode bounded while still exercising the rejection path.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +441,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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]
+ 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
Review Comment:
As with the array<null> decode test, declaring a 200,000,000-element block here can hang or OOM the test process if the skip-path size guard regresses, because skipping null elements consumes no input and the loop could iterate toward that count. Prefer a count just above the default cap (10,000,000) to keep the test's failure mode bounded while still verifying rejection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565068925
##########
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;
}
+ /// <summary>
+ /// When the underlying stream can report its length, verifies that at
+ /// least <paramref name="length"/> 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.
+ /// </summary>
+ /// <param name="length">Number of bytes about to be read.</param>
+ 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");
+ }
+ }
+ }
Review Comment:
EnsureAvailableBytes() only runs when length > 0, so a negative length (e.g., from ReadBytes()) bypasses validation and can flow into array allocation, resulting in runtime exceptions unrelated to Avro input validation. It should reject negative lengths explicitly (even if ReadString already guards its own length).
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +511,71 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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;
+ }
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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");
+ }
+ }
+ }
Review Comment:
EnsureCollectionAvailable() returns early when minBytesPerElement <= 0 (e.g., arrays of null), but ReadArray/ReadMap still cast the raw long block count to int. A crafted block count > int.MaxValue will overflow that cast and can lead to negative sizes/incorrect allocations or infinite/incorrect loops. Add an explicit int.MaxValue guard on the raw long count before any casts (independent of minBytesPerElement).
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +511,71 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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:
Review Comment:
MinBytesPerElement() sums record field minima into an int, which can overflow and become <= 0. Because EnsureCollectionAvailable() treats minBytesPerElement <= 0 as “skip checks”, an attacker-controlled schema with enough fixed/record nesting could bypass the block-size validation. Use a long accumulator and clamp to int.MaxValue to avoid overflow-induced bypass.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(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();
Review Comment:
NonSeekableStream is used in a using block, but it doesn't dispose the wrapped inner stream. Overriding Dispose(bool) to dispose the inner stream avoids leaking resources (and matches typical wrapper-stream behavior).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567589054
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -38,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);
+ }
Review Comment:
You're right — the limits are `static readonly`, computed once at class load, so the runtime [SetUp] clearing had no effect. I've reverted the ineffective [SetUp]/[TearDown] and documented that these tests assume the process wasn't started with a custom AVRO_MAX_COLLECTION_ITEMS (the normal case). Making the tests fully override-independent would require the caps to be re-readable rather than static readonly (a production change), which is out of scope here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567324589
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +533,164 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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 <c>null</c>, 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.
+ /// </summary>
+ 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;
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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.
+ if (count < 0)
+ {
+ throw new AvroException($"Invalid negative collection block count: {count}");
+ }
+
+ total += count;
Review Comment:
Good catch — real overflow. Fixed in 345a803a86: `EnsureCollectionAvailable` now checks `count > MaxCollectionStructural - total` *before* adding, rejecting the overflow (and any over-cap count) deterministically with an AvroException. The running total is always <= MaxCollectionStructural on entry and count >= 0, so the subtraction can't underflow/overflow. Added a non-seekable-stream regression test (block1 count=1, block2 count=long.MaxValue).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567358924
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,38 @@ 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())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n);
+ // 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 = 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
+ // 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);
Review Comment:
Fixed in the latest commit — the growth is now computed in `long` (avoiding int overflow) and clamped to `MaxCollectionStructural` (which is <= the runtime max array length). Since the validated element count never exceeds that cap, the clamp can't starve a legitimate collection, and `Array.Resize` is never handed an over-large/negative size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567559889
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -38,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);
+ }
Review Comment:
Clearing AVRO_MAX_COLLECTION_ITEMS in [SetUp] may not reliably make these tests deterministic because GenericReader caches the env var into static readonly fields (MaxCollectionItems/MaxCollectionStructural) at first type initialization. If any other test fixture touches GenericReader before this fixture runs, these limits will already be fixed and changing the env var here won’t affect behavior, reintroducing flakiness when the env var is set in CI/shell. Consider moving the env-var override into a test-assembly/namespace [SetUpFixture] that runs before any tests (or otherwise ensuring GenericReader is initialized only after unsetting the variable).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3568052481
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +441,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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
Review Comment:
Reduced to 10,000,001 (just over the default zero-byte item cap) so the rejection is still exercised but a future guard regression stays bounded.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +441,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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]
+ 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
Review Comment:
Reduced the skip test to 10,000,001 for the same bounded-failure reason.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ Assert.AreEqual(100000, result.Length);
+ }
Review Comment:
The count here is 100,000 (well under the default cap) exercising a normal large array; a lowered AVRO_MAX_COLLECTION_ITEMS could reject it, but this fixture documents that it assumes default limits. The catastrophic-regression concern applies to the null tests, now reduced to 10,000,001.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(ms)));
+ }
Review Comment:
Reduced to 10,000,001 to keep the failure mode bounded. Full hermetic env isolation isn't reliably possible because GenericReader captures the limits into static readonly fields at type-init (see SetUp note).
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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]
+ 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<object>(writer, reader);
+ Assert.Throws<AvroException>(() => r.Read(null, new BinaryDecoder(ms)));
+ }
Review Comment:
Reduced the skip test to 10,000,001; same static-readonly caching caveat as above.
##########
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;
Review Comment:
Already handled — ReadLong rejects a 10th byte whose payload exceeds bit 63 via `shift == 63 && (b & 0x7E) != 0` (BinaryDecoder.cs:90).
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,49 @@ 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())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n);
+ // 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 = 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
+ // 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);
Review Comment:
Already handled — prealloc is computed in long (`Math.Min((long)i + Math.Min(n, MaxCollectionPrealloc), MaxCollectionStructural)`) then cast to int (GenericReader.cs:425).
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +533,164 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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 <c>null</c>, 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.
+ /// </summary>
+ 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;
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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.
+ if (count < 0)
+ {
+ throw new AvroException($"Invalid negative collection block count: {count}");
+ }
+
+ total += count;
Review Comment:
Already handled — the pre-add bound check `count > MaxCollectionStructural - total` rejects before adding, so total can't overflow (GenericReader.cs:679).
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,38 @@ 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())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n);
+ // 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 = 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
+ // 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);
Review Comment:
Already handled — the geometric grow clamps to MaxCollectionStructural (which is itself Math.Min'd with MaxDotNetArrayLength) before ResizeArray (GenericReader.cs:445-450, 633).
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +514,134 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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);
+ private static readonly long MaxCollectionStructural = ReadCollectionLimit(2147483639L);
+
+ 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;
+ }
Review Comment:
Addressed — MaxCollectionStructural now clamps ReadCollectionLimit with Math.Min(..., MaxDotNetArrayLength), so even a higher AVRO_MAX_COLLECTION_ITEMS can't push the structural cap past the runtime array limit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567395792
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +544,166 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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 <c>null</c>, 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.
+ /// </summary>
+ 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;
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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.
+ 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 block count {count} exceeds the maximum allowed size of {MaxCollectionStructural}");
Review Comment:
The structural-cap exception message says the *block count* exceeds the maximum allowed size, but the guard is actually for `total + count` exceeding the cap. This can produce misleading errors when `count` is valid on its own but would overflow the remaining capacity.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,49 @@ 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())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n);
+ // 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 = 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
+ // 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);
Review Comment:
`prealloc` is computed using `int` arithmetic (`i + Math.Min(n, MaxCollectionPrealloc)`), which can overflow (wrapping or throwing under checked builds) when `i` is near the structural cap. Compute the sum in `long`, clamp to `MaxCollectionStructural`, then cast to `int` before resizing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567401034
##########
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;
Review Comment:
Fixed — when `shift == 63` (the 10th byte) ReadLong now rejects the input if `(b & 0x7E) != 0`, since those higher payload bits would be dropped by `<< 63` and silently decode a malformed varint to a wrong value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567335620
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,38 @@ 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())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n);
+ // 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 = 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
+ // 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);
Review Comment:
The dynamic growth path in ReadArray can resize the backing array beyond the enforced collection limits. `grown = current + (current >> 1) + 1` can overshoot the allowed element count (e.g., zero-byte arrays capped by MaxCollectionItems) and can even exceed MaxDotNetArrayLength/MaxCollectionStructural, causing Array.Resize to throw a runtime exception despite the declared count being within limits. Clamp `grown` to the applicable cap before resizing so allocations stay within the deterministic AvroException bounds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3566787638
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +514,139 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
Review Comment:
MinBytesPerElement() can return 0 not only for `null` but also for composite schemas that encode to zero bytes (e.g., records made entirely of zero-byte fields). The summary currently implies only `null` can be zero-byte, which is misleading given the behavior and the later zero-byte collection cap logic.
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,72 @@ public void SkipFixed(int len)
// Read p bytes into a new byte buffer
private byte[] read(long p)
{
- byte[] buffer = new byte[p];
+ if (p < 0)
+ {
+ 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");
Review Comment:
The inline comment says this guard "Matches ReadString()", but the thrown message differs from ReadString’s max-length error (and this path is for bytes allocations, not strings). Either align the wording or adjust the comment so it doesn’t claim an exact match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia closed pull request #3860: AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections
URL: https://github.com/apache/avro/pull/3860
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567378836
##########
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;
Review Comment:
ReadLong() now rejects varints that exceed 10 bytes, but it still accepts some malformed 10-byte encodings where the 10th byte has more than 1 significant bit set. Because the shift is 63 on the 10th byte, `(b & 0x7F) << 63` will silently drop bits above bit 63, producing an incorrect decoded value rather than rejecting the input. Add a guard so when `shift == 63` the next byte’s payload is only 0 or 1 (i.e., `(b & 0x7E) == 0`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565199659
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -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())
Review Comment:
Fixed in 7376905: both the array and map minima are computed as long (1L + ...) and EnsureCollectionAvailable takes a long, so 1 + int.MaxValue no longer overflows to a negative value that would disable the map check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3566941832
##########
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!");
Review Comment:
Fixed in d308962430: ReadString() now reads the length as a long, applies the remaining-bytes guard and the max-array-length check, then casts to int. A length above int.MaxValue no longer overflows ReadInt() to a negative int and throws a misleading 'negative length'.
##########
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);
Review Comment:
Fixed in d308962430: same change to the non-netstandard2.0 decoder (read length as long, validate, then cast to int). Added a regression test via a non-seekable stream so the length check is reached.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on PR #3860:
URL: https://github.com/apache/avro/pull/3860#issuecomment-5219775463
Thanks for the review. The latest pass reported no blocking comments; I addressed the two suppressed documentation notes in `c5e4306b88`:
- The collection-limit test `NOTE` now refers to `CollectionBounds` (the shared type that captures `AVRO_MAX_COLLECTION_ITEMS` at class load) instead of the outdated `GenericReader`.
- The fragmented comment on `TestReadArrayHugeCountOnStreamClampsPreallocation` is rewritten as a complete sentence that explicitly names the non-seekable-stream case.
No functional changes. Re-requesting a review to confirm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3737303587
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +441,571 @@
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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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(10_000_001); // just over the zero-byte item cap; bounded if the guard regresses
+ ms.Position = 0;
+ var reader = new GenericReader<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(ms)));
+ }
+
+ // 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<null> 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(ms)));
+ }
+
+ // The complement: a record whose array<null> 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<object>(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
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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]
+ 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(10_000_001); // just over the zero-byte item cap; bounded if the guard regresses
+ ms.Position = 0;
+ var r = new GenericReader<object>(writer, reader);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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]
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(new MemoryStream(data))));
+ }
+ }
+
+ [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<AvroException>(() => 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<AvroException>(() => 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
+ // 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<AvroException>(() => d.ReadString());
+ StringAssert.Contains("not supported", ex.Message);
+ }
+ }
+
+ // C# has a second reader implementation, GenericDatumReader<T> (based on
+ // PreresolvingDatumReader<T>), 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<null> 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(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<object>(writer, reader);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var errors = new System.Collections.Concurrent.ConcurrentQueue<Exception>();
+ 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);
+ }
Review Comment:
## CodeQL / Generic catch clause
Generic catch clause.
[Show more details](https://github.com/apache/avro/security/code-scanning/3372)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values [avro]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565162311
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -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())
Review Comment:
`minBytes` for maps is computed as `1 + MinBytesPerElement(...)`. Since `MinBytesPerElement` can intentionally clamp to `int.MaxValue`, this addition can overflow to a negative value in unchecked arithmetic, which then makes `minBytesPerElement <= 0` and silently disables the remaining-bytes validation for maps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567549580
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ Assert.AreEqual(100000, result.Length);
+ }
Review Comment:
Fixed — added [SetUp]/[TearDown] that clear and restore AVRO_MAX_COLLECTION_ITEMS for every test in the fixture, so this test runs against the default cap regardless of the ambient environment.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(ms)));
+ }
Review Comment:
Addressed by the same [SetUp] clearing AVRO_MAX_COLLECTION_ITEMS, so the default 10M cap applies. The huge count is rejected by EnsureCollectionAvailable before any allocation/loop (it's a zero-byte array bounded by the item cap), so it isn't catastrophic even if it ran — but the env isolation removes the raise-the-cap flakiness.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,330 @@ 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<AvroException>(() => 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ 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<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<AvroException>(() => d.ReadBytes());
+ }
+ }
+
+ // 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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<object>(schema, schema);
+ Assert.Throws<AvroException>(() => 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]
+ 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<object>(writer, reader);
+ Assert.Throws<AvroException>(() => r.Read(null, new BinaryDecoder(ms)));
+ }
Review Comment:
Same fix — the [SetUp] clears AVRO_MAX_COLLECTION_ITEMS so the skip test is deterministic; the skip loop is bounded by the cap (rejected before iterating) under the default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3566776604
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,70 @@ 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}");
+ }
+
+ 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);
Review Comment:
Fixed in 3671989ce5: read(long p) allocates `new byte[(int)p]`; p is already bounded to <= MaxDotNetArrayLength above, so the cast can't overflow.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +514,134 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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);
+ private static readonly long MaxCollectionStructural = ReadCollectionLimit(2147483639L);
+
+ 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;
+ }
Review Comment:
Fixed in 3671989ce5: MaxCollectionStructural is now clamped to int.MaxValue, so a large AVRO_MAX_COLLECTION_ITEMS override can't push it past the int range the callers cast to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3566893585
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,72 @@ public void SkipFixed(int len)
// Read p bytes into a new byte buffer
private byte[] read(long p)
{
- byte[] buffer = new byte[p];
+ if (p < 0)
+ {
+ 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");
Review Comment:
Fixed in 1af6c0a84f: reworded the comment to say the guard 'mirrors' ReadString()'s max-length guard and notes the message differs, rather than claiming an exact match.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +514,139 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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.
+ /// </summary>
Review Comment:
Fixed in 1af6c0a84f: the summary now notes 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3566979852
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +514,141 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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 <c>null</c>, 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.
+ /// </summary>
+ 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 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);
+
Review Comment:
Fixed in cf8a68752d. Added a per-TFM `MaxDotNetArrayLength` constant to GenericReader (0x7FFFFFC7 on modern .NET, 0x3FFFFFFF on netstandard2.0), mirroring BinaryDecoder, and clamped `MaxCollectionStructural` to it instead of `int.MaxValue` so a size passing EnsureCollectionAvailable can no longer fault inside Array.Resize.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3568053538
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,52 @@ 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())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n);
+ // Reject a block whose element count could not be backed by the
Review Comment:
Correct — PreresolvingDatumReader<T> (the GenericDatumReader<T>/SpecificDatumReader<T> path) is not hardened by this PR. Tracked as a dedicated follow-up: AVRO-4306 ("Harden PreresolvingDatumReader collection allocation"). This PR intentionally scopes the DefaultReader/GenericReader path; AVRO-4306 will apply the same MinBytesPerElement/EnsureCollectionAvailable + bounded prealloc/grow to the preresolving path and add tests over GenericDatumReader/SpecificDatumReader.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +544,166 @@ protected virtual object ReadMap(object reuse, MapSchema writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// 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 <c>null</c>, 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.
+ /// </summary>
+ 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;
+ }
+
+ /// <summary>
+ /// 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.
+ /// </summary>
+ 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.
+ 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 block count {count} exceeds the maximum allowed size of {MaxCollectionStructural}");
Review Comment:
The message includes both operands — `Collection size {total} + {count} exceeds the maximum allowed size of {MaxCollectionStructural}` — so it's clear it's the cumulative total plus this block's count that exceeds the cap, not the block count alone (GenericReader.cs:682).
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -38,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);
+ }
Review Comment:
Acknowledged — the limits are static readonly captured at type-init, so a [SetUp] env change can't reliably reset them if another fixture initialized GenericReader first. Rather than add a [SetUpFixture] ordering dependency, these tests are written to not depend on the env var: they use the default caps and the huge-count tests are now reduced to 10,000,001 so the failure mode stays bounded even if CI sets AVRO_MAX_COLLECTION_ITEMS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]
Posted by "iemejia (via GitHub)" <gi...@apache.org>.
iemejia commented on PR #3860:
URL: https://github.com/apache/avro/pull/3860#issuecomment-5219882644
Addressed the suppressed note on `PreresolvingDatumReader.ReadArray` in `57aae1f5a2`: the geometric growth now adds the current `chunk` instead of the fixed `MaxCollectionPrealloc`, so a small array is no longer over-allocated to 1024 and shrunk again. Large arrays keep the same bounded ~1.5x growth (there `chunk == MaxCollectionPrealloc`). Re-requesting a review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org