Bug 37929 - invalidated session causes pageContext methods to fail
invalidated session causes pageContext methods to fail
Status: RESOLVED FIXED
Product: Tomcat 6
Classification: Unclassified
Component: Jasper
6.0.18
Other other
: P3 normal (vote)
: default
Assigned To: Tomcat Developers Mailing List
:
: 37699 (view as bug list)
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2005-12-16 01:58 UTC by Pierre Delisle
Modified: 2009-04-28 07:29 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Delisle 2005-12-16 01:58:15 UTC
javax.servlet.http.HttpSession methods such as getAttribute(),
getValue(), getAttributeNames(), getValueNames(), etc, throw an
IllegalStateException if called on a session that has been
invalidated.

So, with the following code in a JSP page:

    <% 
       session.invalidate(); 
       Object obj = pageContext.findAttribute("foo");
    %>

An IllegalStateException is thrown because pageContext.findAttribute()
eventually calls session.getAttribute() on a session that has been
invalidated.

The session that has been invalidated should simply be ignored when a
method needs to process the various scopes (page, request, session,
application).

This impacts the following methods in PageContextImpl:

   public int getAttributesScope(final String name)
      which calls -> private int doGetAttributeScope(String name);
   public Object findAttribute(final String name)
      which calls -> private Object doFindAttribute(String name);
   public void removeAttribute(final String name)
      which calls -> private void doRemoveAttribute(String name);

The fix is to catch IllegalStateException and ignore it when processing
the attribute in session scope. The code then simply follows through to 
process application scope.

No need to worry about setAttribute() because it is always invoked
on a specific scope, and the spec already states
that java.lang.IllegalStateException must be thrown when called 
on an invalidated session.

  pageContext.setAttribute("foo", "value of foo", PageContext.SESSION_SCOPE);

  java.lang.IllegalStateException - if the scope is PageContext.SESSION_SCOPE 
  but the page that was requested does not participate in a session or 
  the session has been invalidated.

---------------------------------------------------------------------------

At the same time, a fix should be done to method "doRemoveAttribute(String
name)" where a try/catch block for Exception appears unnecessary.

    private void doRemoveAttribute(String name){
        try {
            removeAttribute(name, PAGE_SCOPE);
            removeAttribute(name, REQUEST_SCOPE);
            if( session != null ) {
                try {
                    removeAttribute(name, SESSION_SCOPE);
                } catch (IllegalStateException ex) {
                    // Session has been invalidated.
                    // Ignore and fall through to application scope.
                }
            }
            removeAttribute(name, APPLICATION_SCOPE);
        } catch (Exception ex) {
            // we remove as much as we can, and
            // simply ignore possible exceptions
        }
    }

Here is a full analysis:

Starting with 'removeAttribute(final String name)'
  - we check for null and throw NPE if necessary

  - we call doRemoveAttribute(name)
  
doRemoveAttribute(name)
  - we call removeAttribute(name, scope) for each scope

removeAttribute(final String name, final int scope)
  - this calls doRemoveAttribute(name, scope)

doRemoveAttribute(name, scope)
  - page scope: attributes.remove -> won't throw an Exception
  - request scope: request.removeAttribute -> no documented Exception thrown
  - session scope: throws IllegalStateException if session is null
  - app scope: context.removeAttribute -> no documented Exception thrown

A null value for name is already checked in removeAttribute(final String name)
and we throw NPE. So this situation (removing an attr from page or request
scope throwing an NPE) won't happen.

In doRemoveAttribute(name), we already check on session != null
before calling removeAttribute(name, SESSION_SCOPE). So there
normally is no IllegalStateException thrown (except for the invalidated
case).

When removing an attribute from application (i.e., ServletContext)
scope, any registered listeners will be notified, but the code that
does that (see
appserv-webtier/src/java/org/apache/catalina/core/ApplicationContext.
removeAttribute()) already catches any Throwable that a listener
may throw.

The try/catch block is therefore unnecessary.
Moreover, if any of the removal actions from the different scopes could
have thrown an exception, each of them would have needed to be wrapped
inside their own try/catch, so as to ensure that an exception in one
scope does not cause any of the subsequent removals to be bypassed.

doRemoveAttribute(String name) has therefore been modified as follows:

    private void doRemoveAttribute(String name){
            removeAttribute(name, PAGE_SCOPE);
            removeAttribute(name, REQUEST_SCOPE);
            if( session != null ) {
                try {
                    removeAttribute(name, SESSION_SCOPE);
                } catch (IllegalStateException ex) {
                    // Session has been invalidated.
                    // Ignore and fall through to application scope.
                }
            }
            removeAttribute(name, APPLICATION_SCOPE);
    }

--------------------------
Changes done on glassfish. ymmv on jasper for the diffs.

