Closed Bug 1376547 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- verified

People

(Reporter: decoder, Assigned: tcampbell)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(1 file)

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.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
Ted, is bug 1169746 a likely regressor?
Blocks: 1169746
Flags: needinfo?(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.
Assignee: nobody → 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.
Flags: needinfo?(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();
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.
Attachment #8882081 - Flags: review?(jdemooij)
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.
Attachment #8882081 - Flags: review?(jdemooij) → review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: