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

RESOLVED FIXED in mozilla2.0

Status

()

Core
HTML: Parser
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

Trunk
mozilla2.0
Points:
---

Firefox Tracking Flags

(status1.9.2 unaffected, status1.9.1 unaffected)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Summary: Ensure that nsHtml5StreamParser::FinalizeSniffing is correct given the Unicode decoder interface → Ensure that nsHtml5StreamParser::WriteStreamBytes is correct given the Unicode decoder interface
(Assignee)

Comment 1

7 years ago
Created attachment 512770 [details] [diff] [review]
Adjust assertions

AFAICT, only assertions needed changing.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #512770 - Flags: review?(smontagu)

Updated

7 years ago
Whiteboard: [sg:nse][keep private until bug 600974 and bug 634257 are public]
Attachment #512770 - Flags: review?(smontagu) → review+
(Assignee)

Comment 2

7 years ago
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-
(Assignee)

Comment 5

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Group: core-security
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.