From 0ed51da67e63a1fafb0ab95ca2320efedcf9aa78 Mon Sep 17 00:00:00 2001 From: Georgios Andrianakis Date: Wed, 7 Jul 2021 14:58:47 +0300 Subject: [PATCH] Prevent non-public classes of method params from breaking interceptor handling Fixes: #18477 --- .../io/quarkus/arc/processor/BeanInfo.java | 12 +++-- .../io/quarkus/arc/processor/Methods.java | 54 ++++++++++++++++--- .../processor/SubclassSkipPredicateTest.java | 4 +- .../methodargs/CustomExecutor.java | 10 ++++ .../methodargs/ExampleResource.java | 26 +++++++++ .../MethodArgsInterceptionTest.java | 38 +++++++++++++ .../test/interceptors/methodargs/Simple.java | 18 +++++++ .../methodargs/SimpleInterceptor.java | 23 ++++++++ .../methodargs/base/BaseExecutor.java | 16 ++++++ 9 files changed, 186 insertions(+), 15 deletions(-) create mode 100644 independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/CustomExecutor.java create mode 100644 independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/ExampleResource.java create mode 100644 independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/MethodArgsInterceptionTest.java create mode 100644 independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/Simple.java create mode 100644 independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/SimpleInterceptor.java create mode 100644 independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/base/BaseExecutor.java diff --git a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java index 43a6b4647ff26..9c92eb089fffa 100644 --- a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java +++ b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java @@ -534,8 +534,10 @@ private Map initDecoratedMethods() { Collections.sort(bound, Comparator.comparingInt(DecoratorInfo::getPriority).thenComparing(DecoratorInfo::getBeanClass)); Map candidates = new HashMap<>(); - addDecoratedMethods(candidates, target.get().asClass(), bound, - new SubclassSkipPredicate(beanDeployment.getAssignabilityCheck()::isAssignableFrom)); + ClassInfo classInfo = target.get().asClass(); + addDecoratedMethods(candidates, classInfo, classInfo, bound, + new SubclassSkipPredicate(beanDeployment.getAssignabilityCheck()::isAssignableFrom, + beanDeployment.getBeanArchiveIndex())); Map decoratedMethods = new HashMap<>(candidates.size()); for (Entry entry : candidates.entrySet()) { @@ -545,8 +547,8 @@ private Map initDecoratedMethods() { } private void addDecoratedMethods(Map decoratedMethods, ClassInfo classInfo, - List boundDecorators, SubclassSkipPredicate skipPredicate) { - skipPredicate.startProcessing(classInfo); + ClassInfo originalClassInfo, List boundDecorators, SubclassSkipPredicate skipPredicate) { + skipPredicate.startProcessing(classInfo, originalClassInfo); for (MethodInfo method : classInfo.methods()) { if (skipPredicate.test(method)) { continue; @@ -560,7 +562,7 @@ private void addDecoratedMethods(Map decoratedMethods if (!classInfo.superName().equals(DotNames.OBJECT)) { ClassInfo superClassInfo = getClassByName(beanDeployment.getBeanArchiveIndex(), classInfo.superName()); if (superClassInfo != null) { - addDecoratedMethods(decoratedMethods, superClassInfo, boundDecorators, skipPredicate); + addDecoratedMethods(decoratedMethods, superClassInfo, originalClassInfo, boundDecorators, skipPredicate); } } } diff --git a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java index fea42a8b7725a..d9d9c41c647f1 100644 --- a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java +++ b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java @@ -159,19 +159,22 @@ static Set addInterceptedMethodCandidates(BeanDeployment beanDeploym Map> candidates, List classLevelBindings, Consumer bytecodeTransformerConsumer, boolean transformUnproxyableClasses) { - return addInterceptedMethodCandidates(beanDeployment, classInfo, candidates, classLevelBindings, + return addInterceptedMethodCandidates(beanDeployment, classInfo, classInfo, candidates, classLevelBindings, bytecodeTransformerConsumer, transformUnproxyableClasses, - new SubclassSkipPredicate(beanDeployment.getAssignabilityCheck()::isAssignableFrom), false); + new SubclassSkipPredicate(beanDeployment.getAssignabilityCheck()::isAssignableFrom, + beanDeployment.getBeanArchiveIndex()), + false); } static Set addInterceptedMethodCandidates(BeanDeployment beanDeployment, ClassInfo classInfo, + ClassInfo originalClassInfo, Map> candidates, List classLevelBindings, Consumer bytecodeTransformerConsumer, boolean transformUnproxyableClasses, SubclassSkipPredicate skipPredicate, boolean ignoreMethodLevelBindings) { Set methodsFromWhichToRemoveFinal = new HashSet<>(); Set finalMethodsFoundAndNotChanged = new HashSet<>(); - skipPredicate.startProcessing(classInfo); + skipPredicate.startProcessing(classInfo, originalClassInfo); for (MethodInfo method : classInfo.methods()) { if (skipPredicate.test(method)) { @@ -220,7 +223,7 @@ static Set addInterceptedMethodCandidates(BeanDeployment beanDeploym ClassInfo superClassInfo = getClassByName(beanDeployment.getBeanArchiveIndex(), classInfo.superName()); if (superClassInfo != null) { finalMethodsFoundAndNotChanged - .addAll(addInterceptedMethodCandidates(beanDeployment, superClassInfo, candidates, + .addAll(addInterceptedMethodCandidates(beanDeployment, superClassInfo, classInfo, candidates, classLevelBindings, bytecodeTransformerConsumer, transformUnproxyableClasses, skipPredicate, ignoreMethodLevelBindings)); } @@ -230,7 +233,7 @@ static Set addInterceptedMethodCandidates(BeanDeployment beanDeploym ClassInfo interfaceInfo = getClassByName(beanDeployment.getBeanArchiveIndex(), i); if (interfaceInfo != null) { //interfaces can't have final methods - addInterceptedMethodCandidates(beanDeployment, interfaceInfo, candidates, + addInterceptedMethodCandidates(beanDeployment, interfaceInfo, originalClassInfo, candidates, classLevelBindings, bytecodeTransformerConsumer, transformUnproxyableClasses, skipPredicate, true); } @@ -448,22 +451,27 @@ public MethodVisitor visitMethod(int access, String name, String descriptor, Str /** * This stateful predicate can be used to skip methods that should not be added to the generated subclass. *

- * Don't forget to call {@link SubclassSkipPredicate#startProcessing(ClassInfo)} before the methods are processed and + * Don't forget to call {@link SubclassSkipPredicate#startProcessing(ClassInfo, ClassInfo)} before the methods are processed + * and * {@link SubclassSkipPredicate#methodsProcessed()} afterwards. */ static class SubclassSkipPredicate implements Predicate { private final BiFunction assignableFromFun; + private final IndexView beanArchiveIndex; private ClassInfo clazz; + private ClassInfo originalClazz; private List regularMethods; private Set bridgeMethods = new HashSet<>(); - public SubclassSkipPredicate(BiFunction assignableFromFun) { + public SubclassSkipPredicate(BiFunction assignableFromFun, IndexView beanArchiveIndex) { this.assignableFromFun = assignableFromFun; + this.beanArchiveIndex = beanArchiveIndex; } - void startProcessing(ClassInfo clazz) { + void startProcessing(ClassInfo clazz, ClassInfo originalClazz) { this.clazz = clazz; + this.originalClazz = originalClazz; this.regularMethods = new ArrayList<>(); for (MethodInfo method : clazz.methods()) { if (!Modifier.isAbstract(method.flags()) && !method.isSynthetic() && !isBridge(method)) { @@ -505,10 +513,40 @@ public boolean test(MethodInfo method) { // Do not skip default methods - public non-abstract instance methods declared in an interface return false; } + + List parameters = method.parameters(); + if (!parameters.isEmpty() && (beanArchiveIndex != null)) { + String originalClassPackage = determinePackage(originalClazz.name()); + for (Type type : parameters) { + ClassInfo parameterClassInfo = beanArchiveIndex.getClassByName(type.name()); + if (parameterClassInfo == null) { + continue; // hope for the best + } + if (Modifier.isPrivate(parameterClassInfo.flags())) { + return true; // parameters whose class is private can not be loaded, as we would end up with IllegalAccessError when trying to access the use the load the class + } + if (!Modifier.isPublic(parameterClassInfo.flags())) { + if (determinePackage(parameterClassInfo.name()).equals(originalClassPackage)) { + return false; + } + // parameters whose class is package-private and the package is not the same as the package of the method for which we are checking can not be loaded, + // as we would end up with IllegalAccessError when trying to access the use the load the class + return true; + } + } + } + // Note that we intentionally do not skip final methods here - these are handled later return false; } + private String determinePackage(DotName dotName) { + if (dotName.isInner()) { + dotName = dotName.prefix(); + } + return dotName.prefix() == null ? "" : dotName.prefix().toString(); + } + private boolean hasImplementation(MethodInfo bridge) { for (MethodInfo declaredMethod : regularMethods) { if (bridge.name().equals(declaredMethod.name())) { diff --git a/independent-projects/arc/processor/src/test/java/io/quarkus/arc/processor/SubclassSkipPredicateTest.java b/independent-projects/arc/processor/src/test/java/io/quarkus/arc/processor/SubclassSkipPredicateTest.java index c7b0cb88cd939..b5089205f11e3 100644 --- a/independent-projects/arc/processor/src/test/java/io/quarkus/arc/processor/SubclassSkipPredicateTest.java +++ b/independent-projects/arc/processor/src/test/java/io/quarkus/arc/processor/SubclassSkipPredicateTest.java @@ -21,10 +21,10 @@ public class SubclassSkipPredicateTest { public void testPredicate() throws IOException { IndexView index = Basics.index(Base.class, Submarine.class, Long.class, Number.class); AssignabilityCheck assignabilityCheck = new AssignabilityCheck(index, null); - SubclassSkipPredicate predicate = new SubclassSkipPredicate(assignabilityCheck::isAssignableFrom); + SubclassSkipPredicate predicate = new SubclassSkipPredicate(assignabilityCheck::isAssignableFrom, null); ClassInfo submarineClass = index.getClassByName(DotName.createSimple(Submarine.class.getName())); - predicate.startProcessing(submarineClass); + predicate.startProcessing(submarineClass, submarineClass); List echos = submarineClass.methods().stream().filter(m -> m.name().equals("echo")) .collect(Collectors.toList()); diff --git a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/CustomExecutor.java b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/CustomExecutor.java new file mode 100644 index 0000000000000..d98f5c5aeb5e7 --- /dev/null +++ b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/CustomExecutor.java @@ -0,0 +1,10 @@ +package io.quarkus.arc.test.interceptors.methodargs; + +import io.quarkus.arc.test.interceptors.methodargs.base.BaseExecutor; +import javax.inject.Singleton; + +@Singleton +@Simple +public class CustomExecutor extends BaseExecutor { + +} diff --git a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/ExampleResource.java b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/ExampleResource.java new file mode 100644 index 0000000000000..9310e2c6d5d32 --- /dev/null +++ b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/ExampleResource.java @@ -0,0 +1,26 @@ +package io.quarkus.arc.test.interceptors.methodargs; + +import java.util.List; +import javax.inject.Singleton; + +@Singleton +@Simple +public class ExampleResource { + + public String create(List strings) { + return String.join(",", strings); + } + + String otherCreate(PackagePrivate packagePrivate) { + return packagePrivate.toString(); + } + + static class PackagePrivate { + + @Override + public String toString() { + return "PackagePrivate{}"; + } + } + +} diff --git a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/MethodArgsInterceptionTest.java b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/MethodArgsInterceptionTest.java new file mode 100644 index 0000000000000..6fc27dfed31c7 --- /dev/null +++ b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/MethodArgsInterceptionTest.java @@ -0,0 +1,38 @@ +package io.quarkus.arc.test.interceptors.methodargs; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import io.quarkus.arc.Arc; +import io.quarkus.arc.ArcContainer; +import io.quarkus.arc.test.ArcTestContainer; +import io.quarkus.arc.test.interceptors.Counter; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +public class MethodArgsInterceptionTest { + + @RegisterExtension + public ArcTestContainer container = new ArcTestContainer(Simple.class, SimpleInterceptor.class, ExampleResource.class, + CustomExecutor.class, Counter.class); + + @Test + public void testInterception() { + ArcContainer container = Arc.container(); + Counter counter = container.instance(Counter.class).get(); + + counter.reset(); + ExampleResource exampleResource = container.instance(ExampleResource.class).get(); + + assertEquals("first,second", exampleResource.create(List.of("first", "second"))); + assertEquals(1, counter.get()); + + assertEquals("PackagePrivate{}", exampleResource.otherCreate(new ExampleResource.PackagePrivate())); + assertEquals(2, counter.get()); + + CustomExecutor customThreadExecutor = container.instance(CustomExecutor.class).get(); + assertEquals("run", customThreadExecutor.run()); + assertEquals(3, counter.get()); + } + +} diff --git a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/Simple.java b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/Simple.java new file mode 100644 index 0000000000000..04878c01a10a9 --- /dev/null +++ b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/Simple.java @@ -0,0 +1,18 @@ +package io.quarkus.arc.test.interceptors.methodargs; + +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import java.lang.annotation.Documented; +import java.lang.annotation.Retention; +import java.lang.annotation.Target; +import javax.interceptor.InterceptorBinding; + +@Target({ TYPE, METHOD }) +@Retention(RUNTIME) +@Documented +@InterceptorBinding +public @interface Simple { + +} diff --git a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/SimpleInterceptor.java b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/SimpleInterceptor.java new file mode 100644 index 0000000000000..d0c3d0eed7f14 --- /dev/null +++ b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/SimpleInterceptor.java @@ -0,0 +1,23 @@ +package io.quarkus.arc.test.interceptors.methodargs; + +import io.quarkus.arc.test.interceptors.Counter; +import javax.annotation.Priority; +import javax.inject.Inject; +import javax.interceptor.AroundInvoke; +import javax.interceptor.Interceptor; +import javax.interceptor.InvocationContext; + +@Simple +@Priority(1) +@Interceptor +public class SimpleInterceptor { + + @Inject + Counter counter; + + @AroundInvoke + Object mySuperCoolAroundInvoke(InvocationContext ctx) throws Exception { + counter.incrementAndGet(); + return ctx.proceed(); + } +} diff --git a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/base/BaseExecutor.java b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/base/BaseExecutor.java new file mode 100644 index 0000000000000..6b01599bc0ca9 --- /dev/null +++ b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/methodargs/base/BaseExecutor.java @@ -0,0 +1,16 @@ +package io.quarkus.arc.test.interceptors.methodargs.base; + +public class BaseExecutor { + + public String run() { + return "run"; + } + + protected void runWorker(Worker run) { + + } + + static class Worker { + + } +}