The default bug view has changed. See this FAQ.

Crash in nsHtml5TreeBuilder | ASSERTION: The Unicode decoder wrote too much data.: 'end <= NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE' | ASSERTION: The decoder signaled an error other than NS_ERROR_ILLEGAL_INPUT.: 'convResult == NS_ERROR_ILLEGAL_INPUT'

VERIFIED FIXED in Firefox 12

Status

()

Core
History: Global
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bc, Unassigned)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla14
x86
All
assertion, crash, reproducible, testcase
Points:
---

Firefox Tracking Flags

(firefox11 wontfix, firefox12+ verified, firefox13+ verified, firefox14+ verified, firefox-esr1012+ verified)

Details

(Whiteboard: [sg:critical][qa+:ashughes], crash signature, URL)

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
1. http://www.kogakkan-u.ac.jp/users/sirayama/index.html
   perhaps reload

2. Crash Nightly/13. I can't reproduce with Aurora/12 or Beta/11.

###!!! ASSERTION: The Unicode decoder wrote too much data.: 'end <= NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE', file /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5StreamParser.cpp, line 829
###!!! ASSERTION: The decoder signaled an error other than NS_ERROR_ILLEGAL_INPUT.: 'convResult == NS_ERROR_ILLEGAL_INPUT', file /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5StreamParser.cpp, line 836


Mac OS X 10.5
bp-d4c07014-dc00-4446-9f2d-f27532120306
Firefox 13.0a1 Crash Report [@ small_free_list_remove_ptr | small_malloc_from_free_list | szone_malloc ] 

bp-1cdb6ee0-3e92-4686-8a82-bb2d72120306
Firefox 13.0a1 Crash Report [@ small_free_list_remove_ptr | szone_free | free | nsHtml5DataAvailable::~nsHtml5DataAvailable ] 

Windows XP
bp-fafa95bc-780e-4162-a0e0-7d8cd2120306
Firefox 13.0a1 Crash Report [@ nsHtml5TreeBuilder::flushCharacters() ] 

Windows 7
bp-f10abc9a-e10d-4d73-aaa5-99d232120306
Firefox 13.0a1 Crash Report [@ RtlEnterCriticalSection ] 

On Windows 7 with a debug build I saw 
Assertion failure: local < script->nfixed, at c:/work/mozilla/builds/nightly/moz
illa/js/src/jsanalyze.cpp:186

And in Crash automation on Mac OS X 10.6 I saw
Assertion failure: (ptrBits & 0x7) == 0

Operating system: Mac OS X
                  10.6.8 10K549
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0

