Closed Bug 1607670 Opened 2 years ago Closed 2 years ago

Figure out what to do with CreateThis

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 --- wontfix
firefox72 - wontfix
firefox73 - wontfix
firefox74 + fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Keywords: sec-other, Whiteboard: [post-critsmash-triage][adv-main74-])

Attachments

(8 files)

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 before JSOP_NEW and JSOP_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 use JSOP_CREATETHIS with an op that's not JSOP_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...

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.)

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...

Priority: -- → P1

Current plan is to have calls with non-function new.target always go through the slow invoke path, for now. So basically:

  1. In IonBuilder always use MCreateThis if callee != new.target.
  2. In jit::CreateThis, if new.target is not a JSFunction, return a magic value similar to some other cases.
  3. In scripted call codegen check for new.target not being a function and take the slow path.

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.

Keywords: sec-other

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...

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
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.
Attached file Simple testcase

I crafted a new test-case that also trips the assert.

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()>

(In reply to Ted Campbell [:tcampbell] from comment #9)

Created attachment 9119929 [details]
Simple testcase

I crafted a new test-case that also trips the assert.

Hah, your test is nicer, you win ;)

We can call isDerivedClassConstructor() on the function instead.

Depends on D59502

The callers are very different and have different constraints, especially in
later patches in this stack.

Depends on D59503

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

Depends on D59507

Per tcampbell "[this bug] is internally identified, probably-not-exploitable, and complex to fix so it is not a good uplift target."

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.

Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main74-]
Duplicate of this bug: 1380953
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.