1395919 - Assertion failure: MIR instruction returned object with unexpected type, at js/src/jit/MacroAssembler.cpp:1715 or Assertion failure: Unexpected object type, at jit/MacroAssembler.cpp:1715

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision a3585c77e2b1 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

try {
    (function(global) {
        "use strict";
        var ObjectCreate = global.Object.create;
        var ObjectDefineProperty = global.Object.defineProperty;
        function ArrayPush(arr, val) {
            var desc = ObjectCreate(null);
            ObjectDefineProperty(arr, arr.length, desc);
        }
        var callStack = [];
        function enterFunc(funcName) {
            ArrayPush(callStack, funcName);
        }
        global.enterFunc = enterFunc;
        function TestCase(n, d, e, a) {
            this.name = n;
            ObjectDefineProperty(gTestcases, gTc++, {
                __proto__: null,
                value: this,
            });
        }
        global.TestCase = TestCase;
    })(this);
    var gTestcases = new Array();
    var gTc = gTestcases.length;
    test();
    function test()
    enterFunc('test');
    var g = newGlobal();
    var dbg = TestCase(g);
} catch (lfVare) {}
var SECTION = "15.8.1.8-1";
new TestCase(SECTION, eval("Math.SQRT2=0; Math.SQRT2"));
test();
new TestCase(SECTION, eval("Math.SQRT2=0; Math.SQRT2"));



Backtrace:

 received signal SIGTRAP, Trace/breakpoint trap.
0x00003b0ec2828d00 in ?? ()
#0  0x00003b0ec2828d00 in ?? ()
#1  0x00007ffff47007c0 in ?? ()
#2  0x0000000000000000 in ?? ()
rax	0x7ffff468dc10	140737293900816
rbx	0x7ffff6940118	140737330282776
rcx	0x7ffff4700760	140737294370656
rdx	0x7ffff4627640	140737293481536
rsi	0x7ffff46d7128	140737294201128
rdi	0x0	0
rbp	0x7fffffffc120	140737488339232
rsp	0x7fffffffbfa8	140737488338856
r8	0x0	0
r9	0x7ffff6a00b10	140737331071760
r10	0x7ffff42b3000	140737289859072
r11	0x7ffff6b9f750	140737332770640
r12	0x0	0
r13	0x7ffff42f4180	140737290125696
r14	0x4043	16451
r15	0x7ffff6924000	140737330167808
rip	0x3b0ec2828d00	64934578916608
=> 0x3b0ec2828d00:	push   %r10
   0x3b0ec2828d02:	push   %r9


Marking s-s because this assertion is known for potential security problems.

Comment 1

[Christian Holler (:decoder)]

This is also probably responsible for a few more asserts that popped up today as well:

Assertion failure: Unexpected object type, at jit/MacroAssembler.cpp:1715
Assertion failure: MIR instruction returned value with unexpected type, at jit/MacroAssembler.cpp:1715


Happening quite often, setting fuzzblocker.

Comment 2

[Fuzzing Team]

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/a14fc8f5babc
user:        André Bargull
date:        Mon Aug 28 14:19:34 2017 +0200
summary:     Bug 1303335: Move parts of Object.getOwnProperty and Object.defineProperty to self-hosted code. r=till

This iteration took 268.956 seconds to run.

Comment 3

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Crash [@ js::TypeSet::ObjectType]
Build version: mozilla-central revision a3585c77e2b1
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize
Runtime options: --fuzzing-safe --thread-count=2 --ion-offthread-compile=off --ion-eager

Testcase:

See attachment.

Backtrace:

 received signal SIGSEGV, Segmentation fault.
js::TypeSet::ObjectType (obj=<optimized out>) at js/src/vm/TypeInference-inl.h:157
#0  js::TypeSet::ObjectType (obj=<optimized out>) at js/src/vm/TypeInference-inl.h:157
#1  js::TypeSet::GetValueType (val=...) at js/src/vm/TypeInference-inl.h:182
#2  js::TypeScript::Monitor (cx=cx@entry=0x7ffff6924000, script=script@entry=0x7ffff528be50, pc=pc@entry=0x7ffff51e8736 "7\f\270", types=types@entry=0x7ffff697e440, rval=...) at js/src/vm/TypeInference-inl.h:605
#3  0x000000000071dc65 in js::jit::DoTypeMonitorFallback (cx=0x7ffff6924000, frame=0x7fffffffc848, stub=0x7ffff4fb70c8, value=..., res=...) at js/src/jit/SharedIC.cpp:2521
#4  0x0000006ed6bafd4c in ?? ()
[...]
#16 0x0000000000000000 in ?? ()
rax	0x7ffff5303280	140737306964608
rbx	0x7ffff4fb70c8	140737303507144
rcx	0x7ffff697e440	140737330537536
rdx	0x7ffff51e8736	140737305806646
rsi	0x7ffff528be50	140737306476112
rdi	0x2f2f2f2f2f2f2f2f	3399988123389603631
rbp	0x7ffff6924000	140737330167808
rsp	0x7fffffffc760	140737488340832
r8	0xfffe7ffff5303280	-422212646456704
r9	0x7ffff4feef80	140737303736192
r10	0x2	2
r11	0x7ffff4feef98	140737303736216
r12	0x7ffff528be50	140737306476112
r13	0x7fffffffc838	140737488341048
r14	0x7fffffffc848	140737488341064
r15	0x7ffff697e440	140737330537536
rip	0x5b5ecd <js::TypeScript::Monitor(JSContext*, JSScript*, unsigned char*, js::StackTypeSet*, JS::Value const&)+77>
=> 0x5b5ecd <js::TypeScript::Monitor(JSContext*, JSScript*, unsigned char*, js::StackTypeSet*, JS::Value const&)+77>:	testb  $0x2,0x18(%rdi)
   0x5b5ed1 <js::TypeScript::Monitor(JSContext*, JSScript*, unsigned char*, js::StackTypeSet*, JS::Value const&)+81>:	mov    %rdi,%r8

