1242810 - Assertion failure: MIR instruction returned object with unexpected type, at js/src/jit/MacroAssembler.cpp:1445

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision c2256ee8ae9a (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --ion-offthread-compile=off --ion-eager):

loadFile(`
  function foo({}) {
    for (i = 0 ; i < 100; i++) 
      re = /erwe/;
  }
  RegExp.multiline = 1;
  foo(0);
`);
function loadFile(lfVarx) {
  lfGlobal = newGlobal();
  lfGlobal.offThreadCompileScript(lfVarx);
  lfGlobal.runOffThreadScript();
}



Backtrace:

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffff7fbfdc5 in ?? ()
#0  0x00007ffff7fbfdc5 in ?? ()
[...]
#16 0x00007fffffffa510 in ?? ()
#17 0x00000000006bc38e in EnterIon (data=..., cx=0x404) at js/src/jit/Ion.cpp:2808
#18 js::jit::IonCannon (cx=0x404, state=...) at js/src/jit/Ion.cpp:2903
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
rax	0x7ffff7ead100	140737352749312
rbx	0xfff8800000000001	-2111062325329919
rcx	0x7ffff7ea65b0	140737352721840
rdx	0x7ffff6907818	140737330051096
rsi	0x7ffff7ead100	140737352749312
rdi	0x7ffff7ead150	140737352749392
rbp	0x7fffffffa048	140737488330824
rsp	0x7fffffffa038	140737488330808
r8	0xfff9800000000000	-1829587348619264
r9	0x7fffffff9e40	140737488330304
r10	0x7fffffff9c20	140737488329760
r11	0x7ffff6c27960	140737333328224
r12	0x0	0
r13	0x7fffffffa730	140737488332592
r14	0x404	1028
r15	0x7fffffffa100	140737488331008
rip	0x7ffff7fbfdc5	140737353874885
=> 0x7ffff7fbfdc5:	push   %r10
   0x7ffff7fbfdc7:	push   %r9


Not sure what's going on here, marking s-s.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20151022183103" and the hash "9a1e89df904b953006be78880dbe99ad262c5d6a".
The "bad" changeset has the timestamp "20151022190636" and the hash "6dc0a7e6f79178df535c0c55358cb0c51aa09c10".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a1e89df904b953006be78880dbe99ad262c5d6a&tochange=6dc0a7e6f79178df535c0c55358cb0c51aa09c10

Comment 2

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

Waldo, is bug 1215430 a likely regressor?

Comment 3

[Jeff Walden [:Waldo]]

I think I likely exposed an existing problem.  IonBuilder::jsop_regexp(RegExpObject* reobj) consults TI's OBJECT_FLAG_REGEXP_FLAGS_SET to determine whether cloning is required and notes this in MIR so codegen is accordingly affected.  *But*, it does so without adding a constraint on that value never changing.  If we later change it, as here, nothing promptly invalidates that previously-generated code.  Most likely bug 1215430's refactoring perturbed something just so, to make the assert happen -- but a differently-written testcase probably could fail for the same reason.

I'm currently somewhat trying to understand the problem in a debugger, but my understanding of spew isn't great, so I'm not sure how far I'll get on that tack.  At worst I could just add the constraint, observe that fixes things, and be "good enough".  But this is 1) intellectually unsatisfying and wouldn't aid my understanding of parts of SpiderMonkey internals I don't already grok, and 2) gives no insight into whether the failure merits a higher security rating.  Certainly "sec-high" here can't represent deep understanding of how we go off the rails here, and how controllable that process might be, since nobody investigated this already.

For now, still trying to understand the matter.  At worst I can call in reinforcements, but I hope it doesn't come to that.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch, *without* usable testcase

Comment 3 is a sack of lies written by a lying liar who lied and is a bad person who should feel bad.

The real problem is that IonBuilder::jsop_regexp adds a constraint on OBJECT_FLAG_REGEXP_FLAGS_SET.  We expect, for reasons not 100% clear to me, that the object created for a regexp literal will always have the same ObjectGroup.  Examine https://hg.mozilla.org/integration/mozilla-inbound/rev/bc949d6e3aaa carefully, and you'll see CloneRegExpObject *used* to create a regexp with the provided RegExp's group in all circumstances, but that patch in the modified-statics case made us instead do RegExpAlloc(cx) which did *not* share a group.  The JIT in paranoia-mode added extra logic checking that the type of the created regexp was as expected, but because we got a new regexp without sharing a group, things became Inconsistent, and so we asserted.  This fixes the problem by reverting back to always using the same group.

