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

VERIFIED FIXED in Firefox 41, Firefox OS v2.1S

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla43
x86_64
Linux
assertion, regression, sec-high, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox40 wontfix, firefox41+ verified, firefox42+ verified, firefox43+ verified, firefox-esr3841+ fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

(Whiteboard: [jsbugmon:update][post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
The following testcase crashes on mozilla-central revision eab21ec484bb (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-eager --ion-offthread-compile=off):

gczeal(1);
var lfGlobal = newGlobal();
lfGlobal.offThreadCompileScript("(function* p() {});");
lfGlobal.runOffThreadScript();



Backtrace:

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffff7fd321d in ?? ()
#0  0x00007ffff7fd321d in ?? ()
#1  0x00007ffff7e7c060 in ?? ()
#2  0x00007ffff7fe8bcd in ?? ()
#3  0x0000000000000204 in ?? ()
#4  0x00007ffff7e9e12a in ?? ()
#5  0x0000000000000000 in ?? ()
rax	0x7ffff4800000	140737295417344
rbx	0x7ffff69811c0	140737330549184
rcx	0x7ffff7e91610	140737352635920
rdx	0x7fffffffbf28	140737488338728
rsi	0x7ffff7e00000	140737352040448
rdi	0x7fffffffb7e0	140737488336864
rbp	0x7fffffffb8e0	140737488337120
rsp	0x7fffffffb878	140737488337016
r8	0x0	0
r9	0x7ffff4800000	140737295417344
r10	0xc7ce0c7ce0c7ce0d	-4049285284472828403
r11	0x7ffff6c27960	140737333328224
r12	0x8	8
r13	0x7fffffffbf28	140737488338728
r14	0x204	516
r15	0x7fffffffbee0	140737488338656
rip	0x7ffff7fd321d	140737353953821
=> 0x7ffff7fd321d:	push   %r10
   0x7ffff7fd321f:	push   %r9


Filing s-s because the test involves GC.
(Reporter)

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 1

3 years ago
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/796283553115
user:        Boris Zbarsky
date:        Wed Apr 01 12:05:29 2015 -0400
summary:     Bug 679939 part 5.  Stop using the compileAndGo script flag in the bytecode emitter.  r=luke

This iteration took 184.623 seconds to run.
Duplicate of this bug: 1183125
Boris, can you take a look at this?
Flags: needinfo?(bzbarsky)
Chances are this is a preexisting jit bug, but I'll take a look on Monday if no one else has gotten to it first...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 5

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #4)
> Chances are this is a preexisting jit bug, but I'll take a look on Monday if
> no one else has gotten to it first...

I think I know what's happening. Ion's MLambda for the generator function has a resultTypeSet (ObjectGroup) based on the function object created by the parser. This assumes the lambda cloning process will give the clone the same ObjectGroup.

NewFunctionClone does this for generators:

    if (!proto && fun->isStarGenerator()) {
        cloneProto = GlobalObject::getOrCreateStarGeneratorFunctionPrototype(cx, cx->global());
        ...

The clone's ObjectGroup is based on the Class + proto. So if the proto we get here does not match our original function's proto, we get a different ObjectGroup and we fail this check.

When we parse off-thread, as we're doing here, we'll create a new Zone and at the end we merge it back. This can probably lead to duplicate ObjectGroups: the original function and our clone will have different groups.

In other words, js::Lambda needs to assert fun->group() == clone->group(), since that's an invariant Ion depends on.

I'll see what's the easiest fix here.
(Assignee)

Comment 6

3 years ago
Hm, the prototype fixup in finishParseTask calls IdentifyStandardPrototype, but that doesn't handle the generator function case, since Generator.prototype is not a generator. I'll see if we can special case this...
(Assignee)

Updated

3 years ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 7

3 years ago
Created attachment 8639341 [details] [diff] [review]
Part 1 - Fix bug

This patch fixes mergeParseTaskCompartment to give generator function groups the correct builtin prototype.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8639341 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

3 years ago
Created attachment 8639344 [details] [diff] [review]
Part 2 - Add test, asserts

And this one has the test and adds some asserts to js::Lambda/js::LambdaArrow to catch this sooner.

We can land this one once part 1 is on all branches.
Attachment #8639344 - Flags: review?(jwalden+bmo)

Comment 9

3 years ago
Comment on attachment 8639344 [details] [diff] [review]
Part 2 - Add test, asserts

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

::: js/src/jit-test/tests/basic/bug1183153.js
@@ +1,1 @@
> +gczeal(1);

Name the test file something like offthread-generator-cloning.js or something other than a nondescript bug number.  Ideally indicating what it's testing, but also making it easier to refer to the test file later, not just a long number that's hard to remember/find in a file listing.
Attachment #8639344 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8639341 [details] [diff] [review]
Part 1 - Fix bug

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

Eeeeeugh the tactics being performed in this surrounding code are squirrely.
Attachment #8639341 - Flags: review?(jwalden+bmo) → review+
This sounds bad.

Jandem, is this ready to land? Please put it up for sec-approval if so.
Flags: needinfo?(jdemooij)
Keywords: sec-high
(Assignee)

Comment 12

3 years ago
Comment on attachment 8639341 [details] [diff] [review]
Part 1 - Fix bug

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily. Going from an off-thread parsing change to a TI exploit is not an obvious step.

> 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?
The bisection says 40+, but I doubt that's correct. We should land this on all branches.

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

> How likely is this patch to cause regressions; how much testing does it need?
Pretty unlikely. Also because generators are pretty rare on the web.
Attachment #8639341 - Flags: sec-approval?
I'm giving this sec-approval+ for checkin on 8/25 (two weeks into the cycle).

You should nominate branch patches back to ESR38 so we can go everywhere after trunk checkin.
status-firefox40: --- → wontfix
status-firefox41: --- → affected
status-firefox43: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox41: --- → +
tracking-firefox42: --- → +
tracking-firefox43: --- → +
tracking-firefox-esr38: --- → ?
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 8/25]
Attachment #8639341 - Flags: sec-approval? → sec-approval+
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → wontfix
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → affected
https://hg.mozilla.org/integration/mozilla-inbound/rev/60707ba97960
Flags: needinfo?(jdemooij) → in-testsuite?
Whiteboard: [jsbugmon:update][checkin on 8/25] → [jsbugmon:update]
https://hg.mozilla.org/mozilla-central/rev/60707ba97960

Please nominate this for Aurora/Beta/esr38 approval when you get a chance.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: affected → fixed
status-firefox43: affected → fixed
Flags: needinfo?(jdemooij)
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 16

3 years ago
Comment on attachment 8639341 [details] [diff] [review]
Part 1 - Fix bug

Approval Request Comment
[Feature/regressing bug #]: Older off-thread-parsing/TI bug.
[User impact if declined]: Security bugs.
[Describe test coverage new/current, TreeHerder]: On m-c for a while.
[Risks and why]: Low risk. Should only affect generators and those aren't used much on the web.
[String/UUID change made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8639341 - Flags: approval-mozilla-esr38?
Attachment #8639341 - Flags: approval-mozilla-beta?
Attachment #8639341 - Flags: approval-mozilla-aurora?
Comment on attachment 8639341 [details] [diff] [review]
Part 1 - Fix bug

sec high, taking it everywhere it is necessary
Attachment #8639341 - Flags: approval-mozilla-esr38?
Attachment #8639341 - Flags: approval-mozilla-esr38+
Attachment #8639341 - Flags: approval-mozilla-beta?
Attachment #8639341 - Flags: approval-mozilla-beta+
Attachment #8639341 - Flags: approval-mozilla-aurora?
Attachment #8639341 - Flags: approval-mozilla-aurora+

Updated

3 years ago
Group: core-security → core-security-release
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
tracking-firefox-esr38: ? → 41+

Updated

3 years ago
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
status-firefox42: fixed → verified
status-firefox43: fixed → verified

Comment 22

3 years ago
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx41
JSBugMon: This bug has been automatically verified fixed on Fx42
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main41+][adv-esr38.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.