1190272 - Assertion failure: isInt32(), at dist/include/js/Value.h or Assertion failure: isString(), at dist/include/js/Value.h or Assertion failure: isObjectOrNull(), at dist/include/js/Value.h or Assertion failure: isNumber(), at dist/include/js/Value.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function t(f, inputs) {
    var x = [];
    for (var j = 0; j < inputs.length; ++j) {
        for (var k = 0; k < 33; ++k) {
            x.push(f());
        }
    }
}
function g() {}
t(g, [0]);
relazifyFunctions();
function h() {
    return 0;
}
t(h, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10,
      11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
      21, 22, 23, 24, 25, 26, 27, 28, 29, 30,
      31, 32]);
t(g, [0]);

asserts js debug shell on m-c changeset afa67b6957bb with --fuzzing-safe --no-threads --baseline-eager --unboxed-arrays at Assertion failure: isInt32(), at dist/include/js/Value.h

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r afa67b6957bb

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/57dce88fc620
user:        Brian Hackett
date:        Tue May 26 16:29:19 2015 -0600
summary:     Bug 1165392, Bug 1165463 - Various unboxed array fixes and optimizations, r=jandem.

This iteration took 216.055 seconds to run.

Brian, is bug 1165392 or bug 1165463 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x205411, 0x000000010039aa8c js-dbg-64-dm-nsprBuild-darwin-afa67b6957bb`js::SetUnboxedValueNoTypeChange(JSObject*, unsigned char*, JSValueType, JS::Value const&, bool) + 52 at Value.h:1212, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010039aa8c js-dbg-64-dm-nsprBuild-darwin-afa67b6957bb`js::SetUnboxedValueNoTypeChange(JSObject*, unsigned char*, JSValueType, JS::Value const&, bool) + 52 at Value.h:1212
    frame #1: 0x000000010039aa58 js-dbg-64-dm-nsprBuild-darwin-afa67b6957bb`js::SetUnboxedValueNoTypeChange(unboxedObject=<unavailable>, p=<unavailable>, type=<unavailable>, v=<unavailable>, preBarrier=<unavailable>) + 696 at UnboxedObject-inl.h:60
    frame #2: 0x00000001003c2594 js-dbg-64-dm-nsprBuild-darwin-afa67b6957bb`js::DenseElementResult SetOrExtendBoxedOrUnboxedDenseElementsFunctor::operator()<(JSValueType)1>() + 406 at UnboxedObject-inl.h:502
    frame #3: 0x00000001003c23fe js-dbg-64-dm-nsprBuild-darwin-afa67b6957bb`js::DenseElementResult SetOrExtendBoxedOrUnboxedDenseElementsFunctor::operator(this=<unavailable>)<(JSValueType)1>() + 110 at UnboxedObject.cpp:2034
    frame #4: 0x00000001003be599 js-dbg-64-dm-nsprBuild-darwin-afa67b6957bb`js::DenseElementResult js::CallBoxedOrUnboxedSpecialization<SetOrExtendBoxedOrUnboxedDenseElementsFunctor>(f=SetOrExtendBoxedOrUnboxedDenseElementsFunctor at 0x00007fff5fbfdc80, obj=<unavailable>) + 185 at UnboxedObject-inl.h:604
(lldb)

Comment 2

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

function t(f, inputs) {
    var x = [];
    for (var j = 0; j < inputs.length; ++j) {
        for (var k = 0; k < 33; ++k) {
            x.push(f());
        }
    }
}
function g() {}
t(g, [0]);
relazifyFunctions();
function h() {
    return '';
}
t(h, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10,
      11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
      21, 22, 23, 24, 25, 26, 27, 28, 29, 30,
      31, 32]);
t(g, [0]);

Assertion failure: isString(), at dist/include/js/Value.h

function t(f, inputs) {
    var x = [];
    for (var j = 0; j < inputs.length; ++j) {
        for (var k = 0; k < 33; ++k) {
            x.push(f());
        }
    }
}
function g() {}
t(g, [0]);
relazifyFunctions();
function h() {
    return [];
}
t(h, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10,
      11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
      21, 22, 23, 24, 25, 26, 27, 28, 29, 30,
      31, 32]);
t(g, [0]);

Assertion failure: isObjectOrNull(), at dist/include/js/Value.h

I used to have a variant that asserts at Assertion failure: isNumber(), at dist/include/js/Value.h

Setting [fuzzblocker] because of the many variations these testcases can produce.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

The basic problem here is that we can't rely on type information for an unboxed object's group to only describe values which can be stored to the object --- if some preliminary objects are created and then collected by the GC, they will not be considered when we construct an unboxed layout for the group, but the types of their properties will still be reflected in type information.  This could be fixed by checking or constraining the type information when making the unboxed layout, but this would be making an already bad design even worse.

It would be best if Ion did not assume that checking the type information for an unboxed object is a substitute for making sure the value being stored can actually fit in the unboxed property.  IonBuilder::storeUnboxedValue already takes care of this via type policies on the instructions it emits, but places where code generation calls MacroAssembler::storeUnboxedProperty directly and with a null failure label need to be fixed.  For unboxed plain objects this happens for SetProp ICs and SetPropertyPolymorphic.  Crashes in these ops are triggered (without --unboxed-arrays) with variations of the following testcase:

with({}){}

function Thing(a) {
    this.a = a;
}
function Other() {
}
new Thing(0);
var arr = [];
arr.push(new Other());
arr[0].a = 0;
arr[0].a = "";
arr.push(new Thing(""));
gc();
for (var i = 0; i < 30; i++)
    arr.push(new Thing(""));

for (var i = 0; i < 1005; i++) {
    var index = i % arr.length;
    var v = index ? "" : index;
    f(index, v);
}
f(1, 0);
print(arr[1].a);

function f(i, v) {
    arr[i].a = v;
}

Comment 5

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

For the testcase in comment 4:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c3714f520752
user:        Brian Hackett
date:        Mon Feb 02 09:27:59 2015 -0700
summary:     Bug 1127987 - Fix transposed parent/metadata arguments in EmptyShape::getInitialShape, r=jandem.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

Actually, I'm sorry but I attached an older version of the patch that addressed the Array.push issue but not the unboxed plain objects bugs.

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 8644415 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

not easily.

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?

aurora

If not all supported branches, which bug introduced the flaw?

bug 1162199

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

simple

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

not likely

Approval Request Comment
[Feature/regressing bug #]: bug 1162199
[User impact if declined]: exploitable crashes
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low

Comment 8

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

Comment on attachment 8644415 [details] [diff] [review]
patch

Approvals given.

Comment 9

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f95d4a32526

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f95d4a32526

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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