jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.87k stars 1.91k forks source link

Remove reliance on sun.reflect.Reflection to be compatible with Java 11 #2913

Closed pejobo closed 6 years ago

pejobo commented 6 years ago

When testing with Open-JDK 11 I get the following exception (using Jetty-9.4.11, but the code causing this hasn't changed in the 9.4.x branch):

java.lang.ClassNotFoundException: sun.reflect.Reflection
at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:471)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:588)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
at org.eclipse.jetty.util.Loader.loadClass(Loader.java:65)
at org.eclipse.jetty.server.handler.ContextHandler$Context.getClassLoader(ContextHandler.java:2572)
at org.apache.tomcat.util.descriptor.tagplugin.TagPluginParser.<init>(TagPluginParser.java:49)
at org.apache.jasper.compiler.TagPluginManager.init(TagPluginManager.java:95)
at org.apache.jasper.compiler.TagPluginManager.apply(TagPluginManager.java:58)
at org.apache.jasper.compiler.Compiler.generateJava(Compiler.java:244)
at org.apache.jasper.compiler.Compiler.compile(Compiler.java:374)
at org.apache.jasper.compiler.Compiler.compile(Compiler.java:351)
at org.apache.jasper.compiler.Compiler.compile(Compiler.java:335)
at org.apache.jasper.JspCompilationContext.compile(JspCompilationContext.java:595)
at org.apache.jasper.servlet.JspServletWrapper.service(JspServletWrapper.java:368)
at org.apache.jasper.servlet.JspServlet.serviceJspFile(JspServlet.java:386)
at org.apache.jasper.servlet.JspServlet.service(JspServlet.java:330)
at org.eclipse.jetty.jsp.JettyJspServlet.service(JettyJspServlet.java:112)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:865)
at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1655)
at de.espirit.firstspirit.io.servlet.SecurityFilter.doFilter(SecurityFilter.java:121)
at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1634)
at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:533)
at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:146)
at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:548)
at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:132)
at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:257)
at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1595)
at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:255)
at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1317)
at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:203)
at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:473)
at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1564)
at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:201)
at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1219)
at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:144)
at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:219)
at org.eclipse.jetty.server.handler.HandlerCollection.handle(HandlerCollection.java:126)
at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:132)
at org.eclipse.jetty.server.Server.handle(Server.java:531)
at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:352)
at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:260)
at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:281)
at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:102)
at org.eclipse.jetty.io.ChannelEndPoint$2.run(ChannelEndPoint.java:118)
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:333)
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:310)
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:168)
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.produce(EatWhatYouKill.java:132)
at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:762)
at org.eclipse.jetty.util.thread.QueuedThreadPool$2.run(QueuedThreadPool.java:680)
at java.base/java.lang.Thread.run(Thread.java:834)
joakime commented 6 years ago

The first version of Jetty with Java 11 support is Jetty 9.4.12.v20180830. Please use that version (or newer).

pejobo commented 6 years ago

Thanks for your feedback. I'll report back. Just checked the code in the 9.4.12. release branch, it still uses sun.reflect.Reflect...

sbordet commented 6 years ago

@joakime does not matter, we are doing nasty reflection stuff, so it'll be broken in 9.4.12 as well.

@janbartel the offending code was added to fix https://bugs.eclipse.org/bugs/show_bug.cgi?id=427068, which references the Servlet Spec, which I don't think we can implement in JDK 9+.

@gregw worth opening a bug to the Servlet spec?

joakime commented 6 years ago

The lines causing the problem ...

https://github.com/eclipse/jetty.project/blob/d5fc0523cfa96bfebfbda19606cad384d772f04c/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java#L2568-L2574

joakime commented 6 years ago

I think we need to add a catch(ClassNotFoundException e) and then use the new java.lang.StackWalker API (introduced in Java 9) to get the first frame's Classloader?

joakime commented 6 years ago

Looks like StackWalker can obtain the caller class.

Example from the javadoc ...

