Copilot commented on code in PR #3865:
URL: https://github.com/apache/avro/pull/3865#discussion_r3566991654
##########
lang/java/avro/src/test/java/org/apache/avro/generic/TestGenericDatumReader.java:
##########
@@ -303,4 +305,299 @@ void mapOfRecordsRejectsHugeCountUsingFullRecordSize()
throws Exception {
BinaryDecoder decoder = DecoderFactory.get().binaryDecoder(data, null);
assertThrows(EOFException.class, () -> reader.read(null, decoder));
}
+
+ // --- Zero-byte element collection allocation limit (AVRO-4300) ---
+
+ /**
+ * An array of {@code null} elements encodes each element as 0 bytes, so the
+ * bytes-remaining check cannot bound the block count. A huge count must be
+ * rejected by the heap-aware allocation limit rather than driving an
unbounded
+ * backing-array allocation.
+ */
+ @Test
+ void arrayOfNullsRejectsCountAboveAllocationLimit() throws Exception {
+
System.setProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY,
"1000");
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ try {
+ Schema schema = Schema.createArray(Schema.create(Schema.Type.NULL));
+ GenericDatumReader<Object> reader = new GenericDatumReader<>(schema);
+
+ // A single block declaring 200,000 null elements: only ~4 payload bytes,
+ // but would allocate a 200,000-slot backing array. Exceeds the limit of
+ // 1000, so it must be rejected before allocating.
+ byte[] data = encodeVarints(200_000L, 0L);
+ BinaryDecoder decoder = DecoderFactory.get().binaryDecoder(data, null);
+ assertThrows(SystemLimitException.class, () -> reader.read(null,
decoder));
+
+ // Cumulative growth across multiple blocks is also rejected: two blocks
of
+ // 600 nulls each (1200 > 1000) must throw on the second block.
+ byte[] cumulative = encodeVarints(600L, 600L, 0L);
+ BinaryDecoder decoder2 = DecoderFactory.get().binaryDecoder(cumulative,
null);
+ assertThrows(SystemLimitException.class, () -> reader.read(null,
decoder2));
+ } finally {
+
System.clearProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY);
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ }
+ }
+
+ /**
+ * A legitimate array of {@code null} elements within the allocation limit
still
+ * decodes correctly, so the guard does not reject valid data.
+ */
+ @Test
+ void arrayOfNullsWithinAllocationLimitStillDecodes() throws Exception {
+
System.setProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY,
"1000");
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ try {
+ Schema schema = Schema.createArray(Schema.create(Schema.Type.NULL));
+ GenericDatumReader<Object> reader = new GenericDatumReader<>(schema);
+
+ byte[] data = encodeVarints(1000L, 0L);
+ BinaryDecoder decoder = DecoderFactory.get().binaryDecoder(data, null);
+ GenericData.Array<?> result = (GenericData.Array<?>) reader.read(null,
decoder);
+ assertEquals(1000, result.size());
+ } finally {
+
System.clearProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY);
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ }
+ }
+
+ /**
+ * An array whose element is a record with only {@code null} fields also
encodes
+ * to 0 bytes per element and must be bounded by the allocation limit.
+ */
+ @Test
+ void arrayOfAllNullRecordsRejectsCountAboveAllocationLimit() throws
Exception {
+
System.setProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY,
"1000");
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ try {
+ Schema recWithNull = Schema.createRecord("AllNull", null, "test", false);
+ recWithNull.setFields(Collections.singletonList(new Schema.Field("n",
Schema.create(Schema.Type.NULL))));
+ Schema schema = Schema.createArray(recWithNull);
+ GenericDatumReader<Object> reader = new GenericDatumReader<>(schema);
+
+ byte[] data = encodeVarints(200_000L, 0L);
+ BinaryDecoder decoder = DecoderFactory.get().binaryDecoder(data, null);
+ assertThrows(SystemLimitException.class, () -> reader.read(null,
decoder));
+ } finally {
+
System.clearProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY);
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ }
+ }
+
+ private static GenericDatumReader<Object> arrayReader(Schema elementType,
boolean fastReader) {
+ return readerFor(Schema.createArray(elementType), fastReader);
+ }
+
+ private static GenericDatumReader<Object> readerFor(Schema schema, boolean
fastReader) {
+ GenericData data = new GenericData();
+ data.setFastReaderEnabled(fastReader);
+ return new GenericDatumReader<>(schema, schema, data);
+ }
+
+ /**
+ * Full matrix: every collection kind must be rejected (never OOM) with a
huge
+ * declared block count and no element data, on both the fast (default) and
the
+ * classic reader. Zero-byte-element arrays are bounded by the heap-aware
+ * allocation cap (SystemLimitException); every other kind is bounded by the
+ * bytes-remaining check (EOFException). Maps always carry a >=1-byte key so
+ * they fall in the latter group regardless of the value type.
+ */
+ @Test
+ void hugeCollectionsRejectedOnBothReaderPaths() throws Exception {
+ // element/value type -> expected exception when the count is huge and no
data
+ Schema nullType = Schema.create(Schema.Type.NULL);
+ Schema longType = Schema.create(Schema.Type.LONG);
+ Schema intType = Schema.create(Schema.Type.INT);
+
+ // (schema, expected exception). array<null> is the only zero-byte case
here.
+ Object[][] cases = { { Schema.createArray(nullType),
SystemLimitException.class },
+ { Schema.createArray(longType), EOFException.class }, {
Schema.createArray(intType), EOFException.class },
+ { Schema.createMap(nullType), EOFException.class }, {
Schema.createMap(longType), EOFException.class }, };
+
+ // Small allocation limit so the zero-byte array<null> case is rejected
+ // deterministically regardless of the test JVM heap size.
+
System.setProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY,
"1000");
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ try {
+ for (Object[] c : cases) {
+ Schema schema = (Schema) c[0];
+ @SuppressWarnings("unchecked")
+ Class<? extends Throwable> expected = (Class<? extends Throwable>)
c[1];
+ for (boolean fast : new boolean[] { true, false }) {
+ GenericDatumReader<Object> reader = readerFor(schema, fast);
+ byte[] data = encodeVarints(200_000_000L, 0L);
+ BinaryDecoder decoder = DecoderFactory.get().binaryDecoder(data,
null);
+ assertThrows(expected, () -> reader.read(null, decoder), () ->
schema + " fast=" + fast);
+ }
+ }
+ } finally {
+
System.clearProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY);
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ }
+ }
+
+ /**
+ * The fast reader (the default decode path) must apply the same guards as
the
+ * classic reader. A non-zero-byte element array with a huge count and no
data
+ * must be rejected by the bytes-remaining check rather than pre-allocating a
+ * huge backing array. Also verifies the cumulative allocation cap across
+ * multiple blocks for the zero-byte case, which the single-block matrix does
+ * not exercise.
+ */
+ @Test
+ void fastAndClassicReaderRejectCumulativeNullBlocks() throws Exception {
+
System.setProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY,
"1000");
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ try {
+ for (boolean fast : new boolean[] { true, false }) {
+ GenericDatumReader<Object> reader =
arrayReader(Schema.create(Schema.Type.NULL), fast);
+ // Two blocks of 600 nulls each (1200 > 1000) must throw on the second.
+ byte[] data = encodeVarints(600L, 600L, 0L);
+ BinaryDecoder decoder = DecoderFactory.get().binaryDecoder(data, null);
+ assertThrows(SystemLimitException.class, () -> reader.read(null,
decoder), "fastReader=" + fast);
+ }
+ } finally {
+
System.clearProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY);
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ }
+ }
+
+ /**
+ * A negative block count encodes {@code abs(count)} zero-byte elements
preceded
+ * by a block byte-size; the decoder normalizes it to a positive count, which
+ * must still be bounded by the allocation cap on both reader paths (matching
+ * the negative-block-count coverage of the C and Python SDKs).
+ */
+ @Test
+ void fastAndClassicReaderRejectNegativeNullBlockCount() throws Exception {
+
System.setProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY,
"1000");
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ try {
+ for (boolean fast : new boolean[] { true, false }) {
+ GenericDatumReader<Object> reader =
arrayReader(Schema.create(Schema.Type.NULL), fast);
+ // -200000 items (zigzag negative), followed by a block byte-size of 0,
+ // then the end-of-array terminator. Normalized to 200000 > 1000.
+ byte[] data = encodeVarints(-200_000L, 0L, 0L);
+ BinaryDecoder decoder = DecoderFactory.get().binaryDecoder(data, null);
+ assertThrows(SystemLimitException.class, () -> reader.read(null,
decoder), "fastReader=" + fast);
+ }
+ } finally {
+
System.clearProperty(SystemLimitException.MAX_COLLECTION_ALLOCATION_PROPERTY);
+ org.apache.avro.TestSystemLimitException.resetLimits();
+ }
+ }
+
+ /**
+ * {@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.)
Review Comment:
The Long.MIN_VALUE block-count case is described as being “normalized to an
empty collection” and “non-exploitable”, but BinaryDecoder.doReadItemCount()
negates the count (overflowing back to Long.MIN_VALUE) and readArrayStart()
then casts it via checkMaxCollectionLength(long) to 0. That can cause the array
decode to terminate without consuming the subsequent end-of-array marker,
potentially desynchronizing decoding of following fields. Consider rejecting
Long.MIN_VALUE counts as malformed (matching the C SDK behavior) rather than
normalizing to empty, and adjust this test accordingly (or add an assertion
that decoding remains aligned by appending/reading a sentinel value after the
array).
--
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]
