Crash [@ js::CanReuseScriptForClone] or Assertion failure: hasValue(), at jit/RegisterSets.h:210

RESOLVED FIXED in Firefox 59

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: decoder, Assigned: bhackett)

Tracking

(Blocks 1 bug, 6 keywords)

Trunk
mozilla60
x86
Linux
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 wontfix, firefox59+ fixed, firefox60+ fixed)

Details

(Whiteboard: [jsbugmon:update,bisect][post-critsmash-triage][adv-main59+], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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

2 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
Flags: needinfo?(jdemooij)
Component: JavaScript Engine → JavaScript Engine: JIT
Priority: -- → P1
Keywords: sec-high
Naveed, is there someone who can work on this, assuming we want to fix it for 58? Thanks.
Flags: needinfo?(nihsanullah)
Flags: needinfo?(nihsanullah)
re-ping
Flags: needinfo?(nihsanullah)
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

a year ago
Posted patch patchSplinter Review
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 on attachment 8942958 [details] [diff] [review]
patch

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

Thanks!
Attachment #8942958 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 7

a year 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?
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.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][checkin on 2/6]
Attachment #8942958 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(bhackett1024)
Whiteboard: [jsbugmon:update,bisect][checkin on 2/6] → [jsbugmon:update,bisect]
(Assignee)

Comment 10

a year 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?
Attachment #8942958 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/5fc563433fcf
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][post-critsmash-triage]
Whiteboard: [jsbugmon:update,bisect][post-critsmash-triage] → [jsbugmon:update,bisect][post-critsmash-triage][adv-main59+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.