Closed Bug 634262 Opened 13 years ago Closed 13 years ago

Ensure that nsHtml5StreamParser::WriteStreamBytes is correct given the Unicode decoder interface

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: hsivonen, Assigned: hsivonen)

Details

(Whiteboard: [sg:nse][keep private until bug 600974 and bug 634257 are public])

Attachments

(1 file)

From bug 600974:
> The decode loop in the HTML5 parser assumes that if the result is
> NS_PARTIAL_MORE_OUTPUT the decoder hasn't consumed all input bytes. This is
> wrong, of course, if the decoder consumed a 4-byte sequence but had room to
> write only the first surrogate.

> > What's the new API contract? How should I change nsHtml5StreamParser.cpp?
> 
> I don't think there's any change, except for the assertion that you linked to
> before.
> 
>    * Unless there is not enough output space, this method must consume all the
>    * available input data! 
> 
> This doesn't necessarily imply the converse, i.e. that where there isn't enough
> output space, not all input data will be consumed. I'll add a sentence to
> clarify that.

> > What's the new API contract? How should I change nsHtml5StreamParser.cpp?
> 
> The assertion 
>     NS_ASSERTION(byteCount > 0 || NS_FAILED(convResult),
>         "The decoder consumed too few bytes but did not signal an error.");
> will also need to change to cover the case where the decoder returns
> NS_PARTIAL_MORE_OUTPUT at the very end of the input, and the caller has to call
> Convert() again with no input.

Marking as security-sensitive, since I'm quoting from bug 600974, which is security-sensitive. Also, bug 634257 is related and security-sensitive.
Summary: Ensure that nsHtml5StreamParser::FinalizeSniffing is correct given the Unicode decoder interface → Ensure that nsHtml5StreamParser::WriteStreamBytes is correct given the Unicode decoder interface
AFAICT, only assertions needed changing.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #512770 - Flags: review?(smontagu)
Whiteboard: [sg:nse][keep private until bug 600974 and bug 634257 are public]
Attachment #512770 - Flags: review?(smontagu) → review+
Comment on attachment 512770 [details] [diff] [review]
Adjust assertions

Nominating for approval, because having right assertions in Gecko 2.0 may help avoid confusion about wrong assertions firing when doing branch maintenance.
Attachment #512770 - Flags: approval2.0?
Comment on attachment 512770 [details] [diff] [review]
Adjust assertions

a=beltzner
Attachment #512770 - Flags: approval2.0? → approval2.0+
Comment on attachment 512770 [details] [diff] [review]
Adjust assertions

Didn't land in time.
Attachment #512770 - Flags: approval2.0+ → approval2.0-
Actually, this did land. Looks like I've failed to update the bug. Sorry.
http://hg.mozilla.org/mozilla-central/rev/b1ef0685b2e0
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Group: core-security
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: