1205707 - Assertion failure: this->is<T>(), at js/src/jsobj.h:553

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision e7d613b3bcfe (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --no-threads --ion-eager --ion-extra-checks):

function callCatch(f) {
    try {
        f();
    } catch (exc) {}
};
constructor = Int8Array;
var join = newGlobal()[constructor.name].prototype.join;
join.call(new constructor([1, 2, 3]), " ")
var invalidReceivers = [ constructor, new Proxy(new constructor(), {})];
invalidReceivers.forEach(invalidReceiver => {
  callCatch(() => { constructor.prototype.join.call(invalidReceiver); })
});



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x000000000045d922 in JSObject::as<js::SharedTypedArrayObject> (this=<optimized out>) at js/src/jsobj.h:553
#0  0x000000000045d922 in JSObject::as<js::SharedTypedArrayObject> (this=<optimized out>) at js/src/jsobj.h:553
#1  0x0000000000941d51 in as<js::SharedTypedArrayObject> (this=<optimized out>) at js/src/vm/SharedTypedArrayObject.h:112
#2  AnyTypedArrayViewData (obj=<optimized out>) at js/src/vm/TypedArrayCommon.h:113
#3  js::jit::IonBuilder::addTypedArrayLengthAndData (this=this@entry=0x7fffffff8f60, obj=0x7ffff69a5c00, checking=checking@entry=js::jit::IonBuilder::SkipBoundsCheck, index=index@entry=0x0, length=length@entry=0x7fffffff8a68, elements=elements@entry=0x0) at js/src/jit/IonBuilder.cpp:9087
#4  0x00000000009f9bfe in addTypedArrayLength (obj=<optimized out>, this=0x7fffffff8f60) at js/src/jit/IonBuilder.h:631
#5  js::jit::IonBuilder::inlineTypedArrayLength (this=0x7fffffff8f60, callInfo=...) at js/src/jit/MCallOptimize.cpp:2180
#6  0x000000000099ab5d in js::jit::IonBuilder::inlineSingleCall (this=0x7fffffff8f60, callInfo=..., targetArg=<optimized out>) at js/src/jit/IonBuilder.cpp:5439
#7  0x000000000099c47c in js::jit::IonBuilder::inlineCallsite (this=this@entry=0x7fffffff8f60, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:5503
#8  0x000000000099c80d in js::jit::IonBuilder::jsop_call (this=this@entry=0x7fffffff8f60, argc=1, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:6381
#9  0x00000000009953eb in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffff8f60, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:1839
#10 0x0000000000996540 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffff8f60) at js/src/jit/IonBuilder.cpp:1501
#11 0x000000000099a046 in js::jit::IonBuilder::buildInline (this=this@entry=0x7fffffff8f60, callerBuilder=callerBuilder@entry=0x7fffffff9c00, callerResumePoint=callerResumePoint@entry=0x7ffff69a5cd8, callInfo=...) at js/src/jit/IonBuilder.cpp:1071
#12 0x000000000099a575 in js::jit::IonBuilder::inlineScriptedCall (this=this@entry=0x7fffffff9c00, callInfo=..., target=target@entry=0x7ffff7e7c2e0) at js/src/jit/IonBuilder.cpp:4941
#13 0x000000000099d4ae in js::jit::IonBuilder::jsop_funcall (this=this@entry=0x7fffffff9c00, argc=0) at js/src/jit/IonBuilder.cpp:6204
#14 0x0000000000995a68 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffff9c00, op=op@entry=JSOP_FUNCALL) at js/src/jit/IonBuilder.cpp:1832
#15 0x0000000000996540 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffff9c00) at js/src/jit/IonBuilder.cpp:1501
#16 0x000000000099a046 in js::jit::IonBuilder::buildInline (this=this@entry=0x7fffffff9c00, callerBuilder=callerBuilder@entry=0x7ffff69a31a8, callerResumePoint=callerResumePoint@entry=0x7ffff69a4be0, callInfo=...) at js/src/jit/IonBuilder.cpp:1071
#17 0x000000000099a575 in js::jit::IonBuilder::inlineScriptedCall (this=this@entry=0x7ffff69a31a8, callInfo=..., target=<optimized out>) at js/src/jit/IonBuilder.cpp:4941
#18 0x000000000099ab40 in js::jit::IonBuilder::inlineSingleCall (this=0x7ffff69a31a8, callInfo=..., targetArg=<optimized out>) at js/src/jit/IonBuilder.cpp:5447
#19 0x000000000099c47c in js::jit::IonBuilder::inlineCallsite (this=this@entry=0x7ffff69a31a8, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:5503
#20 0x000000000099c80d in js::jit::IonBuilder::jsop_call (this=this@entry=0x7ffff69a31a8, argc=0, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:6381
#21 0x00000000009953eb in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff69a31a8, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:1839
#22 0x0000000000996540 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff69a31a8) at js/src/jit/IonBuilder.cpp:1501
#23 0x0000000000996985 in js::jit::IonBuilder::build (this=0x7ffff69a31a8) at js/src/jit/IonBuilder.cpp:900
#24 0x00000000009977b1 in js::jit::IonCompile (cx=cx@entry=0x7ffff6907000, script=script@entry=0x7ffff7e62570, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, constructing=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::Optimization_Normal) at js/src/jit/Ion.cpp:2171
#25 0x000000000099f3da in js::jit::Compile (cx=cx@entry=0x7ffff6907000, script=..., script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2402
#26 0x000000000099f5db in js::jit::CanEnter (cx=cx@entry=0x7ffff6907000, state=...) at js/src/jit/Ion.cpp:2561
#27 0x000000000069f9dd in js::RunScript (cx=cx@entry=0x7ffff6907000, state=...) at js/src/vm/Interpreter.cpp:680
#28 0x00000000006a01d5 in js::Invoke (cx=cx@entry=0x7ffff6907000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:781
#29 0x00000000006a1e1d in js::Invoke (cx=cx@entry=0x7ffff6907000, thisv=..., fval=..., argc=argc@entry=1, argv=argv@entry=0x7fffffffb428, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:818
#30 0x00000000008f1f5b in js::jit::DoCallFallback (cx=0x7ffff6907000, frame=0x7fffffffb468, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffb418, res=...) at js/src/jit/BaselineIC.cpp:8899
#31 0x00007ffff7feef9f in ?? ()
[...]
#53 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff69a5c00	140737330699264
rcx	0x7ffff6ca53b0	140737333842864
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffff89d0	140737488325072
rsp	0x7fffffff89d0	140737488325072
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffff8790	140737488324496
r11	0x7ffff6c27960	140737333328224
r12	0x7fffffff8f60	140737488326496
r13	0x0	0
r14	0x0	0
r15	0x0	0
rip	0x45d922 <JSObject::as<js::SharedTypedArrayObject>() const+28>
=> 0x45d922 <JSObject::as<js::SharedTypedArrayObject>() const+28>:	movl   $0x229,0x0
   0x45d92d <JSObject::as<js::SharedTypedArrayObject>() const+39>:	callq  0x4982c0 <abort()>


