CglibAopProxy needs to detect package-visible methods when defined in a different ClassLoader [SPR-11618]

[spring-projects-issues]

Alina Mylka opened SPR-11618 and commented

The CglibAopProxy.getProxy(ClassLoader classLoader) method should ignore the ClassLoader passed as the parameter and instead use the classloader of the parent class.

Instead of this block of code that starts at line 178 in CglibAopProxy.java:

Enhancer enhancer = createEnhancer();
if (classLoader != null) {
	enhancer.setClassLoader(classLoader);	

I think we should have:

Enhancer enhancer = createEnhancer();
enhancer.setClassLoader(proxySuperClass.getClassLoader());

I have an .ear app. I deploy it on Websphere using the Module classloader policy. This means that classes from JARs that live directly in the lib folder of the .ear are loaded by the "application classloader". Additionally, each .war has its own classloader. Classes that live in the .war files are loaded by the .war classloaders. When requests are processed, the .war classloader is set as the context classloader of the thread that processes the request.

I'm attaching a maven project that generates such an .ear. When you build it and deploy it on Websphere with the Module Classloader Policy, and then go to http://localhost:9080/multiclassloader/index.xhtml - you'll get a null pointer exception coming from the PhaseListener. The session-scoped bean, gets processed, the CGLIB proxy of the bean class is generated, but the proxy doesn't work. In the debugger, it looks especially weird:

@Component
@Scope(value = "session", proxyMode = ScopedProxyMode.TARGET_CLASS)
public class SynchronizerTokenState implements Serializable {
    private Map<String, String> visitedTokenMap;
	public SynchronizerTokenState(){visitedTokenMap = new HashMap<>();}
    Map<String, String> getVisitedTokenMap() {
        return visitedTokenMap;
    }
}

... but the visitedTokenMap is NULL.

I debugged it a little and it seems that the problem is very similar to the one described on stack overflow. The hack suggested over there as the workaround doesn't seem to work for me though. The description of the behavior fits: the proxy is generated using the context classloader of the thread that processes the request, which is the .war classloader.

http://stackoverflow.com/questions/11574049/cglib-with-spring-throws-illegalaccesserror

AFAIU cglib proxies will simply not work if they are generated in a different classloader than the one used by the proxied class.

When I made the suggested fix, and installed a modified version of Spring to my local maven repo: both the trivial app I'm attaching and the real app I'm working on started functioning properly, with both classloader policies. The unit tests of the Spring build seem to pass.

---

Affects: 4.0.2, 4.0.3

Attachments:

• 2014-03-28 10_18_07-Proxy-NPE.png (21.77 kB)
• multiclassloader.zip (16.35 kB)

Issue Links:

• "CglibAopProxy: Unable to proxy method" WARN when bean class contains static final method [SPR-11107] #15733 "CglibAopProxy: Unable to proxy method" WARN when bean class contains static final method
• Illegal reflective access operation warning for toString() on CGLIB proxies [SPR-17500] #22032 Illegal reflective access operation warning for toString() on CGLIB proxies
• Revisit CGLIB AOP proxy warnings for final methods [SPR-15436] #19997 Revisit CGLIB AOP proxy warnings for final methods
• Clarification: Spring AOP pointcuts match protected methods when CGLIB is used [SPR-15354] #19917 Clarification: Spring AOP pointcuts match protected methods when CGLIB is used

Referenced from: commits 90309ab, d9cb445

[spring-projects-issues]

Juergen Hoeller commented

CGLIB proxy classes are a tough affair. They do not only extend a parent class, they also have hard references to various interceptors which point back to the Spring context that is creating it, including references to beans etc. Also, creating new proxy class variants in a parent ClassLoader might make them pile up there while child ClassLoaders appear and disappear in dynamic scenarios.

For those reasons, creating a CGLIB proxy class in a parent ClassLoader of the current context's ClassLoader might create a leak... CGLIB proxy classes arguably need to live in a ClassLoader with the same lifecycle as the containing ApplicationContext, which is essentially what we're doing right now through using the current context's "bean class loader" there.

The correct solution in a multi-ClassLoader scenario is usually to make sure that each Spring ApplicationContext uses the right ClassLoader, either through a setClassLoader call on the ApplicationContext before the refresh() call or through setting the correct context ClassLoader during each ApplicationContext's bootstrap phase.

The NullPointerException that you're experiencing probably comes from the proxy instance which doesn't have any state set on it by design; looks like the delegation to the target instance doesn't kick in there. I have no idea why that would be the case though.

Generally speaking, I wonder why CGLIB proxy classes couldn't work in a child ClassLoader. After all, regular subclasses can be declared there as well, extending classes from the parent ClassLoader. CGLIB really just generates such a subclass at runtime, doesn't it?

Juergen

[spring-projects-issues]

Alina Mylka commented

Thanks for the answer. The last paragraph inspired me to debug a bit more. The problem was that the method that was called on the proxy was package-access. It started working when I changed it to public.

The Javadocs to the CGLIB Enhancer class from:

http://cglib.sourceforge.net/apidocs/net/sf/cglib/proxy/Enhancer.html

... state that the generated proxies ??override the non-final methods of the superclass??. This statement isn't qualified any further, so it would mean that the generated proxy overrides all public, protected and package-access methods of the superclass. Now a problem arises if the generated proxy class uses a different classloader than the proxied superclass. Calling package-access methods works only if the class of the calling object and the class of the called object are in the same package in the same classloader. This crucial bit is described in many places, for instance here:

http://www.artima.com/insidejvm/ed2/security2.html

??Although you can grant special access privileges between types belonging to the same package by giving members protected or package access, this special access is granted to members of the same package at runtime only if they were loaded by the same class loader.??

So the real problem seems to be that Spring generates a proxy, that tries to override a package-access method, that it doesn't have access to. Then at runtime, that package-access method is executed on the proxy itself and not delegated to the real instance. Delegation is impossible. The method on the real instance is not accessible from the proxy.

Now I'd see following possibilities

1. Throw an Exception in CglibAopProxy.getProxy(ClassLoader classLoader) with a message that says: "Can't generate the proxy, because I can't override a package-access method from a class that lives in a different classloader". Or, even better completely disallow creating proxies of classes with package-access methods, because such code may or may not work depending on the layout of jars in an ear and the .war classloading policy in Websphere. The same problem will likely happen on plain Tomcat as well, if the proxied class is in the "main" classloader of the container, while the proxies are in the classloader of a particular webapp. If disallowing it completely isn't an option, I'd still throw the exception in cases it's really impossible and at least log a warning in cases where it would work.

2. Use some reflection magic, with setAccessible(true) to call the method on the proxied instance anyway. It works for private methods, it may work for package-access methods.

3. At the very least, throw an Exception with a descriptive message at runtime, when such an invalid invocation is made.

The underlying problem with such package-access invocations is that even if both the caller and the callee classes are in the same classloader (the main classloader of the ear) - the proxy itself may be generated in a different one (the web-classloader of a particular .war app inside an .ear, which is also the context classloader of the current thread).

Juergen, if you agree with this, please close this issue as invalid. I will create a new one.

[spring-projects-issues]

Juergen Hoeller commented

Let's simply turn this issue into a corresponding enhancement request, dealing with it from the perspective that you outlined: We should at least catch invalid invocations but ideally have stronger rules around package-visible methods to begin with.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

I've tried to reproduce this in unit tests, and they consistently pass for me: within the same ClassLoader and with different ClassLoaders... The call on a package-visible method on the proxy always properly ends up in the target instance. Which means that there might be plenty of people out there expecting this to work, since the failures apparently only come with specific ClassLoader implementations? This makes it hard to reliably detect invalid scenarios: We could only really let it try to define the class and see whether it fails...

Juergen

[spring-projects-issues]

Juergen Hoeller commented

With some twisting towards actual state involved, I did succeed making it fail in the meantime. So scratch the above: It seems that once local state is involved, a simple separate ClassLoader in a unit test makes it break, and does allow us to cleanly detect our scenario here...

Juergen

[spring-projects-issues]

Juergen Hoeller commented

We detect not only final methods (as we always did) but also the package-visibility-across-ClassLoaders case now, logging corresponding messages. We also log warnings for protected and package-visible final methods now, not just for public ones.

That said, there might be plenty of potential warnings logged here: in particular for non-public final methods but also for package-visible methods which e.g. are just there for testability. Those warnings might not matter in a specific application since the affected methods might never get called through a proxy... There is unfortunately no way to do this lazily for incoming method invocations; we can just do this at proxy creation time, scanning all declared methods. As a consequence, we log those messages at 'info' level where they are less offensive than at 'warn' level.

Juergen