Closed Bug 953373 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 + fixed
firefox28 + fixed
firefox29 + fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: dbaron, Assigned: jandem)

References

()

Details

(Keywords: assertion, crash, sec-critical, Whiteboard: [qa-][adv-main27+])

Attachments

(3 files)

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
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.
Flags: needinfo?(jdemooij)
How would I map an MIR op back to the filename+lineno?
Let's hide this bug, at least until we know what's going on.
Group: core-security
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?
Forgot to set needinfo.
Flags: needinfo?(bhackett1024)
Shell testcase:

offThreadCompileScript('var s = "abc"; for (var i=0; i<2000; i++) { res = /a*c/.test(s) }; res;');
assertEq(runOffThreadScript(), true);
Attached patch PatchSplinter Review
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.)
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8353948 - Flags: review?(bhackett1024)
Flags: needinfo?(jdemooij)
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.
Attachment #8353948 - Flags: review?(bhackett1024) → review+
Flags: needinfo?(bhackett1024)
Jan, how did you find the relevant bit of script?
(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.
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 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.
Attachment #8353948 - Flags: sec-approval?
Comment on attachment 8353948 [details] [diff] [review]
Patch

sec-approval+ for trunk. We should get Aurora and Beta branches made and nominated too.
Attachment #8353948 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/0764495bc9b8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Attached patch Patch for AuroraSplinter Review
[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.
Attachment #8355520 - Flags: review+
Attachment #8355520 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jdemooij)
Attached patch Patch for BetaSplinter Review
See comment 17 for the approval request comment.
Attachment #8355521 - Flags: review+
Attachment #8355521 - Flags: approval-mozilla-beta?
Attachment #8355520 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8355521 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
blocking-b2g: --- → koi?
blocking-b2g: koi? → koi+
Duplicate of this bug: 956034
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.
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main27+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.