Closed Bug 1384615 Opened 3 years ago Closed 3 years ago

Assertion failure: !wcompartment->lookupWrapper(ObjectValue(*newTarget)), at /home/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:623

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 - wontfix
firefox57 + fixed
firefox58 + fixed

People

(Reporter: jkratzer, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [fixed by bug 1404107][adv-main57+][post-critsmash-triage])

Attachments

(2 files)

Attached file trigger.html
Testcase found while fuzzing mozilla-central rev 20170726-e8400551c2e3.

Assertion failure: !wcompartment->lookupWrapper(ObjectValue(*newTarget)), at /home/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:623

ASAN:DEADLYSIGNAL
=================================================================
==578==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f8f02762b6a bp 0x7fff309eeaf0 sp 0x7fff309ee6a0 T0)
==578==The signal is caused by a WRITE memory access.
==578==Hint: address points to the zero page.
    #0 0x7f8f02762b69 in js::RemapWrapper(JSContext*, JSObject*, JSObject*) /home/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:652:13
    #1 0x7f8f02762f4c in js::RemapAllWrappersForObject(JSContext*, JSObject*, JSObject*) /home/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:690:9
    #2 0x7f8f024fee8a in JS_TransplantObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) /home/worker/workspace/build/src/js/src/jsapi.cpp:918:10
    #3 0x7f8efa4d848e in xpc::TransplantObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) /home/worker/workspace/build/src/js/xpconnect/wrappers/WrapperFactory.cpp:675:34
    #4 0x7f8efcd7f1c1 in mozilla::dom::ReparentWrapper(JSContext*, JS::Handle<JSObject*>) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2285:10
    #5 0x7f8efb6f4200 in nsNodeUtils::CloneAndAdopt(nsINode*, bool, bool, nsNodeInfoManager*, JS::Handle<JSObject*>, nsCOMArray<nsINode>*, nsINode*, nsINode**) /home/worker/workspace/build/src/dom/base/nsNodeUtils.cpp:587:14
    #6 0x7f8efb6f44bc in nsNodeUtils::CloneAndAdopt(nsINode*, bool, bool, nsNodeInfoManager*, JS::Handle<JSObject*>, nsCOMArray<nsINode>*, nsINode*, nsINode**) /home/worker/workspace/build/src/dom/base/nsNodeUtils.cpp:608:12
    #7 0x7f8efb60945c in nsNodeUtils::Adopt(nsINode*, nsNodeInfoManager*, JS::Handle<JSObject*>, nsCOMArray<nsINode>&) /home/worker/workspace/build/src/dom/base/nsNodeUtils.h:228:19
    #8 0x7f8efb5aa4b8 in nsIDocument::AdoptNode(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/nsDocument.cpp:7677:8
    #9 0x7f8efb6090a0 in nsDocument::AdoptNode(nsIDOMNode*, nsIDOMNode**) /home/worker/workspace/build/src/dom/base/nsDocument.cpp:7536:34
    #10 0x7f8efb6abda2 in AdoptNodeIntoOwnerDoc(nsINode*, nsINode*) /home/worker/workspace/build/src/dom/base/nsINode.cpp:1546:16
    #11 0x7f8efb6aeeac in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/nsINode.cpp:2435:14
    #12 0x7f8efb6acce8 in ConvertNodesOrStringsIntoNode(mozilla::dom::Sequence<mozilla::dom::OwningNodeOrString> const&, nsIDocument*, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/nsINode.cpp:1717:15
    #13 0x7f8efb6ad639 in nsINode::Prepend(mozilla::dom::Sequence<mozilla::dom::OwningNodeOrString> const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/nsINode.cpp:1888:5
    #14 0x7f8efc8c2696 in mozilla::dom::DocumentBinding::prepend(JSContext*, JS::Handle<JSObject*>, nsIDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/DocumentBinding.cpp:11833:9
    #15 0x7f8efcd8634e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3060:13
    #16 0x7f8f01cb36b1 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #17 0x7f8f01cb325d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:469:16
    #18 0x7f8f01cb40f5 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:514:12
    #19 0x7f8f01cb430c in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:533:10
    #20 0x7f8f0278f8fe in js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /home/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:169:12
Component: DOM: Core & HTML → JavaScript Engine
Hi Shu-yu,
Can you help find someone to look at this issue?
Flags: needinfo?(shu)
Not so sure about the user impact here. Track 56-. Feel free to nominate again if there is a user impact here.
Flags: needinfo?(shu) → needinfo?(jorendorff)
This bisects down to bug 1355109 as the culprit. Jason says he's got a fix in hand. 57 and 58 technically aren't affected at this point due to the backout, but I'm going to leave them marked as affected since the intent is still to re-land there.
Assignee: nobody → jorendorff
Blocks: 1355109
Group: javascript-core-security
Has Regression Range: --- → yes
Priority: -- → P1
57 and 58 are unaffected due to the backout there.
RyanVM pointed out that these flags are like this so that it isn't dropped, should we decide to reland.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> Jason says he's got a fix in hand.

Is that a fix in one of the other bugs related to bug 1355109? If so please make this "depend on" it. Otherwise can we get them in here and start reviews so this can make 57?
Flags: needinfo?(jorendorff)
Keywords: sec-high
> Is that a fix in one of the other bugs related to bug 1355109?

Yes, in bug 1404107.  I've explicitly been not marking that related to any security bugs to avoid attracting attention to it, fwiw.
I re-applied the Xray IC patch on my machine and this assertion does not happen.

