AVRO-4300: [java] Bound array/map allocation and skipping for zero-byte elements and on the fast reader path#3865
Merged
RyanSkraba merged 25 commits intoJul 19, 2026
Conversation
An array whose element schema encodes to zero bytes (null, a zero-length
fixed, or a record with only zero-byte fields) consumes no input per element,
so the number of elements a block declares cannot be bounded by the bytes
remaining in the stream. ensureAvailableCollectionBytes therefore skips the
check for such elements, and the collection-length cap is Integer.MAX_VALUE-8
(a VM array-size ceiling, not a memory budget). A tiny payload declaring a huge
block count of such elements (e.g. {"type":"array","items":"null"} with a
count of 200,000,000) drives an unbounded backing-array allocation and exhausts
the heap. This affects both the classic GenericDatumReader.readArray path and
the default fast-reader path (FastReaderBuilder), which had no collection guard
at all.
Add SystemLimitException.checkMaxCollectionAllocation, a heap-aware cumulative
cap (default: maxMemory()/4/8 elements, overridable via the
org.apache.avro.limits.collectionItems.maxAllocation system property, mirroring
the existing decompression limit). Enforce it before allocating in both reader
paths, keyed on GenericDatumReader.isZeroByteSchema so only the unbounded
zero-byte case is affected; all other element types remain bounded by
ensureAvailableCollectionBytes and are unchanged. Maps are already bounded
because each entry carries a string key of at least one byte.
Assisted-by: GitHub Copilot:claude-opus-4.8
…path The fast reader (FastReaderBuilder, the default decode path) never received the AVRO-4241 bytes-remaining guard: that change only touched the classic GenericDatumReader. As a result an array of non-zero-byte elements with a huge declared block count and no data (e.g. array<long>/array<int> with a count of 200,000,000) still pre-allocated new GenericData.Array<>((int) count) on the default path and exhausted the heap. Expose GenericDatumReader.ensureAvailableCollectionBytes and apply it, together with the zero-byte allocation cap, before allocating each array block in FastReaderBuilder, so the fast and classic readers enforce identical guards. Maps were already safe on both paths (each entry carries a >=1-byte key). Verified with a matrix of array<null|long|int> and map<null|long> at a huge count under -Xmx256m: every combination is now rejected (SystemLimitException for zero-byte elements, EOFException otherwise) on both reader paths instead of OOM. Assisted-by: GitHub Copilot:claude-opus-4.8
Add a matrix test asserting every collection kind is rejected (never OOM) with a huge block count and no data, on both the fast (default) and classic reader: array<null> via the heap-aware allocation cap (SystemLimitException) and array<long>, array<int>, map<null>, map<long> via the bytes-remaining check (EOFException) -- the full 5x2 set of combinations. Keep a cumulative multi-block null test and a positive within-limit decode test on both readers. Assisted-by: GitHub Copilot:claude-opus-4.8
The C and Python collection-limit tests cover a negative block count (abs(count) zero-byte elements preceded by a block byte-size); the Java tests did not. The decoder normalizes the negative count to a positive one, which must still be bounded by the heap-aware allocation cap. Add a test asserting this on both the fast and classic reader paths. Assisted-by: GitHub Copilot:claude-opus-4.8
Mirror the C SDK's INT64_MIN edge case. Long.MIN_VALUE as a block count is the pathological overflow: negating it overflows back to a negative value. Java's decoder normalizes this to an empty collection (no allocation) rather than rejecting it as the C SDK does, which is equally non-exploitable. Add a test asserting the safe, allocation-free result on both the fast and classic reader paths. Assisted-by: GitHub Copilot:claude-opus-4.8
The skip path was unbounded: BinaryDecoder.skipArray()/skipMap() returned doSkipItems() without calling checkMaxCollectionLength, and GenericDatumReader.skip() looped over the returned count. Skipping a huge block of zero-byte elements (e.g. a writer array<null> field absent from the reader schema, skipped during projection) could therefore loop unboundedly -- a CPU exhaustion even though skipping reads and allocates nothing. Two complementary bounds: - BinaryDecoder.skipArray()/skipMap() now apply the structural collection cap (checkMaxCollectionLength), mirroring readArrayStart()/readMapStart() and covering the resolving-decoder projection skip path on both reader types. - GenericDatumReader.skip() additionally bounds the cumulative count, using the heap-aware allocation cap for zero-byte element arrays and the structural cap otherwise, matching the read path and the other language SDKs. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
This PR addresses AVRO-4300 by adding heap-aware bounds for allocating and skipping collections whose elements encode to zero bytes (notably array<null> and similar zero-byte schemas), and by ensuring the fast reader path enforces the same guards as the classic GenericDatumReader path to prevent OOM and unbounded skip loops from tiny malicious payloads.
Changes:
- Introduces
SystemLimitException.checkMaxCollectionAllocationand a new system property (org.apache.avro.limits.collectionItems.maxAllocation) to cap allocations for zero-byte element arrays using a heap-derived default. - Applies the same “available bytes” / allocation guards to the fast reader array path and tightens
BinaryDecoder.skipArray()/skipMap()with structural collection caps. - Adds regression tests covering fast vs classic readers, multi-block cumulative cases, negative block counts, and skip/projection behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lang/java/avro/src/main/java/org/apache/avro/SystemLimitException.java | Adds heap-aware allocation limit for zero-byte element arrays and new limit property. |
| lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java | Enforces zero-byte allocation cap during array reads; exposes helpers; bounds skip loops cumulatively. |
| lang/java/avro/src/main/java/org/apache/avro/io/FastReaderBuilder.java | Applies classic-reader guards (bytes remaining + zero-byte allocation cap) before fast-path array allocation. |
| lang/java/avro/src/main/java/org/apache/avro/io/BinaryDecoder.java | Applies structural collection-length cap when skipping arrays/maps. |
| lang/java/avro/src/test/java/org/apache/avro/TestSystemLimitException.java | Adds unit tests for checkMaxCollectionAllocation and clears the new property in cleanup. |
| lang/java/avro/src/test/java/org/apache/avro/generic/TestGenericDatumReader.java | Adds end-to-end tests for huge collection rejection and bounded skip behavior on fast and classic readers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…M limit Addresses review feedback: - Fix the malformed <li> markup in the limit-properties list (each item closed </li> prematurely after the <tt> tag) and correct the bytes property name (org.apache.avro.limits.bytes.maxLength) so the Javadoc renders correctly. - Clamp maxCollectionAllocation to MAX_ARRAY_VM_LIMIT when refreshing limits, so a configured (or large-heap-derived) zero-byte allocation cap stays consistent with the other collection caps and cannot exceed the VM array ceiling. Assisted-by: GitHub Copilot:claude-opus-4.8
Comment on lines
33
to
35
| * <li><tt>org.apache.avro.limits.collectionItems.maxLength</tt> limits the | ||
| * maximum number of <tt>map</tt> and <tt>list</tt> items that can be read at | ||
| * once single sequence.</li> |
Member
Author
There was a problem hiding this comment.
Fixed in e47f206: 'read at once single sequence' -> 'read in a single sequence'.
…ministic Addresses review feedback: - Javadoc: "read at once single sequence" -> "read in a single sequence". - testCheckMaxCollectionAllocation asserts with MAX_ARRAY_VM_LIMIT + 1 instead of Integer.MAX_VALUE - 8. Since the default allocation cap is derived from the heap and then clamped to MAX_ARRAY_VM_LIMIT, a value equal to that limit may not exceed the computed default on a very large heap; +1 guarantees it does, keeping the test deterministic across environments. Assisted-by: GitHub Copilot:claude-opus-4.8
Comment on lines
+298
to
+300
| * Elements whose schema encodes to zero bytes (e.g. {@code null} or a | ||
| * self-referencing record) consume no input bytes, so the number that may be | ||
| * declared is not bounded by the bytes remaining in the stream. Without a cap, |
Member
Author
There was a problem hiding this comment.
Fixed in 6bf53e3: same rewording applied to this occurrence.
Addresses review feedback: - Javadoc: "a self-referencing record" is not inherently zero-byte (the code only computes a 0-byte minimum for some recursive schemas to break recursion). Reword the three occurrences to describe actual zero-byte encodings: null, a zero-length fixed, or a record whose fields all encode to zero bytes. - skipMapRejectsHugeCount now asserts UnsupportedOperationException (a count of Integer.MAX_VALUE deterministically hits the VM structural-limit path), and resolvingSkipOfHugeNullArrayFieldIsBounded asserts SystemLimitException, so the tests pin the intended exception rather than a generic RuntimeException. Assisted-by: GitHub Copilot:claude-opus-4.8
The zero-byte Javadoc rewording did not match the Eclipse formatter's line-wrapping, failing the spotless check. Reflowed via `mvn spotless:apply`. Assisted-by: GitHub Copilot:claude-opus-4.8
| * instead. | ||
| * | ||
| * @param schema the element (or map value) schema | ||
| * @return {@code true} if the schema encodes to zero bytes |
Member
Author
…icate Addresses review feedback: the "encodes to zero bytes" wording in GenericDatumReader (the array cap comment and isZeroByteSchema Javadoc) and FastReaderBuilder is imprecise. The guard is minBytesPerElement(schema) == 0, which is also true for recursive schemas whose cycle is broken by returning a 0 minimum. Reword to describe the "minimum encoded size is zero" predicate so the docs match the actual condition under which the heap-aware cap applies. Assisted-by: GitHub Copilot:claude-opus-4.8
Comment on lines
+492
to
+496
| * {@code Long.MIN_VALUE} as a block count is the pathological overflow case: | ||
| * negating it overflows back to a negative value. It must be handled safely | ||
| * without allocating -- the decoder normalizes it to an empty collection rather | ||
| * than a huge one -- on both reader paths. (The C SDK rejects it outright; this | ||
| * normalization is equally non-exploitable.) |
Member
Author
There was a problem hiding this comment.
You're right — this was a genuine desync, not merely an empty-collection normalization. doReadItemCount() negates the count, but Long.MIN_VALUE overflows back to Long.MIN_VALUE, and the single-arg checkMaxCollectionLength doesn't reject negatives, so (int) Long.MIN_VALUE == 0 silently ended the collection without consuming the end marker. Fixed in c2ab33b by rejecting Long.MIN_VALUE block counts as malformed (AvroRuntimeException), matching the C SDK. The test now asserts the rejection on both reader paths.
Addresses review feedback: doReadItemCount() negates a negative block count, but Long.MIN_VALUE negates back to Long.MIN_VALUE (still negative). The single-arg checkMaxCollectionLength does not reject negatives, so it was truncated via (int) cast to 0, silently ending the collection without consuming the end-of-array/map marker and desynchronizing decoding of subsequent fields. Reject Long.MIN_VALUE outright as malformed, matching the C SDK, instead of normalizing to an empty collection. Update the test to assert the rejection. Assisted-by: GitHub Copilot:claude-opus-4.8
doReadItemCount consumed the block byte-size for a negative-count block without validating it. doSkipItems now rejects a negative byte-size, so the read path does the same for consistency and to reject malformed encodings. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
lang/java/avro/src/main/java/org/apache/avro/SystemLimitException.java:48
- The class-level Javadoc says all limits default to permitting sizes up to MAX_ARRAY_VM_LIMIT, but MAX_COLLECTION_ALLOCATION_PROPERTY defaults to a heap-derived fraction (then clamped). Updating this sentence will keep the public docs accurate for the new allocation cap.
*
* The default is to permit sizes up to {@link #MAX_ARRAY_VM_LIMIT}.
*/
…ata" The huge-collection matrix payload includes a trailing 0L varint (an element value for arrays, a 0-length key for maps); reword "no element data" to reflect that. Assisted-by: GitHub Copilot:claude-opus-4.8
Reword the DEFAULT_MAX_COLLECTION_ALLOCATION_HEAP_FRACTION Javadoc from "zero-byte elements" to "elements whose minimum encoded size is zero". Assisted-by: GitHub Copilot:claude-opus-4.8
arrayHugeCountOnStreamClampsPreallocation only asserted an EOFException, which a reintroduced new Object[(int) count] preallocation could still satisfy on a large-heap JVM (allocating ~200M slots then hitting EOF). Override newArray to record the largest requested capacity and assert it stays clamped to initialCollectionCapacity, and assert remainingBytes() == -1 to confirm the unknown-remaining-bytes precondition. Assisted-by: GitHub Copilot:claude-opus-4.8
RyanSkraba
approved these changes
Jul 19, 2026
RyanSkraba
left a comment
Contributor
There was a problem hiding this comment.
I've read through this -- good and necessary work with only minor nit-picks while I was reading, and nothing that would block this PR from being merged as it is.
I definitely don't have any objection to LLM assisted pull requests, but this one has a lot of repetitive comments and so many detailed unit tests to read... there's probably some work to be done in choosing a judicious balance!
…ck-count guards Apply RyanSkraba's review feedback: - GenericDatumReader.readArray uses the isZeroByteSchema helper instead of inlining minBytesPerElement == 0, matching skip(). - BinaryDecoder.doReadItemCount rejects Long.MIN_VALUE before consuming the block byte-size, so no readLong runs once the count is invalid. - BinaryDecoder.doSkipItems drops the readLong before throwing on an invalid Long.MIN_VALUE count, consistent with doReadItemCount. - FastReaderBuilder.createArrayReader drops the redundant comment; the rationale is documented on checkArrayBlock.
RyanSkraba
approved these changes
Jul 19, 2026
RyanSkraba
pushed a commit
that referenced
this pull request
Jul 19, 2026
…te elements and on the fast reader path (#3865) * AVRO-4300: [java] Bound zero-byte array element allocation An array whose element schema encodes to zero bytes (null, a zero-length fixed, or a record with only zero-byte fields) consumes no input per element, so the number of elements a block declares cannot be bounded by the bytes remaining in the stream. ensureAvailableCollectionBytes therefore skips the check for such elements, and the collection-length cap is Integer.MAX_VALUE-8 (a VM array-size ceiling, not a memory budget). A tiny payload declaring a huge block count of such elements (e.g. {"type":"array","items":"null"} with a count of 200,000,000) drives an unbounded backing-array allocation and exhausts the heap. This affects both the classic GenericDatumReader.readArray path and the default fast-reader path (FastReaderBuilder), which had no collection guard at all. Add SystemLimitException.checkMaxCollectionAllocation, a heap-aware cumulative cap (default: maxMemory()/4/8 elements, overridable via the org.apache.avro.limits.collectionItems.maxAllocation system property, mirroring the existing decompression limit). Enforce it before allocating in both reader paths, keyed on GenericDatumReader.isZeroByteSchema so only the unbounded zero-byte case is affected; all other element types remain bounded by ensureAvailableCollectionBytes and are unchanged. Maps are already bounded because each entry carries a string key of at least one byte. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Apply the available-bytes check on the fast reader path The fast reader (FastReaderBuilder, the default decode path) never received the AVRO-4241 bytes-remaining guard: that change only touched the classic GenericDatumReader. As a result an array of non-zero-byte elements with a huge declared block count and no data (e.g. array<long>/array<int> with a count of 200,000,000) still pre-allocated new GenericData.Array<>((int) count) on the default path and exhausted the heap. Expose GenericDatumReader.ensureAvailableCollectionBytes and apply it, together with the zero-byte allocation cap, before allocating each array block in FastReaderBuilder, so the fast and classic readers enforce identical guards. Maps were already safe on both paths (each entry carries a >=1-byte key). Verified with a matrix of array<null|long|int> and map<null|long> at a huge count under -Xmx256m: every combination is now rejected (SystemLimitException for zero-byte elements, EOFException otherwise) on both reader paths instead of OOM. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Cover all collection/reader combinations in tests Add a matrix test asserting every collection kind is rejected (never OOM) with a huge block count and no data, on both the fast (default) and classic reader: array<null> via the heap-aware allocation cap (SystemLimitException) and array<long>, array<int>, map<null>, map<long> via the bytes-remaining check (EOFException) -- the full 5x2 set of combinations. Keep a cumulative multi-block null test and a positive within-limit decode test on both readers. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Test negative block count is bounded on both readers The C and Python collection-limit tests cover a negative block count (abs(count) zero-byte elements preceded by a block byte-size); the Java tests did not. The decoder normalizes the negative count to a positive one, which must still be bounded by the heap-aware allocation cap. Add a test asserting this on both the fast and classic reader paths. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Test Long.MIN_VALUE block count is handled safely Mirror the C SDK's INT64_MIN edge case. Long.MIN_VALUE as a block count is the pathological overflow: negating it overflows back to a negative value. Java's decoder normalizes this to an empty collection (no allocation) rather than rejecting it as the C SDK does, which is equally non-exploitable. Add a test asserting the safe, allocation-free result on both the fast and classic reader paths. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Bound the array/map skip path The skip path was unbounded: BinaryDecoder.skipArray()/skipMap() returned doSkipItems() without calling checkMaxCollectionLength, and GenericDatumReader.skip() looped over the returned count. Skipping a huge block of zero-byte elements (e.g. a writer array<null> field absent from the reader schema, skipped during projection) could therefore loop unboundedly -- a CPU exhaustion even though skipping reads and allocates nothing. Two complementary bounds: - BinaryDecoder.skipArray()/skipMap() now apply the structural collection cap (checkMaxCollectionLength), mirroring readArrayStart()/readMapStart() and covering the resolving-decoder projection skip path on both reader types. - GenericDatumReader.skip() additionally bounds the cumulative count, using the heap-aware allocation cap for zero-byte element arrays and the structural cap otherwise, matching the read path and the other language SDKs. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Fix limit Javadoc markup; clamp allocation cap to VM limit Addresses review feedback: - Fix the malformed <li> markup in the limit-properties list (each item closed </li> prematurely after the <tt> tag) and correct the bytes property name (org.apache.avro.limits.bytes.maxLength) so the Javadoc renders correctly. - Clamp maxCollectionAllocation to MAX_ARRAY_VM_LIMIT when refreshing limits, so a configured (or large-heap-derived) zero-byte allocation cap stays consistent with the other collection caps and cannot exceed the VM array ceiling. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Fix limit Javadoc grammar; make heap-cap test deterministic Addresses review feedback: - Javadoc: "read at once single sequence" -> "read in a single sequence". - testCheckMaxCollectionAllocation asserts with MAX_ARRAY_VM_LIMIT + 1 instead of Integer.MAX_VALUE - 8. Since the default allocation cap is derived from the heap and then clamped to MAX_ARRAY_VM_LIMIT, a value equal to that limit may not exceed the computed default on a very large heap; +1 guarantees it does, keeping the test deterministic across environments. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Reword zero-byte Javadoc; tighten skip test assertions Addresses review feedback: - Javadoc: "a self-referencing record" is not inherently zero-byte (the code only computes a 0-byte minimum for some recursive schemas to break recursion). Reword the three occurrences to describe actual zero-byte encodings: null, a zero-length fixed, or a record whose fields all encode to zero bytes. - skipMapRejectsHugeCount now asserts UnsupportedOperationException (a count of Integer.MAX_VALUE deterministically hits the VM structural-limit path), and resolvingSkipOfHugeNullArrayFieldIsBounded asserts SystemLimitException, so the tests pin the intended exception rather than a generic RuntimeException. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Apply spotless formatting to reworded Javadoc The zero-byte Javadoc rewording did not match the Eclipse formatter's line-wrapping, failing the spotless check. Reflowed via `mvn spotless:apply`. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Clarify zero-byte comments to match the actual predicate Addresses review feedback: the "encodes to zero bytes" wording in GenericDatumReader (the array cap comment and isZeroByteSchema Javadoc) and FastReaderBuilder is imprecise. The guard is minBytesPerElement(schema) == 0, which is also true for recursive schemas whose cycle is broken by returning a 0 minimum. Reword to describe the "minimum encoded size is zero" predicate so the docs match the actual condition under which the heap-aware cap applies. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Reject Long.MIN_VALUE block count as malformed Addresses review feedback: doReadItemCount() negates a negative block count, but Long.MIN_VALUE negates back to Long.MIN_VALUE (still negative). The single-arg checkMaxCollectionLength does not reject negatives, so it was truncated via (int) cast to 0, silently ending the collection without consuming the end-of-array/map marker and desynchronizing decoding of subsequent fields. Reject Long.MIN_VALUE outright as malformed, matching the C SDK, instead of normalizing to an empty collection. Update the test to assert the rejection. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Clamp collection preallocation from declared block count The array/map readers passed the declared block count straight to newArray/ newMap as the initial capacity. On a stream source the bytes-available guard is skipped (BinaryDecoder.remainingBytes() returns -1 for sources other than ByteArrayInputStream/ByteBufferInputStream), so a large declared count reaches the allocation path directly and drives a huge up-front allocation (e.g. new Object[count]) before a single element is read. Clamp the initial capacity to a modest bound (MAX_COLLECTION_PREALLOC) via initialCollectionCapacity(); the backing array/map still grows on demand as elements are decoded, so a truncated or hostile stream now fails with EOFException after a bounded allocation instead of attempting to preallocate hundreds of millions of slots. Applies to both the classic and fast readers. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Reject out-of-range union and enum indices with AvroTypeException The union branch index and enum symbol index were used to index the branch/ symbol tables without an explicit range check on several paths, surfacing a malformed index as a generic IndexOutOfBoundsException instead of a clear Avro-specific error: - Union: Symbol.Alternative.getSymbol (used by both the validating and resolving decoders, and therefore by GenericDatumReader and the fast reader) indexed symbols[] directly. Add a [0, size) check throwing AvroTypeException. - Enum: ResolvingDecoder.readEnum returned the raw index in the no-adjustments case and indexed the adjustment table otherwise, both without validation (ValidatingDecoder.readEnum already checked). Add the range checks. - The fast reader reads from a plain decoder (resolution is pre-baked), so its union and enum readers need their own [0, length) checks. Adds tests covering negative and too-large union and enum indices on both the classic and fast reader paths. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Reword remaining "zero-byte" docs to the actual predicate The maxAllocation limit is applied when a schema's minimum encoded size is zero (GenericDatumReader.isZeroByteSchema, i.e. minBytesPerElement == 0), which also covers recursive schemas whose cycle is conservatively broken with a 0 minimum. Reword the remaining "encodes to zero bytes" wording -- the class Javadoc, the MAX_COLLECTION_ALLOCATION_PROPERTY Javadoc, the SystemLimitException message, and the skipArray comment -- to say "minimum encoded size is zero" so the docs match the condition under which the cap actually applies. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Half-open range in enum messages; finish zero-byte reword Address review feedback: - The enum out-of-range messages said "max is <size>", implying <size> is a valid index. Reword to the half-open "must be in [0, <size>)" to match the actual check and the union messages, in ResolvingDecoder (both the no-adjustments and adjustments branches), FastReaderBuilder, and ValidatingDecoder. - Reword the remaining "encodes to zero bytes" wording in the checkMaxCollectionAllocation Javadoc to "minimum encoded size is zero" (including the recursive-schema case), matching isZeroByteSchema. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Enforce structural cap when skipping zero-byte arrays; tidy Address review feedback: - skip(ARRAY) bounded zero-byte-element arrays only by the heap-aware allocation cap, so the configurable structural collection limit (MAX_COLLECTION_LENGTH_PROPERTY) was not enforced cumulatively and a large count split into many blocks could drive a long skip loop. Always enforce checkMaxCollectionLength cumulatively, and additionally checkMaxCollectionAllocation for zero-byte schemas. - FastReaderBuilder.checkArrayBlock: skip the ensureAvailableCollectionBytes call for zero-byte elements (it recomputes minBytesPerElement and no-ops), applying the allocation cap directly instead. - Reword the checkMaxCollectionAllocation summary line from "zero-byte-encoded" to "minimum encoded size is zero". Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Reword defaultMaxCollectionAllocation Javadoc Update the last "zero-byte-encoded" wording (the private defaultMaxCollectionAllocation helper) to "minimum encoded size is zero", consistent with the public property/Javadoc and the isZeroByteSchema predicate. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Reword last zero-byte field doc; sort test imports - Reword the maxCollectionAllocation field Javadoc from "zero-byte-encoded" to "minimum encoded size is zero". - Sort the java.io imports in TestGenericDatumReader (BufferedInputStream before the ByteArray* imports) to satisfy Spotless. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Reject Long.MIN_VALUE and negative byte-size in doSkipItems doSkipItems accepted a Long.MIN_VALUE block count (treating it as a byte-sized block and continuing to skip), inconsistent with doReadItemCount which rejects it. Reject Long.MIN_VALUE, and also reject a negative block byte-size, so the skip path fails fast on malformed input like the read path. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Reject negative block byte-size in doReadItemCount doReadItemCount consumed the block byte-size for a negative-count block without validating it. doSkipItems now rejects a negative byte-size, so the read path does the same for consistency and to reject malformed encodings. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Clarify test Javadoc: trailing 0L varint, not "no data" The huge-collection matrix payload includes a trailing 0L varint (an element value for arrays, a 0-length key for maps); reword "no element data" to reflect that. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Reword heap-fraction Javadoc to the actual predicate Reword the DEFAULT_MAX_COLLECTION_ALLOCATION_HEAP_FRACTION Javadoc from "zero-byte elements" to "elements whose minimum encoded size is zero". Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Make array preallocation-clamp test regression-proof arrayHugeCountOnStreamClampsPreallocation only asserted an EOFException, which a reintroduced new Object[(int) count] preallocation could still satisfy on a large-heap JVM (allocating ~200M slots then hitting EOF). Override newArray to record the largest requested capacity and assert it stays clamped to initialCollectionCapacity, and assert remainingBytes() == -1 to confirm the unknown-remaining-bytes precondition. Assisted-by: GitHub Copilot:claude-opus-4.8 * AVRO-4300: [java] Address review nits: use isZeroByteSchema, tidy block-count guards Apply RyanSkraba's review feedback: - GenericDatumReader.readArray uses the isZeroByteSchema helper instead of inlining minBytesPerElement == 0, matching skip(). - BinaryDecoder.doReadItemCount rejects Long.MIN_VALUE before consuming the block byte-size, so no readLong runs once the count is invalid. - BinaryDecoder.doSkipItems drops the readLong before throwing on an invalid Long.MIN_VALUE count, consistent with doReadItemCount. - FastReaderBuilder.createArrayReader drops the redundant comment; the rationale is documented on checkArrayBlock.
Contributor
|
Cherry-picked to branch-1.12. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
Fixes AVRO-4300 (sub-task of AVRO-4292). When
GenericDatumReaderdecodes an array it reads the block item count from the stream and pre-allocates the backing store (newArray→new Object[count]) before decoding any element. Several independent gaps let a tiny payload drive an unbounded allocation — or an unbounded skip loop — and exhaust the heap:Zero-byte elements (classic and fast reader). Elements whose schema encodes to zero bytes (
null, a zero-lengthfixed, or a record with only zero-byte fields) consume no input, soensureAvailableCollectionBytes(AVRO-4241) skips the check for them (minBytesPerElement == 0), and the collection-length cap isInteger.MAX_VALUE - 8(a JVM array-size ceiling, not a memory budget). An array such as{"type":"array","items":"null"}declaring a block count of 200,000,000 is a ~6-byte payload that allocates a 200M-slot array (~1.6 GB).The fast reader never had the AVRO-4241 available-bytes guard at all. That change only modified the classic
GenericDatumReader(andBinaryDecoder/Decoder/ValidatingDecoder); it never touchedFastReaderBuilder, which is the default decode path (avro.io.fastreaddefaults totrue). So on the default reader even a non-zero-byte array such asarray<long>orarray<int>with a huge block count and no data pre-allocatednew GenericData.Array<>((int) count)and exhausted the heap. Only the classic reader was protected.The skip path was unbounded.
BinaryDecoder.skipArray()/skipMap()returneddoSkipItems()without applying the collection cap, andGenericDatumReader.skip()looped over that count. Skipping a huge block of zero-byte elements (e.g. a writerarray<null>field absent from the reader schema, skipped during projection) could therefore loop unboundedly even though skipping reads and allocates nothing.Fix (applied identically on the classic and fast reader paths)
SystemLimitException.checkMaxCollectionAllocation, a heap-aware cumulative cap for zero-byte elements (defaultmaxMemory()/4/8elements, overridable via theorg.apache.avro.limits.collectionItems.maxAllocationsystem property, mirroring the existing decompression limit).GenericDatumReader.ensureAvailableCollectionBytesand apply it, together with the zero-byte cap, before allocating each array block inFastReaderBuilder, so the fast and classic readers enforce identical guards.BinaryDecoder.skipArray()/skipMap()now apply the structural collection cap (checkMaxCollectionLength), covering the resolving-decoder projection skip on both reader types, andGenericDatumReader.skip()additionally bounds the cumulative count — using the heap-aware cap for zero-byte elements and the structural cap otherwise.ensureAvailableMapByteson the classic path; key reads consume bytes on the fast path).Verifying this change
This change added tests and can be verified as follows:
SystemLimitException.checkMaxCollectionAllocationtests (single/cumulative/negative/overflow and heap-derived default).array<null>→SystemLimitException;array<long>,array<int>,map<null>,map<long>→EOFException(the full 5×2 set of combinations), plus a cumulative multi-block null case and a positive within-limit decode.skipArrayOfNullRejectsHugeCount,skipSmallNullArraySucceeds,skipMapRejectsHugeCount, andresolvingSkipOfHugeNullArrayFieldIsBounded(bounded on both readers under schema resolution).array<null>, block count 200,000,000) under-Xmx256m: rejected with a cleanSystemLimitException(no allocation) instead ofOutOfMemoryError, on both reader paths; legitimate collections within the limit still decode.mvn -pl avro testfor thegenericandiopackages passes (3860 tests, no regressions); Spotless and Checkstyle are clean.Documentation
org.apache.avro.limits.collectionItems.maxAllocationsystem property, documented inSystemLimitExceptionJavaDoc alongside the existing limit properties)