Copilot commented on code in PR #3865:
URL: https://github.com/apache/avro/pull/3865#discussion_r3567382376
##########
lang/java/avro/src/main/java/org/apache/avro/SystemLimitException.java:
##########
@@ -246,6 +293,42 @@ public static int checkMaxCollectionLength(long items) {
return (int) items;
}
+ /**
+ * Check to ensure that allocating storage for the specified number of
+ * zero-byte-encoded array elements remains within the heap-aware limit.
+ * <p>
+ * Elements whose schema encodes to zero bytes (e.g. {@code null}, a
zero-length
+ * fixed, or a record whose fields all encode to zero bytes) consume no input
+ * bytes, so the number that may be declared is not bounded by the bytes
Review Comment:
The Javadoc for checkMaxCollectionAllocation() still describes
“zero-byte-encoded” elements and says the schema “encodes to zero bytes”, but
the guard is used for schemas whose *minimum encoded size is zero*
(minBytesPerElement == 0), including some recursive schemas where the minimum
is conservatively treated as 0. Updating this wording will keep the public docs
consistent with the actual predicate and the exception message.
##########
lang/java/avro/src/main/java/org/apache/avro/io/FastReaderBuilder.java:
##########
@@ -469,39 +474,78 @@ private Optional<Class<?>> findClass(String clazz) {
@SuppressWarnings("unchecked")
private FieldReader createArrayReader(Schema readerSchema, Container action)
throws IOException {
FieldReader elementReader = getReaderFor(action.elementAction, null);
+ Schema elementType = readerSchema.getElementType();
+
+ // Elements whose minimum encoded size is zero (null, a record with only
+ // zero-byte fields, or a recursive schema whose cycle is broken with a 0
+ // minimum) consume no guaranteed input, so the block count cannot be
bounded
+ // by the bytes remaining in the stream. Cap such collections against a
+ // heap-aware limit so a tiny payload cannot declare a huge block count and
+ // drive an unbounded backing-array allocation (AVRO-4300).
+ boolean zeroByteElements =
GenericDatumReader.isZeroByteSchema(elementType);
return reusingReader((reuse, decoder) -> {
if (reuse instanceof GenericArray) {
GenericArray<Object> reuseArray = (GenericArray<Object>) reuse;
long l = decoder.readArrayStart();
+ long total = 0;
+ checkArrayBlock(decoder, elementType, zeroByteElements, total, l);
reuseArray.clear();
while (l > 0) {
for (long i = 0; i < l; i++) {
reuseArray.add(elementReader.read(reuseArray.peek(), decoder));
}
+ total += l;
l = decoder.arrayNext();
+ checkArrayBlock(decoder, elementType, zeroByteElements, total, l);
}
return reuseArray;
} else {
long l = decoder.readArrayStart();
+ long total = 0;
+ checkArrayBlock(decoder, elementType, zeroByteElements, total, l);
List<Object> array = (reuse instanceof List) ? (List<Object>) reuse
- : new GenericData.Array<>((int) l, readerSchema);
+ : new
GenericData.Array<>(GenericDatumReader.initialCollectionCapacity(l),
readerSchema);
array.clear();
while (l > 0) {
for (long i = 0; i < l; i++) {
array.add(elementReader.read(null, decoder));
}
+ total += l;
l = decoder.arrayNext();
+ checkArrayBlock(decoder, elementType, zeroByteElements, total, l);
}
return array;
}
});
}
+ /**
+ * Validates an array block count before its elements are allocated, applying
+ * the same guards as the classic {@code GenericDatumReader}: the
+ * bytes-remaining check for elements with a positive minimum size, and the
+ * heap-aware allocation cap for zero-byte elements (which the bytes check
+ * cannot bound).
+ */
+ private static void checkArrayBlock(Decoder decoder, Schema elementType,
boolean zeroByteElements, long total,
+ long count) throws IOException {
+ if (count <= 0) {
+ return;
+ }
+ GenericDatumReader.ensureAvailableCollectionBytes(decoder, count,
elementType);
+ if (zeroByteElements) {
+ SystemLimitException.checkMaxCollectionAllocation(total, count);
+ }
+ }
Review Comment:
checkArrayBlock() always calls
GenericDatumReader.ensureAvailableCollectionBytes(), which recomputes
minBytesPerElement(elementType). Since zeroByteElements is already precomputed,
this is unnecessary work for the zero-byte case (the method will no-op after
recomputing minBytes==0). Skipping the call when zeroByteElements is true
avoids repeated schema-walk overhead on multi-block arrays.
##########
lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java:
##########
@@ -747,15 +802,32 @@ public static void skip(Schema schema, Decoder in) throws
IOException {
break;
case ARRAY:
Schema elementType = schema.getElementType();
+ // Bound the cumulative element count: skipping a huge block of elements
+ // whose minimum encoded size is zero (e.g. null) would otherwise loop
+ // unboundedly (a CPU exhaustion) even though it reads nothing. Such
+ // elements use the heap-aware allocation cap; others the structural
+ // collection cap.
+ boolean zeroByteElements = isZeroByteSchema(elementType);
+ long arrayTotal = 0;
for (long l = in.skipArray(); l > 0; l = in.skipArray()) {
+ if (zeroByteElements) {
+ SystemLimitException.checkMaxCollectionAllocation(arrayTotal, l);
+ } else {
+ SystemLimitException.checkMaxCollectionLength(arrayTotal, l);
+ }
+ arrayTotal += l;
Review Comment:
In GenericDatumReader.skip(ARRAY), zero-byte-element arrays are only bounded
by checkMaxCollectionAllocation(), which means the configured structural
collection cap (MAX_COLLECTION_LENGTH_PROPERTY) is not enforced cumulatively
across blocks for array<null>/other min-bytes==0 schemas. An attacker can split
a large count into many <=limit blocks and force long skip loops even when
maxCollectionLength is set small. Consider always enforcing
checkMaxCollectionLength(arrayTotal, l) (cumulative) and additionally enforcing
checkMaxCollectionAllocation for zero-byte schemas.
--
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]