Thread 0 (crashed)
 0  libmozglue.dylib!MOZ_Crash [Assertions.cpp : 76 + 0x5]
    rbx = 0x00000001002078d0   r12 = 0x0000000103082502
    r13 = 0x0000000100107bb0   r14 = 0x0000000000000010
    r15 = 0x0000000100107bb0   rip = 0x0000000100010e17
    rsp = 0x00007fff5fbf8f80   rbp = 0x00007fff5fbf8f80
    Found by: given as instruction pointer in context
 1  libmozglue.dylib!MOZ_Assert [Assertions.cpp : 88 + 0x4]
    rbx = 0x00000001002078d0   r12 = 0x0000000103082502
    r13 = 0x0000000100107bb0   r14 = 0x0000000000000010
    r15 = 0x0000000100107bb0   rip = 0x0000000100010e79
    rsp = 0x00007fff5fbf8f90   rbp = 0x00007fff5fbf8fb0
    Found by: call frame info
 2  XUL!JSVAL_TO_OBJECT_IMPL [jsval.h : 766 + 0x23]
    rbx = 0x00000001002078d0   r12 = 0x0000000103082502
    r13 = 0x0000000100107bb0   r14 = 0x0000000000000010
    r15 = 0x0000000100107bb0   rip = 0x0000000103654032
    rsp = 0x00007fff5fbf8fc0   rbp = 0x00007fff5fbf8fe0
    Found by: call frame info
 3  XUL!JS::Value::toObject [jsapi.h : 544 + 0xb]
    rbx = 0x00000001002078d0   r12 = 0x0000000103082502
    r13 = 0x0000000100107bb0   r14 = 0x0000000000000010
    r15 = 0x0000000100107bb0   rip = 0x0000000103678308
    rsp = 0x00007fff5fbf8ff0   rbp = 0x00007fff5fbf9000
    Found by: call frame info
 4  XUL!js::GCMarker::processMarkStackTop [jsgcmark.cpp : 1090 + 0x8]
    rbx = 0x00000001002078d0   r12 = 0x0000000103082502
    r13 = 0x0000000100107bb0   r14 = 0x0000000000000010
    r15 = 0x0000000100107bb0   rip = 0x0000000103701752
    rsp = 0x00007fff5fbf9010   rbp = 0x00007fff5fbf90b0
    Found by: call frame info
 5  XUL!js::GCMarker::drainMarkStack [jsgcmark.cpp : 1172 + 0xc]
    rbx = 0x00000001002078d0   r12 = 0x0000000103082502
    r13 = 0x0000000100107bb0   r14 = 0x0000000000000010
    r15 = 0x0000000100107bb0   rip = 0x00000001036fe751
    rsp = 0x00007fff5fbf90c0   rbp = 0x00007fff5fbf9100
    Found by: call frame info
 6  XUL!MarkAndSweep [jsgc.cpp : 3293 + 0x13]
    rbx = 0x00000001002078d0   r12 = 0x0000000103082502
    r13 = 0x0000000100107bb0   r14 = 0x0000000000000010
    r15 = 0x0000000100107bb0   rip = 0x00000001036e2efb
    rsp = 0x00007fff5fbf9110   rbp = 0x00007fff5fbf9190
    Found by: call frame info
 7  XUL!GCCycle [jsgc.cpp : 3644 + 0xb]
    rbx = 0x00000001002078d0   r12 = 0x0000000103082502
    r13 = 0x0000000100107bb0   r14 = 0x0000000000000010
    r15 = 0x0000000100107bb0   rip = 0x00000001036e30e6
    rsp = 0x00007fff5fbf91a0   rbp = 0x00007fff5fbf9210
    Found by: call frame info

s-s due to all of the various crashes related to memory and gc.
(Reporter)

Comment 1

5 years ago
Created attachment 603165 [details]
sirayama.tar.bz2

1. install https://www.squarefree.com/extensions/quitter.xpi
2. tar jxvf sirayama.tar.bz2
3. firefox ./sirayama/index.html

Another nice assertion:

###!!! ASSERTION: Pointer overrun even before check: 'aDest <= aDestEnd', file /work/mozilla/builds/nightly/mozilla/intl/uconv/ucvja/../util/nsUCSupport.h, line 51
(Reporter)

Updated

5 years ago
Keywords: testcase
Henri, can you look into this? I'm guessing sg:critical here as this is a buffer overrun, which could lead to controllable crashes. If you disagree with that rating please let me know.
Assignee: nobody → hsivonen
Whiteboard: [sg:critical]

Updated

