1098961 - Crash [@ js::TypeDescrIsSimpleType] or Assertion failure: args[0].isObject(), at builtin/TypedObject.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Array.prototype[Symbol.iterator] = function() {
    for (var i = 3; --i >= 0;) {
        yield this[i]
    }
}
new TypedObject.ArrayType(TypedObject.int32, 0).build(x => 1)

asserts js debug shell on m-c changeset 7f0d92595432 with --no-ion --no-threads at Assertion failure: args[0].isObject(), at builtin/TypedObject.cpp.

Debug 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-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

This was found by combining random jit-tests together with jsfunfuzz, the specific files are:

http://hg.mozilla.org/mozilla-central/file/7f0d92595432/js/src/jit-test/tests/for-of/semantics-03.js
http://hg.mozilla.org/mozilla-central/file/7f0d92595432/js/src/jit-test/tests/ion/inlining/TypedObject-TypeDescrIsArrayType-unknown.js

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ceca39a1a154
user:        Brian Hackett
date:        Fri Nov 07 08:37:21 2014 -0700
summary:     Bug 1092318 - Remove unsized array typed objects, r=nmatsakis.

Brian, is bug 1092318 a possible regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x1b27b1, 0x00000001000c7c4c js-dbg-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::TypeDescrIsSimpleType((null)=<unavailable>, argc=<unavailable>, vp=<unavailable>) + 396 at TypedObject.cpp:2713, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001000c7c4c js-dbg-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::TypeDescrIsSimpleType((null)=<unavailable>, argc=<unavailable>, vp=<unavailable>) + 396 at TypedObject.cpp:2713
    frame #1: 0x0000000100631492 js-dbg-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [inlined] js::CallJSNative(native=0x00000001006df5b0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 166 at jscntxtinlines.h:231
    frame #2: 0x00000001006313ec js-dbg-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::Invoke(cx=0x0000000101c01b90, args=CallArgs at 0x00007fff5fbfcad0, construct=<unavailable>) + 428 at Interpreter.cpp:482
    frame #3: 0x000000010064e491 js-dbg-opt-64-dm-nsprBuild-darwin-7f0d92595432`Interpret(cx=<unavailable>, state=<unavailable>) + 43393 at Interpreter.cpp:2527
    frame #4: 0x0000000100643ae9 js-dbg-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::RunScript(cx=0x0000000101c01b90, state=0x00007fff5fbfd918) + 345 at Interpreter.cpp:432
(lldb)

Comment 2

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

This also causes a null de-ref in js opt shells compiled using:

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 --disable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Comment 3

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

Attachment: stack for opt crash

(lldb) bt 5
* thread #1: tid = 0x1b2b23, 0x0000000100095bbe js-dbgDisabled-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::TypeDescrIsSimpleType(js::ThreadSafeContext*, unsigned int, JS::Value*) [inlined] js::BarrieredBase<js::types::TypeObject*>::operator->(this=<unavailable>) const at Barrier.h:455, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: 0x0000000100095bbe js-dbgDisabled-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::TypeDescrIsSimpleType(js::ThreadSafeContext*, unsigned int, JS::Value*) [inlined] js::BarrieredBase<js::types::TypeObject*>::operator->(this=<unavailable>) const at Barrier.h:455
    frame #1: 0x0000000100095bbe js-dbgDisabled-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::TypeDescrIsSimpleType(js::ThreadSafeContext*, unsigned int, JS::Value*) [inlined] JSObject::getClass(this=0x0000000000000000) const at jsobj.h:138
    frame #2: 0x0000000100095bbe js-dbgDisabled-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::TypeDescrIsSimpleType(js::ThreadSafeContext*, unsigned int, JS::Value*) [inlined] js::ValueOperations<JS::MutableHandle<JS::Value> >::toObject(this=0x0000000000000000, i=<unavailable>) const + 4 at TypedObject.h:1054
    frame #3: 0x0000000100095bba js-dbgDisabled-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::TypeDescrIsSimpleType((null)=0x0000000101613c90, argc=1, vp=0x000000010187fa70) + 10 at TypedObject.cpp:2716
    frame #4: 0x0000000100456830 js-dbgDisabled-opt-64-dm-nsprBuild-darwin-7f0d92595432`js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [inlined] js::CallJSNative(native=0x00000001004bafa0, args=0x000000010187fa80)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 65 at jscntxtinlines.h:231
(lldb)

Comment 5

[Brian Hackett [Laid off!]]

This isn't a regression from bug 1092318.  The testcase below crashes in 527d5142b7c5, which is the parent revision to bug 1092318:

Array.prototype[Symbol.iterator] = function() {
    for (var i = 3; --i >= 0;) {
        yield this[i]
    }
}
new TypedObject.ArrayType(TypedObject.int32, 0).build(1, x => 1)

The problem here is that the typed object self hosting code uses destructuring internally, and when that destructuring code runs it accesses this iteration stuff whose behavior can be changed in arbitrary ways by the referring script.  I guess this could be fixed by either banning array-style destructuring in self hosted code, or modifying its semantics so it uses the builtin array iterator.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

After talking with Till we decided that banning operations which can invoke these iteration hooks is the best course of action here.  This patch adds an assert to that effect and fixes up the places in TypedObject.js where this bites.  Other self hosted code doesn't use array style destructuring.

Comment 7

[Jeff Walden [:Waldo]]

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

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

Nice find by the fuzzers.  (And reason 1 of N that I am not a huge fan of destructuring sugar.)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3426,5 @@
>  EmitIteratorNext(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn=nullptr)
>  {
> +    // Disallow use of .next() iteration in self hosted code, as this can
> +    // access user modifiable iteration hooks.
> +    MOZ_ASSERT(bce->emitterMode != BytecodeEmitter::SelfHosting);

Put the rationale in the assertion itself so that people who encounter this know immediately why this is prohibited without having to find the assertion in source code:

    MOZ_ASSERT(bce->emitterMode != BytecodeEmitter::SelfHosting,
               ".next() iteration is prohibited in self-hosted code because it "
               "could run user-modifiable iteration code");

Comment 8

[Niko Matsakis [:nmatsakis]]

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

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

I agree with Jeff's comment regarding having a message in the assertion.

Comment 9

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52f500342c88

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/52f500342c88