Comment 4

[Christian Holler (:decoder)]

Attachment: Testcase for comment 3

Comment 5

[Christian Holler (:decoder)]

Marking sec-critical based on use-after-free in comment 3.

Comment 6

[Christian Holler (:decoder)]

NI on André based on comment 2.

Comment 7

[André Bargull [:anba]]

The assertion failure is still present in this reduced test case which doesn't call Object.defineProperty, so it's unlikely that bug 1303335 is the culprit of the error.

---
function _DefineProperty() {}
function IsObject() {}
function ThrowTypeError() {}

function ObjectOrReflectDefineProperty(obj, propertyKey, attributes) {
    if (!IsObject(obj))
        ThrowTypeError(0);

    propertyKey = propertyKey;

    if (!IsObject(attributes))
        ThrowTypeError(0);

    var attrs = 0, hasValue = false;
    var value, getter = null, setter = null;

    if ("enumerable" in attributes)
        attrs |= attributes.enumerable ? 0x01 : 0x08;

    if ("configurable" in attributes)
        attrs |= attributes.configurable ? 0x02 : 0x10;

    if ("value" in attributes) {
        attrs |= 0x100;
        value = attributes.value;
        hasValue = true;
    }

    if ("writable" in attributes) {
        attrs |= 0x100;
        attrs |= attributes.writable ? 0x04 : 0x20;
    }

    if ("get" in attributes) {
        attrs |= 0x200;
        getter = attributes.get;
        if (!IsCallable(getter) && getter !== undefined)
            ThrowTypeError(0);
    }

    if ("set" in attributes) {
        attrs |= 0x200;
        setter = attributes.set;
        if (!IsCallable(setter) && setter !== undefined)
            ThrowTypeError(0);
    }

    _DefineProperty(obj, propertyKey, attrs, value, null, true);
}

function enterFunc() {
    var desc = Object.create(null);
    ObjectOrReflectDefineProperty([], 0, desc);
}

function TestCase() {
    "use strict";
    this.name = 0;
    ObjectOrReflectDefineProperty([], 0, {
        __proto__: null,
        value: this,
    });
}

enterFunc();
try {
    TestCase({});
} catch (e) {}
new TestCase();
enterFunc();
new TestCase();
---

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function f() {}
function g(x) {
    j = [];
    y = f(f())
    f();
    y = f(f())
    f();
    s = r = null;
    if ("" in x) {
        s = e ? 1 : 8("" in x);
    }
    t = x.o;
    if ("" in x) {
        s = 0;
    }
    v = x.value;
    e = true;
    s = t = x.w ? 4 : 0;
    if ("" in x) {
        a |= 2;
        g(e(s) && d);
        f(0);
    }
    if ("" in x) {
        s |= 0;
        s = f(!s(s) && s != d);
        f(0);
    }
    (function () {})(s, e, null, e);
}
function h() {
    "use strict";
    this.e = 0;
    g({
        __proto__: null,
        value: this
    })
}
try {
    h();
} catch (e) {}
new h();
function m() {
    g(Object.create(null));
}
m()
m()
new h();

asserts js debug shell on m-c rev 04b6be50a252 compiled with --enable-debug --enable-more-deterministic --without-intl-api with --fuzzing-safe --no-threads --ion-eager at:

Assertion failure: MIR instruction returned value with unexpected type, at jit/MacroAssembler.cpp:1715

This testcase has been slightly reduced from the one in comment 7 by Andre.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/b6315d186b4b
user:        Jan de Mooij
date:        Tue May 02 14:26:44 2017 +0200
summary:     Bug 1357680 part 3 - Don't mark the new group as having unknown properties when changing an object's proto. r=bhackett

Jan, is bug 1357680 a likely regressor?

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch

Took me quite a while to track this down.

The problem is that AddPropertyTypesAfterProtoChange uses MarkObjectGroupFlags to copy the dynamic flags from the old object. That doesn't work correctly when the old group has unknown properties. The fix is to call MarkObjectGroupUnknownProperties in that case. 

I also added an assert to ObjectGroup::setFlags to make sure it's not called to set OBJECT_FLAG_UNKNOWN_PROPERTIES.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch

Hm this one might be nicer - the previous patch would regress bug 1357680 I'm afraid.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 56/57 since this is sec-critical.

Comment 12

[Jan de Mooij [:jandem]]

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

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?P
It's possible but pretty difficult.

> 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?
55+.

> If not all supported branches, which bug introduced the flaw?
Bug 1357680.

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

> How likely is this patch to cause regressions; how much testing does it need?
Not very likely but a few days on Nightly would be good.

Comment 13

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

sec-approval+ for trunk.
We'll want a beta patch nominated as well to land after this gets onto mozilla-central.

Comment 14

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66023b60b9aaa6f3e15fda2e644727ae2f3ab3fe

Comment 15

[Jan de Mooij [:jandem]]

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

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1357680.
[User impact if declined]: Security bugs, crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Small/local change but pretty complicated code so there is some risk.
[String changes made/needed]: None.

Comment 16

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision b4c1ad9565ee).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66023b60b9aa

Comment 18

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Gerry Chang [:gchang]]

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

Fix a sec-critical and was verified. Beta56+.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/90cac1d73acc

Comment 21

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx56