Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 334311 - Wrong reuse of buffers in CachedExchange
Summary: Wrong reuse of buffers in CachedExchange
Status: RESOLVED FIXED
Alias: None
Product: Jetty
Classification: RT
Component: client (show other bugs)
Version: 7.2.1   Edit
Hardware: Macintosh Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 7.2.x   Edit
Assignee: Greg Wilkins CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 334309 334310 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-01-13 14:37 EST by Mathieu Poumeyrol CLA
Modified: 2011-02-23 00:18 EST (History)
2 users (show)

See Also:

Attachments
test case (2.08 KB, text/plain)
2011-01-13 14:37 EST, Mathieu Poumeyrol CLA 		no flags Details
junitized test case (2.69 KB, text/plain)
2011-01-14 05:17 EST, Mathieu Poumeyrol CLA 		jesse.mcconnell: iplog+
Details
patch (620 bytes, text/plain)
2011-01-14 05:44 EST, Mathieu Poumeyrol CLA 		no flags Details
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mathieu Poumeyrol CLA 2011-01-13 14:37:31 EST 
Two CachedExchange instances can overwrite each other response "fields", leading to corrupted headers.
Comment 1 Mathieu Poumeyrol CLA 2011-01-13 14:37:59 EST 
Created attachment 186770 [details]
test case
Comment 2 Mathieu Poumeyrol CLA 2011-01-14 04:54:12 EST 
*** Bug 334309 has been marked as a duplicate of this bug. ***
Comment 3 Mathieu Poumeyrol CLA 2011-01-14 04:54:45 EST 
*** Bug 334310 has been marked as a duplicate of this bug. ***
Comment 4 Mathieu Poumeyrol CLA 2011-01-14 05:17:39 EST 
Created attachment 186810 [details]
junitized test case

In the current state of the code, both assertion in testHeaderWhenReadEarly pass, whereas the assert() testHeaderWhenReadLate breaks.

It appears that the bug does not appear when headers are read before the second exchange is triggered.

I am not sure wether or not calling getResponseFields() in onResponseComplete() warrants the headers values will be cached *before* another exchange use the shared buffers again when execution order is not imposed.
Comment 5 Mathieu Poumeyrol CLA 2011-01-14 05:44:14 EST 
Created attachment 186814 [details]
patch
Comment 6 Mathieu Poumeyrol CLA 2011-01-14 05:53:20 EST 
For some reason I don't fully understand, code in HttpFields makes deep copy of headers names, but only build View for "_value" buffer.

The proposed patch makes a deep copy of the value calling asImmutableBuffer() before passing it to HttpFields in onResponsePatch().

It make the test case pass and seams quite innocuous.

As a matter of fact, for the sake of concern isolation and consistency, I think the header name should be passed as an immutable buffer too. Current code in HttpFields.add() will make it immutable anyway so there should be no performance impact at all.
Comment 7 Jesse McConnell CLA 2011-01-14 16:20:15 EST 
hm, thanks for this

will take a deeper look soon
Comment 8 Jesse McConnell CLA 2011-01-14 17:08:26 EST 
Sending        VERSION.txt
Sending        jetty-client/src/main/java/org/eclipse/jetty/client/CachedExchange.java
Adding         jetty-client/src/test/java/org/eclipse/jetty/client/CachedHeadersIsolationTest.java
Transmitting file data ...
Committed revision 2663.

I have committed this fix but before I close this issue I am going to assign to Greg for some additional review.
Comment 9 Greg Wilkins CLA 2011-02-23 00:18:37 EST 
thanks