The setHeader( ) method in the org.apache.coyote.Response class does not clear existing values for a header with multiple values. According to the Servlet Specification version 2.4 (SRV.5.2, para 2): ----- The setHeader method sets a header with a given name and value. A previous header is replaced by the new header. Where a set of header values exist for the name, the values are cleared and replaced with the new value. ----- This requirement has been true since at least Servlets 2.2 (same location in Serlvets 2.3 specification; SRV.6.2, para 2 in the Servlets 2.2 specification). Where only one value exists, the setHeader( ) method properly replaces it with the specified value. Attached is a patch that fixes the bug. In creating the patch, I found that: a) the setHeader( ) method in Response relies on the org.apache.tomcat.util.http.MimeHeaders class's setValue( ) method. b) the setValue( ) method in MimeHeaders relies on getValue( ) in MimeHeaders c) the getValue( ) method in MimeHeaders explicitly states, "If more than one such field is in the header, an arbitrary one is returned." d) the MimeHeaders class's source code is prefixed with: "XXX XXX XXX Need a major rewrite !!!!" Therefore, with the existing MimeHeaders class, all existing header values must be removed before calling getValue( ). If the MimeHeaders class is rewritten, this requirement may be alleviated by a design change in consideration of the Servlet Specification's requirements for setHeader( ). The attached patch simply removes the named header (by calling MimeHeader's removeHeader( ) method) before calling MimeHeader's setValue( ) method. This means that the removeHeader( ) method will be called with every execution of the Response's setHeader( ) method. Since removeHeader( ) iterates over every header, we incur a penalty for every execution of setHeader( ), and the penalty grows as the number of headers increases. However, since: a) there is no way to find out whether multiple values exist for a specified header (other than iterating as is done by removeHeader( )) b) the containsHeader( ) and getHeader( ) methods iterate over the headers (by calling getValue( )) (as is done by removeHeader( )) c) the average number of headers is small (assumed to be about 8) attempting to determine if multiple values exist for the header before calling removeHeader( ) is fruitless since the same penalty is paid whether zero, one, or multiple values exist for the named header and the penalty is identical to that imposed by calling the removeHeader( ) method without first checking for the existence of a header. If checking first, the penalty would double for the case where multiple values exist. If the assumption that the number of headers is small holds true, the penalty should be minimal. I'm marking the severity as minor because the bug is unlikely to affect many users (only those trying to replace all values of a header with multiple values) and workarounds exist (i.e. ensure that the headers are set to their correct value the first time ;), or reset the response while retaining and setting the values that should not be replaced). However, until this bug is fixed, Tomcat is not in compliance with the Servlet Specification. ;) This bug is also confirmed to exist in Tomcat CVS, 5.5.8, 5.5.7, and in 5.0.30 code. It may also exist in Tomcat 4.x and Tomcat 3.x. Attached patch fixes the bug. The patch is against Tomcat 5.5.8 source and applies cleanly to CVS. Attached WAR file contains an example of the problem.
Created attachment 14526 [details] WAR file showing multiple value headers are not reset with call to setHeader( ) The WAR file contains an index.jsp that redirects the user to a servlet that triggers the bug. You will need to view the response headers to see the violation. The servlet may be accessed directly with the request: GET /SetHeaders/SetHeaders HTTP/1.0
Created attachment 14527 [details] Proposed patch Original bug summary describes the decisions made for the patch.
-1 for the proposed patch (quite inefficient). Overall, I think the issue is trivial while the change to make it work efficiently is not exactly trivial, so there is no hurry to fix this.
Created attachment 14531 [details] Slightly more efficient patch - one interation over the list of headers This patch changes the setValue( ) method call to an addValue( ) method call to avoid needlessly iterating over the entire list in the call to setValue( )--since setValue( ) will not find a header with given name after all headers with that name have been removed. After this patch, a call to setHeader( ) will require one full iteration through the list of headers; whereas current CVS iterates over half the list, on average, inside the call to setValue( ). Therefore, assuming the DEFAULT_HEADER_SIZE of 8, this will mean an additional 4 String comparisons, but without using a hash or some kind of custom search tree or redesigning MimeHeaders with multiple header values in mind, a full iteration over the list is unavoidable. So, are the additional String comparisons a small enough price to pay for compliance? :) A better solution is to fix the setValue( ) method of the MimeHeaders class--assuming (since the partial javadoc doesn't say so) that setValue( ) should replace all existing headers having the given name. If this is the intended behavior of the setValue( ) method, the next patch could be used instead. It's consequences (and implementation) are identical to those of this patch.
Created attachment 14532 [details] Patch to fix the setValue( ) method of MimeHeaders (on which Response's setHeader( ) method relies) This patch implements the changes given in http://issues.apache.org/bugzilla/attachment.cgi?id=14531 in the MimeHeaders method setValue( ) instead of the Response method setHeader( ). The consequences and implementation are the same as those described in the patch for Response. Only one of the patches should be used. Since the partial javadoc for the setValue( ) method was not clear about its intended behavior, I was unsure which class should be changed.
The problem is not the cost of the comparisons, but of removeHeader if it matches anything, so -1 for any of the proposed patches. I will fix it eventually, but the issue is small enough that it is ok to not fix it immediately.
This is fixed now in the CVS, and will appear in 5.5.9.