AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections - #3860
Open
iemejia wants to merge 26 commits into
Open
AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections#3860iemejia wants to merge 26 commits into
iemejia wants to merge 26 commits into
Conversation
…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
There was a problem hiding this comment.
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()andEnsureAvailableBytes()and applied the check before allocating for strings (and via the sharedread()path). - Added pre-allocation validation for array/map blocks in
GenericReaderusing a computedMinBytesPerElement()heuristic. - Extended
BinaryCodecTeststo 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.
…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
…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
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
…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
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
changed the title
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
…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
…t per collection The zero-byte-element item cap (array<null>-style elements that consume no input, which the bytes-remaining check cannot bound) was enforced per collection: ReadArray/ReadMap and Skip each started their running total from zero. Because a container file carries its own schema, an attacker can declare a record with many such collection fields, each block individually under the limit but jointly unbounded, so a tiny payload still drives a huge aggregate allocation. Track the cumulative zero-byte element count on the DefaultReader instance (zeroByteItemsRead), reset at the start of each top-level Read<T>, and check it in EnsureCollectionAvailable (now an instance method). The structural and bytes-remaining checks stay per collection. Adds regression tests for a multi-field record rejected cumulatively and a within-limit record that still decodes (resetting between datums).
Commit ed310ad hardened only DefaultReader (used by GenericReader<T>). The other public readers -- GenericDatumReader<T>/SpecificDatumReader<T> (via PreresolvingDatumReader<T>), SpecificDefaultReader, and ReflectDefaultReader -- read array/map blocks without any bound, so a tiny payload could still decode a huge collection (e.g. 100,000,000 zero-byte elements from a few bytes) or preallocate an oversized array. Extract the shared guards (min-bytes-per-element, structural/item caps, and EnsureCollectionAvailable) into an internal CollectionBounds helper so the caps cannot drift between the reader implementations, and wire every reader's ReadArray/ReadMap/skip paths through it. The block count is read as a long and validated before the int cast, and PreresolvingDatumReader grows its backing array in bounded, geometric chunks so a huge count on a non-seekable stream cannot preallocate the whole block up front. The per-datum zero-byte-element budget is tracked in a thread-static nesting scope (mirroring the Java fix) rather than on the reader instance, preserving PreresolvingDatumReader's documented thread-sharing contract; nested scopes accumulate into the enclosing datum and only the outermost resets. Adds regression tests for the Generic, Specific, and Reflect reader paths, including a concurrency test for a shared reader.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:44
- This note says
GenericReadercapturesAVRO_MAX_COLLECTION_ITEMSat type init, but the caps are now read and stored inCollectionBounds(which all readers share). Referring to the right type will avoid confusion for future maintainers.
// NOTE: the collection-limit tests assume the default caps. GenericReader
// captures AVRO_MAX_COLLECTION_ITEMS into static readonly fields at class
// load, so the value is fixed for the process; these tests therefore
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:654
- This comment starts mid-sentence ("count bounded by the bytes remaining"), which reads like a fragment and is confusing. Consider rephrasing to explicitly mention the non-seekable-stream case.
// 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.
Address two Copilot review notes: the collection-limit test NOTE now refers to CollectionBounds (which captures AVRO_MAX_COLLECTION_ITEMS) instead of the outdated GenericReader, and the fragmented comment on TestReadArrayHugeCountOnStreamClampsPreallocation is rewritten as a complete sentence that names the non-seekable-stream case.
Member
Author
|
Thanks for the review. The latest pass reported no blocking comments; I addressed the two suppressed documentation notes in
No functional changes. Re-requesting a review to confirm. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lang/csharp/src/apache/main/Generic/PreresolvingDatumReader.cs:436
- In PreresolvingDatumReader.ReadArray, the geometric growth uses
+ CollectionBounds.MaxCollectionPreallocrather than the actualchunksize. For small arrays (e.g., 3 items), this forces an immediate resize to 1024, then a later shrink viaResize(ref array, i), causing unnecessary allocations/copies on the hot path. Usingchunkkeeps the same bounded-growth behavior for large arrays (where chunk==MaxCollectionPrealloc) without over-allocating small arrays.
// Grow ~1.5x (amortized O(n), so a legitimate large array
// is not resized on every chunk) plus one chunk, then
// clamp to the structural cap (which is <= the runtime's
// max array length). The validated element count never
// exceeds that cap.
long grown = (long)capacity + (capacity >> 1) + CollectionBounds.MaxCollectionPrealloc;
…prealloc bound In PreresolvingDatumReader.ReadArray the geometric growth added the fixed MaxCollectionPrealloc (1024) instead of the actual chunk size, so a small array (e.g. 3 items) was resized to 1024 up front and then shrunk again at the end. Adding the current chunk keeps the bounded ~1.5x growth for large arrays (where chunk == MaxCollectionPrealloc) while avoiding the needless over-allocation and copy for small arrays.
Member
Author
|
Addressed the suppressed note on |
CodeQL flagged the generic `catch (Exception)` in the shared-reader concurrency test. Parallel.For already aggregates any exception thrown in its body (a spurious AvroException from a thread-safety regression, or a failed assertion) into an AggregateException and rethrows it, so the try/catch and the error queue were redundant. Remove them; the test still fails if any worker throws.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
A
bytesorstringvalue is encoded as a length prefix followed by that many bytes of data, and anarrayormapblock 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/ReadStringreject an over-large declared length, andDefaultReader.ReadArray/ReadMapreject a block whose element count could not be backed by the bytes remaining, computingMinBytesPerElement()from the element schema. The count is checked on the rawlongbefore theintcast, 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,EnsureCollectionAvailabletracks 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 belowint.MaxValue - 8— also covering non-seekable decoders and keeping the total within theintrange) and a zero-byte item cap (MaxCollectionItems= 10,000,000). The reader array/map loops and the schema-resolutionSkippath for arrays and maps are all bounded the same way, so skipping a huge zero-byte block cannot loop unboundedly. When set, theAVRO_MAX_COLLECTION_ITEMSenvironment 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:
lang/csharp/src/apache/test/IO/BinaryCodecTests.cswith over-limitbytes/string/array/maprejection, a non-seekable fallback, anarray<null>huge-count rejection, a smallarray<null>that still decodes, and the skip path bounded under schema resolution.cd lang/csharp && dotnet test(net8.0: passing).Documentation