From 982b3d56cbb2ae8573c7e0b1ed0c4301e0ee4836 Mon Sep 17 00:00:00 2001 From: SirYwell Date: Wed, 15 Dec 2021 20:52:30 +0100 Subject: [PATCH 1/2] prevent recursive visiting of cyclic annotation/package structures --- .../java/JavaReflectionTreeBuilder.java | 35 ++++++++++++++++--- .../PackageRuntimeBuilderContext.java | 4 +++ .../java/JavaReflectionTreeBuilderTest.java | 17 +++++++++ .../java/spoon/test/pkg/cyclic/Outside.java | 9 +++++ .../spoon/test/pkg/cyclic/direct/Cyclic.java | 8 +++++ .../test/pkg/cyclic/direct/package-info.java | 5 +++ .../test/pkg/cyclic/indirect/Indirect.java | 8 +++++ .../pkg/cyclic/indirect/package-info.java | 4 +++ 8 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 src/test/java/spoon/test/pkg/cyclic/Outside.java create mode 100644 src/test/java/spoon/test/pkg/cyclic/direct/Cyclic.java create mode 100644 src/test/java/spoon/test/pkg/cyclic/direct/package-info.java create mode 100644 src/test/java/spoon/test/pkg/cyclic/indirect/Indirect.java create mode 100644 src/test/java/spoon/test/pkg/cyclic/indirect/package-info.java diff --git a/src/main/java/spoon/support/visitor/java/JavaReflectionTreeBuilder.java b/src/main/java/spoon/support/visitor/java/JavaReflectionTreeBuilder.java index 21413212031..0a201e1781f 100644 --- a/src/main/java/spoon/support/visitor/java/JavaReflectionTreeBuilder.java +++ b/src/main/java/spoon/support/visitor/java/JavaReflectionTreeBuilder.java @@ -20,6 +20,7 @@ import java.util.ArrayDeque; import java.util.Arrays; import java.util.Deque; +import java.util.Iterator; import java.util.Set; import spoon.reflect.code.CtLiteral; import spoon.reflect.declaration.CtAnnotation; @@ -132,15 +133,39 @@ public > R scan(Class clazz) { @Override public void visitPackage(Package aPackage) { - final CtPackage ctPackage = factory.Package().getOrCreate(aPackage.getName()); - - enter(new PackageRuntimeBuilderContext(ctPackage)); - super.visitPackage(aPackage); - exit(); + CtPackage ctPackage = factory.Package().get(aPackage.getName()); + // this is a dangerous section: + // we DON'T want to visit packages recursively if there are cyclic annotations + // => we only call the super method if: + // - the package is not known by the factory (it wasn't visited before) + // - the package is not in the current context stack + if (ctPackage == null || shouldVisitPackage(ctPackage)) { + ctPackage = factory.Package().getOrCreate(aPackage.getName()); + enter(new PackageRuntimeBuilderContext(ctPackage)); + super.visitPackage(aPackage); + exit(); + } contexts.peek().addPackage(ctPackage); } + // Returns whether the given package is already in the context stack + private boolean shouldVisitPackage(CtPackage ctPackage) { + Iterator iterator = contexts.iterator(); + while (iterator.hasNext()) { + RuntimeBuilderContext next = iterator.next(); + // we don't want to visit the context inserted first, as it's always + // a PackageRuntimeBuilderContext (see scan(...)) but it does not visit + // the package. So yes, the hasNext check is intended here + if (iterator.hasNext() && next instanceof PackageRuntimeBuilderContext) { + if (((PackageRuntimeBuilderContext) next).getPackage() == ctPackage) { + return false; + } + } + } + return true; + } + @Override public void visitClass(Class clazz) { final CtClass ctClass = factory.Core().createClass(); diff --git a/src/main/java/spoon/support/visitor/java/internal/PackageRuntimeBuilderContext.java b/src/main/java/spoon/support/visitor/java/internal/PackageRuntimeBuilderContext.java index e13b1479f39..da994ef7aa3 100644 --- a/src/main/java/spoon/support/visitor/java/internal/PackageRuntimeBuilderContext.java +++ b/src/main/java/spoon/support/visitor/java/internal/PackageRuntimeBuilderContext.java @@ -21,6 +21,10 @@ public PackageRuntimeBuilderContext(CtPackage ctPackage) { this.ctPackage = ctPackage; } + public CtPackage getPackage() { + return ctPackage; + } + @Override public void addType(CtType aType) { ctPackage.addType(aType); diff --git a/src/test/java/spoon/support/visitor/java/JavaReflectionTreeBuilderTest.java b/src/test/java/spoon/support/visitor/java/JavaReflectionTreeBuilderTest.java index 66a1a914765..cf542f96986 100644 --- a/src/test/java/spoon/support/visitor/java/JavaReflectionTreeBuilderTest.java +++ b/src/test/java/spoon/support/visitor/java/JavaReflectionTreeBuilderTest.java @@ -16,6 +16,7 @@ */ package spoon.support.visitor.java; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -93,6 +94,9 @@ import spoon.support.visitor.equals.EqualsVisitor; import spoon.test.generics.testclasses3.ComparableComparatorBug; import spoon.test.pkg.PackageTest; +import spoon.test.pkg.cyclic.Outside; +import spoon.test.pkg.cyclic.direct.Cyclic; +import spoon.test.pkg.cyclic.indirect.Indirect; public class JavaReflectionTreeBuilderTest { @@ -739,4 +743,17 @@ void testShadowPackage() { assertEquals(1, ctPackage.getAnnotations().size()); assertEquals(ctPackage.getAnnotations().get(0).getAnnotationType().getQualifiedName(), "java.lang.Deprecated"); } + + @Test + void testCyclicAnnotationScanning() { + // contract: scanning annotations does not cause StackOverflowError + // due to recursive package -> annotation -> package -> annotation scanning + Factory factory = createFactory(); + // a simple cycle: package a -> annotation a.A -> package a + assertDoesNotThrow(() -> new JavaReflectionTreeBuilder(factory).scan(Cyclic.class)); + // an indirect cycle: package a -> annotation b.B -> package b -> annotation a.A -> package a + assertDoesNotThrow(() -> new JavaReflectionTreeBuilder(factory).scan(Indirect.class)); + // an independent starting point, causing Cyclic and Indirect to be visited too + assertDoesNotThrow(() -> new JavaReflectionTreeBuilder(factory).scan(Outside.class)); + } } diff --git a/src/test/java/spoon/test/pkg/cyclic/Outside.java b/src/test/java/spoon/test/pkg/cyclic/Outside.java new file mode 100644 index 00000000000..b6eef4e5d5a --- /dev/null +++ b/src/test/java/spoon/test/pkg/cyclic/Outside.java @@ -0,0 +1,9 @@ +package spoon.test.pkg.cyclic; + +import spoon.test.pkg.cyclic.direct.Cyclic; +import spoon.test.pkg.cyclic.indirect.Indirect; + +@Cyclic +@Indirect +public class Outside { +} diff --git a/src/test/java/spoon/test/pkg/cyclic/direct/Cyclic.java b/src/test/java/spoon/test/pkg/cyclic/direct/Cyclic.java new file mode 100644 index 00000000000..1ce1ddf41ee --- /dev/null +++ b/src/test/java/spoon/test/pkg/cyclic/direct/Cyclic.java @@ -0,0 +1,8 @@ +package spoon.test.pkg.cyclic.direct; + +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; + +@Retention(RetentionPolicy.RUNTIME) +public @interface Cyclic { +} diff --git a/src/test/java/spoon/test/pkg/cyclic/direct/package-info.java b/src/test/java/spoon/test/pkg/cyclic/direct/package-info.java new file mode 100644 index 00000000000..8789691fe90 --- /dev/null +++ b/src/test/java/spoon/test/pkg/cyclic/direct/package-info.java @@ -0,0 +1,5 @@ +@Cyclic +@Indirect +package spoon.test.pkg.cyclic.direct; + +import spoon.test.pkg.cyclic.indirect.Indirect; diff --git a/src/test/java/spoon/test/pkg/cyclic/indirect/Indirect.java b/src/test/java/spoon/test/pkg/cyclic/indirect/Indirect.java new file mode 100644 index 00000000000..d4d7014c448 --- /dev/null +++ b/src/test/java/spoon/test/pkg/cyclic/indirect/Indirect.java @@ -0,0 +1,8 @@ +package spoon.test.pkg.cyclic.indirect; + +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; + +@Retention(RetentionPolicy.RUNTIME) +public @interface Indirect { +} diff --git a/src/test/java/spoon/test/pkg/cyclic/indirect/package-info.java b/src/test/java/spoon/test/pkg/cyclic/indirect/package-info.java new file mode 100644 index 00000000000..d83841b2e60 --- /dev/null +++ b/src/test/java/spoon/test/pkg/cyclic/indirect/package-info.java @@ -0,0 +1,4 @@ +@Cyclic +package spoon.test.pkg.cyclic.indirect; + +import spoon.test.pkg.cyclic.direct.Cyclic; From f5e6652e4d702201810ed958d501c435f0504f59 Mon Sep 17 00:00:00 2001 From: SirYwell Date: Wed, 15 Dec 2021 21:50:06 +0100 Subject: [PATCH 2/2] add javadoc --- .../visitor/java/internal/PackageRuntimeBuilderContext.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/java/spoon/support/visitor/java/internal/PackageRuntimeBuilderContext.java b/src/main/java/spoon/support/visitor/java/internal/PackageRuntimeBuilderContext.java index da994ef7aa3..06ce0c7ddf7 100644 --- a/src/main/java/spoon/support/visitor/java/internal/PackageRuntimeBuilderContext.java +++ b/src/main/java/spoon/support/visitor/java/internal/PackageRuntimeBuilderContext.java @@ -21,6 +21,11 @@ public PackageRuntimeBuilderContext(CtPackage ctPackage) { this.ctPackage = ctPackage; } + /** + * Returns the package belonging to this context. + * + * @return the package of this context + */ public CtPackage getPackage() { return ctPackage; }