5 years ago
status-firefox-esr10: --- → affected
status-firefox10: --- → wontfix
status-firefox11: --- → wontfix
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox12: --- → +
tracking-firefox13: --- → +
(In reply to Bob Clary [:bc:] from comment #0)
> 1. http://www.kogakkan-u.ac.jp/users/sirayama/index.html
>    perhaps reload

So this is Shift_JIS content mislabeled as iso-2022-jp. There's a bug in the iso-2022-jp decoder that's tickled by Shift_JIS input.

> ###!!! ASSERTION: The Unicode decoder wrote too much data.: 'end <=
> NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE', file
> /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5StreamParser.cpp,
> line 829

This is a bad bug in nsISO2022JPToUnicodeV2::Convert. Investigating.

> ###!!! ASSERTION: The decoder signaled an error other than
> NS_ERROR_ILLEGAL_INPUT.: 'convResult == NS_ERROR_ILLEGAL_INPUT', file
> /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5StreamParser.cpp,
> line 836

At least this one is trivial: nsISO2022JPToUnicodeV2::Convert can return NS_ERROR_UNEXPECTED even though it shouldn't do that.
Status: NEW → ASSIGNED
Created attachment 604370 [details] [diff] [review]
Fix for the bug

I changed from == to >= to mitigate the damage in case a buffer overrun happens in the future by other means. When the operation is correct, == can be true but > should never.
Attachment #604370 - Flags: review?
Attachment #604370 - Flags: review? → review?(smontagu)
Created attachment 604371 [details] [diff] [review]
Documentation, test and converter result code correctness
Attachment #604371 - Flags: review?(smontagu)
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #2)
> I'm guessing sg:critical here as this is a
> buffer overrun, which could lead to controllable crashes. If you disagree
> with that rating please let me know.

This bug indeed would allow data of the attackers choice to be written past the end of the buffer with the limitations that such a run of data would be prefixed with the REPLACEMENT CHARACTER and the data would need to be representable as iso-2022-jp.

(In reply to Henri Sivonen (:hsivonen) from comment #3)
> There's a bug in the iso-2022-jp decoder

Well, hunting that "bug" was the wrong approach. The problem was that the HTML5 parser assumed that the Unicode decoders work in a certain way (signal that the output is full when it is full), but they aren't guaranteed to work that way. Instead, a decoder can check for illegal input before checking for the space in the output buffer and can signal illegal input when the output buffer is full also.

So this is yet another case of the decoder API contract being insufficiently documented. :-(

I split the fix into two patches: The first one can be landed as a security fix and the second one as a follow-up after the first one has been delivered to users so that it's OK to reveal more details.
(Reporter)

Comment 7

5 years ago
Henri, could this be a root cause for bug 608994 ?
(In reply to Bob Clary [:bc:] from comment #7)
> Henri, could this be a root cause for bug 608994 ?

I'm not authorized to see that bug.
(In reply to Bob Clary [:bc:] from comment #7)
> Henri, could this be a root cause for bug 608994 ?

Looks unrelated. Different converter back end. Different direction of conversion. Different caller.
Comment on attachment 604370 [details] [diff] [review]
Fix for the bug

Review of attachment 604370 [details] [diff] [review]:
-----------------------------------------------------------------

::: parser/html/nsHtml5StreamParser.cpp
@@ +855,5 @@
> +          nsHtml5OwningUTF16Buffer::FalliblyCreate(
> +            NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE);
> +        if (!newBuf) {
> +          return NS_ERROR_OUT_OF_MEMORY;
> +        }

Can you not share this code instead of adding another copy of it?
Comment on attachment 604371 [details] [diff] [review]
Documentation, test and converter result code correctness

Review of attachment 604371 [details] [diff] [review]:
-----------------------------------------------------------------

::: parser/htmlparser/tests/crashtests/733282-1.html
@@ +1,1 @@
> +<meta charset=iso-2022-jp>€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€€

The raw bytes make this rather obfuscated. Perhaps you can make it clearer by using <iframe src="data:text/html;charset=iso-2022-jp,%80%80%80%80.... or something similar?
Attachment #604371 - Flags: review?(smontagu) → review+
(In reply to Simon Montagu from comment #10)
> Comment on attachment 604370 [details] [diff] [review]
> Fix for the bug
> 
> Review of attachment 604370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: parser/html/nsHtml5StreamParser.cpp
> @@ +855,5 @@
> > +          nsHtml5OwningUTF16Buffer::FalliblyCreate(
> > +            NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE);
> > +        if (!newBuf) {
> > +          return NS_ERROR_OUT_OF_MEMORY;
> > +        }
> 
> Can you not share this code instead of adding another copy of it?

The repeated bit needs to be able to return early and needs to set a local variable in one flavor of the repeated bit. Having a function whose return value needs to be checked in order to return early didn't seem much of a win.

Should I share the code and have a return value check and early return outside the shared bit?
Comment on attachment 604370 [details] [diff] [review]
Fix for the bug

Review of attachment 604370 [details] [diff] [review]:
-----------------------------------------------------------------

> The repeated bit needs to be able to return early and needs to set a local
> variable in one flavor of the repeated bit. Having a function whose return
> value needs to be checked in order to return early didn't seem much of a win.

A matter of taste, I suppose.
Attachment #604370 - Flags: review?(smontagu) → review+
Thanks. I landed the fix without landing the test & documentation patch yet:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3760562b6c
status-firefox-esr10: affected → ---
status-firefox10: wontfix → ---
status-firefox11: wontfix → ---
status-firefox12: affected → ---
status-firefox13: affected → ---
tracking-firefox12: + → ---
tracking-firefox13: + → ---
Target Milestone: --- → mozilla13
Version: Trunk → Other Branch
(Restoring flags reset by the last comment; also trunk is now mozilla14)
status-firefox-esr10: --- → affected
status-firefox11: --- → wontfix
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox12: --- → ?
tracking-firefox13: --- → ?
Target Milestone: mozilla13 → mozilla14
Version: Other Branch → Trunk
https://hg.mozilla.org/mozilla-central/rev/2e3760562b6c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox14: --- → fixed
tracking-firefox-esr10: ? → 12+
tracking-firefox12: ? → +
tracking-firefox13: ? → +
tracking-firefox14: --- → +
(Reporter)

Comment 17

5 years ago
Another example: http://www.tbs.co.jp/anime/kmb/radio/radio.html
Comment on attachment 604370 [details] [diff] [review]
Fix for the bug

[Approval Request Comment]
Regression caused by (bug #): Not really a regression unless you count the HTML5 landing as the cause.
User impact if declined: The user's system might get attacked by an attacker writing data past the end of a buffer. Non-malicious mislabeled pages can crash the browser.
Testing completed (on m-c, etc.): Has baked on m-c for a few days.
Risk to taking this patch (and alternatives if risky): Very low risk. Adds a buffer allocation in one place.
String changes made by this patch: None.
Attachment #604370 - Flags: approval-mozilla-esr10?
Attachment #604370 - Flags: approval-mozilla-beta?
Attachment #604370 - Flags: approval-mozilla-aurora?
Comment on attachment 604370 [details] [diff] [review]
Fix for the bug

[Triage Comment]
Approving to land given low risk and being early in the beta cycle.  For ESR see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Attachment #604370 - Flags: approval-mozilla-esr10?
Attachment #604370 - Flags: approval-mozilla-esr10+
Attachment #604370 - Flags: approval-mozilla-beta?
Attachment #604370 - Flags: approval-mozilla-beta+
Attachment #604370 - Flags: approval-mozilla-aurora?
Attachment #604370 - Flags: approval-mozilla-aurora+
Thanks for the approvals.
http://hg.mozilla.org/releases/mozilla-aurora/rev/772477595ea4
http://hg.mozilla.org/releases/mozilla-beta/rev/35f34594bd5e
http://hg.mozilla.org/releases/mozilla-esr10/rev/0325ae38d384
status-firefox-esr10: affected → fixed
status-firefox12: affected → fixed
status-firefox13: affected → fixed
(Reporter)

Comment 21

5 years ago
tested using Nightly/14, Aurora/13, Beta/12 on Linux, OS X 10.6, Windows XP/7
http://www.tbs.co.jp/anime/kmb/radio/radio.html
http://www.kogakkan-u.ac.jp/users/sirayama/index.html
-> No Crashes
Whiteboard: [sg:critical] → [sg:critical][qa+]
(In reply to Bob Clary [:bc:] from comment #21)
> tested using Nightly/14, Aurora/13, Beta/12 on Linux, OS X 10.6, Windows XP/7
> http://www.tbs.co.jp/anime/kmb/radio/radio.html
> http://www.kogakkan-u.ac.jp/users/sirayama/index.html
> -> No Crashes

So we can call this verified fixed on all three.
Assignee: hsivonen → nobody
Status: RESOLVED → VERIFIED
status-firefox12: fixed → verified
status-firefox13: fixed → verified
status-firefox14: fixed → verified
Component: HTML: Parser → History: Global
QA Contact: parser → history.global
Group: core-security
Verified fixed in Firefox 10.0.4esr and 10.0.5esrpre.
status-firefox-esr10: fixed → verified
Whiteboard: [sg:critical][qa+] → [sg:critical][qa+:ashughes]
You need to log in before you can comment on or make changes to this bug.