class Util {
     private final StackWalker walker = StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);  // <-- constant
     public ResourceBundle getResourceBundle(String bundleName) {
         Class<?> caller = walker.getCallerClass();  // <-- replacement call
         return ResourceBundle.getBundle(bundleName, Locale.getDefault(), caller.getClassLoader());
     }
 }

 class MyTool {
     private final Util util = new Util();
     private void init() {
         ResourceBundle rb = util.getResourceBundle("mybundle");
     }
 }
joakime commented 6 years ago

More references:

sbordet commented 6 years ago

@joakime StackWalker.getCallerClass() is there, but it's a JDK 9+ API.

Point being whether we really need to know the caller's ClassLoader, especially in light that lambdas or reflection may displace the real caller (so depth is not 2 anymore but more depending on how the call is done). @janbartel?

joakime commented 6 years ago

Hard to implement the javadoc Security Manager requirement of ServletContext.getClassLoader() without knowledge of the caller class classloader.

See: https://docs.oracle.com/javaee/7/api/javax/servlet/ServletContext.html#getClassLoader--

joakime commented 6 years ago

I went ahead an filed a new issue at eclipse-ee4j/servlet-api#205 about the evolution of java and this requirement. Perhaps they will have feedback on this.

janbartel commented 6 years ago

I note that tomcat kind of side-steps the issue and is testing whether the thread context classloader is the same as the designated classloader for the webapp.

janbartel commented 6 years ago

Also, the original motivation on the servlet 3.0 spec committee was to ensure that the concerns elaborated in https://docs.oracle.com/javase/1.5.0/docs/api/java/lang/RuntimePermission.html "getClassLoader" were addressed, in the same way that the internal implementation of Thread.getContextClassLoader performs this check when there is a security manager (albeit using non-public and native classes).

joakime commented 6 years ago

Isn't RuntimePermission.checkPermission("classloader") for checking the permission of using Class.getClassLoader() ? The use of ServletContext.getClassLoader() is a different thing.

From the RuntimePermission javadoc ...

This would grant an attacker permission to get the class loader for a particular class. This is dangerous because having access to a class's class loader allows the attacker to load other classes available to that class loader. The attacker would typically otherwise not have access to those classes.

Using ServletContext.getClassLoader() isn't "access to a class's class loader", and seems that the other rules in the Servlet spec that dictate what the scope of access of the ServletContext solve for the worry about "attacker would typically otherwise not have access to those classes".

janbartel commented 6 years ago

The discussion at the time was that the ServletContext would be passed to a ServletContainerInitializer that may not be part of the webapp, so called "foreign" code, and that a way to access the the classloader of the webapp would be needed. It was pointed out that calls to Thread.getContextClassLoader involves security permission checks and it was felt that access to the webapp's classloader should have the same checks.

sbordet commented 6 years ago

FTR, turns out that class sun.reflect.Reflection is still present in JDK 9 and 10, but has been removed from JDK 11, hence the ClassNotFoundException happening with 11 but not with previous versions.

pejobo commented 6 years ago

Is adding another bunch of reflection code using java.lang.StackWalker in case where sun.reflect.Reflection isn't available an option?

sbordet commented 6 years ago

@pejobo it is a viable (and the simplest) option, but the problem is how far we go deep in the stack. Right now we hardcode depth=2, but that may not be the right depth. And sniffing stack frames until finding what needs to be found seems fragile.

@janbartel perhaps we do a best effort, catch ClassNotFoundException and return?

pejobo commented 6 years ago

Not sure if it would be simple - it's more than one static method call.. Catching the ClassNotFoundException would be simpler ;)

pejobo commented 6 years ago

