Last Comment Bug 600974 - crash (nsUTF8ToUnicode overruns buffer by 2 bytes outputting surrogate pair) on :first-letter punctuation test in CSS 2.1 test suite
: crash (nsUTF8ToUnicode overruns buffer by 2 bytes outputting surrogate pair) ...
Status: RESOLVED FIXED
: [sg:critical?] [qa-examined-191] [qa-...
: crash, css2
Product: Core
Classification: Components
Component: Internationalization
: Trunk
: All All
: -- normal (vote)
: mozilla2.0
Assigned To: Simon Montagu
: i18n
: http://test.csswg.org/suites/css2.1/2...
:
:
  Show dependency treegraph
 
Reported: 2010-09-30 14:23 PDT by David Baron [:dbaron]
Modified: 2011-05-18 18:18 PDT (History)
12 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  final+
  ---
  .14+
  .14-fixed
  .17+
  .17-fixed


Attachments
patch (3.59 KB, patch)
2010-09-30 15:02 PDT, David Baron [:dbaron]
no flags Details | Diff | Splinter Review
patch (4.39 KB, patch)
2010-09-30 19:01 PDT, David Baron [:dbaron]
smontagu: review-
Details | Diff | Splinter Review
Patch (3.48 KB, patch)
2010-10-12 10:56 PDT, Simon Montagu
VYV03354: review+
dbaron: review+
dveditz: approval1.9.2.14+
dveditz: approval1.9.1.17+
Details | Diff | Splinter Review
Test (32.74 KB, patch)
2010-10-12 10:57 PDT, Simon Montagu
no flags Details | Diff | Splinter Review
UTF-32 testcase (12.00 KB, text/html; charset=UTF-32BE)
2010-10-14 09:29 PDT, Simon Montagu
no flags Details
Patch for GB18030 and UTF-16 (9.21 KB, patch)
2010-10-19 03:45 PDT, Simon Montagu
VYV03354: review+
dveditz: approval1.9.2.14+
dveditz: approval1.9.1.17+
Details | Diff | Splinter Review
Tests for GB18030 and UTF-16 (40.54 KB, patch)
2010-10-19 03:46 PDT, Simon Montagu
no flags Details | Diff | Splinter Review
1.8 version (10.18 KB, patch)
2011-01-28 07:43 PST, Martin Stránský
no flags Details | Diff | Splinter Review

Summon comment box

Description David Baron [:dbaron] 2010-09-30 14:23:25 PDT
Loading the following test in the CSS 2.1 test suite (which is derived from a test I wrote):
crashes Firefox intermittently.

Steps to reproduce:
 1. load http://test.csswg.org/suites/css2.1/20100917/html4/first-letter-punct-before-036.htm
 2. reload the page a few times

Actual results: crash, generally after less than 10 reloads

I've tested that this occurs on both 4.0b6 and on the 2010-09-30-03-mozilla-central nightly, in both cases on 64-bit Linux.


In a debug build, loading the page reliably triggers the assertions:

###!!! ASSERTION: The Unicode decoder wrote too much data.: 'end <= NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE', file /home/dbaron/builds/mozilla-central/mozilla/parser/html/nsHtml5StreamParser.cpp, line 495

A buffer overrun causing a crash may well be exploitable (it's harder, but still possible, if it's a heap buffer overrun), so marking security-sensitive.
Comment 1 David Baron [:dbaron] 2010-09-30 14:26:06 PDT
I suspect this is related to incorrect handling of characters that are represented in UTF-16 as surrogate pairs, i.e., non-BMP characters.
Comment 2 David Baron [:dbaron] 2010-09-30 14:32:35 PDT
Actually, the bug may well be that nsUTF8ToUnicode::Convert will happily write one PRUnichar past the end of the buffer when it's outputting a surrogate pair.
Comment 3 David Baron [:dbaron] 2010-09-30 14:35:56 PDT
These steps to reproduce are specific to the HTML5 parser, which isn't on old branches, but the bug is on those old branches and it's likely possible to exploit it there.

I think it only allows overrunning the buffer by only two bytes, though.
Comment 4 David Baron [:dbaron] 2010-09-30 15:02:41 PDT
Created attachment 479926 [details] [review]
patch

I think this is probably the safest approach to fixing it.

I started to write a different approach on the assumption that nobody would be silly enough to repeatedly call convert with a 1 char output buffer... but then (a) again, it's possible that somebody could be and (b) fixing it by backing up introduces significant complexity that I wasn't confident I could get right.

This patch should apply all the way back to 1.9.0, with fuzz.  (I actually accidentally wrote it in my 1.9.0 tree while looking to see how far back the bug went.)
Comment 5 David Baron [:dbaron] 2010-09-30 15:13:17 PDT
I should probably also add a comment here:

+            if (out >= outend) {
+              mRemainingSurrogate = lowSurr;

noting that out >= outend means this is our last iteration through the loop, but we don't want to |break| because we need the assignments below.
Comment 6 David Baron [:dbaron] 2010-09-30 19:01:10 PDT
Created attachment 479989 [details] [review]
patch
Comment 7 David Baron [:dbaron] 2010-09-30 20:48:30 PDT
Actually, maybe this isn't the best approach, since it seems like the other decoders don't split surrogate pairs across output blocks.
Comment 8 Simon Montagu 2010-09-30 21:14:17 PDT
Crashtest?
Comment 9 Simon Montagu 2010-09-30 21:15:58 PDT
(In reply to comment #7)
> Actually, maybe this isn't the best approach, since it seems like the other
> decoders don't split surrogate pairs across output blocks.

I was wondering about that too. Do you think splitting the surrogate pair will cause problems?
Comment 10 Simon Montagu 2010-09-30 21:24:06 PDT
FWIW, nsUTF32ToUnicode and nsGBKToUnicode seem to have the same bug.
Comment 11 David Baron [:dbaron] 2010-09-30 21:32:55 PDT
nsUTF32ToUnicode doesn't look like it overruns the buffer; it just drops a character on the floor, I think.

ucvlatin/nsUCS2BEToUnicode.cpp also has correctness bug when a surrogate pair
ought to cross a block boundary in the destination buffer (it outputs a replacement char instead of the surrogates it ought to)
nsGBKToUnicode::ConvertNoBuff does indeed look broken; I hadn't yet gotten to any of the ConvertNoBuff implementations, or for that matter even figured out how nsBufferDecoderSupport::Convert worked or who derived from it.

We probably ought to have a more general audit here...

(In reply to comment #9)
> I was wondering about that too. Do you think splitting the surrogate pair will
> cause problems?

It depends what the callers do with the result.  If they're just pasting the chunks back together then they should be fine.  But if they look into them before doing so, there could be problems.
Comment 12 Henri Sivonen (:hsivonen) 2010-09-30 23:51:46 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > Actually, maybe this isn't the best approach, since it seems like the other
> > decoders don't split surrogate pairs across output blocks.
> 
> I was wondering about that too. Do you think splitting the surrogate pair will
> cause problems?

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.
http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5StreamParser.cpp#536
My bad. :-(

In any case, the decoder interface contract should be documented to cover this case so that users of the API know what to do.

If the decode loop in the HTML5 parser were fixed to take into account the case mentioned above, the worst that could happen with the high and low surrogates going into different output buffers would be a discretionary flush happening in the middle of a surrogate pair and the first surrogate got notified to layout before the second is available. However, if the decoder doesn't emit the high surrogate until it has consumed the entire 4-bytes UTF-8 sequence, AFAICT, there's no way for a discretionary flush to take place in the middle of a surrogate pair.

So the HTML5 parser should be OK once the decode loop itself has been fixed--assuming that the new API contract makes it possible to grab the second surrogate out of the decoder without pushing new bytes to it so that the parser can always obtain the complete surrogate pair inside the same runnable task that pushed the 4-byte sequence into the decoder.
Comment 13 Daniel Veditz 2010-10-01 10:25:24 PDT
Comment on attachment 479989 [details] [review]
patch

From the comments sounds like we don't need a separate branch patch. We'll wait for trunk landing (or at least blocking) first.
Comment 14 David Baron [:dbaron] 2010-10-01 10:44:26 PDT
(In reply to comment #12)
> If the decode loop in the HTML5 parser were fixed to take into account the case
> mentioned above, the worst that could happen with the high and low surrogates
> going into different output buffers would be a discretionary flush happening in
> the middle of a surrogate pair and the first surrogate got notified to layout
> before the second is available.

I think we don't want that to happen.
Comment 15 Simon Montagu 2010-10-03 02:45:58 PDT
Comment on attachment 479989 [details] [review]
patch

So the consensus seems to be that we don't want to do it this way. David, do you want to make a new patch, or would you rather punt to me (in view of what you said in comment 4)?
Comment 16 David Baron [:dbaron] 2010-10-03 08:23:14 PDT
I'd be happy if you wrote the new patch.
Comment 17 David Baron [:dbaron] 2010-10-11 17:59:07 PDT
There are a bunch of other bugs (bug 597849, bug 596874, bug 599600) at least some of the occurrences of which are probably this bug... and they're public.

Do you know when you'll have a chance to work on this?  Should I take it back?
Comment 18 David Baron [:dbaron] 2010-10-11 19:07:01 PDT
*** Bug 603525 has been marked as a duplicate of this bug. ***
Comment 19 David Baron [:dbaron] 2010-10-11 19:09:40 PDT
There are also public comments in bug 564008.
Comment 20 Simon Montagu 2010-10-11 20:57:00 PDT
This is about 90% done, but I got distracted by other bugs.
Comment 21 Simon Montagu 2010-10-12 10:56:26 PDT
Created attachment 482587 [details] [review]
Patch
Comment 22 Simon Montagu 2010-10-12 10:57:15 PDT
Created attachment 482589 [details] [review]
Test

Reftest rather than crashtest in order to test for correct decoding.
Comment 23 Jonas Sicking (:sicking) 2010-10-12 14:21:28 PDT
Did you forget to request a reviewer? This needs to get on someones review queue.
Comment 24 Simon Montagu 2010-10-12 20:55:13 PDT
(In reply to comment #23)
> Did you forget to request a reviewer? This needs to get on someones review
> queue.

Bug 372539 must die!
Comment 25 David Baron [:dbaron] 2010-10-13 22:21:49 PDT
Comment on attachment 482587 [details] [review]
Patch

r=dbaron
Comment 26 Masatoshi Kimura [:emk] 2010-10-13 23:55:02 PDT
Comment on attachment 482587 [details] [review]
Patch

I couldn't run reftests (even without patches), but the patch looks fine.
Comment 27 Simon Montagu 2010-10-14 00:53:58 PDT
http://hg.mozilla.org/mozilla-central/rev/4c146ed860b3
http://hg.mozilla.org/mozilla-central/rev/3a24c22ddd4f

Keeping open for other decoders per comment 10 and 11.

I'm not so convinced that we need this on branches without STR, and if we do take it we should probably take fixes for other buffer overruns that were exposed by the HTML5 parser. I remember bug 563618, and I think there were others.
Comment 28 Simon Montagu 2010-10-14 02:21:52 PDT
Both UTF-32 and GB18030 trigger the assertions. Since HTML5 recommends against supporting UTF-32, maybe the time has come to remove it rather than invest time in fixing it. I opened bug 604317 on that.
Comment 29 Henri Sivonen (:hsivonen) 2010-10-14 05:08:01 PDT
(In reply to comment #12)
> 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.
> http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5StreamParser.cpp#536
> My bad. :-(
> 
> In any case, the decoder interface contract should be documented to cover this
> case so that users of the API know what to do.

What's the new API contract? How should I change nsHtml5StreamParser.cpp?
Comment 30 Masatoshi Kimura [:emk] 2010-10-14 09:22:55 PDT
Sayre, we need to fix the UTF-32 decoder unless we remove the encoder itself.
Comment 31 Simon Montagu 2010-10-14 09:29:22 PDT
Created attachment 483185 [details]
UTF-32 testcase

UTF-32 testcase (asserts and crashes)
Comment 32 Simon Montagu 2010-10-14 09:40:22 PDT
umm, actually the UTF-32 testcase has different assertions and doesn't seem to crash, so maybe it's not such high priority after all.

###!!! ASSERTION: The Unicode decoder consumed the wrong number of bytes.: 'totalByteCount == (PRInt32)aCount', file /home/smontagu/mozwork/hgtree/mozilla/parser/html/nsHtml5StreamParser.cpp, line 540
###!!! ASSERTION: Wrong number of stream bytes written/sniffed.: 'writeCount == aLength', file /home/smontagu/mozwork/hgtree/mozilla/parser/html/nsHtml5StreamParser.cpp, line 682
Comment 33 Simon Montagu 2010-10-14 11:23:18 PDT
(In reply to comment #29)
> 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.
Comment 34 Simon Montagu 2010-10-18 08:58:35 PDT
(In reply to comment #29)
> 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.
Comment 35 Simon Montagu 2010-10-19 03:45:23 PDT
Created attachment 484264 [details] [review]
Patch for GB18030 and UTF-16
Comment 36 Simon Montagu 2010-10-19 03:46:13 PDT
Created attachment 484265 [details] [review]
Tests for GB18030 and UTF-16
Comment 37 Masatoshi Kimura [:emk] 2010-10-19 09:38:25 PDT
Comment on attachment 484264 [details] [review]
Patch for GB18030 and UTF-16

Please set a language and a font-family to the <html> element explicitly. Otherwise reftests will fail on non-English locales because those locales may select a non-Western or sans-serif font for UTF-8 documents. At least the tests don't pass on Japanese locale unless I add lang="en" style="font-family: serif" to the <html> element of 600974-1.html and 600974-3.html.

Otherwise looks good.
Comment 38 Simon Montagu 2010-10-19 12:02:20 PDT
(In reply to comment #37)
> At least the tests
> don't pass on Japanese locale unless I add lang="en" style="font-family: serif"
> to the <html> element of 600974-1.html and 600974-3.html.

I originally did this in 600974-2.html, because it didn't pass on en-US locale, but even with the language specified it still fails. I couldn't find a solution, so I ended up giving up and making 2 reference versions, 600974-1-ref.html and 600974-2-ref.html, which are identical except for the charset. Do you have any idea how to avoid this? I'm not sure if it's a bug in font selection or expected behaviour.
Comment 39 Masatoshi Kimura [:emk] 2010-10-19 15:41:58 PDT
Did you also specify font-family: serif? On Japanese and Chinese locale, the default font-family is sans-serif which seems to affect a font selection for comma characters (U+002C). I could match 600974-2-ref.html rednering to 600974-1-ref.html by adding style="font-family: serif".
Comment 40 Simon Montagu 2010-10-20 09:28:37 PDT
http://hg.mozilla.org/mozilla-central/rev/7c6637b28e51
http://hg.mozilla.org/mozilla-central/rev/a7f4a58793c0

font-family: serif wasn't enough, but with font-size as well I was able to remove the duplicate reference rendering.
Comment 41 Simon Montagu 2010-12-18 21:58:42 PST
wrt branches, see comment 27.
Comment 42 Daniel Veditz 2010-12-20 10:51:58 PST
Comment on attachment 482587 [details] [review]
Patch

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers
Comment 43 Daniel Veditz 2010-12-20 10:52:29 PST
Comment on attachment 484264 [details] [review]
Patch for GB18030 and UTF-16

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers
Comment 46 Al Billings [:abillings] 2011-01-04 13:17:00 PST
Have we seen this crash on 1.9.2 or 1.9.1? Using 1.9.2.13 and 1.9.1.16 released builds on XP with either the original URL or the testcase attached, I don't get any crashes.
Comment 47 Martin Stránský 2011-01-28 07:43:03 PST
Created attachment 507856 [details] [review]
1.8 version
Comment 48 Henri Sivonen (:hsivonen) 2011-02-15 07:12:16 PST
(In reply to comment #34)
> (In reply to comment #29)
> > 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.

Filed bug 634262 as a follow-up.
Comment 49 Daniel Veditz 2011-05-18 18:11:09 PDT
*** Bug 597849 has been marked as a duplicate of this bug. ***
Comment 50 Daniel Veditz 2011-05-18 18:18:03 PDT
*** Bug 599600 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.