1437450 - Assertion failure: startOfUninitialized <= nfixed, at js/src/jit/MacroAssembler.cpp:1163

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect, P1, critical)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 2b7d42d527af (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --disable-oom-functions --ion-eager):

function testBug507425() {
    var r = /x/;
    for (var i = 0; i < 3; i++) r.lastIndex = 0;
    var s = ';';
    try {
        for (i = 0; i < 80; testBug507425(s)) s = s;
    } catch (exc) {
        r.DateTimeFormat = 0;
    }
}
testBug507425();


Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000000000824aa5 in js::jit::MacroAssembler::initGCSlots (this=this@entry=0x7ffff5fbe048, obj=obj@entry=..., temp=..., temp@entry=..., templateObj=templateObj@entry=0x7ffff5899080, initContents=initContents@entry=true) at js/src/jit/MacroAssembler.cpp:1163
#1  0x0000000000824e77 in js::jit::MacroAssembler::initGCThing (this=0x7ffff5fbe048, obj=..., temp=..., templateObj=templateObj@entry=0x7ffff5899080, initContents=initContents@entry=false, convertDoubleElements=convertDoubleElements@entry=false) at js/src/jit/MacroAssembler.cpp:1257
#2  0x000000000082a12f in js::jit::MacroAssembler::createGCObject (this=<optimized out>, obj=..., obj@entry=..., temp=..., temp@entry=..., templateObj=templateObj@entry=0x7ffff5899080, initialHeap=initialHeap@entry=js::gc::DefaultHeap, fail=fail@entry=0x7ffff5fceee8, initContents=true, convertDoubleElements=false) at js/src/jit/MacroAssembler.cpp:921
#3  0x000000000068b435 in js::jit::CodeGenerator::visitRegExp (this=0x7ffff5fbe000, lir=0x7ffff5fb6fa8) at js/src/jit/CodeGenerator.cpp:1239
#4  0x00000000006cbcef in js::jit::CodeGenerator::generateBody (this=this@entry=0x7ffff5fbe000) at js/src/jit/CodeGenerator.cpp:5568
#5  0x00000000006cc815 in js::jit::CodeGenerator::generate (this=this@entry=0x7ffff5fbe000) at js/src/jit/CodeGenerator.cpp:9748
#6  0x00000000006f83a6 in js::jit::GenerateCode (mir=mir@entry=0x7ffff5fc4270, lir=0x7ffff5fb6230) at js/src/jit/Ion.cpp:1966
#7  0x0000000000751949 in js::jit::CompileBackEnd (mir=mir@entry=0x7ffff5fc4270) at js/src/jit/Ion.cpp:1988
#8  0x0000000000bb8db8 in js::HelperThread::handleIonWorkload (this=this@entry=0x7ffff5f28098, locked=...) at js/src/vm/HelperThreads.cpp:1838
[...]
#14 0x00007ffff6c383dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
rax	0x0	0
rbx	0x1	1
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7ffff5cfcb90	140737317424016
rsp	0x7ffff5cfcb20	140737317423904
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff5cfe700	140737317431040
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x3	3
r13	0x4	4
r14	0x7ffff5899080	140737312821376
r15	0xfff9800000000000	-1829587348619264
rip	0x824aa5 <js::jit::MacroAssembler::initGCSlots(js::jit::Register, js::jit::Register, js::NativeObject*, bool)+997>
=> 0x824aa5 <js::jit::MacroAssembler::initGCSlots(js::jit::Register, js::jit::Register, js::NativeObject*, bool)+997>:	movl   $0x0,0x0
   0x824ab0 <js::jit::MacroAssembler::initGCSlots(js::jit::Register, js::jit::Register, js::NativeObject*, bool)+1008>:	ud2


Marking s-s because violating this assertion looks bad.

Comment 1

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

I've looked at this only because it happened to come up while fuzzing nursery strings. The crashing stack is nice and short. The main thread is reporting over-recursion from within jit code. I haven't looked any further than that.

Comment 2

[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/76b0524a77e4
user:        Jan de Mooij
date:        Sun Jul 09 11:46:57 2017 +0200
summary:     Bug 1115355 - Optimize RegExpObject allocation in Ion. r=evilpie

This iteration took 266.287 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Needinfo for :jandem based on comment 2.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

The underlying bug predates bug 1115355.

The problem is that the MakeMRegExpHoistable pass looks at all uses of MRegExp (regexp literals) but if the script has a try-catch block it's possible we're missing uses in catch blocks (because Ion skips these). The easiest fix is to check hasTryBlock, similar to what we do in EliminateDeadResumePointOperands.

This is pretty bad because it lets you modify the template RegExpObject; I think we should backport this patch to beta/ESR.

FWIW, the following test fails with --ion-eager --no-threads. I'm pretty sure this failed before bug 1115355 landed.

function f() {
    var arr = [];
    for (var i = 0; i < 50; i++) {
        try {
            var re = /foo/;
            if (i % 5 === 0)
                throw 1;
        } catch (e) {
            arr.push(re);
        }
    }
    for (var i = 0; i < arr.length; i++) {
        for (var j = 0; j < arr.length; j++) {
            assertEq(arr[i] === arr[j], i === j);
        }
    }
}
f();

Comment 5

[Jan de Mooij [:jandem]]

Let's treat this as sec-high at least.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8950554 [details] [diff] [review]
Patch

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

This patch fixes the try-catch issue, but I will note that IsRegExpHoistable does not handle the implictlyUsed / removedUses flags, which might cause similar issues with polymorphic callsites.

One alternative solution would be to allocate a proper RegExp object when we bailout instead of re-using the template object after the bailout.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8950554 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
With some effort it's possible probably.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. (There's a comment, but the code change says it all anyway.)

> Which older supported branches are affected by this flaw?
All.

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

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely. Maybe a perf regression but I don't expect it.

Comment 8

[Al Billings [:abillings]]

Comment on attachment 8950554 [details] [diff] [review]
Patch

sec-approval+ for trunk.
Please make and nominate Beta and ESR52 patches as well, to land after trunk.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is there an ETA for this landing?

Comment 10

[Liz Henry (:lizzard)]

This can still make the Beta 14 build tomorrow if you can land it and uplift to beta tonight.  
Nicolas, maybe you can help with that but I'll also follow up in email with jandem.

Comment 11

[Jan de Mooij [:jandem]]

I wanted to land this yesterday but the tree was closed for most of the day.

And now it's also closed :/

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61b461277369e0cec89c79d8526a82c575818e94

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8950554 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Security issues, crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: On inbound.
[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?]: No.
[Why is the change risky/not risky?]: It just disables an optimization in some cases. It might affect performance in some cases but I don't expect it to be serious.
[String changes made/needed]: None.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8950554 [details] [diff] [review]
Patch

m-b was merged to m-r today, so approving for m-r and the Fennec b15 relbranch.

Jan, does this need an ESR52 approval request as well?

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8950554 [details] [diff] [review]
Patch

Yeah this patch is so small and safe that we should just take it on ESR52 IMO.

[Approval Request Comment]
User impact if declined: Security issues.
Fix Landed on Version: 60, backport to 59.
Risk to taking this patch (and alternatives if risky): Very low risk.
String or UUID changes made by this patch: None.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8950554 [details] [diff] [review]
Patch

Fixes a sec-high. Approved for ESR 52.7.0.

Comment 17

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/61b461277369

Comment 18

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/36f58000ec41 (FIREFOX_59b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/806696d49430

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/16b8073d5c30

Comment 21

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx59