1345160 - Assertion failure: maybeTempDouble != InvalidFloatReg, at js/src/jit/IonCacheIRCompiler.cpp:1367

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision b7e42143bbbc (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-eager --ion-offthread-compile=off):

function test(UTCDate, constructor2, from = [1, 2, 3, 4, 5], to = [2, 4]) {
    var modifiedConstructor = new constructor(from);
    modifiedConstructor.constructor = constructor2;
    modifiedConstructor.filter(x => x % 2 == 0);
}
test(Int8Array, Uint8Array);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x00000000006c60e8 in js::jit::IonCacheIRCompiler::emitStoreTypedElement (this=this@entry=0x7fffffffa510) at js/src/jit/IonCacheIRCompiler.cpp:1367
#0  0x00000000006c60e8 in js::jit::IonCacheIRCompiler::emitStoreTypedElement (this=this@entry=0x7fffffffa510) at js/src/jit/IonCacheIRCompiler.cpp:1367
#1  0x00000000006cc6a0 in js::jit::IonCacheIRCompiler::compile (this=this@entry=0x7fffffffa510) at js/src/jit/IonCacheIRCompiler.cpp:438
#2  0x00000000006d8599 in js::jit::IonIC::attachCacheIRStub (this=this@entry=0x7ffff69e72b0, cx=cx@entry=0x7ffff6948000, writer=..., kind=<optimized out>, ionScript=ionScript@entry=0x7ffff69e7000, typeCheckInfo=typeCheckInfo@entry=0x7fffffffb798) at js/src/jit/IonCacheIRCompiler.cpp:1787
#3  0x000000000072a18b in js::jit::IonSetPropertyIC::update (cx=0x7ffff6948000, outerScript=..., ic=0x7ffff69e72b0, obj=..., idVal=..., rhs=...) at js/src/jit/IonIC.cpp:232
#4  0x0000304ee37b4886 in ?? ()
[...]
#20 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x100	256
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffa2a0	140737488331424
rsp	0x7fffffffa1e0	140737488331232
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7fffffffa530	140737488332080
r13	0x7fffffffa510	140737488332048
r14	0x1	1
r15	0x5	5
rip	0x6c60e8 <js::jit::IonCacheIRCompiler::emitStoreTypedElement()+1336>
=> 0x6c60e8 <js::jit::IonCacheIRCompiler::emitStoreTypedElement()+1336>:	movl   $0x0,0x0
   0x6c60f3 <js::jit::IonCacheIRCompiler::emitStoreTypedElement()+1347>:	ud2    


I'm not exactly sure what the assertion here implies but it sounds like it could potentially be security related, so marking s-s until triaged.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Oops, since bug 1344463 we use the Ion SetProperty IC for JSOP_INITELEM (in addition to JSOP_SETELEM) and JSOP_INITELEM is emitted for _DefineDataProperty in self-hosted code.

So we just need to fix the check in LIRGenerator::visitSetPropertyCache to account for JSOP_INITELEM too.

Comment 2

[Tooru Fujisawa [:arai]]

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

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

r+ assuming the extra tempDouble for JSOP_INITHIDDENELEM is negligible.

