tomcat-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kulessa <kule...@flexsecure.de>
Subject Re: HttpServletRequest - getHeaders() vs getCookies()
Date Fri, 25 Jul 2014 11:14:19 GMT
Hi Christopher,

Am 22.07.2014 17:00, schrieb Christopher Schultz:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Simon,
>
> On 7/22/14, 5:47 AM, Simon Kulessa wrote:
>> Hi Christopher,
>>
>> see below
>>
>> Am 17.07.2014 16:18, schrieb Christopher Schultz: Simon,
>>
>> On 7/17/14, 3:52 AM, Simon Kulessa wrote:
>>>>> Hi Christopher,
>>>>>
>>>>> Am 16.07.2014 14:45, schrieb Christopher Schultz: Simon,
>>>>>
>>>>> On 7/16/14, 4:02 AM, Simon Kulessa wrote:
>>>>>>>> Am 15.07.2014 16:12, schrieb Christopher Schultz:
>>>>>>>> Simon,
>>>>>>>>
>>>>>>>> On 7/9/14, 4:51 AM, Simon Kulessa wrote:
>>>>>>>>>>> I had a look at the documentation and the tomcat
>>>>>>>>>>> source to get a better understanding of what
the
>>>>>>>>>>> '|org.apache.catalina.connector.RECYCLE_FACADE'
>>>>>>>>>>> parameter actually does.|
>>>>>>>>>>>
>>>>>>>>>>> I have seen that Tomcat objects like Cookies,
>>>>>>>>>>> Request etc. are designed to be reusable.
>>>>>>>> Requests, yes. I haven't looked to see if Cookies would
>>>>>>>> be re-usable but it seems plausible.
>>>>>>>>
>>>>>>>>>>> What I currently do not understand is: In which
>>>>>>>>>>> scenario and what context are they going to be
>>>>>>>>>>> reused? I see there are Endpoints classes (like
>>>>>>>>>>> NIOEndpoint) which are used to process the
>>>>>>>>>>> different requests. This seems to be the most
>>>>>>>>>>> likely entry point into the scenario.
>>>>>>>>>>>
>>>>>>>>>>> Maybe somebody can provide some general outline
>>>>>>>>>>> of how requests and the reusing of the object
>>>>>>>>>>> actually works together? Is there some kind of
>>>>>>>>>>> relation to the IP of an incoming request?
>>>>>>>> The client's IP is irrelevant: Tomcat uses a pool of
>>>>>>>> objects and re-fills each object with data from an
>>>>>>>> incoming request. These objects, as far as the web
>>>>>>>> application should be concerned, should have a valid
>>>>>>>> lifetime equal to the request itself. The servlet spec
>>>>>>>> requires these semantics, so this isn't some weird
>>>>>>>> Tomcat thing. Tomcat has chosen to pool these objects
>>>>>>>> for a small performance gain when it comes to memory
>>>>>>>> management and garbage collection.
>>>>>>>>
>>>>>>>> If your application retains references to these objects
>>>>>>>> after they become invalid, they may contain invalid
>>>>>>>> data or valid data from another request after they
>>>>>>>> should have become invalid form the perspective of the
>>>>>>>> original request.
>>>>>>>>
>>>>>>>> If you need data from a request, cookie, etc. then you
>>>>>>>> should copy it somewhere safe before the request ends.
>>>>>>>>> We do not cache any request specific information. The
>>>>>>>>> IP relation itself is irrelevant - it seems that the
>>>>>>>>> reused object's contains more 'old' informations than
>>>>>>>>> I previously assumed. The header itself and the
>>>>>>>>> requestedSessionId seems to be changed, the sourceIP
>>>>>>>>> and the cookie stay the same.
>>>>> I checked, and Tomcat does not appear to cache Cookie objects
>>>>> in any way. If the request object is cached and there is a
>>>>> terrible bug, it might be possible to re-read an incorrect
>>>>> session cookie from an old set of request headers.
>>>>>
>>>>>> At least the cookies have a recycle() method, which is
>>>>>> called if a request is recycled.
>> I don't see a recycle() method in the Cookie class. Where do you
>> see that?
>>
>>> I checked out the src for Tomcat the Tags 7.0.29 and 7.0.54 from
>>> http://svn.apache.org/repos/asf/tomcat
>>> In both version org.apache.tomcat.util.http.Cookies as well as
>>> org.apache.tomcat.util.http.ServerCookie have a recycle() method
> Aah, I see. The server does cache the "server cookie" structure, etc.,
> but when parsing a request, the "server cookies" are copied into a
> local (unshared) array with unshared javax.servlet.http.Cookie objects
> in it. Those are the only objects available to the application.
>
> See org.apache.catalina.connector.Request.parseCookies() for the code
> that does that.
>
> So, it's entirely possible that a request under the covers is
> retaining cookie information from a previous request. If there is an
> error during cookie parsing, I could see that a request would think
> that the cookies had been (re)parsed when they hadn't been, but
> recycling the request ought to set the "cookiesParsed" flag back to
> false and so cookies ought to be re-parsed for that later request.
> Also, in the event of an error, there should be some indication of the
> error in the log file.
>
>> I don't even understand the "session id change" that you described
>> so it would be helpful to have a test case for that.
>>
>>> Basically it looks like this
>>> 0 request comes in via Servlet.doPost(...)
>>> 1 HttpSession session = request.getSession(); 2 String sId1 =
>>> request.getSession().getId(); 3 session.invalidate(); 4 String
>>> sId2 = request.getSession().getId();
>>> 5 response is written
> Why are you doing 4 after 3? Are you trying to trigger the creation of
> a new session cookie?
No that was not the intention here. It was just that the request passed 
as parameter
to another method and nobody considered that the session should be 
transported
separately from this.