I think the patches in bug 1404107 fixed it. But I don't see why. I don't know what's causing the assertion. The obvious mechanism would be that cloning the marquee causes XBL to have an xray wrapper to the clone. But if that were the case, then my patches would not have fixed the bug. And anyway bz already told me that XBL does not get hold of the clone so early, because we use the low-level JS_CloneObject to create it.
I just confirmed that backing out the three revisions below (keeping the Xray IC patch) causes the assertion to happen again.

But it doesn't crash as a load-only crashtest! Not imagining it.

39d2b1b24e0f (bug 1396466)
208cf9b36e87 (bug 1404107)
a9c5ab491f2f (also bug 1404107)

$ ./mach crashtest dom/base/crashtests/1384615.html
REFTEST INFO | Running with e10s: True
REFTEST INFO | Application command: /Users/jorendorff/work/gecko/obj-x86_64-apple-darwin16.7.0/dist/NightlyDebug.app/Contents/MacOS/firefox -marionette -foreground -profile /var/folders/2f/vxcscyq11kg3439lb05pmj8c0000gn/T/tmp6Z3SHv.mozrunner
...
REFTEST SUITE-START | Running 1 tests
REFTEST TEST-START | file:///Users/jorendorff/work/gecko/dom/base/crashtests/1384615.html
REFTEST TEST-LOAD | file:///Users/jorendorff/work/gecko/dom/base/crashtests/1384615.html | 0 / 1 (0%)
++DOMWINDOW == 3 (0x128a10800) [pid = 67989] [serial = 4] [outer = 0x12fb70800]
++DOCSHELL 0x18e5db800 == 2 [pid = 67989] [id = {e7ba520e-207c-4446-8253-c2aea21225b9}]
++DOMWINDOW == 4 (0x18e5dc000) [pid = 67989] [serial = 5] [outer = 0x0]
++DOMWINDOW == 5 (0x18fdc3800) [pid = 67989] [serial = 6] [outer = 0x18e5dc000]
--DOCSHELL 0x18e5db800 == 1 [pid = 67989] [id = {e7ba520e-207c-4446-8253-c2aea21225b9}]
REFTEST TEST-PASS | file:///Users/jorendorff/work/gecko/dom/base/crashtests/1384615.html | (LOAD ONLY)
REFTEST TEST-END | file:///Users/jorendorff/work/gecko/dom/base/crashtests/1384615.html
++DOMWINDOW == 6 (0x18e5d4800) [pid = 67989] [serial = 7] [outer = 0x12fb70800]
REFTEST INFO | Result summary:
REFTEST INFO | Successful: 1 (0 pass, 1 load only)
REFTEST INFO | Unexpected: 0 (0 unexpected fail, 0 unexpected pass, 0 unexpected asserts, 0 failed load, 0 exception)
REFTEST INFO | Known problems: 0 (0 known fail, 0 known asserts, 0 random, 0 skipped, 0 slow)
REFTEST SUITE-END | Shutdown
...
Flags: needinfo?(jorendorff) → needinfo?(bzbarsky)
I'm on vacation today and tomorrow, and on planes all day Monday. bz is taking over here (and, generally, the effort to reland bug 1355109 and uplift fixes to beta 57).
> I don't know what's causing the assertion.

This part I know.

During cloning, we copy the expando object chain over.  This used (before bug 1404107) to happen before the transplant.

Also, the XBL scope is a sandbox, so gets its own exclusive expandos.  bhackett's IC patch made it so when copying expandos on something that has an exclusive expando, we instantiate an Xray to the new target so we can store it on the new wrapper holder.

So the sequence of events in the testcase looks like this, with bhackett's IC patch but without bug 1404107:

1) The marquee is created, XBL is bound, expandos are set.  We now have a marquee JSObject (M1) living in compartment C1, and an Xray to it (X1) from the XBL sandbox (which has compartment S1, let's say, for "sandbox").

2) The marquee is adopted into a new document.  We create a new JSObject (M2) living in the new document's compartment (C2).  We clone the expando chain, which creates a new Xray (X2) pointing from compartment S1 to M2.

3) We transplant M1 and M2.  This involves remapping wrappers pointing to M1 to now point to M2.  When we go to remap X1, we discover that we already have a wrapper for M2 in S1; that's the assert that fires.

That is, the steps are basically the same as bug 1403716 comment 5, but when transplanting there is NOT an existing CCW, so we keep using the M2 we created, don't hit the issues of bug 1403716 but instead hit this assertion.

I'm looking into the crashtest situation now.
Ah, the crashtest doesn't catch it because all of that stuff I described relies on the use of a separate XBL scope.  And that's disabled by default in reftests/crashtests; see http://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/layout/tools/reftest/reftest-preferences.js#1-5

Simply changing the manifest line to:

  pref(dom.use_xbl_scopes_for_remote_xul,true) load 1384615.html

makes the crashtest fail with the IC patch applied and the patches mentioned in comment 9 backed out.
Flags: needinfo?(bzbarsky)
So fixed, by bug 1404107.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Whiteboard: [fixed by bug 1404107]
Target Milestone: --- → mozilla58
Group: javascript-core-security → core-security-release
Whiteboard: [fixed by bug 1404107] → [fixed by bug 1404107][adv-main57+]
Flags: qe-verify-
Whiteboard: [fixed by bug 1404107][adv-main57+] → [fixed by bug 1404107][adv-main57+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.