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