Marked s-s because this assertion is known to be associated with security problems.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/310b3af47e93
user:        Jan de Mooij
date:        Fri Mar 20 13:45:35 2015 +0100
summary:     Bug 1142669 part 2 - Lower the script inlining size limit if off-thread compilation is not available. r=h4writer

changeset:   https://hg.mozilla.org/mozilla-central/rev/157929ef51b3
user:        Jan de Mooij
date:        Fri Mar 20 13:45:36 2015 +0100
summary:     Bug 1142669 part 3 - Limit the total inlined bytecode size to avoid excessive inlining. r=h4writer

changeset:   https://hg.mozilla.org/mozilla-central/rev/f7299a88c59c
user:        Jan de Mooij
date:        Thu Mar 19 15:10:07 2015 +0100
summary:     Bug 1142669 part 4 - Fix some inlining issues and inline scripts with loops. r=h4writer

changeset:   https://hg.mozilla.org/mozilla-central/rev/4d744eeea51c
user:        Mike Shal
date:        Tue Mar 17 15:29:07 2015 -0400
summary:     Bug 1137000 - Enable SDK building on nightlies; r=glandium

changeset:   https://hg.mozilla.org/mozilla-central/rev/847f9159b3e9
user:        Jan de Mooij
date:        Fri Mar 20 14:14:06 2015 +0100
summary:     Bug 1142669 followup - Move OffThreadCompilationAvailable definition outside namespace block. r=red CLOSED TREE

This iteration took 102.424 seconds to run.

Comment 2

