Repository: tomee Updated Branches: refs/heads/master a37132fee -> dc2b7374e
TOMEE-1855 limiting jsp/tag leaks Project: http://git-wip-us.apache.org/repos/asf/tomee/repo Commit: http://git-wip-us.apache.org/repos/asf/tomee/commit/dc2b7374 Tree: http://git-wip-us.apache.org/repos/asf/tomee/tree/dc2b7374 Diff: http://git-wip-us.apache.org/repos/asf/tomee/diff/dc2b7374 Branch: refs/heads/master Commit: dc2b7374e50190c29f1235ecd0ec0925de24768b Parents: a37132f Author: Romain manni-Bucau <[email protected]> Authored: Fri Jul 1 18:42:01 2016 +0200 Committer: Romain manni-Bucau <[email protected]> Committed: Fri Jul 1 18:42:01 2016 +0200 ---------------------------------------------------------------------- .../staticresources/AvoidConflictTest.java | 2 + .../org/apache/openejb/core/WebContext.java | 45 +++++--- .../server/httpd/BeginWebBeansListener.java | 5 + .../tomee/catalina/JavaeeInstanceManager.java | 102 +++++++++++-------- .../apache/tomee/embedded/FailingJspTest.java | 76 ++++++++++++++ .../test/resources/META-INF/resources/fail.jsp | 30 ++++++ 6 files changed, 207 insertions(+), 53 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tomee/blob/dc2b7374/arquillian/arquillian-tomee-tests/arquillian-tomee-jaxrs-tests/src/test/java/org/apache/openejb/arquillian/tests/jaxrs/staticresources/AvoidConflictTest.java ---------------------------------------------------------------------- diff --git a/arquillian/arquillian-tomee-tests/arquillian-tomee-jaxrs-tests/src/test/java/org/apache/openejb/arquillian/tests/jaxrs/staticresources/AvoidConflictTest.java b/arquillian/arquillian-tomee-tests/arquillian-tomee-jaxrs-tests/src/test/java/org/apache/openejb/arquillian/tests/jaxrs/staticresources/AvoidConflictTest.java index 111bf6b..0c93110 100644 --- a/arquillian/arquillian-tomee-tests/arquillian-tomee-jaxrs-tests/src/test/java/org/apache/openejb/arquillian/tests/jaxrs/staticresources/AvoidConflictTest.java +++ b/arquillian/arquillian-tomee-tests/arquillian-tomee-jaxrs-tests/src/test/java/org/apache/openejb/arquillian/tests/jaxrs/staticresources/AvoidConflictTest.java @@ -22,6 +22,7 @@ import org.jboss.arquillian.junit.Arquillian; import org.jboss.arquillian.test.api.ArquillianResource; import org.jboss.shrinkwrap.api.Archive; import org.jboss.shrinkwrap.api.ShrinkWrap; +import org.jboss.shrinkwrap.api.asset.EmptyAsset; import org.jboss.shrinkwrap.api.asset.StringAsset; import org.jboss.shrinkwrap.api.spec.WebArchive; import org.junit.Test; @@ -38,6 +39,7 @@ public class AvoidConflictTest { public static Archive<?> war() { return ShrinkWrap.create(WebArchive.class, "AvoidConflictTest.war") .addClasses(TheResource.class, SimpleServlet.class, PreviousFilter.class) + .addAsWebInfResource(EmptyAsset.INSTANCE, "beans.xml") .addAsWebResource(new StringAsset("static"), "index.html") .addAsWebResource(new StringAsset("JSP <%= 5 %>"), "sample.jsp"); } http://git-wip-us.apache.org/repos/asf/tomee/blob/dc2b7374/container/openejb-core/src/main/java/org/apache/openejb/core/WebContext.java ---------------------------------------------------------------------- diff --git a/container/openejb-core/src/main/java/org/apache/openejb/core/WebContext.java b/container/openejb-core/src/main/java/org/apache/openejb/core/WebContext.java index 073be91..0e090c0 100644 --- a/container/openejb-core/src/main/java/org/apache/openejb/core/WebContext.java +++ b/container/openejb-core/src/main/java/org/apache/openejb/core/WebContext.java @@ -22,6 +22,8 @@ import org.apache.openejb.Injection; import org.apache.openejb.InjectionProcessor; import org.apache.openejb.OpenEJBException; import org.apache.openejb.cdi.ConstructorInjectionBean; +import org.apache.openejb.util.LogCategory; +import org.apache.openejb.util.Logger; import org.apache.webbeans.component.InjectionTargetBean; import org.apache.webbeans.config.WebBeansContext; @@ -44,6 +46,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; public class WebContext { private String id; @@ -52,7 +55,7 @@ public class WebContext { private Context jndiEnc; private final AppContext appContext; private Map<String, Object> bindings; - private final Map<Object, CreationalContext<?>> creationalContexts = new ConcurrentHashMap<>(); + private final ConcurrentMap<Object, CreationalContext<?>> creationalContexts = new ConcurrentHashMap<>(); private WebBeansContext webbeansContext; private String contextRoot; private String host; @@ -149,24 +152,28 @@ public class WebContext { final Context unwrap = InjectionProcessor.unwrap(getInitialContext()); final InjectionProcessor injectionProcessor = new InjectionProcessor(o, injections, unwrap); - final Object beanInstance = injectionProcessor.createInstance(); + final Object beanInstance; + try { + beanInstance = injectionProcessor.createInstance(); - if (webBeansContext != null) { - final InjectionTargetBean<Object> bean = InjectionTargetBean.class.cast(beanDefinition); - bean.getInjectionTarget().inject(beanInstance, creationalContext); - if (shouldBeReleased(bean.getScope())) { - creationalContexts.put(beanInstance, creationalContext); + if (webBeansContext != null) { + final InjectionTargetBean<Object> bean = InjectionTargetBean.class.cast(beanDefinition); + bean.getInjectionTarget().inject(beanInstance, creationalContext); + if (shouldBeReleased(bean.getScope())) { + creationalContexts.put(beanInstance, creationalContext); + } + } + } catch (final OpenEJBException oejbe) { + if (creationalContext != null) { + creationalContext.release(); } + throw oejbe; } return new Instance(beanInstance, creationalContext); } public Object newInstance(final Class beanClass) throws OpenEJBException { - final Instance instance = newWeakableInstance(beanClass); - if (instance.getCreationalContext() != null) { - creationalContexts.put(instance.getValue(), instance.getCreationalContext()); - } - return instance.getValue(); + return newWeakableInstance(beanClass).getValue(); } private ConstructorInjectionBean<Object> getConstructorInjectionBean(final Class beanClass, final WebBeansContext webBeansContext) { @@ -237,7 +244,7 @@ public class WebContext { } return beanInstance; - } catch (final NamingException e) { + } catch (final NamingException | OpenEJBException e) { throw new OpenEJBException(e); } } @@ -277,6 +284,18 @@ public class WebContext { } } + public void release() { + for (final CreationalContext<?> cc : creationalContexts.values()) { + try { + cc.release(); + } catch (final RuntimeException re) { + Logger.getInstance(LogCategory.OPENEJB, WebContext.class.getName()) + .warning("Can't release properly a creational context", re); + } + } + creationalContexts.clear(); + } + public static class Instance { private final Object value; private final CreationalContext<?> cc; http://git-wip-us.apache.org/repos/asf/tomee/blob/dc2b7374/server/openejb-http/src/main/java/org/apache/openejb/server/httpd/BeginWebBeansListener.java ---------------------------------------------------------------------- diff --git a/server/openejb-http/src/main/java/org/apache/openejb/server/httpd/BeginWebBeansListener.java b/server/openejb-http/src/main/java/org/apache/openejb/server/httpd/BeginWebBeansListener.java index ad73556..262e793 100644 --- a/server/openejb-http/src/main/java/org/apache/openejb/server/httpd/BeginWebBeansListener.java +++ b/server/openejb-http/src/main/java/org/apache/openejb/server/httpd/BeginWebBeansListener.java @@ -20,6 +20,7 @@ import org.apache.openejb.cdi.CdiAppContextsService; import org.apache.openejb.cdi.OpenEJBLifecycle; import org.apache.openejb.cdi.ThreadSingletonServiceImpl; import org.apache.openejb.cdi.WebappWebBeansContext; +import org.apache.openejb.core.WebContext; import org.apache.openejb.util.LogCategory; import org.apache.openejb.util.Logger; import org.apache.webbeans.config.OWBLogConst; @@ -193,6 +194,10 @@ public class BeginWebBeansListener implements ServletContextListener, ServletReq @Override public void contextDestroyed(final ServletContextEvent servletContextEvent) { WebBeansListenerHelper.destroyFakedRequest(this); + final WebContext wc = WebContext.class.cast(servletContextEvent.getServletContext().getAttribute("openejb.web.context")); + if (wc != null) { + wc.release(); + } } } http://git-wip-us.apache.org/repos/asf/tomee/blob/dc2b7374/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java ---------------------------------------------------------------------- diff --git a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java index d51934f..8df120d 100644 --- a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java +++ b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java @@ -46,11 +46,16 @@ import java.util.Map; public class JavaeeInstanceManager implements InstanceManager { private final WebContext webContext; private final StandardContext webapp; + private final String[] skipContainerTags; + private final String[] skipPrefixes; private volatile InstanceManager defaultInstanceManager; public JavaeeInstanceManager(final StandardContext webapp, final WebContext webContext) { this.webContext = webContext; this.webapp = webapp; + this.skipContainerTags = SystemInstance.get().getProperty( + "tomee.tomcat.instance-manager.skip-container-tags", "org.apache.taglibs.standard.,javax.servlet.jsp.jstl.").split(" *, *"); + this.skipPrefixes = SystemInstance.get().getProperty("tomee.tomcat.instance-manager.skip-cdi", "").split(" *, *"); } public ServletContext getServletContext() { @@ -69,18 +74,9 @@ public class JavaeeInstanceManager implements InstanceManager { return clazz.newInstance(); } - final Object object = webContext.newInstance(clazz); + final Object object = isSkip(name, skipPrefixes) ? clazz.newInstance() : webContext.newInstance(clazz); if (isJsp(object.getClass())) { - if (defaultInstanceManager == null) { // lazy cause can not be needed - synchronized (this) { - if (defaultInstanceManager == null) { - defaultInstanceManager = new DefaultInstanceManager( - webapp.getNamingContextListener().getEnvContext(), - TomcatInjections.buildInjectionMap(webapp.getNamingResources()), webapp, - ParentClassLoaderFinder.Helper.get()); - } - } - } + initDefaultInstanceMgr(); defaultInstanceManager.newInstance(object); } else { postConstruct(object, clazz); @@ -91,6 +87,19 @@ public class JavaeeInstanceManager implements InstanceManager { } } + private void initDefaultInstanceMgr() { + if (defaultInstanceManager == null) { // lazy cause can not be needed + synchronized (this) { + if (defaultInstanceManager == null) { + defaultInstanceManager = new DefaultInstanceManager( + webapp.getNamingContextListener().getEnvContext(), + TomcatInjections.buildInjectionMap(webapp.getNamingResources()), webapp, + ParentClassLoaderFinder.Helper.get()); + } + } + } + } + private boolean isJsp(final Class<?> type) { return type.getSuperclass().getName().equals("org.apache.jasper.runtime.HttpJspBase"); } @@ -107,8 +116,7 @@ public class JavaeeInstanceManager implements InstanceManager { @Override public Object newInstance(final String className) throws IllegalAccessException, InvocationTargetException, NamingException, InstantiationException, ClassNotFoundException { - final ClassLoader classLoader = webContext.getClassLoader(); - return newInstance(className, classLoader); + return newInstance(className, webContext.getClassLoader()); } @Override @@ -120,17 +128,30 @@ public class JavaeeInstanceManager implements InstanceManager { public void newInstance(final Object o) throws IllegalAccessException, InvocationTargetException, NamingException { final String name = o.getClass().getName(); if ("org.apache.tomee.webservices.CXFJAXRSFilter".equals(name) - || "org.apache.tomcat.websocket.server.WsFilter".equals(name)) { + || "org.apache.tomcat.websocket.server.WsFilter".equals(name) + || isSkip(name, skipContainerTags)) { return; } try { - webContext.inject(o); + if (isSkip(name, skipPrefixes)) { + webContext.inject(o); + } postConstruct(o, o.getClass()); } catch (final OpenEJBException e) { + destroyInstance(o); throw new InjectionFailedException(e); } } + private boolean isSkip(final String name, final String[] prefixes) { + for (final String prefix : prefixes) { + if (name.startsWith(prefix)) { + return true; + } + } + return false; + } + @Override public void destroyInstance(final Object o) throws IllegalAccessException, InvocationTargetException { if (o == null) { @@ -146,20 +167,23 @@ public class JavaeeInstanceManager implements InstanceManager { } final Object unwrapped = unwrap(o); - if (isJsp(o.getClass())) { - defaultInstanceManager.destroyInstance(o); - } else { - preDestroy(unwrapped, unwrapped.getClass()); - } - webContext.destroy(unwrapped); - if (unwrapped != o) { // PojoEndpointServer, they create and track a cc so release it - webContext.destroy(o); + try { + if (isJsp(o.getClass())) { + defaultInstanceManager.destroyInstance(o); + } else { + preDestroy(unwrapped, unwrapped.getClass()); + } + } finally { + webContext.destroy(unwrapped); + if (unwrapped != o) { // PojoEndpointServer, they create and track a cc so release it + webContext.destroy(o); + } } } private Object unwrap(final Object o) { return "org.apache.tomcat.websocket.pojo.PojoEndpointServer".equals(o.getClass().getName()) ? - WebSocketTypes.unwrapWebSocketPojo(o) : o; + WebSocketTypes.unwrapWebSocketPojo(o) : o; } public void inject(final Object o) { @@ -175,9 +199,8 @@ public class JavaeeInstanceManager implements InstanceManager { * * @param instance object to call postconstruct methods on * @param clazz (super) class to examine for postConstruct annotation. - * @throws IllegalAccessException if postConstruct method is inaccessible. - * @throws java.lang.reflect.InvocationTargetException - * if call fails + * @throws IllegalAccessException if postConstruct method is inaccessible. + * @throws java.lang.reflect.InvocationTargetException if call fails */ public void postConstruct(final Object instance, final Class<?> clazz) throws IllegalAccessException, InvocationTargetException { @@ -224,9 +247,8 @@ public class JavaeeInstanceManager implements InstanceManager { * * @param instance object to call preDestroy methods on * @param clazz (super) class to examine for preDestroy annotation. - * @throws IllegalAccessException if preDestroy method is inaccessible. - * @throws java.lang.reflect.InvocationTargetException - * if call fails + * @throws IllegalAccessException if preDestroy method is inaccessible. + * @throws java.lang.reflect.InvocationTargetException if call fails */ protected void preDestroy(final Object instance, final Class<?> clazz) throws IllegalAccessException, InvocationTargetException { @@ -268,8 +290,8 @@ public class JavaeeInstanceManager implements InstanceManager { Method tmp; try { tmp = WebSocketTypes.class.getClassLoader() - .loadClass("org.apache.tomcat.websocket.pojo.PojoEndpointBase") - .getDeclaredMethod("getPojo"); + .loadClass("org.apache.tomcat.websocket.pojo.PojoEndpointBase") + .getDeclaredMethod("getPojo"); tmp.setAccessible(true); } catch (final NoSuchMethodException e) { if ("true".equals(SystemInstance.get().getProperty("tomee.websocket.skip", "false"))) { @@ -299,25 +321,25 @@ public class JavaeeInstanceManager implements InstanceManager { private static Map<String, Map<String, String>> buildInjectionMap(final NamingResourcesImpl namingResources) { final Map<String, Map<String, String>> injectionMap = new HashMap<>(); - for (final Injectable resource: namingResources.findLocalEjbs()) { + for (final Injectable resource : namingResources.findLocalEjbs()) { addInjectionTarget(resource, injectionMap); } - for (final Injectable resource: namingResources.findEjbs()) { + for (final Injectable resource : namingResources.findEjbs()) { addInjectionTarget(resource, injectionMap); } - for (final Injectable resource: namingResources.findEnvironments()) { + for (final Injectable resource : namingResources.findEnvironments()) { addInjectionTarget(resource, injectionMap); } - for (final Injectable resource: namingResources.findMessageDestinationRefs()) { + for (final Injectable resource : namingResources.findMessageDestinationRefs()) { addInjectionTarget(resource, injectionMap); } - for (final Injectable resource: namingResources.findResourceEnvRefs()) { + for (final Injectable resource : namingResources.findResourceEnvRefs()) { addInjectionTarget(resource, injectionMap); } - for (final Injectable resource: namingResources.findResources()) { + for (final Injectable resource : namingResources.findResources()) { addInjectionTarget(resource, injectionMap); } - for (final Injectable resource: namingResources.findServices()) { + for (final Injectable resource : namingResources.findServices()) { addInjectionTarget(resource, injectionMap); } return injectionMap; @@ -327,7 +349,7 @@ public class JavaeeInstanceManager implements InstanceManager { final List<InjectionTarget> injectionTargets = resource.getInjectionTargets(); if (injectionTargets != null && !injectionTargets.isEmpty()) { final String jndiName = resource.getName(); - for (final InjectionTarget injectionTarget: injectionTargets) { + for (final InjectionTarget injectionTarget : injectionTargets) { final String clazz = injectionTarget.getTargetClass(); Map<String, String> injections = injectionMap.get(clazz); if (injections == null) { http://git-wip-us.apache.org/repos/asf/tomee/blob/dc2b7374/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/FailingJspTest.java ---------------------------------------------------------------------- diff --git a/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/FailingJspTest.java b/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/FailingJspTest.java new file mode 100644 index 0000000..94cf0e9 --- /dev/null +++ b/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/FailingJspTest.java @@ -0,0 +1,76 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomee.embedded; + +import org.apache.openejb.core.WebContext; +import org.apache.openejb.loader.IO; +import org.apache.openejb.loader.JarLocation; +import org.apache.openejb.loader.SystemInstance; +import org.apache.openejb.spi.ContainerSystem; +import org.apache.openejb.util.NetworkUtil; +import org.apache.openejb.util.reflection.Reflections; +import org.junit.Test; + +import java.io.IOException; +import java.net.MalformedURLException; +import java.net.URL; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Map; + +import static org.junit.Assert.assertEquals; + +public class FailingJspTest { + @Test + public void run() throws MalformedURLException { // this test passes just cause we skip container tags + final Collection<Object> tracked1; + final WebContext ctx; + try (final Container container = new Container( + new Configuration() + .http(NetworkUtil.getNextAvailablePort()) + .property("openejb.container.additional.exclude", "org.apache.tomee.embedded.") + .property("openejb.additional.include", "tomee-") + .user("tomee", "tomeepwd") + .loginConfig(new LoginConfigBuilder().basic()) + .securityConstaint(new SecurityConstaintBuilder().addAuthRole("**").authConstraint(true).addCollection("api", "/api/resource2/"))) + .deployPathsAsWebapp(JarLocation.jarLocation(FailingJspTest.class)) + .inject(this)) { + + ctx = SystemInstance.get().getComponent(ContainerSystem.class).getWebContextByHost("", "localhost"); + + tracked1 = getTrackedContexts(ctx); + + for (int i = 0; i < 5; i++) { + try { + IO.slurp(new URL("http://localhost:"; + container.getConfiguration().getHttpPort() + "/fail.jsp")); + } catch (final IOException e) { + // no-op + } + } + + final Collection<Object> tracked2 = getTrackedContexts(ctx); + + // bug in org.apache.jasper.servlet.JspServletWrapper.destroy() + tracked2.removeAll(tracked1); + assertEquals(String.valueOf(tracked2), 0, tracked2.size()); + } + } + + private Collection<Object> getTrackedContexts(final WebContext ctx) { + return new ArrayList<>(Map.class.cast(Reflections.get(ctx, "creationalContexts")).keySet()); + } +} http://git-wip-us.apache.org/repos/asf/tomee/blob/dc2b7374/tomee/tomee-embedded/src/test/resources/META-INF/resources/fail.jsp ---------------------------------------------------------------------- diff --git a/tomee/tomee-embedded/src/test/resources/META-INF/resources/fail.jsp b/tomee/tomee-embedded/src/test/resources/META-INF/resources/fail.jsp new file mode 100644 index 0000000..6d1fc59 --- /dev/null +++ b/tomee/tomee-embedded/src/test/resources/META-INF/resources/fail.jsp @@ -0,0 +1,30 @@ +<% + /* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +%> +<%@ page session="false" %> +<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core"; %> +<%! +public void jspDestroy() { + //throw new RuntimeException("bouh"); +} +%> + + + +<c:out value="${'<tag> , &'}"/>
