You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2007/01/28 01:55:24 UTC
svn commit: r500716 - in /tomcat/tc6.0.x/trunk:
java/org/apache/catalina/core/StandardWrapper.java
java/org/apache/catalina/valves/ErrorReportValve.java
webapps/docs/changelog.xml
Author: markt
Date: Sat Jan 27 16:55:24 2007
New Revision: 500716
URL: http://svn.apache.org/viewvc?view=rev&rev=500716
Log:
Port fix bug 39088 that prevents infinite loops when an exception is thrown the returns itself for getRootCause(). Also port changes that enable the root cause to be found when the nesting is particularly extreme.
Modified:
tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/StandardWrapper.java
tomcat/tc6.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml
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?view=diff&rev=500716&r1=500715&r2=500716
==============================================================================
--- 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 Sat Jan 27 16:55:24 2007
@@ -31,6 +31,8 @@
import java.security.AccessController;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
+import java.sql.SQLException;
+
import javax.servlet.Servlet;
import javax.servlet.ServletConfig;
import javax.servlet.ServletContext;
@@ -60,6 +62,7 @@
import org.apache.catalina.security.SecurityUtil;
import org.apache.catalina.util.Enumerator;
import org.apache.catalina.util.InstanceSupport;
+import org.apache.tomcat.util.IntrospectionUtils;
import org.apache.tomcat.util.log.SystemLogHandler;
import org.apache.tomcat.util.modeler.Registry;
@@ -291,7 +294,19 @@
*/
protected static Properties restrictedServlets = null;
+
+ private static Class jspExceptionClazz;
+ static {
+ try {
+ jspExceptionClazz = Class.forName("javax.servlet.jsp.JspException");
+ } catch (ClassNotFoundException e) {
+ // Expected if jsp-api not on classpath, eg when embedding
+ jspExceptionClazz = null;
+ }
+ }
+
+
// ------------------------------------------------------------- Properties
@@ -675,18 +690,41 @@
* @param e The servlet exception
*/
public static Throwable getRootCause(ServletException e) {
- Throwable rootCause = e;
- Throwable rootCauseCheck = null;
- // Extra aggressive rootCause finding
- int loops = 0;
- do {
- loops++;
- rootCauseCheck = rootCause.getCause();
- if (rootCauseCheck != null)
- rootCause = rootCauseCheck;
- } while (rootCauseCheck != null && (loops < 20));
- return rootCause;
+ Throwable rootCause = e.getRootCause();
+ return findRootCause(e, rootCause);
}
+
+
+ /*
+ * Work through the root causes using specific methods for well known types
+ * and getCause() for the rest. Stop when the next rootCause is null or
+ * an exception is found that has itself as its own rootCause.
+ */
+ private static final Throwable findRootCause(Throwable theException,
+ Throwable theRootCause) {
+
+ Throwable deeperRootCause = null;
+
+ if (theRootCause == null || theRootCause == theException) {
+ return theException;
+ }
+
+ if (theRootCause instanceof ServletException) {
+ deeperRootCause = ((ServletException) theRootCause).getRootCause();
+ } else if (jspExceptionClazz!=null &&
+ jspExceptionClazz.isAssignableFrom(theRootCause.getClass())) {
+ deeperRootCause = (Throwable)IntrospectionUtils.getProperty(
+ theRootCause, "rootCause");
+ } else if (theRootCause instanceof SQLException) {
+ deeperRootCause = ((SQLException) theRootCause).getNextException();
+ }
+ if (deeperRootCause == null) {
+ deeperRootCause = theRootCause.getCause();
+ }
+
+ return findRootCause(theRootCause, deeperRootCause);
+ }
+
/**
Modified: tomcat/tc6.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java?view=diff&rev=500716&r1=500715&r2=500716
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java Sat Jan 27 16:55:24 2007
@@ -21,6 +21,7 @@
import java.io.IOException;
import java.io.Writer;
+import java.sql.SQLException;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
@@ -33,6 +34,7 @@
import org.apache.catalina.util.RequestUtil;
import org.apache.catalina.util.ServerInfo;
import org.apache.catalina.util.StringManager;
+import org.apache.tomcat.util.IntrospectionUtils;
/**
* <p>Implementation of a Valve that outputs HTML error pages.</p>
@@ -71,6 +73,18 @@
StringManager.getManager(Constants.Package);
+ private static Class jspExceptionClazz;
+
+ static {
+ try {
+ jspExceptionClazz = Class.forName("javax.servlet.jsp.JspException");
+ } catch (ClassNotFoundException e) {
+ // Expected if jsp-api not on classpath, eg when embedding
+ jspExceptionClazz = null;
+ }
+ }
+
+
// ------------------------------------------------------------- Properties
@@ -216,9 +230,9 @@
sb.append(RequestUtil.filter(stackTrace));
sb.append("</pre></p>");
- int loops = 0;
Throwable rootCause = throwable.getCause();
- while (rootCause != null && (loops < 10)) {
+ Throwable nestedRootCause = null;
+ while (rootCause != null) {
stackTrace = getPartialServletStackTrace(rootCause);
sb.append("<p><b>");
sb.append(sm.getString("errorReportValve.rootCause"));
@@ -226,10 +240,33 @@
sb.append(RequestUtil.filter(stackTrace));
sb.append("</pre></p>");
// In case root cause is somehow heavily nested
- rootCause = rootCause.getCause();
- loops++;
+ try {
+ if (rootCause instanceof ServletException) {
+ nestedRootCause =
+ ((ServletException) rootCause).getRootCause();
+ } else if (jspExceptionClazz!=null &&
+ jspExceptionClazz.isAssignableFrom(
+ rootCause.getClass())) {
+ nestedRootCause = (Throwable)IntrospectionUtils.
+ getProperty(rootCause, "rootCause");
+ } else if (rootCause instanceof SQLException) {
+ nestedRootCause = ((SQLException) rootCause).
+ getNextException();
+ }
+ if (nestedRootCause == null) {
+ nestedRootCause = rootCause.getCause();
+ }
+
+ if (rootCause == nestedRootCause)
+ rootCause = null;
+ else {
+ rootCause = nestedRootCause;
+ nestedRootCause = null;
+ }
+ } catch (ClassCastException e) {
+ rootCause = null;
+ }
}
-
sb.append("<p><b>");
sb.append(sm.getString("errorReportValve.note"));
sb.append("</b> <u>");
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?view=diff&rev=500716&r1=500715&r2=500716
==============================================================================
--- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Sat Jan 27 16:55:24 2007
@@ -31,6 +31,10 @@
web.xml. (markt)
</fix>
<fix>
+ <bug>39088</bug>: Prevent infinte loops when an exception is thrown
+ that returns itself for getRootCause(). (markt)
+ </fix>
+ <fix>
<bug>41217</bug>: Set secure attribute on SSO cookie when cookie is
created during a secure request. Patch provided by Chris Halstead.
(markt)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r500716 - in /tomcat/tc6.0.x/trunk: java/org/apache/catalina/core/StandardWrapper.java
java/org/apache/catalina/valves/ErrorReportValve.java webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> Mark Thomas wrote:
>> Fair enough. My biggest concern was the lack of JspException
>> unwrapping. The SQLException support was a nice to have but I'm not
>> that bothered about it.
>
> I never used the SQL exception nesting, so I don't have an opinion about
> it, but I think it's ok to add special support for the SQLException
> chain if you want to.
OK. I'll look at this later.
>>> Note: I see you have fixed JspException as ServletException has been
>>> fixed earlier.
>>
>> I am assuming the veto doesn't apply to this.
>
> I didn't know JspException used the same rootCause field (for some
> reason, I thought it extended ServletException), and needed the same
> changes. However, I think the update to JspException should be revised,
> since getRootCause will return null now (I think it should return
> getCause() for compatibility, and the rootCause field could be removed).
Doh. I though I had done that. I am not having a good week with this
patch. Hopefully this will be the last change.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r500716 - in /tomcat/tc6.0.x/trunk: java/org/apache/catalina/core/StandardWrapper.java
java/org/apache/catalina/valves/ErrorReportValve.java webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
Mark Thomas wrote:
> Fair enough. My biggest concern was the lack of JspException
> unwrapping. The SQLException support was a nice to have but I'm not
> that bothered about it.
I never used the SQL exception nesting, so I don't have an opinion about
it, but I think it's ok to add special support for the SQLException
chain if you want to.
>> Note: I see you have fixed JspException as ServletException has been
>> fixed earlier.
>
> I am assuming the veto doesn't apply to this.
I didn't know JspException used the same rootCause field (for some
reason, I thought it extended ServletException), and needed the same
changes. However, I think the update to JspException should be revised,
since getRootCause will return null now (I think it should return
getCause() for compatibility, and the rootCause field could be removed).
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r500716 - in /tomcat/tc6.0.x/trunk: java/org/apache/catalina/core/StandardWrapper.java
java/org/apache/catalina/valves/ErrorReportValve.java webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> Mark Thomas wrote:
>> An alternative approach that has since occurred to me is to modify
>> JspException in the same way as ServletException so the standard
>> exception chaining is always used. This would enable getCause to be
>> used in all cases apart from SQLException and allow a much cleaner
>> patch whilst retaining improved root cause identification.
>>
>> Having now just looked at the JavaDoc for JspException in the JSP 2.1
>> final spec we have to make this change to be spec compliant.
>> getRootCause has been deprecated in favour of getCause.
>>
>> I'll make the changes later today.
>
> I think it's better if I veto this patch, then. The previous code was
> simpler, complied with the new Java standard for exception nesting, and
> had better protections against infinite loops. Using getCause is the
> standard way of unwrapping exceptions, and should be used moving forward.
Fair enough. My biggest concern was the lack of JspException
unwrapping. The SQLException support was a nice to have but I'm not
that bothered about it.
> Note: I see you have fixed JspException as ServletException has been
> fixed earlier.
I am assuming the veto doesn't apply to this.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r500716 - in /tomcat/tc6.0.x/trunk: java/org/apache/catalina/core/StandardWrapper.java
java/org/apache/catalina/valves/ErrorReportValve.java webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
Mark Thomas wrote:
> Remy Maucherat wrote:
>> markt@apache.org wrote:
>>> Author: markt
>>> Date: Sat Jan 27 16:55:24 2007
>>> New Revision: 500716
>>>
>>> URL: http://svn.apache.org/viewvc?view=rev&rev=500716
>>> Log:
>>> Port fix bug 39088 that prevents infinite loops when an exception is
>>> thrown the returns itself for getRootCause(). Also port changes that
>>> enable the root cause to be found when the nesting is particularly
>>> extreme.
>> Arrrrr, my eyes :D :D
>>
>> TC 6 already used the clean mechanism (using getCause, which can also
>> display a lot more about the exception if proper wrapping was done by
>> the application), so reverting to the TC 5.5 hack is super evil. It also
>> had a recursion limit which seemed reasonable to me, and is going to be
>> more efficient than your check.
>
> I have already done some cleaning and corrected the change log.
>
> There are a couple of problem with getCause. The first is that is
> doesn't unwrap a fairly frequent exception for web applications:
> SQLException. The second is that it doesn't unwrap a JspException that
> has used the JspException(Throwable) constructor.
>
> An alternative approach that has since occurred to me is to modify
> JspException in the same way as ServletException so the standard
> exception chaining is always used. This would enable getCause to be
> used in all cases apart from SQLException and allow a much cleaner
> patch whilst retaining improved root cause identification.
>
> Having now just looked at the JavaDoc for JspException in the JSP 2.1
> final spec we have to make this change to be spec compliant.
> getRootCause has been deprecated in favour of getCause.
>
> I'll make the changes later today.
I think it's better if I veto this patch, then. The previous code was
simpler, complied with the new Java standard for exception nesting, and
had better protections against infinite loops. Using getCause is the
standard way of unwrapping exceptions, and should be used moving forward.
Note: I see you have fixed JspException as ServletException has been
fixed earlier.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r500716 - in /tomcat/tc6.0.x/trunk: java/org/apache/catalina/core/StandardWrapper.java
java/org/apache/catalina/valves/ErrorReportValve.java webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> markt@apache.org wrote:
>> Author: markt
>> Date: Sat Jan 27 16:55:24 2007
>> New Revision: 500716
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=500716
>> Log:
>> Port fix bug 39088 that prevents infinite loops when an exception is
>> thrown the returns itself for getRootCause(). Also port changes that
>> enable the root cause to be found when the nesting is particularly
>> extreme.
>
> Arrrrr, my eyes :D :D
>
> TC 6 already used the clean mechanism (using getCause, which can also
> display a lot more about the exception if proper wrapping was done by
> the application), so reverting to the TC 5.5 hack is super evil. It also
> had a recursion limit which seemed reasonable to me, and is going to be
> more efficient than your check.
I have already done some cleaning and corrected the change log.
There are a couple of problem with getCause. The first is that is
doesn't unwrap a fairly frequent exception for web applications:
SQLException. The second is that it doesn't unwrap a JspException that
has used the JspException(Throwable) constructor.
An alternative approach that has since occurred to me is to modify
JspException in the same way as ServletException so the standard
exception chaining is always used. This would enable getCause to be
used in all cases apart from SQLException and allow a much cleaner
patch whilst retaining improved root cause identification.
Having now just looked at the JavaDoc for JspException in the JSP 2.1
final spec we have to make this change to be spec compliant.
getRootCause has been deprecated in favour of getCause.
I'll make the changes later today.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r500716 - in /tomcat/tc6.0.x/trunk: java/org/apache/catalina/core/StandardWrapper.java
java/org/apache/catalina/valves/ErrorReportValve.java webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
markt@apache.org wrote:
> Author: markt
> Date: Sat Jan 27 16:55:24 2007
> New Revision: 500716
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=500716
> Log:
> Port fix bug 39088 that prevents infinite loops when an exception is thrown the returns itself for getRootCause(). Also port changes that enable the root cause to be found when the nesting is particularly extreme.
Arrrrr, my eyes :D :D
TC 6 already used the clean mechanism (using getCause, which can also
display a lot more about the exception if proper wrapping was done by
the application), so reverting to the TC 5.5 hack is super evil. It also
had a recursion limit which seemed reasonable to me, and is going to be
more efficient than your check.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org