Another option which works with every Java version between 7 and 11 is to use [SecurityManager.getClassContext()](https://docs.oracle.com/javase/7/docs/api/java/lang/SecurityManager.html#getClassContext()).

Here a unit test method demonstrating this:

    @Test
    @SuppressWarnings("rawtypes")
    public void getCallerContext() {
        final Class[] classContext = new MySecurityManager().getClassContext();
        Arrays.stream(classContext).forEach(System.out::println);
        assertThat(classContext[2].getName()).startsWith("org.junit.");
    }

    private static class MySecurityManager extends SecurityManager {

        @Override
        @SuppressWarnings("rawtypes")
        protected Class[] getClassContext() {
            return super.getClassContext();
        }

    }
sbordet commented 6 years ago

Ah! I used this in the past but completely forgot about it! Thanks!

joakime commented 6 years ago

seems nice and simple. plus we only need this test if a security manager exists.

sbordet commented 6 years ago

@joakime I'm on it.

pejobo commented 6 years ago

Ah! I used this in the past but completely forgot about it! Thanks!

Glad this helps. Don't forget to add one to the offset because of the "hop" with the "super" call..

joakime commented 6 years ago

Some questions on the offset.

What happens if the caller is a lambda? What happens if the caller is an invoker?

Does it matter?
Is it still a sane offset? Is it still a sane check?

sbordet commented 6 years ago

@pejobo can you try it out?

Calling from class A, we have the following.

Direct

[0] = ContextHandler$Caller
[1] = ContextHandler$Context
[2] = A

Method Reference

[0] = ContextHandler$Caller
[1] = ContextHandler$Context
[2] = A$$Lambda$...
[3] = A

Lambda

[0] = ContextHandler$Caller
[1] = ContextHandler$Context
[2] = A
[3] = A$$Lambda$...
[4] = A

Reflection

[0] = ContextHandler$Caller
[1] = ContextHandler$Context
[2] = A

In all cases we end up in the class that called us or one of its lambdas, so we should be good.

pejobo commented 6 years ago

I remember that it used to be documented that reflection code and trampolin methods are skipped (there were no lambdas in Java at this time). I was a bit surprised that the linked documentation is so.. unspecific.

The actual behaviour is still like I remembered it, I tested it with Open-JDK-11 and Oracle-JDK-8 for reflection code.

codeconuts commented 6 years ago

sun.reflect.Reflection is removed in Java 11

pejobo commented 6 years ago

Correct, and the usage of this class has been removed with this fix.

pejobo commented 6 years ago

Does anybody has an idea when a release with this fix will be available?

joakime commented 6 years ago

We are testing a staged (not yet blessed/approved) version of 9.4.13

This is attempt 3 with the inclusion of the Dumpable backwards compat.

Staging Repository: https://oss.sonatype.org/content/repositories/jetty-1426/

Version: 9.4.13.v20181109

Give it a test. Tell us what you think.

joakime commented 6 years ago

@pejobo the 9.4.13.v20181109 staged release has been dropped and a new reroll has occurred.

New Staged 9.4.13 release as follows ...

Staging Repo: https://oss.sonatype.org/content/repositories/jetty-1428/

Version: 9.4.13.v20181111

This is the fourth reroll of the potential 9.4.13 release.

This includes an HttpClient executor / updateBeans fix for behavior differences reported by external projects that use Jetty. and is using a new jetty-schemas artifact that includes the new LICENSE/NOTICE file requirements as well as a needed OSGi manifest correction to eliminate the irrelevant javax.servlet export.

joakime commented 6 years ago

Jetty version 9.4.13.v20181111 is now available on Maven Central.

Bukhtawar commented 5 years ago

The first version of Jetty with Java 11 support is Jetty 9.4.12.v20180830. Please use that version (or newer).

@joakime Do we have a list of breaking changes and migration guide from Jetty 8 to Jetty 9.4.13. We also wanted to understand the runtime issues faced with Jetty 8 on JDK11

joakime commented 5 years ago

@Bukhtawar Jetty 8 is EOL (End of Life).

See: https://www.eclipse.org/jetty/documentation/current/what-jetty-version.html

It is not recommended to use Jetty 8 in a production capacity on any JVM/JDK level.

Bukhtawar commented 5 years ago

Thanks @joakime I understand it is not recommended but we might need some guidance on migration. For instance 1) We are using org.eclipse.jetty.server.AsyncContinuation which is not present in Jetty 9.4.18 2) We are using AbstractHttpConnection, specifically AbstractHttpConnection.getCurrentConnection() method. Which is not present in Jetty 9.4.18. 3) We are using org.eclipse.jetty.security.MappedLoginService which is not present in Jetty 9.4.18. 4) we are using connector.getConnection() method. which is not present in Connector class in Jetty 9.4.18.

It would be great if you could help us with the same

joakime commented 5 years ago

See jetty-users mailing list, those questions have been answered there. As well as stackoverflow.com