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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Mike Taylor [:miketaylr]]

Naveed, is there someone who can work on this, assuming we want to fix it for 58? Thanks.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

re-ping

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8942958 [details] [diff] [review]
patch

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

Thanks!

Comment 7

[Brian Hackett [Laid off!]]

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.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 10

[Brian Hackett [Laid off!]]

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

Comment 11

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

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

Comment 12

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/367b18b6007b