1357022 - Assertion failure: obj->is<CrossCompartmentWrapperObject>(), at js/src/jscompartment.cpp:433 with nukeCCW

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision ce69b6e1773e (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

const root3 = newGlobal();
function test(constructor) {
    if (!nukeCCW(root3.load)) {}
}
test(Map);
test(Set);


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000095e2b0 in JSCompartment::getOrCreateWrapper (this=this@entry=0x7ffff692b000, cx=cx@entry=0x7ffff694c000, existing=..., existing@entry=..., obj=obj@entry=...) at js/src/jscompartment.cpp:433
#0  0x000000000095e2b0 in JSCompartment::getOrCreateWrapper (this=this@entry=0x7ffff692b000, cx=cx@entry=0x7ffff694c000, existing=..., existing@entry=..., obj=obj@entry=...) at js/src/jscompartment.cpp:433
#1  0x000000000095e3d2 in JSCompartment::wrap (this=this@entry=0x7ffff692b000, cx=cx@entry=0x7ffff694c000, obj=obj@entry=...) at js/src/jscompartment.cpp:485
#2  0x000000000047188b in JSCompartment::wrap (this=0x7ffff692b000, cx=0x7ffff694c000, vp=...) at js/src/jscompartmentinlines.h:134
#3  0x0000000000a487fc in js::CrossCompartmentWrapper::get (this=0x1e984e0 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff694c000, wrapper=..., receiver=..., id=..., vp=...) at js/src/proxy/CrossCompartmentWrapper.cpp:229
#4  0x0000000000a60ed9 in js::Proxy::get (cx=0x7ffff694c000, proxy=..., receiver_=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:325
#5  0x0000000000529e09 in js::GetProperty (vp=..., name=<optimized out>, receiver=..., obj=..., cx=0x7ffff694c000) at js/src/jsobj.h:845
#6  js::GetProperty (cx=0x7ffff694c000, v=..., name=..., vp=...) at js/src/vm/Interpreter.cpp:4386
#7  0x000000000052dd1d in GetPropertyOperation (vp=..., lval=..., pc=<optimized out>, script=..., fp=<optimized out>, cx=<optimized out>) at js/src/vm/Interpreter.cpp:193
#8  Interpret (cx=0x7ffff694c000, state=...) at js/src/vm/Interpreter.cpp:2725
[...]
#18 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8683
rax	0x0	0
rbx	0x7ffff694c000	140737330331648
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffc840	140737488341056
rsp	0x7fffffffc770	140737488340848
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7fffffffc7c0	140737488340928
r13	0x7fffffffc7a0	140737488340896
r14	0x7fffffffc7e0	140737488340960
r15	0x7fffffffc940	140737488341312
rip	0x95e2b0 <JSCompartment::getOrCreateWrapper(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JSObject*>)+752>
=> 0x95e2b0 <JSCompartment::getOrCreateWrapper(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JSObject*>)+752>:	movl   $0x0,0x0
   0x95e2bb <JSCompartment::getOrCreateWrapper(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JSObject*>)+763>:	ud2    


Test involves nukeCCW, not sure if this is shell-only or also affects the browser, marking s-s.

Comment 1

[Daniel Veditz [:dveditz]]

calling "sec-other" on the theory that web content can't trigger nukeCCW directly, and that we'll be passing valid objects to it.

Comment 2

[Jon Coppeard (:jonco)]

Tom, you added nukeCCW recently.  Any chance you could take a look at this?

Comment 3

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/d439fa74bf05
user:        Tom Schuster
date:        Thu Feb 23 15:26:49 2017 +0100
summary:     Bug 1319087 - Add nukeCCW to the shell and test it. r=jandem

This iteration took 264.472 seconds to run.

Comment 4

[Tom S. (please needinfo tschuster)]

Looks like this happens because NukeCrossCompartmentWrapper doesn't remove the wrapper from the compartments' crossCompartmentWrappers Map. NukeCrossCompartmentWrappers does remove it from the list before calling it with removeFront()

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Tom Schuster [:evilpie] from comment #4)
Thanks for looking into this.

I thought it was expected that the wrapper stayed in the map, but this assertion argues otherwise.

Unfortunately, GC depends on this (in JSCompartment::findDeadProxyZoneEdges, see bug 1343261).

Comment 6

[Jon Coppeard (:jonco)]

Maybe we should relax this assertion and allow getOrCreateWrapper to return dead object proxies.  NukeCrossCompartmentWrappers should probably also not remove the wrappers.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1357022-nuke-ccw

Allow getOrCreateWrapper to return a DeadObjectProxy for a nuked CCW.  Stop NukeCrossCompartmentWrappers remove these from the map since the GC depends on there presence.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1357022-remove-nuked-ccws

Back in bug 1343261 we added code to the GC to sweep compartments whose wrapper map contained nuked CCWs at the same time as their targets so that those wrappers would be successfully swept from the map.

We went with this fairly complex approach because we wanted the map to return the nuked CCW again if it was subsequently queried for the target.

I'm beginning to think that this was a mistake.  Apart from introducing all that complexity it turns out that js::NukeCrossCompartmentWrappers always removes the nuked CCWs from the map anyway.  And the assertion in JSCompartment::getOrCreateWrapper indicates that it was not expected for the map to retain dead CCWs.

Removing nuked CCWs does give some slightly strange behaviour (you gave an example in bug 1343261) but nuking is already violating a bunch of expected behaviour anyway so I don't think that's a problem.

Here's a patch to revert the behaviour and remove nuked CCWs from the map.

Try is looking OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2db6125ace7ca88494df608aed797b99f0c6741

Sorry for the flip-flopping on this.  What do you think?

Comment 9

[Nathan Froyd [:froydnj]]

Gentle review ping on this one, since it's probably responsible for fixing two sec bugs, and bug 1363229 might be more general than this?

Comment 10

[Steve Fink [:sfink] [:s:]]

Sorry, looks like a mail filtering problem. I thought I had finally fixed everything. :(

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 8867648 [details] [diff] [review]
bug1357022-remove-nuked-ccws

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? Everything back to 53.

If not all supported branches, which bug introduced the flaw? Bug 1343261 

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be trivial.

How likely is this patch to cause regressions; how much testing does it need? Unlikely.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

This is a sec-other rated bug so technically doesn't need sec-approval+ but...

Normally I'd say not to check in the tests until after we ship as well. We don't usually want to zero day ourselves by checking in a test that illustrates the problem.

So, this is either a "sec-other" so you can do what you want with it (but it won't be back ported normally) or this is mis-rated and we should put another rating on it and not check in the test until we release Firefox with this fix. 

Can you clarify, Jon?

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Al Billings [:abillings] from comment #12)
In all uses from gecko that I could find we remove CCWs that we nuke, so this affects just the shell.  So I guess this is sec-other.

The patch does remove some fairly hairy code though so I would like to backport it to affected branches.

Comment 14

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d2fe983c62ff24968247a37bde80746f3d5f70

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8d2fe983c62

Comment 17

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Jon Coppeard (:jonco)]

Attachment: bug1357022-beta

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1343261.
[User impact if declined]: I'm pretty sure that affects the shell only but it does remove some hairy code so I'd like to see if fixed where possible.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: None.
[Why is the change risky/not risky?]: This is mainly code removal and has been in nightly since 25th May.
[String changes made/needed]: None.

Comment 20

[Jon Coppeard (:jonco)]

Attachment: bug1357022-esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Removes hairy code and simplifies GC.
User impact if declined: Probably none.
Fix Landed on Version: 55.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8872577 [details] [diff] [review]
bug1357022-beta

We are about to gtb the last beta build and enter RC week on Monday. Given that this isn't a sec-high/sec-crit, I'd prefer this fix ride the 55 train. Wontfix for 54 and esr52.

Comment 22

[Daniel Veditz [:dveditz]]

(In reply to Ritu Kothari (:ritu) from comment #21)
> We are about to gtb the last beta build and enter RC week on Monday. Given
> that this isn't a sec-high/sec-crit, I'd prefer this fix ride the 55 train.
> Wontfix for 54 and esr52.

I agree we shouldn't rush this into 54, but I'd like to get it fixed for ESR 52.3 (or whatever the 55-equivalent is).

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8872578 [details] [diff] [review]
bug1357022-esr52

Make this ESR52+ based on Dan's last comment. Let's get this on ESR52.3 (which ships with 55).

Comment 24

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5bd51bc3f5873e0e7aa5a56553f9f0cd0e04bc21