I would like to have a clearer test for this than comment 0.  My attempt is in this patch, but it doesn't work.  I'd *like* to have a test that fails even if the asserts aren't compiled in, but this doesn't cut it.  The apparent consequence of this bug, is that we'd create a RegExp with a different TypeSet from expected.  How to expose that difference?  Not sure.  Note that we're fighting against logic to avoid cloning RegExps at all, and if we look too funny at the /foo/ we'll perturb logic and possibly suppress an otherwise-bug.

Anyway.  Halp, I would like to have a better/clearer test for this than fuzzer garbage.  :-)

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Anyway.  Halp, I would like to have a better/clearer test for this than
> fuzzer garbage.  :-)

If we parse on the main thread, the regex literal will get the 'canonical' group for the regex Class + proto. In that case there's no problem, because CloneRegExpObject will use that same group.

The problem shows up when we parse off-thread: the off-thread parse compartment will create a *different* group for the regex literal. Then we merge the parse-compartment's groups with the main compartment and we end up with a regex literal that has a group != the canonical group for that Class + proto.

The JIT's 'paranoia-mode' type checks are there for a reason: these TI bugs are sec-crits. The test below crashes an opt build (it's a near-null crash, but it should be possible to modify it to do worse).

Hope that helps.

offThreadCompileScript(`
function foo(x, {}) {
    for (var i = 0; i < 100; i++) {
        re = /erwe/;
        if (x === 1)
            re.x = 1;
        else
            re.x = "a";
        assertEq(re.x.length, (x === 1) ? undefined : 1);
    }
}
foo(0, 0);
RegExp.multiline = 1;
foo(1, 0);
`);
runOffThreadScript();

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8724005 [details] [diff] [review]
Patch, *without* usable testcase

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

Nice simplification. Let's land without a testcase, given how easy it is to exploit this (comment 5).

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Test for eventual landing

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Patch for landing (does not include test)

Given the trickiness in understanding TI well enough to exploit this, I'm not super-worried about this being found in the field.  So I think it's probably best not to try landing this super-late in a cycle, even if the patch seems pretty clear and unlikely to cause any regressions.  A few weeks into the next cycle seems like a good time to land this, to me.  Here's the final patch for use when the time comes.

Comment 9

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 46+.
Can you also request sec approval? Thanks!

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8726490 [details] [diff] [review]
Patch for landing (does not include test)

Note that this change and bug 1253099 touch the same area of code, so these two changes must mildly coordinate landing.  Both are sec bugs we should fix, of similar-ish danger IMO, so coordination should be easy enough.

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

If you understand TI well...maybe easily?  I dunno.  The set of people capable of exploiting this is probably fairly low, but why take unnecessary risks.

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

No.  And tests are separated out to land a bit after the fact.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?

44 onward, judging by the TM of bug 1215430 that introduced this.  Spot-checking 45 says it's there for sure.

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

Should be easy to make.

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

Fairly low-risk.  I'd wait 2-3 weeks into the cycle to land in m-c, then percolate through branches over the subsequent few days.

Comment 12

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

sec-approval+ to checkin on March 22 (a few weeks into the cycle).

Comment 13

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e251884f69f

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Aurora patch

Approval Request Comment
[Feature/regressing bug #]: bug 1215430
[User impact if declined]: sec-critical
[Describe test coverage new/current, TreeHerder]: tryservering of trunk patch with the tests attached here, landed on trunk earlier today; straightforward backport
[Risks and why]: not too much, patch restores with relative simplicity prior code's behavior
[String/UUID change made/needed]: N/A

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Beta and esr45 patch

Approval Request Comment: Same comments as on the Aurora patch.

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 8733679 [details] [diff] [review]
Beta and esr45 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined:  sec-critical
Fix Landed on Version: trunk is 48 now, right?
Risk to taking this patch (and alternatives if risky): not too risky a patch, localized changes to a single function
String or UUID changes made by this patch:  N/A

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e251884f69f

Comment 18

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

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

Waldo, could you please nominate an esr38 patch as well? Ideally we want sec patches to land in all affected branches at the same time. Thanks!

Comment 20

[Jeff Walden [:Waldo]]

Bug 1215430 landed in Mozilla 44, if its TM is believable.  Certainly it landed *well* after ESR38.

Comment 21

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> Bug 1215430 landed in Mozilla 44, if its TM is believable.  Certainly it
> landed *well* after ESR38.

Got it. Thanks! Will approve for all the affected branches now.

Comment 22

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

Comment on attachment 8733678 [details] [diff] [review]
Aurora patch

Sec-crit, Aurora47+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/094bebe3dafd

Comment 24

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx47

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c29c84ce79b3

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/37d3b14b5e8b

Comment 27

[Jeff Walden [:Waldo]]

Five months later, landed the test:

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

Comment 28

[Carsten Book [:Tomcat]]

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