208845 - multiple content-type headers combined breaks mozilla

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030210
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030210

these are the headers from www22.verizon.com.. note that they produce 2
identical content-type headers

HTTP/1.0 200 OK
Server: Microsoft-IIS/5.0
CP: ALL DSP COR CURa ADMa DEVa TAIa PSAa PSDa IVAa IVDa CONa HISa TELa OTPa OUR
BUS IND PHY ONL UNI COM NAV INT DEM OTC
Content-Type: text/html
Cache-Control: no-cache
Content-Type: text/html
Expires: Mon, 09 Jun 2003 21:07:17 GMT
Date: Mon, 09 Jun 2003 20:37:36 GMT
Content-Length: 63366
Connection: keep-alive

mozilla renders this page fine.

after going through a proxy, the content-type headers are combined according to
the rules in 4.2 of rfc 2616.

HTTP/1.1 200 OK
Server: Microsoft-IIS/5.0
CP: ALL DSP COR CURa ADMa DEVa TAIa PSAa PSDa IVAa IVDa CONa HISa TELa OTPa OUR
BUS IND PHY ONL UNI COM NAV INT DEM OTC
Content-Type: text/html,text/html
Cache-Control: no-cache
Expires: Mon, 09 Jun 2003 20:40:17 GMT
Date: Mon, 09 Jun 2003 20:36:39 GMT
Content-Length: 63366
Via: 1.0 proxy
Connection: Keep-Alive

and mozilla can no longer render it.

2616 says "Multiple message-header fields with the same field-name MAY be
present in a message if and only if the entire field-value for that header field
is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to
combine the multiple header fields into one "field-name: field-value" pair,
without changing the semantics of the message, by appending each subsequent
field-value to the first, each separated by a comma. The order in which header
fields with the same field-name are received is therefore significant to the
interpretation of the combined field value, and thus a proxy MUST NOT change the
order of these field values when a message is forwarded."

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Darin Fisher]

yup, servers do wacky things.  i'm not sure why mozilla would work in one case
and not in the other since we do in fact coalesce like-headers before processing
anything.

i can repro this using today's trunk build.  investigating...

Comment 2

[Darin Fisher]

Attachment: v1 patch

Comment 3

[Darin Fisher]

Comment on attachment 125359 [details] [diff] [review]
v1 patch

simple patch to recursively parse multiple media-type specifications.  this is
done intentionally so as to not ignore the charset info in a header like this:

  Content-Type: text/html; charset=ISO-8859-1, text/html

Comment 4

[Darin Fisher]

Comment on attachment 125359 [details] [diff] [review]
v1 patch

nevermind, this patch is slightly wrong.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 125359 [details] [diff] [review]
v1 patch


I will assume that val isn't needed.  

okay, i will bite: What happened to the parsing code in nsMultiMixedConv.cpp,
or more importantly do we need modify NS_ParseContentType to deal with multiple
content types joined to gether or doc it to indicate its behavor?

it would be nice to indicate what section(s) of the RFC you are talking about:

+    // Augmented BNF (from RFC 2616):

Comment 6

[Darin Fisher]

Attachment: v2 patch

ok, this is way overkill for this bug, but since this patch is for alpha, i
decided to go hog wild and cleanup some other things.

doug: to answer your questions,

>I will assume that val isn't needed.  

right, there was a compiler warning.


>okay, i will bite: What happened to the parsing code in nsMultiMixedConv.cpp,
>or more importantly do we need modify NS_ParseContentType to deal with
multiple
>content types joined to gether or doc it to indicate its behavor?

i don't think we need to worry about that situation.  i could be wrong, but i
think it is highly unlikely that we'll encounter this sort of thing in the
multipart/x-mixed-replace case.  also since NS_ParseContentType is used by
numerous nsIChannel::SetContentType implementations to separate out charset
from mime-type, i don't think we want to burden NS_ParseContentType with this
stuff.	i.e., it certainly doesn't make sense to pass "text/html,text/html"
into SetContentType ;-)

ok, now what about all these other changes in this patch?  what are these new
net_*Find* functions.  well... these functions don't already exist, and they
help me clean up some code in necko.  eventually it might make sense to move
them out of necko, but for now since they are really small, i think necko is
fine home.  eventually, i think it'd be good to use them elsewhere in necko
besides the HTTP implementation so i put them in nsURLHelper.{h,cpp} for lack
of any less URL-specific internal .h,.cpp files.

Comment 7

[Darin Fisher]

Comment on attachment 125496 [details] [diff] [review]
v2 patch

see my previous comment for more info on this revised patch.  thx!

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 125496 [details] [diff] [review]
v2 patch

if null is passed to any of the net functions, death is quick.

The names of the parameters of these net functions in the declarations do not
match where they are implemented.

can't/shouldn't net_FindCharInSet(const char *str, const char *set) be
implemented as strpbrk?

I am not sure i like the bounds checking on these functions.  Maybe we should
be carefully in the comments to indicate gigo.

was there any reason that you used \x9 instead of \t originally?

two returns in these net functions instead of the goto?

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 125496 [details] [diff] [review]
v2 patch

spoke to darin.  r=dougt

Comment 10

[Darin Fisher]

Attachment: v2.01

revised per comments from dougt.

Comment 11

[Alec Flett]

Comment on attachment 125920 [details] [diff] [review]
v2.01

wow, servers suck :)

+	     p2 = net_RFindCharNotInSet(p, HTTP_LWS);
+	     *++p2 = 0; // null terminate header value

I understand what's going on here, because FindCharInSet will return the last
character.. but you might want to add more to the comment here, otherwise it
looks like you're null terminating the wrong thing on failure.

-	     SetHeader(atom, nsDependentCString(p), PR_TRUE);
+	     SetHeader(atom, Substring(p, p2), PR_TRUE);

p2 is unquestionably the null termination now - can't you use
nsDependentCString(p, p2) now? (not sure if there is a perf benefit.. but if
SetHeader() does a PromiseFlatCString() then you'll want that)

sr=alecf with or without responses/updates to the above comments.

Comment 12

[Darin Fisher]

thanks alec!  i'll make the changes you suggested.

Comment 13

[Darin Fisher]

fixed-on-trunk

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

This bgu caused an infinite loop hang at espn.com - see bug 210334