Copilot commented on code in PR #3928:
URL: https://github.com/apache/avro/pull/3928#discussion_r3737310043
##########
lang/java/avro/src/main/java/org/apache/avro/io/FastReaderBuilder.java:
##########
@@ -637,11 +651,18 @@ public boolean canReuse() {
@Override
public Object read(Object reuse, Decoder decoder) throws IOException {
- Object object = supplier.newInstance(reuse, schema);
- for (ExecutionStep thisStep : readSteps) {
- thisStep.execute(object, decoder);
+ // Bound decode nesting depth: a recursive schema fed deeply nested data
+ // would otherwise overflow the stack via this recursive descent.
+ SystemLimitException.incrementDecodeDepth();
+ try {
+ Object object = supplier.newInstance(reuse, schema);
+ for (ExecutionStep thisStep : readSteps) {
+ thisStep.execute(object, decoder);
+ }
+ return object;
+ } finally {
+ SystemLimitException.decrementDecodeDepth();
Review Comment:
`RecordReader.read(...)` is now depth-guarded, but
`initializeRecordReader(...)` still installs skip steps that call
`GenericDatumReader.skip(...)`, which (currently) has no decode-depth checks
and can still recurse arbitrarily deep on hostile data. This can leave the
fast-reader path vulnerable when skipping writer-only fields for recursive
schemas.
##########
lang/java/avro/src/test/java/org/apache/avro/TestSystemLimitException.java:
##########
@@ -205,4 +205,67 @@ void testCheckMaxCollectionLengthFromNonZero() {
ex = assertThrows(SystemLimitException.class, () ->
checkMaxCollectionLength(25, 999));
assertEquals("Collection length 1024 exceeds maximum allowed",
ex.getMessage());
}
+
+ @Test
+ void testDecodeDepthDefaultAllowsModerateNestingAndRejectsBeyondLimit() {
+ resetLimits();
+ // Descend exactly to the default limit: all increments must succeed.
+ for (int i = 0; i < DEFAULT_MAX_DECODE_DEPTH; i++) {
+ incrementDecodeDepth();
+ }
+ // One level too deep is rejected with a clear, bounded error (not a
crash).
+ SystemLimitException ex = assertThrows(SystemLimitException.class,
SystemLimitException::incrementDecodeDepth);
+ assertTrue(
+ ex.getMessage().contains("Decode nesting depth exceeds the maximum
allowed of " + DEFAULT_MAX_DECODE_DEPTH),
+ ex.getMessage());
+ // The rejected increment must not have advanced the counter: after
unwinding
+ // all successful descents the depth returns to zero.
+ for (int i = 0; i < DEFAULT_MAX_DECODE_DEPTH; i++) {
+ decrementDecodeDepth();
+ }
+ // Now at zero again; a fresh descent is permitted.
+ incrementDecodeDepth();
+ decrementDecodeDepth();
+ }
+
+ @Test
+ void testDecodeDepthHonoursCustomLimit() {
+ System.setProperty(MAX_DECODE_DEPTH_PROPERTY, "3");
+ resetLimits();
+ incrementDecodeDepth();
+ incrementDecodeDepth();
+ incrementDecodeDepth();
+ SystemLimitException ex = assertThrows(SystemLimitException.class,
SystemLimitException::incrementDecodeDepth);
+ assertTrue(ex.getMessage().contains("maximum allowed of 3"),
ex.getMessage());
+ assertTrue(ex.getMessage().contains(MAX_DECODE_DEPTH_PROPERTY),
ex.getMessage());
+ decrementDecodeDepth();
+ decrementDecodeDepth();
+ decrementDecodeDepth();
+ }
+
+ @Test
+ void testDecodeDepthResetAtOutermostScope() {
+ System.setProperty(MAX_DECODE_DEPTH_PROPERTY, "5");
+ resetLimits();
+ // Simulate a decode that terminated abnormally leaving a stale depth.
+ incrementDecodeDepth();
+ incrementDecodeDepth();
+ // Opening a fresh outermost datum scope must clear the stale depth so the
+ // next decode starts from zero.
+ beginCollectionAllocationScope();
+ try {
+ for (int i = 0; i < 5; i++) {
+ incrementDecodeDepth();
+ }
+ assertThrows(SystemLimitException.class,
SystemLimitException::incrementDecodeDepth);
+ for (int i = 0; i < 5; i++) {
+ decrementDecodeDepth();
+ }
+ } finally {
+ endCollectionAllocationScope();
+ }
+ // Balance the two stale increments left before the scope reset.
+ decrementDecodeDepth();
+ decrementDecodeDepth();
Review Comment:
This test comment is misleading: after `beginCollectionAllocationScope()`
the outermost-scope reset clears `decodeDepth`, so there are no “stale
increments” left to balance. The extra `decrementDecodeDepth()` calls are also
redundant (they currently no-op at depth 0), which makes the intent of the test
harder to follow.
##########
lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java:
##########
@@ -214,15 +214,21 @@ protected Object readWithConversion(Object old, Schema
expected, LogicalType log
protected Object readWithoutConversion(Object old, Schema expected,
ResolvingDecoder in) throws IOException {
switch (expected.getType()) {
case RECORD:
- return readRecord(old, expected, in);
- case ENUM:
- return readEnum(expected, in);
case ARRAY:
- return readArray(old, expected, in);
case MAP:
- return readMap(old, expected, in);
case UNION:
- return read(old, expected.getTypes().get(in.readIndex()), in);
+ // Descending into a structural value grows the decode call stack. Bound
the
+ // nesting depth so a recursive schema fed a deeply nested payload fails
with
+ // a SystemLimitException instead of a StackOverflowError. The counter is
+ // decremented on exit via the finally so it stays balanced even on
error.
+ SystemLimitException.incrementDecodeDepth();
+ try {
+ return readStructural(old, expected, in);
+ } finally {
+ SystemLimitException.decrementDecodeDepth();
+ }
Review Comment:
The new decode-depth guard only wraps the main read path.
`GenericDatumReader.skip(...)` / `skipInternal(...)` still recursively descends
into RECORD/ARRAY/MAP/UNION without calling
`SystemLimitException.incrementDecodeDepth()`, so deeply nested payloads can
still trigger a `StackOverflowError` when skipping (e.g.,
`BinaryData.compare(...)` or when resolving/fast reader skips writer-only
fields). Consider applying the same increment/decrement guard to structural
cases inside `skipInternal(...)` as well.
--
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]
