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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

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.

Comment 3

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

Boris, can you take a look at this?

Comment 4

[Boris Zbarsky [: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...

Comment 5

[Jan de Mooij [:jandem]]

(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.

Comment 6

[Jan de Mooij [:jandem]]

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...

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix bug

This patch fixes mergeParseTaskCompartment to give generator function groups the correct builtin prototype.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[Jeff Walden [:Waldo]]

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.

Comment 11

[Andrew McCreight [:mccr8]]

This sounds bad.

Jandem, is this ready to land? Please put it up for sec-approval if so.

Comment 12

[Jan de Mooij [:jandem]]

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.

Comment 13

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

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60707ba97960

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60707ba97960

Please nominate this for Aurora/Beta/esr38 approval when you get a chance.

Comment 16

[Jan de Mooij [:jandem]]

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.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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

sec high, taking it everywhere it is necessary

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a8b96fe39d92

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/64de1bde2558

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/555c7d4a6575

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/504665629f49
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/504665629f49
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/9ef38fe25fc7

Comment 22

[Fuzzing Team]

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