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)
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)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
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();
Assignee | ||
Comment 6•7 years ago
|
||
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)
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Comment 8•7 years ago
|
||
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+
Assignee | ||
Comment 9•7 years ago
|
||
Comment 10•7 years ago
|
||
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 11•7 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•