632419 - HTTP 100 level parser race condition

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

I have seen this case arise for an HTTP response:

1: HTTP/1.1 100 Continue


2: 
3: HTTP/1.1 200 OK

4: Server: Moz-PipelineTester.r2

5: Content-Length: 7

6: Cache-Control: no-cache


Specifically we read that in two segments, separated between lines 3 and 4.

nsHTTPTransaction::ParseHead correctly locates the 100 line and then starts feeding lines 1, 2, and 3 into parselinesegment() waiting for the headers to be complete.. they aren't complete so we do the next read and come back into parsehead() which blows up because mHttpResponseHead is false again (a side effect of 100 detection) and the code goes looking for "HTTP/1" starting at line 4.

This is a race condition because if either 2 lines or 6 lines are presented in the first segment it all works out and that seems to be the natural segmentation points. It also mixes any response headers from the 100 into the final response, which is bad.

I'll patch it by recursively calling parsehead again to look for "HTTP/1" after a 100 is detected.

Comment 1

[Patrick McManus [:mcmanus]]

>  It also mixes any response headers from the 100 into the
> final response, which is bad.
>

actually it doesn't do that. But it still fails to parse with the race condition.

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: 100-continue-parsing.v1

Comment 3

[Boris Zbarsky [:bzbarsky]]

Shouldn't we propagate the nsresult from the recursive call out?

Also, with this setup a stream of 1xx responses will cause us to recurse to death, right?

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to comment #3)
> Shouldn't we propagate the nsresult from the recursive call out?
> 

yes, that was dumb. thanks.

> Also, with this setup a stream of 1xx responses will cause us to recurse to
> death, right?

we will only recurse a single read buffer full of them (4KB iirc)... that's ~200 stack frames for a minimally crafted "HTTP/1.1 100 C\r\n\r\n".. I had initially thought that was ok, but I guess for some platforms it could be pushing it. I'll refactor.

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: 100-continue-parsing v2

reflects comments 3 and 4

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 511376 [details] [diff] [review]
100-continue-parsing v2

r=me.  Thanks!

Comment 7

[Patrick McManus [:mcmanus]]

http://hg.mozilla.org/mozilla-central/rev/dbe8ef0649ac

Comment 8

[Joe Drew (not getting mail)]

Backed out on suspicion of causing a Tp4 regression: http://hg.mozilla.org/mozilla-central/rev/25a266b7d18e

Comment 9

[Joe Drew (not getting mail)]

Er, http://hg.mozilla.org/mozilla-central/rev/27d5f3938369

Comment 10

[Patrick McManus [:mcmanus]]

this is cleared of being the tp4 regression (it was 628561).. ready to push again

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/77415b6ad9da

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/77415b6ad9da