1379686 - Assertion failure: IsConstructor(args.CallArgs::newTarget()) (provided new.target value must be a constructor), at js/src/vm/Interpreter.cpp:552

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision a708f80289b2 (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-offthread-compile=off --ion-eager):

var g = newGlobal();
var w = g.eval("() => {}");
var v = g.eval("Array");
try {
    Reflect.construct(v, Reflect, w);
} catch (e) {
    Reflect.construct(v, Reflect, w);
}


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000053ae86 in InternalConstruct (cx=cx@entry=0x7ffff6952000, args=...) at js/src/vm/Interpreter.cpp:551
#0  0x000000000053ae86 in InternalConstruct (cx=cx@entry=0x7ffff6952000, args=...) at js/src/vm/Interpreter.cpp:551
#1  0x000000000053b432 in js::Construct (cx=cx@entry=0x7ffff6952000, fval=fval@entry=..., args=..., newTarget=..., objp=..., objp@entry=...) at js/src/vm/Interpreter.cpp:612
#2  0x0000000000a7eb2d in js::Wrapper::construct (this=this@entry=0x1ef2710 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7ffff6952000, proxy=..., proxy@entry=..., args=...) at js/src/proxy/Wrapper.cpp:188
#3  0x0000000000a6bb44 in js::CrossCompartmentWrapper::construct (this=0x1ef2710 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff6952000, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:379
#4  0x0000000000a765aa in js::Proxy::construct (cx=cx@entry=0x7ffff6952000, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:501
#5  0x0000000000a7669c in js::proxy_Construct (cx=0x7ffff6952000, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:750
#6  0x00000000005445eb in js::CallJSNative (cx=cx@entry=0x7ffff6952000, native=native@entry=0xa76620 <js::proxy_Construct(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293
#7  0x00000000005447f0 in js::CallJSNativeConstructor (cx=cx@entry=0x7ffff6952000, native=0xa76620 <js::proxy_Construct(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:326
#8  0x000000000053af2f in InternalConstruct (cx=cx@entry=0x7ffff6952000, args=...) at js/src/vm/Interpreter.cpp:573
#9  0x000000000053b2cb in js::ConstructFromStack (cx=cx@entry=0x7ffff6952000, args=...) at js/src/vm/Interpreter.cpp:599
#10 0x000000000061680f in js::jit::DoCallFallback (cx=0x7ffff6952000, frame=0x7fffffffcc28, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffcbc0, res=...) at js/src/jit/BaselineIC.cpp:2530
#11 0x000009934b4fb9db in ?? ()
[...]
#22 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffc320	140737488339744
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffc210	140737488339472
rsp	0x7fffffffc1b0	140737488339376
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff6952000	140737330356224
r13	0x1ed0000	32309248
r14	0x7fffffffc32d	140737488339757
r15	0x7fffffffc380	140737488339840
rip	0x53ae86 <InternalConstruct(JSContext*, js::AnyConstructArgs const&)+278>
=> 0x53ae86 <InternalConstruct(JSContext*, js::AnyConstructArgs const&)+278>:	movl   $0x0,0x0
   0x53ae91 <InternalConstruct(JSContext*, js::AnyConstructArgs const&)+289>:	ud2

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Hey Till, any progress update here?

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/e32dc83928e5
user:        Till Schneidereit
date:        Fri May 12 15:48:41 2017 +0200
summary:     Bug 1355016 - Properly check for constructability of wrapped objects in intrinsic_IsConstructor. r=bz,bholley

Till, is bug 1355016 a likely regressor?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Till, re-ping?

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

// jsfunfuzz-generated
try {
    for (let i = 0; i < 1;
        // Adapted from randomly chosen test: js/src/jit-test/tests/self-hosting/is-constructor-on-wrapper.js
        (eval("\
            try {\
                Reflect.construct((function(){}),\
                                   [],\
                                   (newGlobal()).eval(\"()=>{}\"));\
                a;\
            } catch (e) {\
                assertEq(e.constructor, TypeError);\
            }\
        "))) {
        print(i);
    }
} catch (e) {}

