Figure out what to do with CreateThis
Categories
(Core :: JavaScript Engine, task, P1)
Tracking
()
People
(Reporter: jandem, Assigned: jandem)
References
Details
(Keywords: sec-other, Whiteboard: [post-critsmash-triage][adv-main74-])
Attachments
(8 files)
6.71 KB,
text/plain
|
Details | |
261 bytes,
text/plain
|
Details | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
Fixing 0-day bug 1607443 made us discover some weirdness with MCreateThis
. It made sense before, but since ES6 classes it's now an effectful operation (see the GetPrototypeFromConstructor
call in CreateThisForFunction
) and we don't handle that well.
We have a number of options:
-
We add
JSOP_CREATETHIS
and use that right beforeJSOP_NEW
andJSOP_SUPERCALL
. A bit annoying is that we have to emit this after evaluating the arguments due to side-effects. -
We keep
JSOP_NEW
as-is, but disallow using it with an arbitrary new.target. The complicated cases where new.target != callee (Reflect.construct, super calls?) would then useJSOP_CREATETHIS
with an op that's notJSOP_NEW
. This results in two different paths which isn't great. -
More radical: move this-allocation into the callee with a
JSOP_CREATETHIS
in the prologue. This is nice because it matches the spec better and is consistent with native functions (where callee constructs |this|) so it means we can remove special cases for bound functions, derived class constructors, native functions. It could also improve performance of polymorphic constructor calls.
Downside is that it would add an extra guard to non-constructing calls (although Ion could probably avoid that), and would require rethinking |this| TI type checks...
Comment 1•3 years ago
|
||
The current approach isn't actually spec-compliant: bug 1380953, which shows we retrieve callee.prototype
too often. (IonBuilder::createThisScripted()
emits code which isn't idempotent, contrary to the comment in that method.)
Assignee | ||
Comment 2•3 years ago
|
||
You're right, that's one of the issues we need to fix.
For what it's worth, JSC does callee-side |this| allocation, but they have specialized bytecode for constructing vs non-constructing so that's nicer for them than it is for us...
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
|
||
Current plan is to have calls with non-function new.target always go through the slow invoke path, for now. So basically:
- In IonBuilder always use MCreateThis if callee != new.target.
- In jit::CreateThis, if new.target is not a JSFunction, return a magic value similar to some other cases.
- In scripted call codegen check for new.target not being a function and take the slow path.
Comment 4•3 years ago
|
||
Bug 1607443 comment 10 says "We've looked at the MCreateThis case and while it is weird, it is not a sec concern" so I'm going to mark this sec-other.
Assignee | ||
Comment 5•3 years ago
|
||
I'm working on this. It's a bit complicated because some (scripted) constructor functions don't have a builtin .prototype property (bound functions..), but this is work that should have happened when classes/newTarget were added...
Comment 6•3 years ago
|
||
This is an automated crash issue comment: Summary: Assertion failure: !cx->runtime()->jitRuntime()->disallowArbitraryCode, at js/src/vm/Interpreter.cpp:394 Build version: mozilla-central revision 9f55d547e196+ Build flags: --enable-gczeal --enable-optimize --enable-debug Runtime options: --fuzzing-safe --no-threads --ion-warmup-threshold=1 --baseline-warmup-threshold=1 --ion-full-warmup-threshold=1 --blinterp-warmup-threshold=1 --ion-extra-checks --disable-oom-functions Testcase: See attachment. Backtrace: received signal SIGSEGV, Segmentation fault. #0 js::RunScript (cx=0x7ffff5f27000, state=...) at js/src/vm/Interpreter.cpp:393 #1 0x0000555556bc6e91 in js::InternalCallOrConstruct (cx=0x7ffff5f27000, args=..., construct=js::NO_CONSTRUCT, reason=(unknown: -168772608)) at js/src/vm/Interpreter.cpp:583 #2 0x0000555556bc7b50 in js::Call (cx=0x7ffff6eea540 <_IO_2_1_stderr_>, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:628 #3 0x0000555556d728bb in js::ScriptedProxyHandler::get (this=<optimized out>, cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) atjs/src/proxy/ScriptedProxyHandler.cpp:1152 #4 0x0000555556d7c5da in js::Proxy::getInternal (cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:332 #5 0x0000555556d684c5 in js::Proxy::get (cx=0x7ffff5f27000, proxy=..., receiver_=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:340 #6 0x0000555556b34b3e in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:114 #7 js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=<optimized out>, vp=...) at js/src/vm/ObjectOperations-inl.h:124 #8 0x0000555556b349d5 in js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=0x387e69d25e20, vp=...) at js/src/vm/ObjectOperations-inl.h:138 #9 0x0000555556d6c003 in GetProxyTrap (cx=0x7ffff5f27000, handler=..., name=..., func=...) at js/src/proxy/ScriptedProxyHandler.cpp:184 #10 0x0000555556d725db in js::ScriptedProxyHandler::get (this=<optimized out>, cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1128 #11 0x0000555556d7c5da in js::Proxy::getInternal (cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:332 #12 0x0000555556d684c5 in js::Proxy::get (cx=0x7ffff5f27000, proxy=..., receiver_=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:340 #13 0x0000555556b34b3e in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:114 #14 js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=<optimized out>, vp=...) at js/src/vm/ObjectOperations-inl.h:124 #15 0x0000555556bcbd72 in js::GetProperty (cx=0x7ffff5f27000, v=..., name=..., vp=...) at js/src/vm/Interpreter.cpp:4453 #16 0x000000c1ad70f00b in ?? () [...] #31 0x0000000000000000 in ?? () rax 0x555555a019b7 93824997136823 rbx 0x4 4 rcx 0x55555838c380 93825040696192 rdx 0x0 0 rsi 0x7ffff6eeb770 140737336227696 rdi 0x7ffff6eea540 140737336223040 rbp 0x7fffffff8440 140737488323648 rsp 0x7fffffff83f0 140737488323568 r8 0x7ffff6eeb770 140737336227696 r9 0x7ffff7fe6c80 140737354034304 r10 0x58 88 r11 0x7ffff6b927a0 140737332717472 r12 0x7ffff5f27000 140737319694336 r13 0x7fffffff85f0 140737488324080 r14 0x7fffffff8490 140737488323728 r15 0xfffdffffffffffff -562949953421313 rip 0x555556bb1c07 <js::RunScript(JSContext*, js::RunState&)+919> => 0x555556bb1c07 <js::RunScript(JSContext*, js::RunState&)+919>: movl $0x18a,0x0 0x555556bb1c12 <js::RunScript(JSContext*, js::RunState&)+930>: callq 0x555556b48be0 <abort()> Note: This testcase is only partly reduced and still contains fuzzing logic (including code rewriting). I am working on a reduced version, but filing this for analysis in the meantime.
Comment 7•3 years ago
|
||
[Tracking Requested - why for this release]:
Comment 9•3 years ago
•
|
||
I crafted a new test-case that also trips the assert.
Comment 10•3 years ago
|
||
This is an automated crash issue comment: Summary: Assertion failure: !cx->runtime()->jitRuntime()->disallowArbitraryCode, at js/src/vm/Interpreter.cpp:394 Build version: mozilla-central revision 9f55d547e196+ Build flags: --enable-gczeal --enable-optimize --enable-debug Runtime options: --fuzzing-safe --no-threads --ion-warmup-threshold=1 --baseline-warmup-threshold=1 --ion-full-warmup-threshold=1 --blinterp-warmup-threshold=1 --ion-extra-checks --disable-oom-functions Testcase: function await () {} function LoggingProxyHandlerWrapper(name, handler={}) { return new Proxy(handler, { get(t6, id) { return function (...args) { return Reflect[id].apply(null, args); }; } }); } function LoggingProxy(name, target) { return new Proxy(await, new LoggingProxyHandlerWrapper(name)); } let proto_proxy = new LoggingProxy("proto", new Date); niche = proto_proxy; new niche; new niche; new niche; new niche; new niche; new niche; Backtrace: received signal SIGSEGV, Segmentation fault. js::RunScript (cx=0x7ffff5f27000, state=...) at js/src/vm/Interpreter.cpp:393 #0 js::RunScript (cx=0x7ffff5f27000, state=...) at js/src/vm/Interpreter.cpp:393 #1 0x0000555556bc6e91 in js::InternalCallOrConstruct (cx=0x7ffff5f27000, args=..., construct=js::NO_CONSTRUCT, reason=(unknown: -168772608)) at js/src/vm/Interpreter.cpp:583 #2 0x0000555556bc7b50 in js::Call (cx=0x7ffff6eea540 <_IO_2_1_stderr_>, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:628 #3 0x0000555556d728bb in js::ScriptedProxyHandler::get (this=<optimized out>, cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1152 #4 0x0000555556d7c5da in js::Proxy::getInternal (cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:332 #5 0x0000555556d684c5 in js::Proxy::get (cx=0x7ffff5f27000, proxy=..., receiver_=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:340 #6 0x0000555556b34b3e in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:114 #7 js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=<optimized out>, vp=...) at js/src/vm/ObjectOperations-inl.h:124 #8 0x0000555556b349d5 in js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=0x57a83425e20, vp=...) at js/src/vm/ObjectOperations-inl.h:138 #9 0x0000555556d6c003 in GetProxyTrap (cx=0x7ffff5f27000, handler=..., name=..., func=...) at js/src/proxy/ScriptedProxyHandler.cpp:184 #10 0x0000555556d725db in js::ScriptedProxyHandler::get (this=<optimized out>, cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1128 #11 0x0000555556d7c5da in js::Proxy::getInternal (cx=0x7ffff5f27000, proxy=..., receiver=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:332 #12 0x0000555556d684c5 in js::Proxy::get (cx=0x7ffff5f27000, proxy=..., receiver_=..., id=..., vp=...) at js/src/proxy/Proxy.cpp:340 #13 0x0000555556b34b3e in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:114 #14 js::GetProperty (cx=0x7ffff5f27000, obj=..., receiver=..., name=<optimized out>, vp=...) at js/src/vm/ObjectOperations-inl.h:124 #15 0x0000555556bcbd72 in js::GetProperty (cx=0x7ffff5f27000, v=..., name=..., vp=...) at js/src/vm/Interpreter.cpp:4453 #16 0x00003ac8eb19e00b in ?? () #17 0x0000000000000000 in ?? () rax 0x555555a019b7 93824997136823 rbx 0x4 4 rcx 0x55555838c380 93825040696192 rdx 0x0 0 rsi 0x7ffff6eeb770 140737336227696 rdi 0x7ffff6eea540 140737336223040 rbp 0x7fffffffa750 140737488332624 rsp 0x7fffffffa700 140737488332544 r8 0x7ffff6eeb770 140737336227696 r9 0x7ffff7fe6c80 140737354034304 r10 0x58 88 r11 0x7ffff6b927a0 140737332717472 r12 0x7ffff5f27000 140737319694336 r13 0x7fffffffa900 140737488333056 r14 0x7fffffffa7a0 140737488332704 r15 0xfffdffffffffffff -562949953421313 rip 0x555556bb1c07 <js::RunScript(JSContext*, js::RunState&)+919> => 0x555556bb1c07 <js::RunScript(JSContext*, js::RunState&)+919>: movl $0x18a,0x0 0x555556bb1c12 <js::RunScript(JSContext*, js::RunState&)+930>: callq 0x555556b48be0 <abort()>
Comment 11•3 years ago
|
||
(In reply to Ted Campbell [:tcampbell] from comment #9)
Created attachment 9119929 [details]
Simple testcaseI crafted a new test-case that also trips the assert.
Hah, your test is nicer, you win ;)
Assignee | ||
Comment 12•3 years ago
|
||
Assignee | ||
Comment 13•3 years ago
|
||
We can call isDerivedClassConstructor() on the function instead.
Depends on D59502
Assignee | ||
Comment 14•3 years ago
|
||
The callers are very different and have different constraints, especially in
later patches in this stack.
Depends on D59503
Assignee | ||
Comment 15•3 years ago
|
||
Depends on D59505
Assignee | ||
Comment 16•3 years ago
|
||
In particular, we can't do effectful .prototype lookups in CreateThis and
elsewhere before the call instruction. This patch returns NullValue from
CreateThisFromIon in that case and changes LCallGeneric to take the slow
invoke path when it sees this value.
Depends on D59506
Assignee | ||
Comment 17•3 years ago
|
||
Depends on D59507
Comment 18•3 years ago
|
||
Per tcampbell "[this bug] is internally identified, probably-not-exploitable, and complex to fix so it is not a good uplift target."
Comment 19•3 years ago
|
||
Given comment 18, I don't think we want to rush this into Fx73 either. I'm going to leave ESR68 set to affected for now in case we want to look at uplifting it there next cycle (i.e. for 68.6esr shipping alongside Fx74), but I'd be OK with calling it wontfix too if others agree.
Assignee | ||
Comment 20•3 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/596958878df0b51e5e672cd6c274509d5d249dd2
https://hg.mozilla.org/integration/autoland/rev/878930cde05bd79f5dcbd03b3b80e3cac3442206
https://hg.mozilla.org/integration/autoland/rev/e062ea07194dbd8ef065d150ff5ecf01f8d84636
https://hg.mozilla.org/integration/autoland/rev/b2162495abd782b2fa3637935dbcccd7e0eb1561
https://hg.mozilla.org/integration/autoland/rev/c20655f4a4c12382c1fb71f332fc52ff78ea935b
https://hg.mozilla.org/integration/autoland/rev/a1d01d3c0bafc51dc745cc7f66f2b595aa4b4934
![]() |
||
Comment 21•3 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/596958878df0
https://hg.mozilla.org/mozilla-central/rev/878930cde05b
https://hg.mozilla.org/mozilla-central/rev/e062ea07194d
https://hg.mozilla.org/mozilla-central/rev/b2162495abd7
https://hg.mozilla.org/mozilla-central/rev/c20655f4a4c1
https://hg.mozilla.org/mozilla-central/rev/a1d01d3c0baf
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•2 years ago
|
Description
•