1254122 - Assertion failure: returnAddr > method_->raw(), at js/src/jit/BaselineJIT.cpp:731 with OOM and TypedObject

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision b6acf4d4fc20 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

for (lfLocal in this) {}
function foo() {
var T = TypedObject;
var ObjectStruct = new T.StructType({
    f: T.Object
});
var o = new ObjectStruct();
function writeObject(o, v, expected) {
    o.f = v;
}
for (var i = 0; i < 5; i++)
    writeObject(o, {}, "helo");
for (var i = 0; i < 5; i++)
    writeObject(o, null, "null");
writeObject(o, "three", "three");
for (var i = 0; oomAfterAllocations(1); i++)
    writeObject(o, 4.5, "4.5");
writeObject(o, undefined, "");
for (var i = 0;
    (class get {}.get++) < 5; i++)
    writeString(s, 4.5, "4.5");var lfcode = new Array();
} foo();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000005f94cc in js::jit::BaselineScript::icEntryFromReturnAddress (this=<optimized out>, returnAddr=<optimized out>) at js/src/jit/BaselineJIT.cpp:731
#0  0x00000000005f94cc in js::jit::BaselineScript::icEntryFromReturnAddress (this=<optimized out>, returnAddr=<optimized out>) at js/src/jit/BaselineJIT.cpp:731
#1  0x00000000006bc0fd in js::jit::JitFrameIterator::baselineScriptAndPc (this=this@entry=0x7fffffffae50, scriptRes=scriptRes@entry=0x0, pcRes=pcRes@entry=0x7fffffffad90) at js/src/jit/JitFrames.cpp:238
#2  0x00000000006e329d in js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:834
#3  0x00007ffff7fe8608 in ?? ()
#4  0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffae50	140737488334416
rcx	0x7ffff6ca588d	140737333844109
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffaca0	140737488333984
rsp	0x7fffffffaca0	140737488333984
r8	0x7ffff7fdf7c0	140737354004416
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffaa60	140737488333408
r11	0x7ffff6c27ee0	140737333329632
r12	0x7ffff7ea1090	140737352700048
r13	0x7fffffffad90	140737488334224
r14	0x0	0
r15	0x7fffffffae50	140737488334416
rip	0x5f94cc <js::jit::BaselineScript::icEntryFromReturnAddress(unsigned char*)+60>
=> 0x5f94cc <js::jit::BaselineScript::icEntryFromReturnAddress(unsigned char*)+60>:	movl   $0x2db,0x0
   0x5f94d7 <js::jit::BaselineScript::icEntryFromReturnAddress(unsigned char*)+71>:	callq  0x4a6f30 <abort()>


Marking this s-s because the assertion doesn't look safe to me. If it depends on TypedObject, it might be Nightly-only.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20151009120049" and the hash "2c91f257f53d9c2d1e0df1578ccc0fcf9c740bf3".
The "bad" changeset has the timestamp "20151009120613" and the hash "250cd0bf3ce07627f08057cc7a38bc2d67174f9f".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2c91f257f53d9c2d1e0df1578ccc0fcf9c740bf3&tochange=250cd0bf3ce07627f08057cc7a38bc2d67174f9f

Comment 2

[Daniel Veditz [:dveditz]]

ni? Eric due to possible regressor bug 1105463

Comment 3

[Eric Faust [:efaust]]

bug 1105463 is almost certainly not a regressor, here. It just made |class get{}.get++| not a syntax error. Jan has graciously agreed to help me look into this anyway.

Comment 4

[Eric Faust [:efaust]]

Attachment: Fix

OK, I've tracked this down.

We incorrectly set up the stack for callTypeUpdateIC() in several SetProp stubs including TypedObject and Unboxed, which leads to a bogus value in ICTailCallReg, consulted in HandleException after an oom-related failure in DoTypeUpdateFallback.

Jan, do you think we should land tests with this? It seems like it's reasonably easy to get this to happen, and if it does, we'll get the wrong stub there. It seems like the repercussions could be quite bad.

It also seems like this should go to Beta, since ICSetProp_Unboxed is older and not Nightly-only.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8733249 [details] [diff] [review]
Fix

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

Looks great. I'd land the test a few weeks after this has made it into a stable release.

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8733249 [details] [diff] [review]
Fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Since we're not gonna land the test immediately, you'd have to be already familiar, and then get pretty lucky.

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?

41+. This is headed for Aurora47 Beta46 and esr45. 

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This should apply everywhere.

How likely is this patch to cause regressions; how much testing does it need?

quite unlikely. Tested locally, and we can let it sit on central for a day or two before uplifting.

Comment 7

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

sec-approval+.

Please nominate for branches as well.

Comment 8

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a5e93a96d8

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0a5e93a96d8

Comment 10

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 11

[Eric Faust [:efaust]]

Comment on attachment 8733249 [details] [diff] [review]
Fix

[Approval Request Comment]
User impact if declined: 
Potential possibly exploitable crash with OOM. Incorrect JITCode.

Fix Landed on Version:
Landed cleanly with no visible regressions on 48.

Risk to taking this patch (and alternatives if risky):
 
Changing jitcode always introduces the possibiliy of regressions, but this one is quite small, safe, and intuitive. The failure is quite localized, so I am quite confident this will not provide new risk.

String or UUID changes made by this patch: 
None.


See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8733249 [details] [diff] [review]
Fix

Fix for (potential) crash, let's uplift to aurora and beta.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5629a22932f7

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8111b50b52e3

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8733249 [details] [diff] [review]
Fix

sec-high, taking it.
Should be in 45.1.0

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/845dd3bf121f