Closed Bug 632419 Opened 9 years ago Closed 9 years ago

HTTP 100 level parser race condition

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → 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.
Attached patch 100-continue-parsing.v1 (obsolete) — Splinter Review
Attachment #510639 - Flags: review?(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?
(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.
reflects comments 3 and 4
Attachment #510639 - Attachment is obsolete: true
Attachment #511376 - Flags: review?(bzbarsky)
Attachment #510639 - Flags: review?(bzbarsky)
Comment on attachment 511376 [details] [diff] [review]
100-continue-parsing v2

r=me.  Thanks!
Attachment #511376 - Flags: review?(bzbarsky) → review+
Blocks: 603503
http://hg.mozilla.org/mozilla-central/rev/dbe8ef0649ac
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out on suspicion of causing a Tp4 regression: http://hg.mozilla.org/mozilla-central/rev/25a266b7d18e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
this is cleared of being the tp4 regression (it was 628561).. ready to push again
Pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/77415b6ad9da
Flags: in-testsuite?
Whiteboard: fixed-in-cedar
http://hg.mozilla.org/mozilla-central/rev/77415b6ad9da
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.