1376547 - Crash [@ JSObject::getClass] or Assertion failure: isObject(), at js/Value.h:642

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

class foo extends Array {}
var arrs = [];
arrs.push(new foo(1));
function g() {
    f(0);
}
function f(y) {
    return (undefined <= foo.call(g).f);
}
try {
    let buffer = new g(137);
} catch (e) {}
f()()



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000081e3ea in JSObject::getClass (this=<optimized out>) at js/src/jsobj.h:127
#0  0x000000000081e3ea in JSObject::getClass (this=<optimized out>) at js/src/jsobj.h:127
#1  JSObject::getOpsGetProperty (this=0x0) at js/src/jsobj.h:139
#2  js::GetProperty (vp=..., id=..., receiver=..., obj=..., cx=0x7ffff6924000) at js/src/vm/NativeObject.h:1534
#3  js::GetProperty (vp=..., name=<optimized out>, receiver=..., obj=..., cx=0x7ffff6924000) at js/src/jsobj.h:853
#4  js::GetProperty (vp=..., name=0x7ffff46269a0, receiver=..., obj=..., cx=0x7ffff6924000) at js/src/jsobj.h:869
#5  js::GetPrototypeFromConstructor (cx=0x7ffff6924000, newTarget=newTarget@entry=..., proto=...) at js/src/jsobj.cpp:999
#6  0x000000000081e4e5 in js::GetPrototypeFromCallableConstructor (cx=cx@entry=0x7ffff6924000, args=..., proto=..., proto@entry=...) at js/src/jsobj.cpp:1013
#7  0x00000000004d62db in ArrayConstructorImpl (isConstructor=true, args=..., cx=0x7ffff6924000) at js/src/jsarray.cpp:3516
#8  js::ArrayConstructor (cx=0x7ffff6924000, argc=0, vp=0x7fffffffc768) at js/src/jsarray.cpp:3559
#9  0x00001ffeec39ecbf in ?? ()
#10 0x00007fffffffc7f8 in ?? ()
#11 0x00007fffffffc740 in ?? ()
#12 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffc6d0	140737488340688
rcx	0x7fffffffc640	140737488340544
rdx	0x7fffffffc620	140737488340512
rsi	0x7fffffffc670	140737488340592
rdi	0x7ffff6924000	140737330167808
rbp	0x7fffffffc6c0	140737488340672
rsp	0x7fffffffc5f0	140737488340464
r8	0x7fffffffc600	140737488340480
r9	0x7ffff4681970	140737293850992
r10	0x0	0
r11	0x7ffff69161c8	140737330110920
r12	0x0	0
r13	0x7ffff691e270	140737330143856
r14	0x7ffff469ff80	140737293975424
r15	0x7ffff46a3380	140737293988736
rip	0x81e3ea <js::GetPrototypeFromConstructor(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JSObject*>)+170>
=> 0x81e3ea <js::GetPrototypeFromConstructor(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JSObject*>)+170>:	mov    (%rax),%rax
   0x81e3ed <js::GetPrototypeFromConstructor(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JSObject*>)+173>:	mov    (%rax),%rax


This looks like a null pointer crash, but I remember some of the getClass crashes being potentially exploitable due to type constraints being violated. Marking s-s until a JS developer confirmes this as ok.

Comment 1

[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/68c84d4736ca
user:        Ted Campbell
date:        Tue Jun 06 10:34:08 2017 -0400
summary:     Bug 1169746 - Support |super()| in Baseline. r=jandem

This iteration took 225.654 seconds to run.

Comment 2

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

Ted, is bug 1169746 a likely regressor?

Comment 3

[Ted Campbell [:tcampbell]]

Looks like me. I've got a debug build throwing an assert that I am looking in to.

It seems that our |new.target| value is undefined somehow.

Comment 4

[Ted Campbell [:tcampbell]]

The crash is due to a derived class constructor being run without |new|. By spec we should throw before we execute the function but instead some our various call ICs don't have this check. The result is the Derived constructor passes an |undefined| |new.target| value into base class constructor. We then cast this to an object and deference causing the crash.

The issue of invoking constructors when not allowed happens independently of Bug 1169746, but don't appear to cause crashes (just incorrect behavior). We probably want an assert that class constructors always receive a valid |new.target| so we can catch these regressions sooner.

All our call ICs need to be checked for this. I'll also look into what fallout there is in Ion.

Comment 5

[Ted Campbell [:tcampbell]]

Simplified test case. There are variants for a bunch of the different ICs.

> class Base { }
> class Derived extends Base { }
> 
> new Derived();
> function f() { try { Derived.call(null); } catch (e) { } }
> f();
> f();

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1376547-JITs-should-throw-if-ClassConstructor-us.patch

The underlying correctness issue is that we are invoking ClassConstructors in context where we must instead throw. This is a long standing issue. With the addition of |super()| support in Baseline, end up with |new.target| being |undefined| but used as an object in VM code resulting in a crash.

This patch includes a testcase and corrects the half dozen areas we did not handle this behavior. Some are handled with emitted masm checks and other check in the decision to attach an IC.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbba4e7e888c879c50738ca09aa4ce703b2c2c3b

This appears to a simple assertion/null-deref where an UndefinedValue is unpacked to a (nullptr) Object and then accessed. I don't believe uplifts are necessary.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8882081 [details] [diff] [review]
0001-Bug-1376547-JITs-should-throw-if-ClassConstructor-us.patch

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

Looks good. I was wondering about LCallGeneric but I see that one has this check already.

Comment 9

[Ted Campbell [:tcampbell]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e37016d4dbe

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5e37016d4dbe

Comment 11

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.