Closed Bug 395651 Opened 12 years ago Closed 12 years ago

Atom table crash triggered by DOM mutation events and unicode surrogates


(Core :: DOM: Core & HTML, defect, critical)

Not set





(Reporter: jruderman, Assigned: jst)


(Blocks 1 open bug)


(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical])


(2 files)

Loading the testcase triggers:

###!!! ASSERTION: got a High Surrogate but no low surrogate: 'Error', file ../../../dist/include/string/nsUTF8Utils.h, line 322

It also triggers a crash on shutdown within NS_PurgeAtomTable.  The crash looks exploitable.

An interesting thing about the testcase is that the mutation event listener functions don't actually *do* anything.  They just have to be present.  I'm curious why.

Two atom table crashes involving surrogates have already been fixed:
* Bug 377360
* Bug 394275
Flags: blocking1.9?
Whiteboard: [sg:critical]
Wondering whether we should factor harder to have less duplicated or triplicated UTF-* (error) handling code.

Attached patch Fix.Splinter Review
This fixes this crash and hopefully all other similar ones as well. The problem here was in the string comparison code used by the atom table hash wasn't comparing an invalid UTF-16 string correctly to what you got when converting that string to UTF-8. Why this wasn't caught and needed to fix the previous related bugs Jesse pointed at is beyond me at this point.

The testcase in this bug I believe covers all the cases of invalid UTF-16 strings, high surrogate at end of string, high surrogate not followed by a low surrogate, and low surrogate not preceded by a high surrogate. And in addition to that I also tested with valid strings around and in between invalid characters etc. All seems good with this patch.

And this patch will change how our CompareUTF8toUTF16() function works with invalid strings. With this change an invalid character in the UTF-16 string will be treated as 0xFFFD, and thus if the UTF-8 contains that, then, and only then, will the strings compare equal. Invalid UTF-8 will still never compare equal to anything.
Assignee: nobody → jst
Attachment #280509 - Flags: superreview?(jonas)
Attachment #280509 - Flags: review?(jonas)
Comment on attachment 280509 [details] [diff] [review]

Is |err| still used in CompareUTF8toUTF16? Or could you simply pass nsnull?

Attachment #280509 - Flags: superreview?(jonas)
Attachment #280509 - Flags: superreview+
Attachment #280509 - Flags: review?(jonas)
Attachment #280509 - Flags: review+
Fix landed on the trunk (with Jonas' suggestion applied). And I filed bug 396240 on making the character iterators more consistent in how they deal with invalid unicode data.
Closed: 12 years ago
Resolution: --- → FIXED
Branch hits some assertions but doesn't crash on shutdown.
Group: security
Flags: wanted1.8.1.x-
This should probably be tested with a C++ test in xpcom/tests.
Flags: in-testsuite?
Blocks: 489041
crash test landed

leaving in-testsuite? for an xpcom/tests C++ test.
Group: core-security
Is there a reason this bug is still hidden, beyond no one having gotten around to unhiding it yet?  The crash test gives away the attachment. The clone of this opened as bug 489041 is re-fixed on 1.9.0.  The rest of this bug beyond the attachment doesn't really contain anything not in bug 489041 already.  What am I missing?
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.