Closed Bug 1254122 Opened 4 years ago Closed 4 years ago

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


(Core :: JavaScript Engine, defect, critical)

Not set



Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + verified
firefox-esr38 --- unaffected
firefox-esr45 46+ fixed


(Reporter: decoder, Assigned: efaust)


(Blocks 2 open bugs)


(4 keywords, Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.1+])


(1 file)

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();


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.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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:
Group: core-security → javascript-core-security
ni? Eric due to possible regressor bug 1105463
Blocks: 1105463
Flags: needinfo?(efaustbmo)
Keywords: sec-high
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.
Attached patch FixSplinter Review
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.
Assignee: nobody → efaustbmo
Flags: needinfo?(efaustbmo)
Attachment #8733249 - Flags: review?(jdemooij)
Comment on attachment 8733249 [details] [diff] [review]

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.
Attachment #8733249 - Flags: review?(jdemooij) → review+
Comment on attachment 8733249 [details] [diff] [review]

[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?


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.
Attachment #8733249 - Flags: sec-approval?

Please nominate for branches as well.
Attachment #8733249 - Flags: sec-approval? → sec-approval+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 8733249 [details] [diff] [review]

[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: 

See for more info.
Attachment #8733249 - Flags: approval-mozilla-esr45?
Attachment #8733249 - Flags: approval-mozilla-beta?
Attachment #8733249 - Flags: approval-mozilla-aurora?
Comment on attachment 8733249 [details] [diff] [review]

Fix for (potential) crash, let's uplift to aurora and beta.
Attachment #8733249 - Flags: approval-mozilla-beta?
Attachment #8733249 - Flags: approval-mozilla-beta+
Attachment #8733249 - Flags: approval-mozilla-aurora?
Attachment #8733249 - Flags: approval-mozilla-aurora+
Group: javascript-core-security → core-security-release
Comment on attachment 8733249 [details] [diff] [review]

sec-high, taking it.
Should be in 45.1.0
Attachment #8733249 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main46+][adv-esr45.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.