You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by kk...@apache.org on 2013/01/11 10:23:06 UTC

svn commit: r1431946 - in /tomcat/tc6.0.x/trunk: STATUS.txt java/org/apache/catalina/core/ApplicationFilterConfig.java java/org/apache/catalina/core/StandardWrapper.java webapps/docs/changelog.xml

Author: kkolinko
Date: Fri Jan 11 09:23:06 2013
New Revision: 1431946

URL: http://svn.apache.org/viewvc?rev=1431946&view=rev
Log:
Fix leak of servlet instances when running with SecurityManager:
a) In case when Servlet.init() or destroy() fail.
b) In case of a SingleThreadModel servlet. (fix wrong argument)

Includes the same fix for filter instances when running with SecurityManager
and an Error is thrown by destroy().

It is based on r1429186 and partly on r1428993.

Modified:
    tomcat/tc6.0.x/trunk/STATUS.txt
    tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationFilterConfig.java
    tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/StandardWrapper.java
    tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml

Modified: tomcat/tc6.0.x/trunk/STATUS.txt
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1431946&r1=1431945&r2=1431946&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Fri Jan 11 09:23:06 2013
@@ -94,14 +94,6 @@ PATCHES PROPOSED TO BACKPORT:
   +1: kkolinko, markt
   -1:
 
-* Fix memory leak of servlet instances when running with a
-  SecurityManager and either init() or destroy() methods fail
-  or the servlet is a SingleThreadModel one.
-  It is based on r1429186
-  http://people.apache.org/~kkolinko/patches/2013-01-05_tc6_SecurityUtil_remove.patch
-  +1: kkolinko, schultz, markt
-  -1:
-
 * Improve method cache handling in SecurityUtil class.
   Add caching for Comet methods and simplify cache lookup code.
   It is backport of r728776 (BZ 46304) and r1429360

Modified: tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationFilterConfig.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationFilterConfig.java?rev=1431946&r1=1431945&r2=1431946&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationFilterConfig.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationFilterConfig.java Fri Jan 11 09:23:06 2013
@@ -351,8 +351,9 @@ public final class ApplicationFilterConf
                     SecurityUtil.doAsPrivilege("destroy", filter); 
                 } catch(java.lang.Exception ex){                    
                     context.getLogger().error("ApplicationFilterConfig.doAsPrivilege", ex);
+                } finally {
+                    SecurityUtil.remove(filter);
                 }
-                SecurityUtil.remove(filter);
             } else { 
                 filter.destroy();
             }
@@ -401,8 +402,9 @@ public final class ApplicationFilterConf
                         SecurityUtil.doAsPrivilege("destroy", filter);  
                     } catch(java.lang.Exception ex){    
                         context.getLogger().error("ApplicationFilterConfig.doAsPrivilege", ex);
+                    } finally {
+                        SecurityUtil.remove(filter);
                     }
-                    SecurityUtil.remove(filter);
                 } else { 
                     filter.destroy();
                 }

Modified: tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/StandardWrapper.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/StandardWrapper.java?rev=1431946&r1=1431945&r2=1431946&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/StandardWrapper.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/StandardWrapper.java Fri Jan 11 09:23:06 2013
@@ -1195,13 +1195,20 @@ public class StandardWrapper
                                                   servlet);
 
                 if( Globals.IS_SECURITY_ENABLED) {
-
-                    Object[] args = new Object[]{((ServletConfig)facade)};
-                    SecurityUtil.doAsPrivilege("init",
-                                               servlet,
-                                               classType,
-                                               args);
-                    args = null;
+                    boolean success = false;
+                    try {
+                        Object[] args = new Object[]{ facade };
+                        SecurityUtil.doAsPrivilege("init",
+                                                   servlet,
+                                                   classType,
+                                                   args);
+                        success = true;
+                    } finally {
+                        if (!success) {
+                            // destroy() will not be called, thus clear the reference now
+                            SecurityUtil.remove(servlet);
+                        }
+                    }
                 } else {
                     servlet.init(facade);
                 }
@@ -1429,9 +1436,12 @@ public class StandardWrapper
               (InstanceEvent.BEFORE_DESTROY_EVENT, instance);
 
             if( Globals.IS_SECURITY_ENABLED) {
-                SecurityUtil.doAsPrivilege("destroy",
-                                           instance);
-                SecurityUtil.remove(instance);                           
+                try {
+                    SecurityUtil.doAsPrivilege("destroy",
+                                               instance);
+                } finally {
+                    SecurityUtil.remove(instance);
+                }
             } else {
                 instance.destroy();
             }
@@ -1477,8 +1487,11 @@ public class StandardWrapper
                 while (!instancePool.isEmpty()) {
                     Servlet s = (Servlet) instancePool.pop();
                     if (Globals.IS_SECURITY_ENABLED) {
-                        SecurityUtil.doAsPrivilege("destroy", s);
-                        SecurityUtil.remove(instance);                           
+                        try {
+                            SecurityUtil.doAsPrivilege("destroy", s);
+                        } finally {
+                            SecurityUtil.remove(s);
+                        }
                     } else {
                         s.destroy();
                     }

Modified: tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml?rev=1431946&r1=1431945&r2=1431946&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Fri Jan 11 09:23:06 2013
@@ -62,6 +62,12 @@
         <bug>54220</bug>: Ensure the ErrorReportValve only generates an error
         report if the error flag on the response has been set. (markt)
       </fix>
+      <fix>
+        Fix memory leak of servlet instances when running with a
+        SecurityManager and either init() or destroy() methods fail
+        or the servlet is a SingleThreadModel one, and of filter instances
+        if their destroy() method fails with an Error. (kkolinko)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Web applications">



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org