395651 - Atom table crash triggered by DOM mutation events and unicode surrogates

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (causes shutdown crash)

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

Comment 1

[Brendan Eich [:brendan]]

Wondering whether we should factor harder to have less duplicated or triplicated UTF-* (error) handling code.

/be

Comment 2

[Johnny Stenback (:jst)]

Attachment: Fix.

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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 280509 [details] [diff] [review]
Fix.

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

r/sr=me

Comment 4

[Johnny Stenback (:jst)]

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.

Comment 5

[Jesse Ruderman]

Branch hits some assertions but doesn't crash on shutdown.

Comment 6

[Jesse Ruderman]

This should probably be tested with a C++ test in xpcom/tests.

Comment 7

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/6ce912e80cf2

leaving in-testsuite? for an xpcom/tests C++ test.

Comment 8

[Jeff Walden [:Waldo]]

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?