::: js/src/jit-test/tests/ion/bug1345160.js
@@ +1,5 @@
> +function f() {
> +    var o = [1, 2, 3];
> +    o.constructor = Uint8Array;
> +    for (var i=0; i<10; i++)
> +        o.filter(x => true);

not related to this bug tho, es6draft and V8 throws TypeError for this operation. (SpiderMonkey and JSC don't)

es6draft says:
  uncaught exception: TypeError: cannot create property "0"

and V8 says:
  TypeError: Cannot redefine property: 0

haven't yet figured out where it throws tho...

::: js/src/jit/Lowering.cpp
@@ +3996,3 @@
>      LDefinition tempD = LDefinition::BogusTemp();
>      LDefinition tempF32 = LDefinition::BogusTemp();
> +    if (IsElemPC(ins->resumePoint()->pc())) {

MSetPropertyCache is also used by JSOP_INITHIDDENELEM that I think not necessary here, but IsElemPC matches it too.
is it negligible?

Comment 3

[Tooru Fujisawa [:arai]]

https://tc39.github.io/ecma262/#sec-array.prototype.filter
> 22.1.3.7 Array.prototype.filter ( callbackfn [ , thisArg ] )
> ...
> 5. Let A be ? ArraySpeciesCreate(O, 0).
> ...
> 8.
> ...
>   c.
> ...
>     iii.
>         1. Perform ? CreateDataPropertyOrThrow(A, ! ToString(to), kValue).

https://tc39.github.io/ecma262/#sec-createdatapropertyorthrow
> 7.3.6 CreateDataPropertyOrThrow ( O, P, V )
> ...
> 3. Let success be ? CreateDataProperty(O, P, V).
> 4. If success is false, throw a TypeError exception.
> ...

https://tc39.github.io/ecma262/#sec-createdataproperty
> 7.3.4 CreateDataProperty ( O, P, V )
> ...
> 4. Return ? O.[[DefineOwnProperty]](P, newDesc).

https://tc39.github.io/ecma262/#sec-integer-indexed-exotic-objects-defineownproperty-p-desc
> 9.4.5.3 [[DefineOwnProperty]] ( P, Desc )
> ...
> 3. If Type(P) is String, then
>   a. Let numericIndex be ! CanonicalNumericIndexString(P).
>   b. If numericIndex is not undefined, then
>     ...
>     iv. Let length be O.[[ArrayLength]].
>      v. If numericIndex ≥ length, return false.


[[DefineOwnProperty]] returns false for creating 0-th element, because `O.[[ArrayLength]]` is 0,
and CreateDataPropertyOrThrow throws TypeError at step 4.
So, the expected result of the testcase is throwing TypeError.

Similar thing can be observed by `Object.defineProperty(new Uint8Array(), 0, { value: 10 })`

anyway, I think it should be fixed separately than this bug.

Comment 4

[Tooru Fujisawa [:arai]]

it's here. (so, apparently known)

https://dxr.mozilla.org/mozilla-central/rev/58753259bfeb3b818eac7870871b0aae1f8de64a/js/src/vm/TypedArrayObject.cpp#2267-2271
>     // Steps iv-v.
>     // We (wrongly) ignore out of range defines with a value.
>     uint32_t length = obj->as<TypedArrayObject>().length();
>     if (index >= length)
>         return result.succeed();

Comment 5

[Andrew McCreight [:mccr8]]

Sounds potentially bad so I'll just mark it sec-high.

Comment 6

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c87ea81036b7
user:        Jan de Mooij
date:        Sat Mar 04 15:24:44 2017 +0100
summary:     Bug 1344463 - Optimize JSOP_INITELEM in Ion and emit it for 3-arguments _DefineDataProperty in self-hosted code. r=till,evilpie

This iteration took 266.344 seconds to run.

Comment 7

[Frederik Braun [:freddy]]

The r+ is 2 weeks old and it seems the remaining work is supposed to happen in a follow-up (according to comment 3).
Can you please get this landed, Jan?

Comment 8

[Jan de Mooij [:jandem]]

Sorry for the delay. I don't think this is a sec-high; changing to sec-moderate.

(In reply to Tooru Fujisawa [:arai] (almost away until April) from comment #2)
> MSetPropertyCache is also used by JSOP_INITHIDDENELEM that I think not
> necessary here, but IsElemPC matches it too.
> is it negligible?

Yes that's fine :)

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f87b8596e04521ed99443159edfe5b8c446fc3

Comment 10

[Jan de Mooij [:jandem]]

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

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1344463.
[User impact if declined]: Crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet. It does fix the testcase.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Very small and straight-forward patch.
[String changes made/needed]: None.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/f7f87b8596e0

Comment 12

[Gerry Chang [:gchang]]

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

Fix a security issue. Aurora54+.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e67736a44a5

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx54