Re: [PR] AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections [avro]

["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