I change that now that the Session reference is only obtained once per 
request.
>>> The session from 1 is saved in global map, so 3 is actually done
>>> from another thread that has nothing to do with the request. The
>>> other steps are done in same request and thread.
>>> Problem is that 'sometimes' sId1 does not equal sId2.
> I would never expect that they are the same: you are invalidating the
> session and then asking for the session id: this will create a new
> session, with a new id.
>
> If you want to know if your session was invalidated (checking from
> another thread), then you should call request.getSession(false) and
> check for null. Since you are playing games with sessions accessed
> from different places (which in general I think is mostly okay), then
> you might also want to check to see if your session has been emptied
> of all data (since there is no HttpSession.isValid method, but Tomcat
> unbinds all attributes from the session when it is invalidated).
>
> If step 4 is in fact calling request.getSession() instead of using a
> local reference to the session obtained in step 1, then the session
> returned should be a new one. If you are using a previously-obtained
> reference from step 1, then you will have an invalid session object on
> your hands.
>> Are you caching the AsyncContext yourself, or are you using an
>> AsyncListener to capture events and using the AsyncContext that
>> comes from the event?
>>
>>> We are caching the AsyncContext and we have a Listener as well to
>>> react on the events. In the listener we mostly (with the
>>> exception of onComplete, see below) use the cached reference, not
>>> the one from the event.
> I'm pretty sure that you are always going to want to use the context
> from the event.
>
> I'm not async expert (and I've never written any servlet 3.0
> async-powered code) but you do have to be very careful with the
> references you store and when you use them.
>
>> Can you even give us some pseudocode that shows what you are trying
>> to do? So far, all you've really said is "Tomcat is messing-up my
>> sessions". It took you 3 posts to mention that you were using
>> Servlet 3.0 async, which might be the most important piece of
>> information after the version you are using. Since your version is
>> old, you really should upgrade regardless of the outcome of this
>> conversation. In fact, there have been so many changes and tweaks
>> to the async-related code over the years (/literally/ years since
>> your release came out) that this problem may not even exist
>> anymore.
>>> The code for starting the context looks like this:
>>> // listener is an application scoped class that is used for all
>>> AsyncContext's public void startAsync(HttpServletRequest
>>> servletRequest, AsyncListener listener) {
>>> AsyncContext asyncContext = servletRequest.startAsync(); if
>>> (asyncContext == null) { throw new IllegalStateException(); }
>>> HttpSession session = ((HttpServletRequest)
>>> asyncContext.getRequest()).getSession(); if (session == null) {
>>> asyncContext.complete(); throw new IllegalStateException(); }
>>> asyncContext.addListener(listener); // store asyncContext in
>>> global map, mapped to the current session }
> This is likely when everything goes wrong: storing the async context
> and session in a global map sounds like a recipe for disaster. Again,
> I'm no expert, but my spidey sense is tingling.
You need the asyncContext to send something later.
This will happen while the httpRequest is still alive (somewhere in tomcat),
but after the Servlet has finished the processing.

In other example I see that some people use a running Thread to store
this reference in, but this should not be very different from what we 
are doing.

You have to maintain this reference somewhere. So by design there 
(hopefully) shouldn't
be any problem with 'caching' this object.

The async listener only covers the event's that occur, but the listener 
does not necessarily
have anything to do with the sending of an answer.

>>> At this point of time the processing of the request which came
>>> from a Servlet is done. The listener itself looks like this:
>>> public void onTimeout(AsyncEvent event) throws IOException {
>>> HttpSession httpSession = ((HttpServletRequest)
>>> event.getSuppliedRequest()).getSession(false);
>>> dispose(httpSession.getId()); }
> Here, it looks like you are using the context from the event. This
> usage should be okay.
>
>>> public void onError(AsyncEvent event) throws IOException {
>>> HttpSession httpSession = ((HttpServletRequest)
>>> event.getSuppliedRequest()).getSession(false);
>>> dispose(httpSession.getId()); }
>>> private void dispose(String sessionId) { // retrieve related
>>> session from global map AsyncContext ctx = ... if (ctx != null)
>>> { // Send some message that we close the session Message message
>>> = ... sendMessageOnAsyncContext(message, ctx); }
>>> httpSession.invalidate(); }
> Instead of looking-up the context in the global map, why not just pass
> the context into the dispose() method?
>
>>> public void onComplete(AsyncEvent event) throws IOException {
>>> // retrieve session based on the sessionId contained in the event
>>> from the global map and take the AsyncContext from there
>>> AsyncContext ctx = ...
> I think you want to use the event's context, here.
>
>>> if (event.getAsyncContext() == ctx) {  // <--- maybe equals would
>>> be better here? but the object should have the same reference //
>>> if same remove async context from global map } }
> Under what conditions would you expect these async objects to be the
> same/different? It looks like a sanity check, but you seem to be using
> that check to trigger an important state-transition.
If a client loses the connection after a 1st asyncContext has been created
and sends another command to our server, then a 2nd asyncContext would 
be created and stored in the global Map.

