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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
