Closed
Bug 1396904
Opened 7 years ago
Closed 7 years ago
Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region)
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: jkratzer, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(4 files)
772 bytes,
text/html
|
Details | |
360.73 KB,
text/plain
|
Details | |
6.95 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
2.81 KB,
patch
|
Details | Diff | Splinter Review |
Testcase found while fuzzing mozilla-central rev 20170904-1401e3eec44d.
Flags: in-testsuite?
Reporter | ||
Comment 1•7 years ago
|
||
Updated•7 years ago
|
Group: core-security → javascript-core-security
Comment 3•7 years ago
|
||
Boris, any ideas? It looks like the interesting part of the stack from the log is we're removing a script blocker, which calls into nsXULCommandDispatcher::UpdateCommands, and then we try to dispatch an event, which has a pending exception, so we try to create a proxy object, but for some reason we're not expecting that to happen here. 0|2|libxul.so|js::ProxyObject::create|hg:hg.mozilla.org/mozilla-central:js/src/vm/ProxyObject.cpp:3ecda4678c49|174|0x16 ... 0|10|libxul.so|JS_GetPendingException|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:3ecda4678c49|6759|0xb 0|12|libxul.so|mozilla::dom::AutoEntryScript::AutoEntryScript|hg:hg.mozilla.org/mozilla-central:dom/script/ScriptSettings.cpp:3ecda4678c49|674|0x5 0|13|libxul.so|mozilla::dom::CallbackObject::CallSetup::CallSetup|hg:hg.mozilla.org/mozilla-central:mfbt/Maybe.h:3ecda4678c49|459|0x5 0|14|libxul.so|mozilla::JSEventHandler::HandleEvent|s3:gecko-generated-sources:d0cd062a8f2e61a1842e705e1539dafa6e2559f266e0cf39cc24ecbc1de67828060d8f7d4015631bcfe7c22d53ac77e562e84587fe96e76b41b346b32ed4aeb6/dist/include/mozilla/dom/EventHandlerBinding.h:|352|0x29 ... 0|19|libxul.so|mozilla::EventDispatcher::Dispatch|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:3ecda4678c49|823|0x5 0|20|libxul.so|nsXULCommandDispatcher::UpdateCommands|hg:hg.mozilla.org/mozilla-central:dom/xul/nsXULCommandDispatcher.cpp:3ecda4678c49|402|0x1f 0|21|libxul.so|nsContentUtils::RemoveScriptBlocker|hg:hg.mozilla.org/mozilla-central:dom/base/nsContentUtils.cpp:3ecda4678c49|5681|0xe 0|22|libxul.so|nsDocument::EndUpdate|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:3ecda4678c49|5357|0x5 0|23|libxul.so|nsHTMLDocument::EndUpdate|hg:hg.mozilla.org/mozilla-central:dom/html/nsHTMLDocument.cpp:3ecda4678c49|2510|0x5 0|24|libxul.so|mozAutoDocUpdate::~mozAutoDocUpdate|hg:hg.mozilla.org/mozilla-central:dom/base/mozAutoDocUpdate.h:3ecda4678c49|40|0x14
Flags: needinfo?(bzbarsky)
Updated•7 years ago
|
Keywords: csectype-uaf,
sec-high
Assignee | ||
Comment 4•7 years ago
|
||
So... We're firing an event. That is, we're about to run script. So in general, at this stackframe: 0|14|libxul.so|mozilla::JSEventHandler::HandleEvent it better be ok to allocate, gc, whatever. So far so good. The assert is coming from the fact that in CallSetup::CallSetup we have a block with a JS::AutoSuppressGCAnalysis at the start. Inside that block we do mAutoEntryScript.emplace which leads to the rest of the stack. So obvious thoughts: 1) It's not clear to me that we need this AutoSuppressGCAnalysis at all. I think it used to be the case that we had a bunch more virtual function calls there, and uses of gcthings after those calls, but I think that's all gone. So I think we can take out the AutoSuppressGCAnalysis entirely. I pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=e23e70332598b990f29bc49d7248a1ecef68537c to verify this theory. 2) If we did not hit this assert, then note that we're doing this JS_GetPendingException call: 0|10|libxul.so|JS_GetPendingException|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:3ecda4678c49|6759|0xb from a debug-only block that is just gathering information so it can fatally assert in the most informative possible way at <http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/dom/script/ScriptSettings.cpp#446>. The fact that we're in this block at all (i.e. there _is_ a pending exception when we call AutoJSAPI::InitInternal) is a bug. In this case, I expect it's basically the same as bug 1393806. At least the testcase has the same structure, with infinite recursion in mutation events. I'll double-check that, though.
Assignee | ||
Comment 5•7 years ago
|
||
Note that I haven't been able to reproduce the assertion with the testcase here. But looking at it more closely, it's definitely got the "infinite recursion during adopt" thing from bug 1393806 going on...
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8905644 -
Flags: review?(continuation)
Updated•7 years ago
|
Group: javascript-core-security
Depends on: 1393806
Keywords: csectype-uaf,
sec-high
Summary: Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region) → (bogus) Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•7 years ago
|
||
Comment 9•7 years ago
|
||
Comment on attachment 8905644 [details] [diff] [review] Remove bogus and no longer needed AutoSuppressGCAnalysis Review of attachment 8905644 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thanks!
Attachment #8905644 -
Flags: review?(continuation) → review+
Comment 10•7 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd62e8a31942 Remove bogus and no longer needed AutoSuppressGCAnalysis. r=mccr8
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bd62e8a31942
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Assignee | ||
Comment 12•7 years ago
|
||
To be clear, the assertion failure was not bogus. It was a real assertion failure as far as it went: we had turned off analysis and were doing gc. It happens there were no live things across that block, which is convenient. And the code that triggered GC only ran in debug builds, which is also convenient.
Summary: (bogus) Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region) → Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region)
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
•