--- PageContextImpl.java	9 Dec 2005 18:54:30 -0000	1.7
+++ PageContextImpl.java	16 Dec 2005 00:11:21 -0000	1.8
@@ -452,8 +452,13 @@
             return REQUEST_SCOPE;
 
         if (session != null) {
+            try {
             if (session.getAttribute(name) != null)
                 return SESSION_SCOPE;
+            } catch (IllegalStateException ex) {
+ 	        // Session has been invalidated.
+                // Ignore and fall through to application scope.
+            }
         }
 
         if (context.getAttribute(name) != null)
@@ -495,9 +500,14 @@
             return o;
 
         if (session != null) {
+	    try {
             o = session.getAttribute(name);
-            if (o != null)
-                return o;
+ 	    } catch (IllegalStateException ex) {
+ 	        // Session has been invalidated.
+                // Ignore and fall through to application scope.
+ 	    }
+
+ 	    if (o != null) return o;
         }
 
         return context.getAttribute(name);
@@ -559,19 +569,18 @@
         }
     }
 
-
     private void doRemoveAttribute(String name){
-        try {
             removeAttribute(name, PAGE_SCOPE);
             removeAttribute(name, REQUEST_SCOPE);
             if( session != null ) {
+            try {
                 removeAttribute(name, SESSION_SCOPE);
+            } catch (IllegalStateException ex) {
+                // Session has been invalidated.
+                // Ignore and fall through to application scope.
             }
-            removeAttribute(name, APPLICATION_SCOPE);
-        } catch (Exception ex) {
-            // we remove as much as we can, and
-            // simply ignore possible exceptions
         }
+        removeAttribute(name, APPLICATION_SCOPE);
     }
 
     public JspWriter getOut() {
Comment 1 Pierre Delisle 2005-12-16 02:00:01 UTC
*** Bug 37699 has been marked as a duplicate of this bug. ***
Comment 2 william.barker 2005-12-16 06:57:38 UTC
In the future, please attach patches from GlassFish rather than inlining 
them.  As it is, I can't do anything about this since I've already seen the 
patch, so I'm tainted from an IP persective.

Before this patch can be accepted, we need to know that your submission is 
covered by a CCLA.  Otherwise, Sun owns the code, so it can't be incorporated 
into Tomcat.
Comment 3 Pierre Delisle 2005-12-16 07:32:35 UTC
Hi William,

Thanks for looking into this so quickly.

The situation is the same as the one you encountered
with Kin-Man on bug 35351:

>> Just to be clear, is this patch is still covered by your CCLA?  In English, is 
>> this a grant from glassfish to that ASF?
>> 

> This is just a patch, and there is no license or copyright, so YES.

Let me know if you need anything else.

Comment 4 william.barker 2005-12-16 09:34:32 UTC
(In reply to comment #3)
> Hi William,
> Thanks for looking into this so quickly.
> The situation is the same as the one you encountered
> with Kin-Man on bug 35351:
> >> Just to be clear, is this patch is still covered by your CCLA?  In 
English, is 
> >> this a grant from glassfish to that ASF?
> >> 
> > This is just a patch, and there is no license or copyright, so YES.
> Let me know if you need anything else.

I accepted Kin-Man's patch based on the fact that he is a Tomcat committer, 
and as such should have a CCLA on file with the ASF.  If that is still valid, 
than that means that Sun has trusted him with deciding about donating Sun code 
to the ASF.  Were I to find out that Kin-Man's CCLA has been revoked, I would 
revert the commit in a heart-beat :).

IANAL, but from all of the ASF lists that I have followed, the above reasoning 
doesn't even hold a drop of water.  So again I ask if you have a CCLA on file 
so that the ASF can accept your donation of Sun's code?  

The patch is nice, so I'm hoping that you say yes ;-).


Comment 5 william.barker 2005-12-18 02:09:16 UTC
With very minor style changes, your patch has been committed to SVN trunk, and 
will appear in 5.5.15.

Thanks much!
Comment 6 Pavel Sher 2009-04-14 13:44:15 UTC
It seems the problem still exists in the Tomcat 6.0.18. Moreover, I took a look at the PageContextImpl.java class and I do not see there any fixes related to this bug. I browsed svn repository and it looks like this commit has reverted the fix:

http://svn.apache.org/viewvc/tomcat/jasper/tc6.0.x/src/share/org/apache/jasper/runtime/PageContextImpl.java?view=log

Is it supposed to be so? Or fix was reverted by mistake?
Comment 7 Mark Thomas 2009-04-14 16:24:42 UTC
Moving to Tomcat 6
Comment 8 Mark Thomas 2009-04-16 08:22:44 UTC
Thanks for the report and the research. This has been fixed in trunk and proposed for 6.0.x
Comment 9 Mark Thomas 2009-04-28 06:45:54 UTC
This has been fixed in 6.0.x and will be included in 6.0.20 onwards.