953373 - Assertion failure: MIR instruction returned object with unexpected type, at js/src/jit/IonMacroAssembler.cpp:1223, loading Amazon product page

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Loading http://www.amazon.com/gp/product/0415368197/ref=as_li_tf_tl?ie=UTF8&camp=1789&cr
eative=9325&creativeASIN=0415368197&linkCode=as2&tag=matthygles-20 reliably yields a crash:

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

I can't get gdb to give me a useful stack, nor does this assertion trigger the usual handler that prints a stack trace (which is odd).

I'm using a local debug build built with my local changes on top of https://hg.mozilla.org/mozilla-central/rev/451f47a70238

Comment 2

[Boris Zbarsky [:bzbarsky]]

There's no stack because this is happening as a call directly from jitcode.  It's emitted by the masm.assumeUnreachable() in CodeGenerator::emitObjectOrStringResultChecks.

If I replace that string by mir->opName(), then I get "RegExp" as the assertion text, so presumably this is a MRegExp.

Comment 3

[Boris Zbarsky [:bzbarsky]]

How would I map an MIR op back to the filename+lineno?

Comment 4

[Jan de Mooij [:jandem]]

Let's hide this bug, at least until we know what's going on.

Comment 5

[Jan de Mooij [:jandem]]

We have a function like this:

    isFunction : function(fn) {
        return !!fn && typeof fn!="string" && !fn.nodeName && fn.constructor!=Array && /^[\s[]?function/.test(fn+"");
    }

The RegExpObject stored in the JSScript has TypeObject T1. IonMonkey assumes MRegExp/CloneRegExpObject will return an object with the same TypeObject, but it actually has a different TypeObject, hence the failure. T2. T1.clasp_ == T2.clasp_ and T1.proto_ == T2.proto_ though.

I added the following assert to CloneRegExpObject and it passes jit-tests but fails when I load the website:

    JS_ASSERT(res->type() == obj_->type());

With parallel parsing disabled, it also seems to work fine, so it looks like we can have multiple TypeObjects for a single clasp + proto with parallel parsing. Brian, does this seem reasonable? Can we just use NewObjectWithType instead of NewObjectWithGivenProto in RegExpObjectBuilder::getOrCreateClone?

Comment 6

[Jan de Mooij [:jandem]]

Forgot to set needinfo.

Comment 7

[Jan de Mooij [:jandem]]

Shell testcase:

offThreadCompileScript('var s = "abc"; for (var i=0; i<2000; i++) { res = /a*c/.test(s) }; res;');
assertEq(runOffThreadScript(), true);

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

NewObjectWithType seems to work.

(I also noticed that passing the RegExp proto to CloneRegExpObject is unnecessary; will file a followup bug to clean this up.)

Comment 9

[Brian Hackett [Laid off!]]

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

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

Yeah, with parallel parsing there is initially a different global and set of prototype objects, which would mean a different type object for the regexps.  When finishing the parse the off thread regexp type object's proto is repointed to the target compartment's regexp proto.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Jan, how did you find the relevant bit of script?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Vacation Dec 19 to Jan 1 from comment #10)
> Jan, how did you find the relevant bit of script?

I added some code like this (untested, but you should get the idea):

if (mir->isRegExp()) {
    static unsigned count = 0;
    JSScript *script = mir->block()->info().script();
    printf("%u - %s:%u\n", count, script->filename(), script->lineno());
    masm.breakpoint();
    masm.move32(Imm32(count), temp);
    count++;
}

Then when you hit the breakpoint, you disassemble the code that follows it to find the "count" value. Then you can use the stdout output to map this value back to the script filename/lineno.

Comment 12

[Jan de Mooij [:jandem]]

This is probably sec-critical. We could optimize a property access based on the TypeObject we expect, then get a RegExpObject with a different TypeObject and different property types.

Comment 13

[Jan de Mooij [:jandem]]

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

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily; it requires off-thread parsing and that's not clear from the patch. You'd have to be pretty familiar with the engine to realize what's going on.

> 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?
Firefox 26+ (since it requires parallel parsing, bug 906371).

> If not all supported branches, which bug introduced the flaw?
It's a problem since bug 906371.

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

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

Comment 14

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

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

sec-approval+ for trunk. We should get Aurora and Beta branches made and nominated too.

Comment 15

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0764495bc9b8

Green Linux64 jit-tests/jstests: https://tbpl.mozilla.org/?tree=Try&rev=c58a558bbdea

I'll see if this applies to Aurora/Beta and if not post patches for them.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0764495bc9b8

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 906371
User impact if declined: Security bugs, crashes.
Testing completed (on m-c, etc.): On m-c since yesterday.
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None.

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Patch for Beta

See comment 17 for the approval request comment.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b38b8e812aa7
https://hg.mozilla.org/releases/mozilla-beta/rev/2debd43d9b9e
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a7176f1aca55

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

I'm not able to see a crash with affected builds around the date of reporting. I imagine also that the URL we use to cause the crash could have easily changed.

Unless we get a static test case or more info, I'm going to assume that QA can't do much to verify. Please update the bug if that assumption is incorrect. Thanks.