Bug 55159 - Wasted work in ErrorReportValve.getPartialServletStackTrace
Wasted work in ErrorReportValve.getPartialServletStackTrace
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
7.0.41
PC Linux
: P2 normal (vote)
: ---
Assigned To: Tomcat Developers Mailing List
: PatchAvailable
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2013-06-29 19:12 UTC by Adrian Nistor
Modified: 2013-07-01 11:25 UTC (History)
1 user (show)



Attachments
patch (881 bytes, patch)
2013-06-29 19:12 UTC, Adrian Nistor
Details | Diff
patchShort (567 bytes, patch)
2013-06-29 19:12 UTC, Adrian Nistor
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Nistor 2013-06-29 19:12:03 UTC
Created attachment 30499 [details]
patch

The problem appears in version 7.0.41 and in revision 1497999.  I
attached a two-line patch  (patch.diff) that fixes it.

In method "ErrorReportValve.getPartialServletStackTrace", the loop
over "elements" keeps overriding "pos" with "i".  Therefore, only the
last written value is visible out of the loop and all the other writes
and iterations are not necessary.  The patch iterates from the end of
"elements" and breaks the first time when "pos" is set.

The above fix (in patch.diff) is certainly correct (it's easy to see
through code inspection), but I think we can have an even shorter
patch (one line, in patchShort.diff): just break as soon as "pos" is
set, without reversion the loop order.  patchShort.diff is correct
only if there can be only one "elements[i]" with class name
"org.apache.catalina.core.ApplicationFilterChain" and method name
"internalDoFilter" or if it doesn't matter which such "elements[i]" is
detected (the last, like in the original code, or the first, like in
patchShort.diff).
Comment 1 Adrian Nistor 2013-06-29 19:12:32 UTC
Created attachment 30500 [details]
patchShort
Comment 2 Violeta Georgieva 2013-07-01 10:37:25 UTC
(In reply to Adrian Nistor from comment #0)
> The above fix (in patch.diff) is certainly correct (it's easy to see
> through code inspection), but I think we can have an even shorter
> patch (one line, in patchShort.diff): just break as soon as "pos" is
> set, without reversion the loop order.  patchShort.diff is correct
> only if there can be only one "elements[i]" with class name
> "org.apache.catalina.core.ApplicationFilterChain" and method name
> "internalDoFilter" or if it doesn't matter which such "elements[i]" is
> detected (the last, like in the original code, or the first, like in
> patchShort.diff).

We need the last element and not the first one. So the first patch is the proper one.

Regards
Violeta
Comment 3 Violeta Georgieva 2013-07-01 11:25:36 UTC
Thanks for the report.
The first patch was applied in trunk and 7.0.x and will be included in 7.0.42 onwards.