Closed Bug 1212305 Opened 4 years ago Closed 4 years ago

Assertion failure: calleeScript->hasBaselineScript(), at js/src/jit/Ion.cpp:619


(Core :: JavaScript Engine, defect, critical)

Not set



Tracking Status
firefox41 --- wontfix
firefox42 --- wontfix
firefox43 --- fixed
firefox44 --- fixed
firefox-esr38 --- wontfix
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- wontfix
b2g-v2.2r --- wontfix
b2g-master --- fixed


(Reporter: decoder, Assigned: h4writer)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [jsbugmon:ignore][b2g-adv-main2.5+][adv-main43+][post-critsmash-triage])


(1 file)

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

(No proper testcase available, filing on behalf of h4writer).


Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00000000009b2c0a in js::jit::LazyLink (cx=cx@entry=0x7fce97806c00, calleeScript=..., calleeScript@entry=...) at js/src/jit/Ion.cpp:619
#1  0x00000000009b46f1 in js::jit::CanEnter (cx=cx@entry=0x7fce97806c00, state=...) at js/src/jit/Ion.cpp:2573
#2  0x0000000000705f1d in js::RunScript (cx=cx@entry=0x7fce97806c00, state=...) at js/src/vm/Interpreter.cpp:685
#3  0x000000000070674f in js::Invoke (cx=cx@entry=0x7fce97806c00, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:786
#4  0x00000000007072bd in js::Invoke (cx=cx@entry=0x7fce97806c00, thisv=..., fval=..., argc=argc@entry=1, argv=argv@entry=0x7fff0cdc7770, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:823
#5  0x00000000008d687b in js::jit::DoCallFallback (cx=0x7fce97806c00, frame=0x7fff0cdc77c8, stub_=<optimized out>, argc=<optimized out>, vp=0x7fff0cdc7760, res=...) at js/src/jit/BaselineIC.cpp:8905
#6  0x00007fce98d87f9f in ?? ()
#33 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fce97806c00	140525281766400
rcx	0x7fce97bd688d	140525285763213
rdx	0x0	0
rsi	0x7fce97eab9d0	140525288733136
rdi	0x7fce97eaa1c0	140525288726976
rbp	0x7fff0cdc6f80	140733409161088
rsp	0x7fff0cdc6d90	140733409160592
r8	0x7fce98f1b780	140525305968512
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fce97ea7be0	140525288717280
r11	0x0	0
r12	0x7fce95920258	140525249364568
r13	0x7fff0cdc6ed0	140733409160912
r14	0x7fff0cdc6e20	140733409160736
r15	0x7fff0cdc6e38	140733409160760
rip	0x9b2c0a <js::jit::LazyLink(JSContext*, JS::Handle<JSScript*>)+1514>
=> 0x9b2c0a <js::jit::LazyLink(JSContext*, JS::Handle<JSScript*>)+1514>:	movl   $0x26b,0x0
   0x9b2c15 <js::jit::LazyLink(JSContext*, JS::Handle<JSScript*>)+1525>:	callq  0x4979a0 <abort()>
Attached patch PatchSplinter Review
Could repro using testcase: which isn't reduced.
Rate of segmentation fault on this testcase has reduced from: 1 out of 4 to 0 out of 25.

Forgot to mark baselinescript as active if it is on the stack. As a result the script was getting gc'ed, while we are expecting that we atleast still have the baselinescript after lazy linking.
Assignee: nobody → hv1989
Attachment #8670754 - Flags: review?(jdemooij)
FF35: Bug 1047346: first enabled lazy linking. But only in extreme cases. Very hard to have it (only 0.01% uses it). Even harder to trigger this (which on its own is very hard) 

FF43: Bug 1178834: enabled lazy linking for everything. Still hard to reproduce, though.

Because of this I'm gonna land this on FF43+ only.
Comment on attachment 8670754 [details] [diff] [review]

Review of attachment 8670754 [details] [diff] [review]:

Good find.
Attachment #8670754 - Flags: review?(jdemooij) → review+
Comment on attachment 8670754 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I think it is quite hard to construct an exploit, even knowing the issue. Even harder when not understanding the issue and only looking at the patch. It depends on a lot of timings going right. Also the patch doesn't make it obvious the debugger hooks need to be enabled, before it is possible to exploit this.

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?
FF35+, but on a very minor scale. This feature was used very little back then.
FF43+, this is now enabled by default. Still hard to reproduce. But intend is to land on FF43+ because of this.

If not all supported branches, which bug introduced the flaw?
see above.

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

How likely is this patch to cause regressions; how much testing does it need?
Quite safe.
Attachment #8670754 - Flags: sec-approval?
Is this disabled by default then on 41 and 42?

sec-approval+. We'll want it on Aurora too.
Attachment #8670754 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #5)
> Is this disabled by default then on 41 and 42?

No. There is a small path that allows this in FF41 and FF42, but very very hard to trigger. (Nexto this bug being hard to trigger in itself). As a result I imagine we can see this path as good as not happening in FF41/FF42.
Comment on attachment 8670754 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]:
See bug 1212305 comment 2. Originally bug 1047346, but only bug 1178834 made it wide spread enough to maybe become exploitable?

[User impact if declined]: Unsafe crashes and exploitable.

[Describe test coverage new/current, TreeHerder]:
I haven't pushed to nightly yet (Should I wait before aurora approval? So we can land this security on all branches at the same time.). It passes all our shell testcases and the testcase that gave us this issue.

[Risks and why]: 
Doesn't adjust any code on normal code paths. Only on code paths that would crash currently. I think that this code is definitely better than crashing. Very very worst case scenario this code would introduce a new crash. Which still sounds better than a definite crash. Though I (and the reviewer) think this fully solves the issue ;)

[String/UUID change made/needed]:
Attachment #8670754 - Flags: approval-mozilla-aurora?
Attachment #8670754 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][b2g-adv-main2.5+]
Whiteboard: [jsbugmon:ignore][b2g-adv-main2.5+] → [jsbugmon:ignore][b2g-adv-main2.5+][adv-main43+]
Whiteboard: [jsbugmon:ignore][b2g-adv-main2.5+][adv-main43+] → [jsbugmon:ignore][b2g-adv-main2.5+][adv-main43+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.