$ ./js-64-dm-linux-d53ba311ca2f --fuzzing-safe --no-threads --ion-eager testcase.js
0
0
0
0
0
0
0
0

$ ./js-64-dm-linux-d53ba311ca2f --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
/snip

Tested on m-c rev d53ba311ca2f, compiled with --enable-more-deterministic --without-intl-api

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Naveed, is there someone else who can look into this?

Comment 6

[Naveed Ihsanullah [:naveed]]

Jeff please evaluate and reassign as needed

Comment 7

[Jeff Walden [:Waldo]]

Bug 1355016 changed IsConstructor the intrinsic, i.e. intrinsic_IsConstructor, to not just be a direct call to JSObject::isConstructor.  But it didn't change how IsConstructor the intrinsic gets inlined -- which remains as a call to JSObject::isConstructor.

So what happens is the try-caught Reflect.construct throws because it's passed a newTarget argument that's a wrapped arrow function, that intrinsic_IsConstructor says isn't a constructor.

But the subsequent catch-block Reflect.construct gets Ion-eager'd into inlining the intrinsic_IsConstructor as a call to JSObject::isConstructor.  That function says (because of this little gem

(rr) lis
303	ForwardingProxyHandler::isConstructor(JSObject* obj) const
304	{
305	    // For now, all wrappers are constructable if they are callable. We will want to eventually
306	    // decouple this behavior, but none of the Wrapper infrastructure is currently prepared for
307	    // that.
308	    return isCallable(obj);
309	}

) that the wrapped arrow function *is* a constructor.  So Reflect.construct code continues to execute, and a non-constructor is passed as NewTarget for the construction operation.  Because the construction operation on the wrapper forwards to a construction operation on the wrappee -- trusting that because the wrapper is a constructor, the wrappee is as well, which is entirely proper -- we hit an assertion that the unwrapped NewTarget is a constructor.

We've put off a proper fix for this general issue for years (just look at the revision history for the lines quoted above).  Unless we just bite the bullet and do it right, we're going to keep having weird behaviors like this manifest.  We really need to make the isConstructor operation on proxies into a properly trappable/proxyable operation, identical in spirit to how IsArray and Proxy::isArray are implemented.  And likely add a security operation for construction, distinct from calling, unless security policy goo has been made sane (i.e. parallel to ECMA spec traps) in the last five months.  This is gonna be such fun!

I guess I'll take this.

Comment 8

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision d71e8e0053d8).

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Patch

I think ForwardingProxyHandler::isConstructor, nowadays, should be doing the exact same sort of thing ForwardingProxyHandler::isCallable does: consult the target.  If we do that, the patch for bug 1355016 can be reverted, we fix this testcase, and everything is green with this patch (which contains fixes for the 10 failures) on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edf09ad722a0dbe2e0cbc9a6ca15d6113bc5e77c

The test fixes in this patch are places where we were buggy before, so this is a nice bonus beyond fixing a fuzzbug.

So: why is this fix, which incidentally aligns IsConstructor and intrinsic_IsConstructor again (which un-aligning was questioned in bug 1355016 comment 1), not the right one?  I'm not seeing any immediately obvious things like conflating callable/constructor as we once did.

I'm happy to throw this at more people, just not sure who given the obvious candidate apparently has a kid (woo!  more change in the last five months).

Comment 10

[Till Schneidereit [:till]]

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

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

I think the reason I didn't do this was that I didn't have the time to think through the implications and nobody could tell me off the bat, either. I'm very happy to see it just works!

(Also, sorry for the delay, busy times.)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26b1ee21f365
Make ForwardingProxyHandler::isConstructor query and return whether the target is a constructor, rather than pretending is-constructor is identical to is-callable.  r=till

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/26b1ee21f365