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

RESOLVED FIXED in mozilla36

Status

()

--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gkw, Assigned: bhackett)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla36
x86_64
Mac OS X
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 unaffected, firefox34 unaffected, firefox35 unaffected, firefox36 affected, firefox-esr31 unaffected)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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?
Flags: needinfo?(bhackett1024)
(Reporter)

Comment 1

4 years ago
Created attachment 8522722 [details]
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)
(Reporter)

Comment 2

4 years ago
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
Crash Signature: [@ js::TypeDescrIsSimpleType]
Keywords: crash
Summary: Assertion failure: args[0].isObject(), at builtin/TypedObject.cpp → Crash [@ js::TypeDescrIsSimpleType] or Assertion failure: args[0].isObject(), at builtin/TypedObject.cpp
(Reporter)

Updated

4 years ago
status-firefox33: --- → unaffected
status-firefox34: --- → unaffected
status-firefox35: --- → unaffected
status-firefox-esr31: --- → unaffected
(Reporter)

Comment 3

4 years ago
Created attachment 8522723 [details]
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)
(Reporter)

Updated

4 years ago
Blocks: 1100132
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1098963
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 6

4 years ago
Created attachment 8524062 [details] [diff] [review]
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.
Assignee: nobody → bhackett1024
Attachment #8524062 - Flags: review?(nmatsakis)

Comment 7

4 years ago
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 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.
Attachment #8524062 - Flags: review?(nmatsakis) → review+
https://hg.mozilla.org/mozilla-central/rev/52f500342c88
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.