Closed Bug 1183375 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 + verified
firefox42 + verified
firefox43 --- verified
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: gkw, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update][b2g-adv-main2.5-])

Attachments

(2 files, 1 obsolete file)

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.
Flags: needinfo?(bhackett1024)
Attached file 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)
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8633579 - Flags: review?(jdemooij)
Keywords: sec-high
Is the regressor that Gary identified correct so this only affects 41 and 42?
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?
Attachment #8633579 - Flags: review?(jdemooij) → review+
(In reply to Al Billings [:abillings] from comment #3)
> Is the regressor that Gary identified correct so this only affects 41 and 42?

Yes.
(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.
Attached patch patchSplinter Review
Revised patch after talking with Jan on IRC, I didn't quite understand comment 4.
Attachment #8633579 - Attachment is obsolete: true
Attachment #8635331 - Flags: review?(jdemooij)
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.
Attachment #8635331 - Flags: review?(jdemooij) → review+
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
Attachment #8635331 - Flags: sec-approval?
Attachment #8635331 - Flags: approval-mozilla-aurora?
Comment on attachment 8635331 [details] [diff] [review]
patch

Approvals given!
Attachment #8635331 - Flags: sec-approval?
Attachment #8635331 - Flags: sec-approval+
Attachment #8635331 - Flags: approval-mozilla-aurora?
Attachment #8635331 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/53d79351d89e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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
Group: core-security → core-security-release
Group: core-security-release
Whiteboard: [jsbugmon:update] → [jsbugmon:update][b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.