Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565707447
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,61 @@ 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);
return buffer;
}
+ /// <summary>
+ /// When the underlying stream can report its length, verifies that at
+ /// least <paramref name="length"/> bytes remain before the caller
+ /// allocates a buffer of that size. This guards against an
+ /// out-of-memory attack from a malicious or truncated input that
+ /// declares a huge length prefix but carries little actual data. The
+ /// check is skipped for non-seekable streams, whose remaining length
is
+ /// unknown.
+ /// </summary>
+ /// <param name="length">Number of bytes about to be read.</param>
+ 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");
+ }
+ }
+ }
+
+ /// <summary>
+ /// Returns the number of bytes still available to read from the
+ /// underlying stream when it is seekable, or -1 when that count is not
+ /// known (a non-seekable stream). Used to reject a declared length or
a
+ /// collection block count that exceeds the data actually available
+ /// before allocating for it.
+ /// </summary>
+ /// <returns>The number of bytes remaining, or -1 if unknown.</returns>
+ public long RemainingBytes()
+ {
+ return stream.CanSeek ? stream.Length - stream.Position : -1;
+ }
Review Comment:
RemainingBytes() can return a negative value when stream.CanSeek is true but
Position > Length (e.g., the stream was externally seeked past end or
truncated). That violates the method’s own contract (“bytes remaining” vs -1)
and also causes EnsureAvailableBytes() to skip validation (it only checks
remaining >= 0), reintroducing the large-allocation risk on such streams. Clamp
negative remaining to 0 so callers only ever see -1 (unknown) or a non-negative
remaining count.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +511,101 @@ 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;
+ }
+
+ if (depth > 64)
+ {
+ // A cyclic or pathologically deep schema. Return 1 (not 0) so
the
+ // collection check stays enabled rather than being silently
+ // bypassed; a valid recursive value always encodes to >= 1
byte.
+ return 1;
+ }
+
+ 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:
+ // 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;
+ }
+ }
+
+ /// <summary>
+ /// Rejects a collection (array or map) block whose declared element
count
+ /// could not be backed by the bytes actually remaining, before
+ /// allocating. Skipped when the per-element minimum is zero, or when
the
+ /// decoder cannot report how many bytes remain.
+ /// </summary>
+ private static void EnsureCollectionAvailable(Decoder d, long count,
long minBytesPerElement)
+ {
+ if (count <= 0)
+ {
+ return;
+ }
+
Review Comment:
EnsureCollectionAvailable() silently returns for count < 0.
Decoder.ReadArrayStart/ReadMapStart are documented to return 0 for end and >0
for a chunk size, so a negative count indicates corrupt/malicious data (and can
occur via long.MinValue overflow when negating a negative block count).
Returning early lets the caller cast the negative long to int, which can lead
to incorrect sizing/loops. Reject negative counts explicitly.
--
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]