[Daniel Veditz [:dveditz]]

guessing sec-high on gut feel, no investigation. Please adjust

Comment 3

[Jan de Mooij [:jandem]]

Annoying bug. TypedArrayJoin does:

    if (!IsObject(this) || !IsTypedArray(this)) {
         ...
    }
    ...
    var len = TypedArrayLength(O);

Then IonBuilder::inlineTypedArrayLength does:

    // We assume that when calling this function we always
    // have a TypedArray. The native asserts that as well.

But that's invalid: if the input is not a typed array, the TypedArrayLength call is never *executed* (because IsTypedArray will throw),  but the compiler still sees a TypedArrayLength call with a non-TypedArray argument.

Simpler testcase:

(new Int8Array(3)).join(" ");
[Math.abs, {}].forEach(x => {
    try {
	(() => Int8Array.prototype.join.call(x))();
    } catch(e) {}
});

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Not sure if this is actually exploitable but let's assume it is.

Waldo, you did some work on TA intrinsics so I picked you, feel free to forward the review to someone else.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Add test

Test crashes without the patch with jit_test.py --tbpl.

Comment 6

[Jeff Walden [:Waldo]]

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

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

Ugh, this is subtle.

::: js/src/jit/MCallOptimize.cpp
@@ +2173,5 @@
> +    if (!types)
> +        return false;
> +
> +    if (types->forAllClasses(constraints, IsTypedArrayClass) !=
> +        TemporaryTypeSet::ForAllResult::ALL_TRUE)

Just

    return types->forAllClasses(...) == ALL_TRUE;

maybe?  (w/appropriate formatting, etc.)

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8664753 [details] [diff] [review]
Add test

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

Don't name the test with a meaningless name -- make it ion/inlining/inline-istypedarray-on-nontypedarray.js or something that hints at what's tested.

I could maybe go for a |var repeated = Math.abs| and then use |repeated| multiple times there, so it's most readably obvious the same value treated multiple times.

Comment 8

[Jeff Walden [:Waldo]]

Tentatively, it looks like AnyTypedArrayViewData extracts a void* from a PrivateValue-containing slot, and AnyTypedArrayLength extracts an int32_t from a fixed slot.  Doing those on a non-typed array will give you nonsense that might be inlined in the instruction stream -- but only in unreachable code.  The void* is used further in addTypedArrayLengthAndData, but only to check if it's in the nursery, which I don't believe has bad side effects.

So tentatively I think this isn't exploitable as well.  But I only skimmed quickly, and the consequences of being wrong seem worse than the potential value in opening up this bug.  So I'd say still keep this closed, but mostly out of an abundance of caution.  (And probably open up fairly shortly after this is fixed on branches, which I would still do because abundance of caution, and also because I looked at trunk code and not branch code when determining whether things went wrong "too much".)

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add test

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8666041 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
We don't see an obvious way to exploit this. Maybe with some effort someone finds something though and the fix is simple, so I just want to be overly cautious.

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

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

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

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

Comment 12

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

sec-approval+ for trunk. We'll want this on Aurora, Beta, and ESR38 after it is on trunk.

Comment 13

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19620ea2ebc8

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/19620ea2ebc8

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Jan, could you fill the uplift request to aurora, esr38 and beta? Thanks

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8666041 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1101258.
[User impact if declined]: Crashes, potential security bugs.
[Describe test coverage new/current, TreeHerder]: On m-c for a few days.
[Risks and why]: Low risk, adds an extra check before performing a certain optimization.
[String/UUID change made/needed]: None.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8666041 [details] [diff] [review]
Part 1 - Fix

Thanks, taking it as it fixes a sec high bug.

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5bc99bba04af

Comment 21

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx42
JSBugMon: This bug has been automatically verified fixed on Fx43

Comment 22

[Carsten Book [:Tomcat]]

Hi Jan, this does not apply to esr38 

grafting 304039:19620ea2ebc8 "Bug 1205707 part 1 - Clean up some is-TypedArrayObject code in Ion. r=Waldo"
merging js/src/jit/MCallOptimize.cpp
warning: conflicts during merge.
merging js/src/jit/MCallOptimize.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')

could you take a look ? Thanks!

Comment 23

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/f544a221989e

Comment 24

[Jan de Mooij [:jandem]]

Landed part 2, the test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/89eab1566e86

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89eab1566e86