The old one would be overwritten and the complete would be triggered for it.
In this case the context from the event will differ from the one stored 
in the global map.

For this reason as well we use the globally cached asyncContext in the 
other methods.

>>> public void onStartAsync(AsyncEvent event) throws IOException {
>>> // does nothing }
>>> And then there is the sending of something which is used by the
>>> listener and other places
>>> // message is some internal used object to contain our message
>>> public static void sendMessageOnAsyncContext(Message message,
>>> AsyncContext context) throws IOException {
>>> try { // send something on the context to inform the client that
>>> we are closing it } finally { ctx.complete(); // remove async
>>> context from global map } } }
>> Can you reproduce the problem in a test environment with 7.0.29?
>>> Currently not
> :(
>
> So as of now your only options are:
>
> 1) Read every line of code in Tomcat to prove to yourself that there
> was a bug in the old version and that it's been fixed in the new version
On this point, I mentioned this exception coming from Tomcat before:

... org.apache.coyote.AbstractProtocol$AbstractConnectionHandler process
SEVERE: null
java.lang.IllegalStateException: Calling [asyncPostProcess()] is not 
valid for a request with Async state [STARTED]
     at 
org.apache.coyote.AsyncStateMachine.asyncPostProcess(AsyncStateMachine.java:202)
     at 
org.apache.coyote.AbstractProcessor.asyncPostProcess(AbstractProcessor.java:116)
     at 
org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:589)
     at 
org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.run(NioEndpoint.java:1653)
     at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
     at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
     at java.lang.Thread.run(Unknown Source)

Do you - now that you have looked at the recycling as well - see any 
connection between this exception
and the recycling?

It would interesting for me to actually reproduce this exception but 
without understanding the inner workings of Tomcat
I doubt I can do that.

> 2) Upgrade Tomcat in production and hope for the best
I am pretty sure, that setting the RECYCLE_FACADE flag to true (and 
therefore disabling the recycling)
will prevent this issue from occurring. But this won't help me to track 
down the cause of this error.

> ??
>
> In any case, I'd start using Tomcat 7.latest in your development and
> test environments and upgrade production as soon as you deem it stable.
>
> - -chris
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
> Comment: GPGTools - http://gpgtools.org
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQIcBAEBCAAGBQJTznyPAAoJEBzwKT+lPKRY8BEQALTC6SBbVaPak3HO+VSkVmnL
> vJYACtxk/0ZmaNmAeqQ5nYwIeFdIfy5yMUbNHMf8t+FSG4hhhk0cs4gQOljlZ1DY
> TG2lyBD1xH80YiLVQb7v0KWi13maQ+59TS0AJusz955DYkGBxbzFI0UYNMkyYhIZ
> OMNf+ALf6iVLwooXq6+NaM8QvAh6Zg3q4iqILgcR1RR7BHSBMMb4uXzR1NYPHBKk
> PAmWsAY1HtegeKwANRnVaGgtZKaKO+95y41RLMbv9cQ/3fj5pRRzgM+1esspR880
> zTkEu+zj4EKbY7dUpWYr0g8A1o+ECKuRa7jtdtdoqgMYvjkGSbGFkQT172FHNqzI
> H3h3JI7UxMLOMA8Ex1ysWvnDinewV0pWf6SQgSeotoYk6Z7usT7BCxRgq+bXXCTw
> xLNkJ6tM2ft/kDpjjvbxv3fSrOd8Sr6eQ6TFgymfpTmHNhWY4DzhRPzdaiwBkf/3
> xcw8Q2W2Rd9566v9UdxxHmYn0enFkr3HgxJeFpl2UnLWplVo3me9XZ0rRIO/bdS2
> pOmAzjL+cgAUcubXfKL2vAyMo0yomquodZ/eG4Lm0MojymfQMrrZM1zj1QzKvPhi
> qUXCQp07Ubwucj+ML226J89iiElkXdF3OfqJDvrfTAxcRxJiuPJKgtpxfwOLy5/V
> nebJTCQ7reqv5ztrGsSH
> =t7vf
> -----END PGP SIGNATURE-----
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: users-help@tomcat.apache.org
>
>
Regards,
Simon

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org


Mime
View raw message