Closed Bug 1242810 Opened 5 years ago Closed 4 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + verified
firefox48 + verified
firefox-esr38 - unaffected
firefox-esr45 46+ fixed


(Reporter: decoder, Assigned: Waldo)



(5 keywords, Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.1+])


(4 files, 1 obsolete file)

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):

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


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.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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:
Waldo, is bug 1215430 a likely regressor?
Blocks: 1215430
Flags: needinfo?(jwalden+bmo)
Keywords: sec-high
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.
Flags: needinfo?(jwalden+bmo)
Attached patch Patch, *without* usable testcase (obsolete) — Splinter Review
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 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.  :-)
Attachment #8724005 - Flags: review?(jdemooij)
Assignee: nobody → jwalden+bmo
(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.

function foo(x, {}) {
    for (var i = 0; i < 100; i++) {
        re = /erwe/;
        if (x === 1)
            re.x = 1;
            re.x = "a";
        assertEq(re.x.length, (x === 1) ? undefined : 1);
foo(0, 0);
RegExp.multiline = 1;
foo(1, 0);
Keywords: sec-highsec-critical
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).
Attachment #8724005 - Flags: review?(jdemooij) → review+
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.
Attachment #8724005 - Attachment is obsolete: true
Attachment #8726490 - Flags: review+
[Tracking Requested - why for this release]:
Tracking for 46+.
Can you also request sec approval? Thanks!
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(jwalden+bmo)
Attachment #8726490 - Flags: sec-approval?
sec-approval+ to checkin on March 22 (a few weeks into the cycle).
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 3/22]
Attachment #8726490 - Flags: sec-approval? → sec-approval+
Whiteboard: [jsbugmon:update][checkin on 3/22] → [jsbugmon:update]
Attached patch Aurora patchSplinter Review
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
Attachment #8733678 - Flags: approval-mozilla-aurora?
Approval Request Comment: Same comments as on the Aurora patch.
Attachment #8733679 - Flags: approval-mozilla-beta?
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
Attachment #8733679 - Attachment description: Beta patch → Beta and esr45 patch
Attachment #8733679 - Flags: approval-mozilla-esr45?
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
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!
Flags: needinfo?(jwalden+bmo)
Bug 1215430 landed in Mozilla 44, if its TM is believable.  Certainly it landed *well* after ESR38.
Flags: needinfo?(jwalden+bmo)
(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 on attachment 8733678 [details] [diff] [review]
Aurora patch

Sec-crit, Aurora47+
Attachment #8733678 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8733679 - Flags: approval-mozilla-esr45?
Attachment #8733679 - Flags: approval-mozilla-esr45+
Attachment #8733679 - Flags: approval-mozilla-beta?
Attachment #8733679 - Flags: approval-mozilla-beta+
JSBugMon: This bug has been automatically verified fixed on Fx47
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main46+][adv-esr45.1+]
Five months later, landed the test:
Flags: in-testsuite? → in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.