Bug 39088 - StandardWrapper getRootCause() infinite loop
StandardWrapper getRootCause() infinite loop
Status: RESOLVED FIXED
Product: Tomcat 5
Classification: Unclassified
Component: Catalina
5.5.9
All All
: P3 minor with 1 vote (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on: 14789
Blocks:
  Show dependency tree
 
Reported: 2006-03-23 21:02 UTC by Jonathan Leech
Modified: 2007-01-19 18:48 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Leech 2006-03-23 21:02:08 UTC
The "// Extra aggressive rootCause finding" in StandardWrapper.java is an
understatement.  It causes an infinite loop in certain cases.  For example, the
semantics of the following exception class are that getRootCause() returns the
one and only root cause of the exception chain, which in this case can be the
exception itself.  It is not derived from ServletException, thus is not bound to
return null at the end of the chain as ServletException does.  Its not safe to
use reflection to call an arbitrary method of an arbitrary Exception class, just
because it happens to have the same name as the method in ServletException.

public class MyException 
   extends Exception
{
   public MyException(Throwable cause) {
      this.cause = cause;
   }
   Throwable cause;
   public Throwable getCause() {
      return cause;
   }
   public Throwable getRootCause() {
      if (cause instanceof VCOMException) {
         return ((VCOMException) exception).getRootCause();
      }
      return cause == null ? this : cause;
   }
}
Comment 1 Yoav Shapira 2006-04-17 15:35:08 UTC
Suggested patch?
Comment 2 Jonathan Leech 2006-04-17 15:39:29 UTC
(In reply to comment #1)
> Suggested patch?
Break out of the loop if cause == exception.
Comment 3 Tim Funk 2006-04-17 15:39:48 UTC
The problem is user code:
return cause == null ? this : cause;

If the root cause is null, then it should return null, not itself. 
Comment 4 Jonathan Leech 2006-04-17 15:47:40 UTC
(In reply to comment #3)
> The problem is user code:
> return cause == null ? this : cause;
> 
> If the root cause is null, then it should return null, not itself. 

You can't control every exception class that could be thrown in user code.  This
is getRootCause(), not getCause().  Lots of java code was written before Sun
added getCause() to Throwable and standardized exception chaining.

Tomcat calling getRootCause() of an arbitrary exception class is dangerous, as
the infinite loop demonstrates.  It should either only call it on descendants of
ServletException and a few other known types, or at a minimum protect against
the infinite loop condition.
Comment 5 Tim Funk 2006-04-17 16:02:42 UTC
At best - here is the patch if any (not me) is interested ...
/container/catalina/src/share/org/apache/catalina/core/StandardWrapper.java

@@ -627,13 +627,14 @@
 
     /**
      * Extract the root cause from a servlet exception.
-     * 
+     *
      * @param e The servlet exception
      */
     public static Throwable getRootCause(ServletException e) {
         Throwable rootCause = e;
         Throwable rootCauseCheck = null;
         // Extra aggressive rootCause finding
+        int recursionKlugeDetector = 20;
         do {
             try {
                 rootCauseCheck = (Throwable)IntrospectionUtils.getProperty
@@ -644,6 +645,16 @@
             } catch (ClassCastException ex) {
                 rootCauseCheck = null;
             }
+
+            /*
+                We've done this 20 times .. if you've nested more than
+                20 levels of exceptions. Tomcat throwing the wrong exception
+                is the least of your concerns.
+            */
+
+            if (recursionKlugeDetector-- == 0) {
+                return rootCause;
+            }
         } while (rootCauseCheck != null);
         return rootCause;
     }
Comment 6 Jonathan Leech 2006-05-05 21:12:46 UTC
see http://issues.apache.org/bugzilla/show_bug.cgi?id=37038
Perhaps Tomcat should use the commons-lang ExceptionUtils root cause finding
that already fixes this problem...  It appears to be superior to the // Extra
aggressive rootCause finding remification.
Comment 7 Wouter Zelle 2006-10-04 02:05:26 UTC
This issue has caused a production incident for us. It was rather tough to debug
given the fact that one would not expect that a getRootCause() method in a
non-Tomcat exception actually implements a (hidden) Tomcat interface. IMHO this
is very bad style.

A recursion detector does not address these issues:
1. Tomcat has no business calling a getRootCause() method in user code. That
method may have a very different contract than what Tomcat expects. It may have
side effects or it may be illegal to call the method in some cases. Because it
is not clear to a user that his getRootCause() method is called by Tomcat and
because exception paths are usually not tested that well, production incidents
are likely to still happen to some users.
2. The user still does not know that he is implementing a hidden Tomcat interface.
3. 20 recursions (or any arbitrary number) of reflection, with calls to a method
of unknown specifications may cause performance issues.

I believe that a similar fix to the one for bug 37038 is the best solution. That
fix would result in code that does not surprise Java developers, without the
problems outlined above. It is also unlikely that it would result in breakage,
since I don't think that it's likely that anyone took advantage of this
'feature' to implement some functionality.
Comment 8 Jonathan Leech 2006-10-04 09:34:14 UTC
Wouter,  unfortunately nothing ever gets fixed in Tomcat.  The people who can 
make the changes think there shit doesn't stink, and unless they find the bug 
and it affects them, it doesn't exist.  The patch for this would be like 1 
line of code, and the one Tim provided above (with the typical Tomcat 
developer sarcastic asshole mentality) is almost worse than the current code.  
I won't write a patch, and you shouldn't either.  Noone will incorporate one 
into the Tomcat codebase, ever, or let you put it in.  They will give excuses 
like, "if we fix it, it could break code that depends on it being broken".  
Good luck.
Comment 9 andre 2007-01-03 13:20:33 UTC
it is for a called game Bots 
Comment 10 Mark Thomas 2007-01-14 11:46:05 UTC
I have fixed the infinite loop issue in svn by breaking out of the loop as
Jonathan suggested in comment 2. The fix will be in 5.5.21 onwards.

There are certainly more comprehensive ways of doing this, the commons-lang
approach is just one of them. If someone would like to provide a patch then I
would be happy to review it.
Comment 11 Wouter Zelle 2007-01-15 03:49:25 UTC
Mark,

Thanks for working on this issue. Your change still results in potentially
dangerous invocations of getRootCause on third-party classes, though. This fix
does not have that flaw and is much cleaner overall:

public static Throwable getRootCause(ServletException e) {
    Throwable rootCause = e.getRootCause();
    if (rootCause instanceof ServletException
        && rootCause != e) {
        rootCause = getRootCause((ServletException) rootCause);
    }
    return rootCause;
}

Could you please review it?
Comment 12 Mark Thomas 2007-01-15 06:01:03 UTC
Thanks for the suggested patch. Whilst it is safer, it also less likely to get
at the true root cause and I'd like to keep the behaviour of getting at the root
cause wherever possible.

Using commons-lang is one option but I'd rather not add yet another jar to the
build process. It should be possible to add the ExceptionUtils class to
o.a.c.util and use that. It looks like there might be some dependencies on other
commons-lang classes that would need to be coded around. If you want to take a
look at this - that would be great. If not, no problem and I'll look at it later
this week.
Comment 13 Wouter Zelle 2007-01-15 06:38:50 UTC
Mark,

If you really want the root cause, you should do this:

public static Throwable getRootCause(ServletException e) {
    Throwable rootCause = e.getRootCause();
    if (rootCause instanceof ServletException
            && rootCause != e) {
        rootCause = getRootCause((ServletException) rootCause);
    } else {
        Throwable deepRootCause;
        do {
            deepRootCause = rootCause.getCause();
            if (rootCause == deepRootCause) {
                return rootCause;
            } else if (deepRootCause != null) {
                rootCause = deepRootCause;
            }
        } while (deepRootCause != null);
    }
    return rootCause;
}

This code is actually far more likely to find the true root cause and as an
added bonus, it uses API's whose implementation you should be able to trust.
Comment 14 Tim Funk 2007-01-15 07:31:59 UTC
That won't work since JspException does not extend ServletException and 
JspException is typically the reason we wish to delve deeper into the real root 
cause. 

This issue is fixed in tomcat6 since getCause() and getRootCause() are 
imlpemented as one in the same (since 1.5 jdk is required) but since tomcat 
needs to be able to run on java 1.3 (shudders) - the getCause may not be 
available for 1.3 implementations.

Remarking as fixed for now lness a better patch arrives.
Comment 15 Wouter Zelle 2007-01-15 08:00:14 UTC
Here is some code that works for both ServletException and 
JspException:

public static Throwable getRootCause(ServletException e) {
    Throwable rootCause = e.getRootCause();
    return processRootCause(e, rootCause);
}

private static Throwable processRootCause(Exception e, Throwable rootCause) {
    if (rootCause == null || rootCause == e) {
        return e;   //The root cause is a ServletException or JspException
    }

    if (rootCause instanceof ServletException) {
        rootCause = getRootCause((ServletException) rootCause);
    } else if (rootCause instanceof JspException) {
        JspException jspRootCause = (JspException)rootCause;
        Throwable deeperRootCause = jspRootCause.getRootCause();
        rootCause = processRootCause(jspRootCause, deeperRootCause);
    }
    return rootCause;   //The root cause is not a ServletException or JspException
}
Comment 16 Mark Thomas 2007-01-19 18:48:17 UTC
Thanks for the new patch. I have committed a variation to svn and will be in
5.5.21 onwards.