iemejia commented on code in PR #3928:
URL: https://github.com/apache/avro/pull/3928#discussion_r3737366002
##########
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:
Good catch. The fast reader's skip steps delegate to
`GenericDatumReader.skip(...)`, which now bounds the depth in `skipInternal`
(the structural cases are wrapped with
`incrementDecodeDepth`/`decrementDecodeDepth`). Fixed in 24a1fb5, with a
regression test that a deeply nested payload is rejected when skipped.
##########
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:
Agreed, the comment was misleading and the trailing `decrementDecodeDepth()`
calls were redundant (they no-op at depth 0 after
`beginCollectionAllocationScope()` already reset the stale depth). Rewrote the
test in 24a1fb5 to drop them and clarify that the assertion at exactly the
limit proves the reset happened.
##########
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:
Fixed in 24a1fb5. Applied the same increment/decrement guard to
`skipInternal`'s structural cases (RECORD/ARRAY/MAP/UNION) and to
`BinaryData.compare` (which now also delimits a scope at the top-level
comparison so the depth is reset there). Added regression tests asserting a
deeply nested payload is rejected with a bounded `SystemLimitException` when
skipped and when compared.
--
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]
