Skip to content

AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections#3860

Open
iemejia wants to merge 21 commits into
apache:mainfrom
iemejia:AVRO-4295-csharp-available-bytes
Open

AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections#3860
iemejia wants to merge 21 commits into
apache:mainfrom
iemejia:AVRO-4295-csharp-available-bytes

Conversation

@iemejia

@iemejia iemejia commented Jul 11, 2026

Copy link
Copy Markdown
Member

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 to the C# SDK and extends it to collections. It has two complementary parts.

1. Validate available bytes before allocating

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.

2. Cap collection allocation for zero-byte elements

Zero-byte elements (null, or a record with only zero-byte fields) consume no input, so the available-bytes check cannot bound their count: a tiny payload such as {{"type":"array","items":"null"}} declaring a block count of 200,000,000 would otherwise drive an unbounded allocation. In addition to the available-bytes check, EnsureCollectionAvailable tracks the cumulative count across blocks and applies a structural cap to every collection (MaxCollectionStructural = Math.Min(int.MaxValue - 8, MaxDotNetArrayLength) — i.e. the runtime's maximum array length, which on every target is at or below int.MaxValue - 8 — also covering non-seekable decoders and keeping the total within the int range) and a zero-byte item cap (MaxCollectionItems = 10,000,000). The reader array/map loops and the schema-resolution Skip path for arrays and maps are all bounded the same way, so skipping a huge zero-byte block cannot loop unboundedly. When set, the AVRO_MAX_COLLECTION_ITEMS environment variable caps both limits.

This is a sub-task of AVRO-4292 and resolves 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, an array<null> huge-count rejection, a small array<null> that still decodes, and the skip path bounded under schema resolution.
  • Run: cd lang/csharp && dotnet test (net8.0: passing).

Documentation

  • Does this pull request introduce a new feature? (no — hardening / robustness)
  • If yes, how is the feature documented? (not applicable)

…ngth-prefixed values and collections

A bytes or string value is a length prefix followed by that many bytes, and an
array or map block is an element count followed by that many items. A malicious
or truncated input can declare a huge length or count with little or no data.

- BinaryDecoder.RemainingBytes() reports the bytes still readable for a seekable
  stream (or -1). ReadBytes and both ReadString implementations reject an
  over-large declared length before allocating.
- DefaultReader.ReadArray/ReadMap reject a block whose element count could not be
  backed by the bytes remaining, using MinBytesPerElement() computed from the
  element schema so a zero-byte element type (e.g. null) is not falsely rejected.
  The count is checked on the raw long before the int cast, which also avoids
  the cast overflowing into a bogus pre-allocation.

Mirrors the Java SDK's checks (AVRO-4241). Non-seekable streams and non-binary
decoders are unaffected.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens the C# Avro binary decoding path against malicious or truncated inputs that declare large length-prefixed values or collection block counts by validating available bytes before allocating, when the underlying stream can report remaining length.

Changes:

  • Added BinaryDecoder.RemainingBytes() and EnsureAvailableBytes() and applied the check before allocating for strings (and via the shared read() path).
  • Added pre-allocation validation for array/map blocks in GenericReader using a computed MinBytesPerElement() heuristic.
  • Extended BinaryCodecTests to cover over-limit bytes/string/array/map cases, plus a non-seekable fallback and a “null elements” non-false-positive case.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs Adds regression tests for length/count validation and introduces a non-seekable stream wrapper for coverage.
lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs Adds available-bytes validation before string buffer allocation in the non-netstandard2.0 implementation.
lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs Adds available-bytes validation before string allocation in the netstandard2.0 implementation.
lang/csharp/src/apache/main/IO/BinaryDecoder.cs Adds RemainingBytes() plus centralized EnsureAvailableBytes() and applies it in the shared read() allocation path.
lang/csharp/src/apache/main/Generic/GenericReader.cs Adds block count validation for arrays/maps using per-element minimum on-wire sizes to prevent over-allocation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +283 to +294
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");
}
}
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 04308d0: read(long) now rejects a negative length explicitly before allocating.

Comment on lines +561 to +577
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");
}
}
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +539 to +548
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:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +551 to +555
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();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 04308d0: NonSeekableStream now overrides Dispose(bool) to dispose the wrapped inner stream.

…harden min-bytes sum

Review feedback:
- read(long) now rejects a negative length explicitly instead of letting it flow
  into a negative array allocation.
- EnsureCollectionAvailable rejects a block count above int.MaxValue before the
  callers cast it to int, independently of the per-element size (so it also
  covers null-element arrays where the byte check is skipped).
- MinBytesPerElement accumulates record field minima in a long and clamps to
  int.MaxValue, so deep nesting cannot overflow int into a value <= 0 that would
  disable the check; the depth guard now returns 1 (not 0) for the same reason.
- The NonSeekableStream test helper disposes its wrapped inner stream.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment on lines +499 to +501
// 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())

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

…erflow

