You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tomee.apache.org by rm...@apache.org on 2016/07/01 16:42:25 UTC
tomee git commit: TOMEE-1855 limiting jsp/tag leaks
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 <rm...@gmail.com>
Authored: Fri Jul 1 18:42:01 2016 +0200
Committer: Romain manni-Bucau <rm...@gmail.com>
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> , &'}"/>