RyanSkraba commented on code in PR #3865:
URL: https://github.com/apache/avro/pull/3865#discussion_r3610524618
##########
lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java:
##########
@@ -296,9 +317,20 @@ protected Object readArray(Object old, Schema expected,
ResolvingDecoder in) thr
long base = 0;
if (l > 0) {
ensureAvailableCollectionBytes(in, l, expectedType);
+ // Elements whose minimum encoded size is zero (null, a zero-length
fixed, a
+ // record whose fields are all zero-byte, or a recursive schema where the
+ // cycle is broken with a 0 minimum) consume no guaranteed 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.
+ boolean zeroByteElements = minBytesPerElement(expectedType) == 0;
Review Comment:
```suggestion
boolean zeroByteElements = isZeroByteSchema(expectedType);
```
##########
lang/java/avro/src/main/java/org/apache/avro/io/BinaryDecoder.java:
##########
@@ -408,8 +408,21 @@ protected void doReadBytes(byte[] bytes, int start, int
length) throws IOExcepti
protected long doReadItemCount() throws IOException {
long result = readLong();
if (result < 0L) {
- // Consume byte-count if present
- readLong();
+ // A negative block count is followed by a block byte-size; consume it.
+ final long bytecount = readLong();
+ if (result == Long.MIN_VALUE) {
Review Comment:
Minor nit-pick -- this check should be part of the if statement above
instead of nested, right?
I think if we're throwing an exception, we're in an invalid state anyway and
there's no reason to consume the block byte-size with a `readLong`.
I'm not certain, let me know if I'm wrong.
##########
lang/java/avro/src/main/java/org/apache/avro/io/FastReaderBuilder.java:
##########
@@ -469,39 +474,82 @@ 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).
Review Comment:
```suggestion
```
So many words.
##########
lang/java/avro/src/main/java/org/apache/avro/io/BinaryDecoder.java:
##########
@@ -436,7 +449,17 @@ protected long doReadItemCount() throws IOException {
private long doSkipItems() throws IOException {
long result = readLong();
while (result < 0L) {
+ if (result == Long.MIN_VALUE) {
+ // Consistent with doReadItemCount: Long.MIN_VALUE is not a valid block
+ // count (it cannot be negated), so reject it rather than treating it
as
+ // a byte-sized block and continuing to skip.
+ readLong();
Review Comment:
As above, is there any reason to do this readLong if we're in an
inconsistent state?
--
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]
