Copilot commented on code in PR #3865:
URL: https://github.com/apache/avro/pull/3865#discussion_r3566963281
##########
lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java:
##########
@@ -441,6 +455,20 @@ static int minBytesPerElement(Schema schema) {
return minBytesPerElement(schema, Collections.newSetFromMap(new
IdentityHashMap<>()));
}
+ /**
+ * Whether values of the given schema encode to zero bytes (e.g. {@code
null},
+ * zero-length {@code fixed}, or a record whose fields are all zero-byte).
Such
+ * elements cannot be bounded by the number of bytes remaining in the
stream, so
+ * a collection of them must be bounded by a heap-aware allocation limit
+ * instead.
Review Comment:
The Javadoc claims this returns true when a schema “encodes to zero bytes”,
but the implementation is `minBytesPerElement(schema) == 0`, and
minBytesPerElement returns 0 for recursive schemas to break cycles. Please
update the Javadoc to reflect that this is a conservative “min encoded size is
0” check (not strictly “encodes to zero bytes”).
##########
lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java:
##########
@@ -296,6 +297,15 @@ protected Object readArray(Object old, Schema expected,
ResolvingDecoder in) thr
long base = 0;
if (l > 0) {
ensureAvailableCollectionBytes(in, l, expectedType);
+ // Elements whose schema encodes to zero bytes (null, or a
self-referencing
+ // record) consume no input, so ensureAvailableCollectionBytes cannot
bound
+ // their count from the bytes remaining. 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.
Review Comment:
This comment describes these elements as “encod[ing] to zero bytes”, but the
guard uses minBytesPerElement(elementType) == 0, which can also be true for
some recursive schemas (see the recursion break in minBytesPerElement). Please
reword this comment to match the actual condition being checked so future
readers aren’t misled about when the heap-aware cap applies.
##########
lang/java/avro/src/main/java/org/apache/avro/io/FastReaderBuilder.java:
##########
@@ -469,36 +470,70 @@ 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 schema encodes to zero bytes (null, or a record with only
+ // zero-byte fields) consume no 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).
Review Comment:
This comment says the cap is for schemas that “encode to zero bytes”, but
the code uses GenericDatumReader.isZeroByteSchema(elementType), which is
implemented as minBytesPerElement == 0 and can also be true for some recursive
schemas (cycle-breaking). Reword the comment to match the actual predicate
being applied.
##########
lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java:
##########
@@ -441,6 +455,20 @@ static int minBytesPerElement(Schema schema) {
return minBytesPerElement(schema, Collections.newSetFromMap(new
IdentityHashMap<>()));
}
+ /**
+ * Whether values of the given schema encode to zero bytes (e.g. {@code
null},
+ * zero-length {@code fixed}, or a record whose fields are all zero-byte).
Such
+ * elements cannot be bounded by the number of bytes remaining in the
stream, so
+ * a collection of them must be bounded by a heap-aware allocation limit
+ * instead.
+ *
+ * @param schema the element (or map value) schema
+ * @return {@code true} if the schema encodes to zero bytes
Review Comment:
This `@return` description is no longer accurate if the method is based on
`minBytesPerElement(schema) == 0` (which may also be true for recursive
schemas). Update it to describe the actual condition.
--
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]
