iemejia commented on code in PR #3865:
URL: https://github.com/apache/avro/pull/3865#discussion_r3567419688
##########
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:
Reworded the checkMaxCollectionAllocation summary line too ('array elements
whose minimum encoded size is zero'); the body was already updated.
##########
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:
Fixed — checkArrayBlock now applies the allocation cap directly for
zero-byte elements and only calls ensureAvailableCollectionBytes for
non-zero-byte ones (it would just recompute minBytes==0 and no-op otherwise).
##########
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:
Fixed — skip(ARRAY) now always enforces checkMaxCollectionLength(arrayTotal,
l) cumulatively, plus checkMaxCollectionAllocation additionally for zero-byte
schemas, so a large count split across blocks can't drive an unbounded skip
loop even when maxCollectionLength is set small.
--
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]
