removing log() from error processing in StandardWrapperValve.java ??

[Tim Funk <fu...@joedog.org>]

In the use case of a servlet or filter throwing an ServletException, 
StandardWrapperValve will log the exception to its logger.

If the error is an expected one (such as session timeout or browser 
compatibility check) and there is a configured error-page directive to handle 
the exception, StandardWrapperValve stills logs the message. This causes many 
useless stack traces in the logs for error handling that is already caught.

Does it make sense to remove the log() statement? The the job of logging the 
exception belongs to either ErrorReportValve(?) or the user defined error 
handler?

Just to be clear, this is only the log() in the catch (ServletException e) 
code that I wish to remove.

Or I can my developer rewrite his code to use error handling for "real" errors.

Advice?

-Tim

Re: [5] Infinite loop potential in processing error in StandardWrapperValve.java ??

[Remy Maucherat <re...@apache.org>]

Tim Funk wrote:
> When I added the code to use PropertyUtils.getProperty in determining 
> the root cause, I noticed it can cause an infinite loop.
> 
> // Extra aggressive rootCause finding
> do {
>     try {
>         rootCauseCheck = (Throwable)PropertyUtils.getProperty
>                                     (rootCause, "rootCause");
>         if (rootCauseCheck!=null)
>             rootCause = rootCauseCheck;
> 
>     } catch (...) {
>         rootCauseCheck = null;
>     }
> } while (rootCauseCheck != null);
> 
> ------------------------------------------
> If we have a malicious user who does this:
> {
>   ...
>   ServletException e = new ServletException();
>   throw new ServletException(e);
> }
> 
> We can get a non-recursive infinite loop in the error handling logic.
> 
> 
> Is this a cause for concern? I would guess so in shared environments but 
> this is not a problem in tightly controlled (enterprise/private) 
> environments.
> 
> Comments?

Yes, well, I had seen that flaw in the code. However, if there's a 
"malicious" user out there, he can just add while (true) { 
doSomethingStupid(); } in his code ;-) So I chose not to care about it.

Anyway, +1 to add a max recursion int (there are a few places that use 
this type of code).

Remy

[5] Infinite loop potential in processing error in StandardWrapperValve.java ??

[Tim Funk <fu...@joedog.org>]

When I added the code to use PropertyUtils.getProperty in determining the 
root cause, I noticed it can cause an infinite loop.

// Extra aggressive rootCause finding
do {
     try {
         rootCauseCheck = (Throwable)PropertyUtils.getProperty
                                     (rootCause, "rootCause");
         if (rootCauseCheck!=null)
             rootCause = rootCauseCheck;

     } catch (...) {
         rootCauseCheck = null;
     }
} while (rootCauseCheck != null);

------------------------------------------
If we have a malicious user who does this:
{
   ...
   ServletException e = new ServletException();
   throw new ServletException(e);
}

We can get a non-recursive infinite loop in the error handling logic.


Is this a cause for concern? I would guess so in shared environments but this 
is not a problem in tightly controlled (enterprise/private) environments.

Comments?

-Tim

Re: removing log() from error processing in StandardWrapperValve.java ??

[Tim Funk <fu...@joedog.org>]

Remy Maucherat wrote:
> Tim Funk wrote:
> 
>> In the use case of a servlet or filter throwing an ServletException, 
>> StandardWrapperValve will log the exception to its logger.
>>
>> If the error is an expected one (such as session timeout or browser 
>> compatibility check) and there is a configured error-page directive to 
>> handle the exception, StandardWrapperValve stills logs the message. 
>> This causes many useless stack traces in the logs for error handling 
>> that is already caught.
>>
>> Does it make sense to remove the log() statement? The the job of 
>> logging the exception belongs to either ErrorReportValve(?) or the 
>> user defined error handler?
>>
>> Just to be clear, this is only the log() in the catch 
>> (ServletException e) code that I wish to remove.
>>
> 
> It's a bit more complex than that. (ex: with the request dispatcher, 
> which also logs everything)
> Since this is an uncaught exception, I would log it, personally.
> 
> An IOException which occurs on a socket operation should now be ignored, 
> however. That was the biggest offender.
> 
> Remy
> 

Doh! I thought it wasn't that simple. But the exception is "caught" (loosely 
speaking) by the  error-page directives as defined in web.xml. It is my 
preference that it would be the job of the handler as user declared by 
"error-page" in web.xml to choose to log the exception. If no erroe-page 
directive is defined, than the existing logic is OK.

But such logic might hurt a lot of users at this point due to not having 
things logged as they were before. So I'm content to do nothing or wait for a 
future major release. (like 4.2, or 5.2 if they are ever proposed)

-Tim

Re: removing log() from error processing in StandardWrapperValve.java ??

[Remy Maucherat <re...@apache.org>]

Tim Funk wrote:
> In the use case of a servlet or filter throwing an ServletException, 
> StandardWrapperValve will log the exception to its logger.
> 
> If the error is an expected one (such as session timeout or browser 
> compatibility check) and there is a configured error-page directive to 
> handle the exception, StandardWrapperValve stills logs the message. This 
> causes many useless stack traces in the logs for error handling that is 
> already caught.
> 
> Does it make sense to remove the log() statement? The the job of logging 
> the exception belongs to either ErrorReportValve(?) or the user defined 
> error handler?
> 
> Just to be clear, this is only the log() in the catch (ServletException 
> e) code that I wish to remove.
> 
> Or I can my developer rewrite his code to use error handling for "real" 
> errors.
> 
> Advice?

It's a bit more complex than that. (ex: with the request dispatcher, 
which also logs everything)
Since this is an uncaught exception, I would log it, personally.

An IOException which occurs on a socket operation should now be ignored, 
however. That was the biggest offender.

Remy