If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 54

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
7 months ago
5 months ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla55
x86_64
Linux
assertion, jsbugmon, regression, sec-moderate, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54+ verified, firefox55+ verified)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

7 months ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 1

7 months ago
Created attachment 8844837 [details] [diff] [review]
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8844837 - Flags: review?(arai.unmht)
(Assignee)

Updated

7 months ago
Blocks: 1344463
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
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?
Attachment #8844837 - Flags: review?(arai.unmht) → review+
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.
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();
Sounds potentially bad so I'll just mark it sec-high.
Keywords: sec-high
tracking-firefox54: ? → +
tracking-firefox55: ? → +

Updated

7 months ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 6

7 months ago
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.
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?
Flags: needinfo?(jdemooij)
(Assignee)

Comment 8

6 months ago
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 :)
Flags: needinfo?(jdemooij)
Keywords: sec-high → sec-moderate
(Assignee)

Comment 9

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f87b8596e04521ed99443159edfe5b8c446fc3
(Assignee)

Comment 10

6 months ago
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.
Attachment #8844837 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f7f87b8596e0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8844837 [details] [diff] [review]
Patch

Fix a security issue. Aurora54+.
Attachment #8844837 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 13

6 months ago
uplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e67736a44a5
status-firefox54: affected → fixed
Group: javascript-core-security → core-security-release
Group: core-security-release

Updated

5 months ago
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
status-firefox55: fixed → verified

Comment 14

5 months ago
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx54
You need to log in before you can comment on or make changes to this bug.