This is an automated email from the ASF dual-hosted git repository.
jbonofre pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/karaf.git
The following commit(s) were added to refs/heads/main by this push:
new 1b889e0412 Avoid the non-varargs call warning in AsmProxyFactory
(#2870)
1b889e0412 is described below
commit 1b889e04124d78627a2cb85a58241a949b64b0f3
Author: Holger Friedrich <[email protected]>
AuthorDate: Sat Sep 12 07:26:54 2026 +0200
Avoid the non-varargs call warning in AsmProxyFactory (#2870)
* Avoid the non-varargs call warning in AsmProxyFactory
Object.class.getConstructor(null) is a non-varargs call of a varargs
method, which javac reports in every CI summary. The lookup could not
return anything but the no-arg constructor of Object, so its descriptor
was always the "()V" the variable was already initialized with, and the
NoSuchMethodException was never thrown. Drop the whole construct.
The constant descriptor means the proxied class must have a no-arg
constructor, which nothing verified: the generated INVOKESPECIAL only
failed once the proxy was instantiated. Check it upfront and report it
as an IllegalArgumentException instead.
This PR was created with help of Claude Code.
* Address review findings
* Improve behavior for special cases, extend tests
* Reject final classes and check every proxied interface, not just the first
checkProxyable() validated classesToProxy[0] only, and the interface branch
of createConstructor() skipped it entirely. A public final class with an
accessible no-arg constructor therefore still passed validation and failed
later with a raw VerifyError when the proxy tried to extend it; a non-public
interface passed directly, or as a secondary entry alongside a proxyable
class, likewise still failed later with a raw IllegalAccessError.
Move the call to checkProxyable() up into generateProxy() so it runs for
every entry of classesToProxy, and make it reject final classes and return
early (after the visibility check) for interfaces, which have no
constructor to validate.
---------
Co-authored-by: JB Onofré <[email protected]>
---
.../impl/runtime/proxy/AsmProxyFactory.java | 63 ++++-
.../impl/runtime/proxy/AsmProxyFactoryTest.java | 274 +++++++++++++++++++++
2 files changed, 324 insertions(+), 13 deletions(-)
diff --git
a/services/interceptor/impl/src/main/java/org/apache/karaf/service/interceptor/impl/runtime/proxy/AsmProxyFactory.java
b/services/interceptor/impl/src/main/java/org/apache/karaf/service/interceptor/impl/runtime/proxy/AsmProxyFactory.java
index 9346f3d6d7..ab43be5af4 100644
---
a/services/interceptor/impl/src/main/java/org/apache/karaf/service/interceptor/impl/runtime/proxy/AsmProxyFactory.java
+++
b/services/interceptor/impl/src/main/java/org/apache/karaf/service/interceptor/impl/runtime/proxy/AsmProxyFactory.java
@@ -114,19 +114,11 @@ public class AsmProxyFactory {
private void createConstructor(final ClassWriter cw, final String
proxyClassFileName, final Class<?> classToProxy,
final String classFileName) {
- Constructor superDefaultCt;
- String parentClassFileName = classFileName;
- String descriptor = "()V";
-
- try {
- if (classToProxy.isInterface()) {
- parentClassFileName = Type.getInternalName(Object.class);
- superDefaultCt = Object.class.getConstructor(null);
- descriptor = Type.getConstructorDescriptor(superDefaultCt);
- }
- } catch (final NoSuchMethodException nsme) {
- // no worries
- }
+ // the proxy extends the proxied class, or Object when proxying an
interface; either way
+ // the super constructor it invokes is the no-arg one
+ final String parentClassFileName = classToProxy.isInterface() ?
+ Type.getInternalName(Object.class) : classFileName;
+ final String descriptor = "()V";
MethodVisitor mv = cw.visitMethod(ACC_PUBLIC, "<init>", descriptor,
null, null);
mv.visitCode();
@@ -142,7 +134,52 @@ public class AsmProxyFactory {
mv.visitEnd();
}
+ /**
+ * The proxy is defined by its own class loader, so it lands in a
different runtime package than
+ * the proxied class even when the package names match. It can therefore
only extend or implement a
+ * public type, cannot extend a final class, and the constructor it
invokes must be public or
+ * protected. Checking that upfront turns an IllegalAccessError or
VerifyError raised while the
+ * proxy is linked or instantiated into a readable message.
+ */
+ private void checkProxyable(final Class<?> classToProxy) {
+ if (!Modifier.isPublic(classToProxy.getModifiers())) {
+ throw new IllegalArgumentException("Cannot proxy " +
classToProxy.getName()
+ + ", it is not public and therefore not visible to the
generated proxy");
+ }
+ if (classToProxy.isInterface()) {
+ return;
+ }
+ if (Modifier.isFinal(classToProxy.getModifiers())) {
+ throw new IllegalArgumentException("Cannot proxy " +
classToProxy.getName()
+ + ", it is final and the generated proxy cannot extend
it");
+ }
+
+ final Constructor<?> superCt;
+ try {
+ superCt = classToProxy.getDeclaredConstructor();
+ } catch (final NoSuchMethodException nsme) {
+ throw new IllegalArgumentException("Cannot proxy " +
classToProxy.getName()
+ + ", it has no no-arg constructor", nsme);
+ } catch (final LinkageError le) {
+ // reflecting on the constructors resolves the parameter types of
all of them, which can
+ // fail when one of them is not wired here. Skip the check instead
of rejecting a class
+ // we are probably able to proxy, the no-arg constructor itself
resolves just fine.
+ return;
+ }
+
+ final int modifiers = superCt.getModifiers();
+ if (!Modifier.isPublic(modifiers) && !Modifier.isProtected(modifiers))
{
+ throw new IllegalArgumentException("Cannot proxy " +
classToProxy.getName()
+ + ", its no-arg constructor is not accessible from the
generated proxy");
+ }
+ }
+
private byte[] generateProxy(final Class<?>[] classesToProxy, final String
proxyClassFileName, final Method[] interceptedMethods) {
+ // classesToProxy[0] becomes the superclass (or Object, if it and
every other entry is an
+ // interface) and every entry is added to the proxy's implements
clause when it is an
+ // interface, so all of them must be checked, not just
classesToProxy[0]
+ Stream.of(classesToProxy).forEach(this::checkProxyable);
+
final ClassWriter cw = new ClassWriter(ClassWriter.COMPUTE_FRAMES);
final String classFileName = classesToProxy[0].getName().replace('.',
'/');
diff --git
a/services/interceptor/impl/src/test/java/org/apache/karaf/service/interceptor/impl/runtime/proxy/AsmProxyFactoryTest.java
b/services/interceptor/impl/src/test/java/org/apache/karaf/service/interceptor/impl/runtime/proxy/AsmProxyFactoryTest.java
index 03bcc3f21a..83c5f66df8 100644
---
a/services/interceptor/impl/src/test/java/org/apache/karaf/service/interceptor/impl/runtime/proxy/AsmProxyFactoryTest.java
+++
b/services/interceptor/impl/src/test/java/org/apache/karaf/service/interceptor/impl/runtime/proxy/AsmProxyFactoryTest.java
@@ -23,6 +23,7 @@ import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import java.io.IOException;
+import java.io.InputStream;
import org.junit.Test;
@@ -56,6 +57,279 @@ public class AsmProxyFactoryTest {
}
}
+ @Test
+ public void proxyInterface() {
+ final ProxyFactory.ProxyClassLoader classLoader = new
ProxyFactory.ProxyClassLoader(Thread.currentThread().getContextClassLoader(),
null);
+ final AsmProxyFactory factory = new AsmProxyFactory();
+ final Class<?> proxyClass = factory.createProxyClass(
+ classLoader, Bar.class.getName() + "$$ProxyTestProxy2",
+ new Class<?>[]{Bar.class},
+ Bar.class.getDeclaredMethods());
+ assertNotNull(proxyClass);
+
+ // an interface proxy extends Object, so this also covers the
generated no-arg constructor
+ final Bar instance = Bar.class.cast(factory.create(proxyClass,
+ (method, args) -> method.getName() + "(" + asList(args) +
")"));
+ assertEquals("bar([])", instance.bar());
+ assertEquals("baz([param])", instance.baz("param"));
+ }
+
+ @Test
+ public void proxyWithoutNoArgConstructor() {
+ final ProxyFactory.ProxyClassLoader classLoader = new
ProxyFactory.ProxyClassLoader(Thread.currentThread().getContextClassLoader(),
null);
+ final AsmProxyFactory factory = new AsmProxyFactory();
+ try {
+ factory.createProxyClass(
+ classLoader, Unproxyable.class.getName() +
"$$ProxyTestProxy3",
+ new Class<?>[]{Unproxyable.class},
+ Unproxyable.class.getDeclaredMethods());
+ fail();
+ } catch (final IllegalArgumentException iae) {
+ assertEquals("Cannot proxy " + Unproxyable.class.getName() + ", it
has no no-arg constructor",
+ iae.getMessage());
+ }
+ }
+
+ @Test
+ public void proxyWithPackagePrivateConstructor() {
+ final ProxyFactory.ProxyClassLoader classLoader = new
ProxyFactory.ProxyClassLoader(Thread.currentThread().getContextClassLoader(),
null);
+ final AsmProxyFactory factory = new AsmProxyFactory();
+ try {
+ factory.createProxyClass(
+ classLoader, PackagePrivateConstructor.class.getName() +
"$$ProxyTestProxy4",
+ new Class<?>[]{PackagePrivateConstructor.class},
+ PackagePrivateConstructor.class.getDeclaredMethods());
+ fail();
+ } catch (final IllegalArgumentException iae) {
+ assertEquals("Cannot proxy " +
PackagePrivateConstructor.class.getName()
+ + ", its no-arg constructor is not accessible from
the generated proxy",
+ iae.getMessage());
+ }
+ }
+
+ @Test
+ public void proxyWithProtectedConstructor() {
+ final ProxyFactory.ProxyClassLoader classLoader = new
ProxyFactory.ProxyClassLoader(Thread.currentThread().getContextClassLoader(),
null);
+ final AsmProxyFactory factory = new AsmProxyFactory();
+ final Class<?> proxyClass = factory.createProxyClass(
+ classLoader, ProtectedConstructor.class.getName() +
"$$ProxyTestProxy5",
+ new Class<?>[]{ProtectedConstructor.class},
+ ProtectedConstructor.class.getDeclaredMethods());
+ assertNotNull(proxyClass);
+
+ // a protected super constructor is reachable from a subclass, even in
another runtime package
+ final ProtectedConstructor instance =
ProtectedConstructor.class.cast(factory.create(proxyClass,
+ (method, args) -> method.getName() + "(" + asList(args) +
")"));
+ assertEquals("some([])", instance.some());
+ }
+
+ @Test
+ public void proxyOfNonPublicClass() {
+ final ProxyFactory.ProxyClassLoader classLoader = new
ProxyFactory.ProxyClassLoader(Thread.currentThread().getContextClassLoader(),
null);
+ final AsmProxyFactory factory = new AsmProxyFactory();
+ try {
+ factory.createProxyClass(
+ classLoader, NotPublic.class.getName() +
"$$ProxyTestProxy6",
+ new Class<?>[]{NotPublic.class},
+ NotPublic.class.getDeclaredMethods());
+ fail();
+ } catch (final IllegalArgumentException iae) {
+ assertEquals("Cannot proxy " + NotPublic.class.getName()
+ + ", it is not public and therefore not visible to
the generated proxy",
+ iae.getMessage());
+ }
+ }
+
+ @Test
+ public void proxyOfFinalClass() {
+ final ProxyFactory.ProxyClassLoader classLoader = new
ProxyFactory.ProxyClassLoader(Thread.currentThread().getContextClassLoader(),
null);
+ final AsmProxyFactory factory = new AsmProxyFactory();
+ try {
+ factory.createProxyClass(
+ classLoader, FinalClass.class.getName() +
"$$ProxyTestProxy8",
+ new Class<?>[]{FinalClass.class},
+ FinalClass.class.getDeclaredMethods());
+ fail();
+ } catch (final IllegalArgumentException iae) {
+ assertEquals("Cannot proxy " + FinalClass.class.getName()
+ + ", it is final and the generated proxy cannot
extend it",
+ iae.getMessage());
+ }
+ }
+
+ @Test
+ public void proxyOfNonPublicInterface() {
+ final ProxyFactory.ProxyClassLoader classLoader = new
ProxyFactory.ProxyClassLoader(Thread.currentThread().getContextClassLoader(),
null);
+ final AsmProxyFactory factory = new AsmProxyFactory();
+ try {
+ factory.createProxyClass(
+ classLoader, NotPublicInterface.class.getName() +
"$$ProxyTestProxy9",
+ new Class<?>[]{NotPublicInterface.class},
+ NotPublicInterface.class.getDeclaredMethods());
+ fail();
+ } catch (final IllegalArgumentException iae) {
+ assertEquals("Cannot proxy " + NotPublicInterface.class.getName()
+ + ", it is not public and therefore not visible to
the generated proxy",
+ iae.getMessage());
+ }
+ }
+
+ @Test
+ public void proxyWithNonPublicSecondaryInterface() {
+ // classesToProxy[0] (Foo) alone is proxyable; the check must still
walk the rest of the
+ // array, since every interface in it is added to the proxy's
implements clause
+ final ProxyFactory.ProxyClassLoader classLoader = new
ProxyFactory.ProxyClassLoader(Thread.currentThread().getContextClassLoader(),
null);
+ final AsmProxyFactory factory = new AsmProxyFactory();
+ try {
+ factory.createProxyClass(
+ classLoader, Foo.class.getName() + "$$ProxyTestProxy10",
+ new Class<?>[]{Foo.class, NotPublicInterface.class},
+ Foo.class.getDeclaredMethods());
+ fail();
+ } catch (final IllegalArgumentException iae) {
+ assertEquals("Cannot proxy " + NotPublicInterface.class.getName()
+ + ", it is not public and therefore not visible to
the generated proxy",
+ iae.getMessage());
+ }
+ }
+
+ @Test
+ public void proxyWithUnresolvableConstructorParameter() throws Exception {
+ // reflecting on the constructors resolves the parameter types of all
of them, so a type which
+ // is not wired here must not make the proxyability checks reject an
otherwise fine class
+ final ClassLoader blocking = new
BlockingClassLoader(Thread.currentThread().getContextClassLoader(),
+ Absent.class.getName(), UnresolvableParameter.class.getName());
+ final Class<?> classToProxy =
blocking.loadClass(UnresolvableParameter.class.getName());
+ final ProxyFactory.ProxyClassLoader classLoader = new
ProxyFactory.ProxyClassLoader(blocking, null);
+ final AsmProxyFactory factory = new AsmProxyFactory();
+ final Class<?> proxyClass = factory.createProxyClass(
+ classLoader, UnresolvableParameter.class.getName() +
"$$ProxyTestProxy7",
+ new Class<?>[]{classToProxy},
+ classToProxy.getDeclaredMethods());
+ assertNotNull(proxyClass);
+
+ // the proxied class comes from another loader, so the instance is
driven reflectively
+ final Object instance = factory.create(proxyClass,
+ (method, args) -> method.getName() + "(" + asList(args) + ")");
+ assertEquals("some([])",
proxyClass.getMethod("some").invoke(instance));
+ }
+
+ public interface Bar {
+ String bar();
+
+ String baz(String some);
+ }
+
+ public static class Unproxyable {
+ public Unproxyable(final String some) {
+ // no no-arg constructor on purpose
+ }
+
+ public String some() {
+ return "some";
+ }
+ }
+
+ public static class PackagePrivateConstructor {
+ PackagePrivateConstructor() {
+ // not accessible from the generated proxy on purpose
+ }
+
+ public String some() {
+ return "some";
+ }
+ }
+
+ public static class ProtectedConstructor {
+ protected ProtectedConstructor() {
+ // no-op
+ }
+
+ public String some() {
+ return "some";
+ }
+ }
+
+ static class NotPublic {
+ // an accessible constructor is not enough as long as the class itself
is not visible
+ public NotPublic() {
+ // no-op
+ }
+
+ public String some() {
+ return "some";
+ }
+ }
+
+ public static final class FinalClass {
+ public FinalClass() {
+ // no-op
+ }
+
+ public String some() {
+ return "some";
+ }
+ }
+
+ interface NotPublicInterface {
+ String some();
+ }
+
+ public static class Absent {
+ }
+
+ public static class UnresolvableParameter {
+ public UnresolvableParameter() {
+ // the one the proxy invokes
+ }
+
+ public UnresolvableParameter(final Absent absent) {
+ // its parameter type is hidden by BlockingClassLoader
+ }
+
+ public String some() {
+ return "some";
+ }
+ }
+
+ /**
+ * Defines {@code reloaded} itself and refuses {@code blocked}, to mimic a
type which is not wired
+ * in the bundle the proxied class comes from.
+ */
+ static class BlockingClassLoader extends ClassLoader {
+ private final String blocked;
+ private final String reloaded;
+
+ BlockingClassLoader(final ClassLoader parent, final String blocked,
final String reloaded) {
+ super(parent);
+ this.blocked = blocked;
+ this.reloaded = reloaded;
+ }
+
+ @Override
+ protected Class<?> loadClass(final String name, final boolean resolve)
throws ClassNotFoundException {
+ if (blocked.equals(name)) {
+ throw new ClassNotFoundException(name);
+ }
+ if (!reloaded.equals(name)) {
+ return super.loadClass(name, resolve);
+ }
+ Class<?> clazz = findLoadedClass(name);
+ if (clazz == null) {
+ try (final InputStream stream =
getParent().getResourceAsStream(name.replace('.', '/') + ".class")) {
+ final byte[] bytes = stream.readAllBytes();
+ clazz = defineClass(name, bytes, 0, bytes.length);
+ } catch (final IOException e) {
+ throw new ClassNotFoundException(name, e);
+ }
+ }
+ if (resolve) {
+ resolveClass(clazz);
+ }
+ return clazz;
+ }
+ }
+
public static class Foo {
public String foo1() {
return "first";
