Closed Bug 1190272 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 --- fixed
firefox42 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: gkw, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [fuzzblocker][jsbugmon:update][post-critsmash-triage][b2g-adv-main2.5-])

Attachments

(2 files, 1 obsolete file)

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?
Flags: needinfo?(bhackett1024)
Attached file 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)
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.
Summary: Assertion failure: isInt32(), at dist/include/js/Value.h → 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
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
Group: javascript-core-security
Duplicate of this bug: 1190147
Attached patch patch (obsolete) — Splinter Review
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;
}
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8643913 - Flags: review?(jdemooij)
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.
Blocks: 1127987
No longer blocks: 1165392, 1165463
Attachment #8643913 - Flags: review?(jdemooij) → review+
Group: core-security
Keywords: sec-high
Attached patch patchSplinter Review
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.
Attachment #8643913 - Attachment is obsolete: true
Attachment #8644415 - Flags: review?(jdemooij)
Attachment #8644415 - Flags: review?(jdemooij) → review+
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
Attachment #8644415 - Flags: sec-approval?
Attachment #8644415 - Flags: approval-mozilla-aurora?
Comment on attachment 8644415 [details] [diff] [review]
patch

Approvals given.
Attachment #8644415 - Flags: sec-approval?
Attachment #8644415 - Flags: sec-approval+
Attachment #8644415 - Flags: approval-mozilla-aurora?
Attachment #8644415 - Flags: approval-mozilla-aurora+
Group: javascript-core-security
Group: core-security → core-security-release
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker][jsbugmon:update][post-critsmash-triage]
Group: core-security-release
Whiteboard: [fuzzblocker][jsbugmon:update][post-critsmash-triage] → [fuzzblocker][jsbugmon:update][post-critsmash-triage][b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.