AVRO-4241: [Java] Bound zero-byte collection elements per datum, not per collection - #3927
Open
iemejia wants to merge 1 commit into
Open
AVRO-4241: [Java] Bound zero-byte collection elements per datum, not per collection#3927iemejia wants to merge 1 commit into
iemejia wants to merge 1 commit into
Conversation
…per collection The heap-aware zero-byte-element allocation cap (null, a zero-length fixed, an all-zero-byte record, or a recursive schema broken with a 0 minimum) was enforced per collection: readArray/readCollection and the skip/fast-reader paths each started counting from zero. Because a container file carries its own schema, an attacker can declare a record with many array<null> fields, each block individually under the limit but jointly unbounded, so a tiny payload still drives a huge aggregate allocation (e.g. ~16 array<null> fields near the per-array cap exhaust the heap; a handful burn tens of seconds of CPU). Track the cumulative zero-byte allocation per decode on a per-thread scope in SystemLimitException. GenericDatumReader.read and the static skip open the scope (scopes nest, so a delegated fast reader or a skipped writer field accumulates into the enclosing datum budget instead of resetting it); only the outermost scope resets the running total. All zero-byte call sites (GenericDatumReader read/skip, FastReaderBuilder, ReflectDatumReader) now use the cumulative checkMaxCollectionAllocation(long). Outside any scope the check falls back to the previous per-collection behaviour, so no existing caller becomes stricter. Positive-size elements are unchanged: they remain bounded per collection by the bytes-remaining check, which consumes input as it advances. Adds regression tests for a multi-field record rejected cumulatively and a within-limit record that still decodes (and confirms the budget resets between datums), on both the fast and classic reader paths.
There was a problem hiding this comment.
Pull request overview
This PR hardens the Java Avro decoder’s collection-allocation guards by making the “zero-byte element” allocation cap cumulative across an entire decoded datum (not reset per collection), closing an amplification vector where many array<null>-style fields could jointly drive large heap allocations from tiny inputs.
Changes:
- Added per-thread, per-datum allocation scoping in
SystemLimitExceptionand a new cumulativecheckMaxCollectionAllocation(long items)API. - Delimited allocation scopes around top-level
GenericDatumReader.read(...)andGenericDatumReader.skip(...), and updated classic/reflect/fast-reader call sites to use the cumulative check. - Added new unit tests verifying cumulative rejection across multiple fields and budget reset between datums for both classic and fast reader paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lang/java/avro/src/test/java/org/apache/avro/generic/TestGenericDatumReader.java | Adds tests ensuring cumulative zero-byte allocation caps apply across fields and reset between datums. |
| lang/java/avro/src/main/java/org/apache/avro/SystemLimitException.java | Introduces per-thread nested allocation scopes and a cumulative allocation-check overload. |
| lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java | Switches zero-byte allocation checks to the new cumulative API during array decoding. |
| lang/java/avro/src/main/java/org/apache/avro/io/FastReaderBuilder.java | Updates fast array decoding to use cumulative allocation checks for zero-byte elements. |
| lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java | Opens/closes per-datum allocation scopes for reads and skips; updates zero-byte allocation call sites. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
480
to
+484
| 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); | ||
| checkArrayBlock(decoder, elementType, zeroByteElements, l); |
Comment on lines
505
to
509
| l = decoder.arrayNext(); | ||
| checkArrayBlock(decoder, elementType, zeroByteElements, total, l); | ||
| checkArrayBlock(decoder, elementType, zeroByteElements, l); | ||
| } | ||
| return array; | ||
| } |
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 changes were proposed in this pull request?
Follow-up to the AVRO-4241 / AVRO-4300 collection-allocation guards. Those changes capped the number of zero-byte-minimum collection elements (
null, a zero-lengthfixed, an all-zero-byte record, or a recursive schema whose cycle is broken with a0minimum) that a decoder will allocate, since such elements consume no input and so cannot be bounded by the "bytes remaining" check.However, the cap was enforced per collection:
readArray/readCollection, the staticskip, and the fast-reader path each start counting from zero. Because an Avro container file carries its own schema, an attacker can declare a record with manyarray<null>fields, each block individually under the limit but jointly unbounded. A tiny payload therefore still drives a huge aggregate allocation (a record of ~16array<null>fields near the per-array cap exhausts the heap; a handful burns tens of seconds of CPU). This is the Java counterpart of the Python fix in #3926 (AVRO-4296).Approach
Track the cumulative zero-byte allocation per decode on a per-thread scope in
SystemLimitException:beginCollectionAllocationScope()/endCollectionAllocationScope()delimit a datum. Scopes nest via a depth counter: a delegated fast reader or a skipped writer field accumulates into the enclosing datum budget instead of resetting it; only the outermost scope resets the running total (and clears it on exit so nothing leaks to a later decode on the same thread).GenericDatumReader.read(D, Decoder)and the staticGenericDatumReader.skip(Schema, Decoder)open the scope in atry/finally.readcovers the classic and (delegated) fast paths as well asSpecificDatumReader/ReflectDatumReader, which inherit it;skipcovers schema-projection skips andBinaryData(each top-levelskipis bounded per invocation).checkMaxCollectionAllocation(long items)accumulates into the active scope. Outside any scope it falls back to the existing per-collection check, so no existing caller becomes stricter. The zero-byte call sites inGenericDatumReader(read/skip),FastReaderBuilder, andReflectDatumReadernow use it.Positive-size elements are unchanged: they remain bounded per collection by the bytes-remaining check, which consumes input as the position advances.
How was this patch tested?
recordOfNullArrayFieldsRejectedCumulativelyAcrossDatum(a record with twoarray<null>fields of 600 each is rejected on the second field at a 1000-element cap) andrecordOfNullArrayFieldsWithinCumulativeLimitStillDecodes(two 400-element fields still decode, and the budget resets between datums), both exercised on the fast and classic reader paths.TestGenericDatumReader,TestReflectDatumReader, andTestSystemLimitExceptionpass, plusTestBinaryData,TestDataFile*,TestGenericData,TestSpecificData, andTestResolvingIOas regression coverage for theskip/compare and datafile paths.