Closed
Bug 632419
Opened 14 years ago
Closed 14 years ago
HTTP 100 level parser race condition
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla5
People
(Reporter: mcmanus, Assigned: mcmanus)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•14 years ago
|
Assignee: nobody → mcmanus
Assignee | ||
Comment 1•14 years ago
|
||
> 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.
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #510639 -
Flags: review?(bzbarsky)
Comment 3•14 years ago
|
||
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?
Assignee | ||
Comment 4•14 years ago
|
||
(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.
Assignee | ||
Comment 5•14 years ago
|
||
reflects comments 3 and 4
Attachment #510639 -
Attachment is obsolete: true
Attachment #511376 -
Flags: review?(bzbarsky)
Attachment #510639 -
Flags: review?(bzbarsky)
Comment 6•14 years ago
|
||
Comment on attachment 511376 [details] [diff] [review]
100-continue-parsing v2
r=me. Thanks!
Attachment #511376 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 7•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 8•14 years ago
|
||
Backed out on suspicion of causing a Tp4 regression: http://hg.mozilla.org/mozilla-central/rev/25a266b7d18e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 9•14 years ago
|
||
Assignee | ||
Comment 10•14 years ago
|
||
this is cleared of being the tp4 regression (it was 628561).. ready to push again
Comment 11•14 years ago
|
||
Pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/77415b6ad9da
Flags: in-testsuite?
Whiteboard: fixed-in-cedar
Comment 12•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 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.
Description
•