Review feedback: the map's minBytes was computed as 1 + MinBytesPerElement(...).
Since MinBytesPerElement clamps to int.MaxValue, adding 1 in int arithmetic
overflows to a negative value, which makes minBytesPerElement <= 0 and silently
disables the remaining-bytes validation for maps. Compute both the array and map
minima as long (1L + ...) and take a long in EnsureCollectionAvailable.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment on lines +272 to 274
EnsureAvailableBytes(p);
byte[] buffer = new byte[p];
Read(buffer, 0, buffer.Length);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Review feedback: ReadBytes() -> read(long) allocates new byte[p]. A seekable
stream can declare (and hold) more than the maximum .NET array length, so
new byte[p] would throw an OverflowException/OutOfMemoryException instead of a
consistent AvroException. Reject a length above MaxDotNetArrayLength first, as
ReadString() already does. Added a test using a non-seekable stream.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment on lines +317 to +320
public long RemainingBytes()
{
return stream.CanSeek ? stream.Length - stream.Position : -1;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 601f0ab: 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.

Comment on lines +578 to +582
if (count <= 0)
{
return;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 601f0ab: 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.

…p null returns 0

Review feedback:
- RemainingBytes() clamps a negative result (Position past Length on a truncated
  or externally seeked stream) to 0, so callers only ever see -1 (unknown) or a
  non-negative count and the check is not skipped.
- EnsureCollectionAvailable rejects a negative block count explicitly (it can
  arise from long.MinValue overflow when negating a negative count) instead of
  returning early and letting the caller cast it to int.
- MinBytesPerElement applies the depth guard only in the record case, so a
  zero-byte leaf type (null) nested under deep records still returns 0.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Completes the available-bytes protection for collections. Elements whose schema
encodes to zero bytes (null, a zero-length fixed, or a record with only
zero-byte fields) consume no input, so the bytes-remaining check cannot bound
their count. A tiny payload declaring a huge array block count of such elements
(e.g. {"type":"array","items":"null"} with a count of 200,000,000) therefore
drove an unbounded ResizeArray and exhausted memory.

EnsureCollectionAvailable now tracks the cumulative count across blocks and
enforces, per block: a structural cap on all collections
(MaxCollectionStructural = Integer.MAX_VALUE - 8, also covering non-seekable
decoders and keeping the total within the int range the callers cast to); a
zero-byte item cap (MaxCollectionItems = 10,000,000) when the per-element
minimum is zero; and the existing bytes-remaining check otherwise.
AVRO_MAX_COLLECTION_ITEMS, when set, caps both. The reader array/map loops and
the schema-resolution Skip path for arrays and maps are all bounded the same
way, so skipping a huge zero-byte block cannot loop unboundedly.

Assisted-by: GitHub Copilot:claude-opus-4.8
@iemejia iemejia changed the title AVRO-4295: [csharp] Validate available bytes before allocating for length-prefixed values AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections Jul 12, 2026
@iemejia iemejia requested a review from Copilot July 12, 2026 15:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment on lines +279 to 282

EnsureAvailableBytes(p);
byte[] buffer = new byte[p];
Read(buffer, 0, buffer.Length);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3671989: read(long p) allocates new byte[(int)p]; p is already bounded to <= MaxDotNetArrayLength above, so the cast can't overflow.

Comment on lines +575 to +591
// 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;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3671989: 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

iemejia added 2 commits July 12, 2026 18:16
A negative block count is normalized by negating it (result = -result), but
long.MinValue cannot be negated: under unchecked arithmetic it wraps back to a
negative value, and under checked arithmetic it throws OverflowException. The
zig-zag encoding of long.MinValue is a valid 10-byte varint, so this is
reachable from malformed input.

Reject long.MinValue explicitly in doReadItemCount() with a clear AvroException,
consistent with the C and C++ bindings. Adds tests for a long.MinValue block
count on ReadArrayStart() and ReadMapStart().

Assisted-by: GitHub Copilot:claude-opus-4.8
Addresses review feedback:
 - read(long p) now allocates new byte[(int)p]; p is already bounded to
   <= MaxDotNetArrayLength above, so the cast required for array allocation
   cannot overflow.
 - MaxCollectionStructural is clamped to int.MaxValue. The callers cast the
   cumulative block count to int to size .NET collections, so a structural limit
   above int.MaxValue (e.g. from a large AVRO_MAX_COLLECTION_ITEMS override)
   would otherwise reintroduce an int-overflow on that cast.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comment on lines +407 to +411
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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — this is a real gap specific to C#'s architecture: PreresolvingDatumReader<T> (base of SpecificDatumReader/GenericDatumReader) 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct — PreresolvingDatumReader (the GenericDatumReader/SpecificDatumReader 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Comment on lines +523 to +535
[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);
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +584 to +593
[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)));
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +617 to +632
[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)));
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reduced the skip test to 10,000,001; same static-readonly caching caveat as above.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

…_ITEMS

The collection-limit tests assume the default caps, but AVRO_MAX_COLLECTION_ITEMS
overrides them, so a value set in a shell/CI could make them flaky (fail, or
reject a legitimate array). Add [SetUp]/[TearDown] that clear the env var for
each test and restore it afterward, so they run deterministically against the
defaults.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comment on lines +48 to +53
[SetUp]
public void ClearCollectionItemsEnv()
{
_previousMaxCollectionItems = Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS");
Environment.SetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS", null);
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

GenericReader captures AVRO_MAX_COLLECTION_ITEMS into static readonly fields at
class load, so clearing the env var at runtime in [SetUp] had no effect on the
already-computed limits. Remove the misleading [SetUp]/[TearDown] and document
that the collection-limit tests assume the process was not started with a custom
AVRO_MAX_COLLECTION_ITEMS.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

{
var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}");
var ms = new MemoryStream();
new BinaryEncoder(ms).WriteLong(200_000_000); // ~4 byte payload

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"{\"type\":\"record\",\"name\":\"Foo\",\"fields\":[" +
"{\"name\":\"val\",\"type\":\"int\"}]}");
var ms = new MemoryStream();
new BinaryEncoder(ms).WriteLong(200_000_000); // arr block count; skipped

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reduced the skip test to 10,000,001 for the same bounded-failure reason.

…em cap

The array<null> decode and skip tests declared 200,000,000 elements. Since a
zero-byte element consumes no input, a future regression of the item-cap guard
could let those loops run toward that count and hang/OOM the test process.
Use 10,000,001 (just over the default 10,000,000 zero-byte item cap) so the
rejection path is still exercised but the failure mode stays bounded.

Assisted-by: GitHub Copilot:claude-opus-4.8

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants