Uh oh!
There was an error while loading. Please reload this page.
AVRO-4329: [Java] Bound bytes/string allocation on non-seekable streams - #3930
Open
iemejia wants to merge 3 commits into
Open
AVRO-4329: [Java] Bound bytes/string allocation on non-seekable streams#3930iemejia wants to merge 3 commits into
iemejia wants to merge 3 commits into
Conversation
The available-bytes guard added under AVRO-4292 rejects a declared bytes/string length that exceeds the data remaining only when the decoder can report the number of bytes remaining (memory-backed or seekable sources). On a non-seekable stream (socket, pipe, decompression stream) the check is a no-op, so a huge declared length still drives a single large up-front allocation before any payload is read. When the remaining byte count is unknown, read the bytes/string value into a buffer that grows in bounded chunks rather than allocating the full attacker-declared length up front. A truncated or hostile stream then fails with a bounded EOFException after a bounded allocation instead of an OutOfMemoryError. The existing single-allocation fast path is kept when the remaining byte count is known. The same fix is applied to DirectBinaryDecoder, which reads straight from a stream.
There was a problem hiding this comment.
Pull request overview
Hardens the Java Avro binary decoders against DoS-by-allocation when reading length‑prefixed bytes/string values from non-seekable streams by avoiding single up-front allocations based on attacker-declared lengths, while keeping the existing fast path for sources with known remaining-byte counts.
Changes:
- Add bounded, chunk-growing read path in
BinaryDecoderfor large declaredbytes/stringlengths when remaining bytes are unknown. - Add equivalent bounded allocation behavior in
DirectBinaryDecoderfor largebytesreads from streaming sources. - Add regression tests covering huge declared lengths on non-seekable streams and round-trip behavior for legitimately large values.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lang/java/avro/src/main/java/org/apache/avro/io/BinaryDecoder.java | Adds bounded growing-buffer helper and routes large bytes/string reads on unknown-length sources through it. |
| lang/java/avro/src/main/java/org/apache/avro/io/DirectBinaryDecoder.java | Avoids large up-front allocations for large declared bytes lengths when reading directly from streams. |
| lang/java/avro/src/test/java/org/apache/avro/io/TestBinaryDecoderBoundedRead.java | Adds regression coverage for huge declared lengths on non-seekable streams plus round-trip tests for large payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Address review feedback: the bounded-read branch in BinaryDecoder.readBytes/readString was taken purely on the declared length, so a caller-supplied buffer with sufficient capacity was ignored and a new array allocated, an observable regression of the Decoder buffer reuse contract. Take the bounded growing-buffer path only when a new allocation would otherwise be required (the supplied buffer is null or too small); when a reusable buffer of sufficient capacity is provided there is no large up-front allocation to guard against, so read straight into it. Add a regression test asserting the supplied buffer is reused.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lang/java/avro/src/test/java/org/apache/avro/io/TestBinaryDecoderBoundedRead.java:75
- This comment claims that seeing an EOFException “proves” the decoder didn’t attempt the full up-front allocation, but the same EOFException could still occur even if a large allocation succeeded (e.g., on a large-heap CI runner). This makes the intent of the test misleading; it’s really relying on typical heap limits to make an up-front allocation fail.
/**
* A near-2GB declared length that a single {@code new byte[len]} could not
* satisfy on a normal test heap, so reaching an {@link EOFException} proves the
* decoder never attempted the full up-front allocation.
*/
lang/java/avro/src/main/java/org/apache/avro/io/BinaryDecoder.java:659
- The Javadoc says the length “must be positive”, but the implementation correctly handles a zero length (it returns an empty array without reading). Either validate and reject 0, or update the contract to match the actual behavior.
* @param length the number of bytes to read; must be positive
…ap-independent)
The huge-length rejection tests only asserted assertThrows(EOFException). That
distinguishes the guard from the unguarded path only because a ~2GB up-front
allocation throws OutOfMemoryError on a normal test heap -- on a large-heap
runner the unguarded path would allocate the full length, short-read, and also
throw EOFException, so the tests would pass with the guard removed.
Add a MaxReadRecordingStream that records the largest single read request and
assert it stays <= MAX_UNVERIFIED_ALLOCATION. This directly measures the guard's
bounded allocation and is independent of heap size.
Verified by neutering requiresBoundedRead() with an allocatable declared length:
assertThrows(EOFException) still passes but assertBoundedRead fails ("asked for
100000000 bytes in one read"), and both pass once the guard is restored.
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 freeto 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
Java SDK implementation of AVRO-4303 (parent). The available-bytes guard added
under AVRO-4292 rejects a declared bytes/string length that exceeds the data
remaining only when the decoder can report the number of bytes remaining
(memory-backed or seekable sources). On a non-seekable stream (socket, pipe,
decompression stream) the check is a no-op, so a huge declared length still
drives a single large up-front allocation before any payload is read. A tiny
truncated input can therefore force a multi-hundred-MB allocation.
When the remaining byte count is unknown, this reads the bytes/string value into
a buffer that grows in bounded chunks rather than allocating the full
attacker-declared length up front. A truncated or hostile stream then fails with
a bounded
EOFExceptionafter a bounded allocation instead of anOutOfMemoryError. The existing single-allocation fast path is kept when theremaining byte count is known. The same fix is applied to
DirectBinaryDecoder,which reads straight from a stream.
Verifying this change
This change added tests and can be verified as follows:
TestBinaryDecoderBoundedRead: a near-2GB declared bytes/string lengthon a truncated non-seekable stream fails with a bounded
EOFException(not anOutOfMemoryError) for both the bufferedBinaryDecoderandDirectBinaryDecoder; a legitimately large value on a non-seekable streamstill round-trips; small values and seekable sources keep the direct path.
TestBinaryDecoder(68 tests) continues to pass.Documentation
helper in
BinaryDecoder)