Closed
Bug 1416523
Opened 3 years ago
Closed 3 years ago
Crash [@ js::CanReuseScriptForClone] or Assertion failure: hasValue(), at jit/RegisterSets.h:210
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: decoder, Assigned: bhackett1024)
Details
(6 keywords, Whiteboard: [jsbugmon:update,bisect][post-critsmash-triage][adv-main59+])
Crash Data
Attachments
(1 file)
1.53 KB,
patch
|
jandem
:
review+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 864174ac0707+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe): function f(flag) { o = arguments; } f(true); o.__proto__ = [3]; for (var j = 0; j < 5; { toString() { return null; } }) o[0]; Backtrace: received signal SIGSEGV, Segmentation fault. 0x0844702c in js::CanReuseScriptForClone (compartment=0xf5007c00, fun=..., newParent=...) at js/src/jsfun.cpp:2079 #0 0x0844702c in js::CanReuseScriptForClone (compartment=0xf5007c00, fun=..., newParent=...) at js/src/jsfun.cpp:2079 #1 0x0815a339 in js::CloneFunctionObjectIfNotSingleton (cx=0xf6e17800, fun=..., parent=..., proto=..., newKind=js::GenericObject) at js/src/jsfuninlines.h:88 #2 0x081463f4 in js::Lambda (cx=0xf6e17800, fun=..., parent=...) at js/src/vm/Interpreter.cpp:4392 #3 0x2bf7599b in ?? () #4 0x2bf6f8d8 in ?? () eax 0xffffc3e0 -15392 ebx 0x8aceff4 145551348 ecx 0xf536abe0 -180966432 edx 0xf537e078 -180887432 esi 0x1 1 edi 0xf5007c00 -184517632 ebp 0xffffc46c 4294952044 esp 0xffffc2e0 4294951648 eip 0x844702c <js::CanReuseScriptForClone(JSCompartment*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>)+76> => 0x844702c <js::CanReuseScriptForClone(JSCompartment*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>)+76>: mov (%esi),%eax 0x844702e <js::CanReuseScriptForClone(JSCompartment*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>)+78>: mov (%eax),%eax Seems to involve registers according to assertion, marking s-s.
Reporter | ||
Comment 1•3 years ago
|
||
Backtrace for the assertion: received signal SIGSEGV, Segmentation fault. 0x082cf746 in js::jit::TypedOrValueRegister::valueReg (this=0xffffae14) at js/src/jit/RegisterSets.h:210 #0 0x082cf746 in js::jit::TypedOrValueRegister::valueReg (this=0xffffae14) at js/src/jit/RegisterSets.h:210 #1 0x082c6e24 in js::jit::AutoOutputRegister::valueReg (this=0xffffae14) at js/src/jit/CacheIRCompiler.h:620 #2 js::jit::CacheIRCompiler::emitLoadArgumentsObjectArgResult (this=0xffffafb0) at js/src/jit/CacheIRCompiler.cpp:1874 #3 0x08323fd9 in js::jit::IonCacheIRCompiler::emitLoadArgumentsObjectArgResult (this=0xffffafb0) at js/src/jit/IonCacheIRCompiler.cpp:189 #4 js::jit::IonCacheIRCompiler::compile (this=0xffffafb0) at js/src/jit/IonCacheIRCompiler.cpp:565 #5 0x0832fc6b in js::jit::IonIC::attachCacheIRStub (this=0xf6e2f1dc, cx=0xf6e1b800, writer=..., kind=<incomplete type>, ionScript=0xf6e2f000, attached=0xffffbe83, typeCheckInfo=0x0) at js/src/jit/IonCacheIRCompiler.cpp:2428 #6 0x0837006d in js::jit::IonGetPropertyIC::update (cx=0xf6e1b800, outerScript=..., ic=0xf6e2f1dc, val=..., idVal=..., res=...) at js/src/jit/IonIC.cpp:146 #7 0x27ea9653 in ?? () #8 0x27ea1bf9 in ?? () eax 0x0 0 ebx 0x8db5ff4 148594676 ecx 0xf7da4864 -136689564 edx 0x0 0 esi 0xffffafc0 -20544 edi 0xffffae2c -20948 ebp 0xffffadc8 4294946248 esp 0xffffadc0 4294946240 eip 0x82cf746 <js::jit::TypedOrValueRegister::valueReg() const+70> => 0x82cf746 <js::jit::TypedOrValueRegister::valueReg() const+70>: movl $0x0,0x0 0x82cf750 <js::jit::TypedOrValueRegister::valueReg() const+80>: ud2
Updated•3 years ago
|
Flags: needinfo?(jdemooij)
Updated•3 years ago
|
Component: JavaScript Engine → JavaScript Engine: JIT
Priority: -- → P1
Comment 2•3 years ago
|
||
Naveed, is there someone who can work on this, assuming we want to fix it for 58? Thanks.
Flags: needinfo?(nihsanullah)
Updated•3 years ago
|
Flags: needinfo?(nihsanullah)
Comment 4•3 years ago
|
||
Brian can you look at this? I'm a bit swamped with other (security) bugs atm. The immediate problem is that CacheIR's LoadArgumentsObjectArgResult in Ion assumes the output is a Value register. That's easy to fix, but I think the underlying bug is in IonBuilder where TI incorrectly infers the type of this load.
Flags: needinfo?(nihsanullah)
Flags: needinfo?(jdemooij)
Flags: needinfo?(bhackett1024)
Assignee | ||
Comment 5•3 years ago
|
||
The issue here is similar to other ones we've had in the past with things like array.length and getter calls. TI doesn't account for property types other than normal data properties when deciding whether a type barrier is needed, and in this case the arguments object is being loaded from without making sure that a barrier will be used later in the load. This patch should fix arguments[n], and I also noticed that arguments.length (and other objects.length) may be emitted without making sure the result is allowed to be int32.
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8942958 -
Flags: review?(jdemooij)
Comment 6•3 years ago
|
||
Comment on attachment 8942958 [details] [diff] [review] patch Review of attachment 8942958 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8942958 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 7•3 years ago
|
||
Comment on attachment 8942958 [details] [diff] [review] patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? Probably Firefox 53+. If not all supported branches, which bug introduced the flaw? Probably bug 1322093. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? They should be simple. How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8942958 -
Flags: sec-approval?
Comment 8•3 years ago
|
||
This needs to be delayed until two weeks into the next cycle. So sec-approval+ for checkin on February 6. At that point, we'll want a Beta patch nominated as well.
status-firefox59:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox59:
--- → +
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][checkin on 2/6]
Updated•3 years ago
|
Attachment #8942958 -
Flags: sec-approval? → sec-approval+
Comment 9•3 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc563433fcfc2a08400988c247ccb23e45129d4 This grafts cleanly Beta as-landed. Please request approval on it when you get a chance.
status-firefox60:
--- → affected
tracking-firefox60:
--- → +
Flags: needinfo?(bhackett1024)
Whiteboard: [jsbugmon:update,bisect][checkin on 2/6] → [jsbugmon:update,bisect]
Assignee | ||
Comment 10•3 years ago
|
||
Comment on attachment 8942958 [details] [diff] [review] patch Approval Request Comment [Feature/Bug causing the regression]: bug 1322093 [User impact if declined]: security vulnerability [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: just landed on inbound [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: tweaks conditions under which an optimization is performed [String changes made/needed]: none
Flags: needinfo?(bhackett1024)
Attachment #8942958 -
Flags: approval-mozilla-beta?
Updated•3 years ago
|
Attachment #8942958 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/5fc563433fcf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
![]() |
||
Comment 12•3 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/367b18b6007b
Updated•3 years ago
|
Group: javascript-core-security → core-security-release
Updated•3 years ago
|
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][post-critsmash-triage]
Updated•3 years ago
|
Whiteboard: [jsbugmon:update,bisect][post-critsmash-triage] → [jsbugmon:update,bisect][post-critsmash-triage][adv-main59+]
Updated•3 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•