Closed
Bug 395651
Opened 17 years ago
Closed 17 years ago
Atom table crash triggered by DOM mutation events and unicode surrogates
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: jst)
References
Details
(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical])
Attachments
(2 files)
708 bytes,
text/html
|
Details | |
4.73 KB,
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•17 years ago
|
Whiteboard: [sg:critical]
Comment 1•17 years ago
|
||
Wondering whether we should factor harder to have less duplicated or triplicated UTF-* (error) handling code. /be
Assignee | ||
Comment 2•17 years ago
|
||
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
Status: NEW → ASSIGNED
Attachment #280509 -
Flags: superreview?(jonas)
Attachment #280509 -
Flags: review?(jonas)
Comment on attachment 280509 [details] [diff] [review] Fix. Is |err| still used in CompareUTF8toUTF16? Or could you simply pass nsnull? r/sr=me
Attachment #280509 -
Flags: superreview?(jonas)
Attachment #280509 -
Flags: superreview+
Attachment #280509 -
Flags: review?(jonas)
Attachment #280509 -
Flags: review+
Assignee | ||
Comment 4•17 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 5•16 years ago
|
||
Branch hits some assertions but doesn't crash on shutdown.
Group: security
Flags: wanted1.8.1.x-
Reporter | ||
Comment 6•16 years ago
|
||
This should probably be tested with a C++ test in xpcom/tests.
Flags: in-testsuite?
Comment 7•15 years ago
|
||
crash test landed http://hg.mozilla.org/mozilla-central/rev/6ce912e80cf2 leaving in-testsuite? for an xpcom/tests C++ test.
Updated•15 years ago
|
Group: core-security
Comment 8•15 years ago
|
||
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?
Updated•15 years ago
|
Group: core-security
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•