1183375 - Assertion failure: !IsInsideNursery(&lir->object()->toConstant()->toObject()), at jit/CodeGenerator.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

x = new WeakMap();
y = [];
x.toString = (function() {
    WeakMap.prototype.get = ArrayBuffer(64);
});
Array.prototype.push.call(y, x);
y.toString();
y.toString();

asserts js debug shell on m-c changeset 7ec3e4b2a45f with --fuzzing-safe --no-threads --ion-eager at Assertion failure: !IsInsideNursery(&lir->object()->toConstant()->toObject()), at jit/CodeGenerator.cpp.

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 7ec3e4b2a45f

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/fd36716d1f9d
user:        Brian Hackett
date:        Sat Jun 13 08:10:55 2015 -0700
summary:     Bug 1162986 - Allow objects to be turned into singletons dynamically, r=jandem.

Brian, is bug 1162986 a likely regressor?

Marking this s-s because a recent bug 1175714 with a similar assert was also marked s-s.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x1a7af9, 0x00000001004da17f js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::jit::CodeGenerator::visitPostWriteBarrierO(this=<unavailable>, lir=<unavailable>) + 863 at CodeGenerator.cpp:2745, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001004da17f js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::jit::CodeGenerator::visitPostWriteBarrierO(this=<unavailable>, lir=<unavailable>) + 863 at CodeGenerator.cpp:2745
    frame #1: 0x00000001004e1109 js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::jit::CodeGenerator::generateBody(this=0x00000001028e0000) + 985 at CodeGenerator.cpp:4108
    frame #2: 0x00000001004fa41a js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::jit::CodeGenerator::generate(this=0x00000001028e0000) + 458 at CodeGenerator.cpp:7784
    frame #3: 0x00000001005554ff js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::jit::GenerateCode(mir=0x00000001028be1a8, lir=0x00000001028c0760) + 303 at Ion.cpp:1713
    frame #4: 0x00000001005555e1 js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::jit::CompileBackEnd(mir=0x00000001028be1a8) + 97 at Ion.cpp:1735
(lldb)

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

The code generation for PostWriteBarrier needs to be able to cope with constant objects that are in the nursery when the code is initially compiled.

Comment 3

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

Is the regressor that Gary identified correct so this only affects 41 and 42?

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8633579 [details] [diff] [review]
patch

Review of attachment 8633579 [details] [diff] [review]:
-----------------------------------------------------------------

Good find.

It seems simpler though to nop MPostWriteBarrier during lowering, and then simply use useRegister instead of useRegisterOrConstant for the LIR instructions, so you don't need any extra codegen stuff?

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Al Billings [:abillings] from comment #3)
> Is the regressor that Gary identified correct so this only affects 41 and 42?

Yes.

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Comment on attachment 8633579 [details] [diff] [review]
> patch
> 
> Review of attachment 8633579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good find.
> 
> It seems simpler though to nop MPostWriteBarrier during lowering, and then
> simply use useRegister instead of useRegisterOrConstant for the LIR
> instructions, so you don't need any extra codegen stuff?

This would be simpler, yeah, but it seems nice that LPostWriteBarrier can take a constant since I don't think it would be that unusual to Ion compile code which writes properties to a specific object.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: patch

Revised patch after talking with Jan on IRC, I didn't quite understand comment 4.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8635331 [details] [diff] [review]
patch

Review of attachment 8635331 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/Lowering.cpp
@@ +2390,5 @@
> +
> +    // LPostWriteBarrier assumes that if it has a constant object then that
> +    // object is tenured, and does not need to be tested for being in the
> +    // nursery. Ensure that assumption holds by lowering constant nursery
> +    // objects to a register.

Ah, when I wrote comment 4, I mistakenly thought there was a case where we could simply not add any LIR instructions here. That's what I meant by "nop during lowering".

But of course that's not possible, because the only case is when the object is in the nursery, but that can change by tenuring it.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 8635331 [details] [diff] [review]
patch

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

Not easily.

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?

aurora

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

bug 1162986

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

trivial

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

not likely

Approval Request Comment
[Feature/regressing bug #]: bug 1162986
[User impact if declined]: exploitable crashes
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low

Comment 10

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

Comment on attachment 8635331 [details] [diff] [review]
patch

Approvals given!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53d79351d89e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b4a6171753f2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53d79351d89e

Comment 14

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx41
JSBugMon: This bug has been automatically verified fixed on Fx42