982398 - GIRP game crashes in EnterBaseline

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Install Shumway addon: http://mozilla.github.io/shumway/extension/firefox/shumway.xpi
2. In about:addons, set Shockwave Flash's click-to-play setting to "Ask to Activate".
3. Play GIRP game using Shumway: http://www.foddy.net/GIRP.html
4. Press and hold keys matching the ring letters to climb.
5. Let the man fall into the water.

RESULT:
CRASH! The crash is intermittent and, if it will happen, usually happens on the first fall.

bp-6f23d05d-71ab-470f-ade2-6824e2140311
bp-6f23d05d-71ab-470f-ade2-6824e2140311
bp-c8bb8269-8c77-4c8a-965c-249842140311
bp-3391da3b-b5a8-4cce-b605-85c162140311
bp-c8bb8269-8c77-4c8a-965c-249842140311
bp-3391da3b-b5a8-4cce-b605-85c162140311


Frame 	Module 	Signature 	Source
0 		@0x5300000078 	
1 	XUL 	EnterBaseline 	js/src/jit/BaselineJIT.cpp
2 	XUL 	js::jit::EnterBaselineMethod(JSContext*, js::RunState&) 	js/src/jit/BaselineJIT.cpp
3 	XUL 	CanEnterBaselineJIT 	js/src/jit/BaselineJIT.cpp
4 	XUL 	js::jit::CanEnter(JSContext*, js::RunState&) 	js/src/jit/Ion.cpp
5 	XUL 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp
6 	XUL 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
7 	libsystem_pthread.dylib 	libsystem_pthread.dylib@0x44a9 	
8 	libsystem_pthread.dylib 	libsystem_pthread.dylib@0x48f3 	
9 	HIToolbox 	HIToolbox@0x16995 	
10 	HIToolbox 	HIToolbox@0x2e8ad 	
11 	HIToolbox 	HIToolbox@0x2e844 	
12 	libnss3.dylib 	PR_GetCurrentThread 	
13 	XUL 	nsThreadManager::GetIsMainThread(bool*) 	xpcom/threads/nsThreadManager.cpp
14 	XUL 	NS_IsMainThread() 	xpcom/glue/nsThreadUtils.cpp
15 	XUL 	XPCWrappedNative::FindTearOff(XPCNativeInterface*, bool, tag_nsresult*) 	js/xpconnect/src/nsCxPusher.cpp
16 	XUL 	xpc::WrapperFactory::WrapForSameCompartment(JSContext*, JS::Handle<JSObject*>) 	js/xpconnect/wrappers/WrapperFactory.cpp
17 	XUL 	JSCompartment::wrap(JSContext*, JS::MutableHandle<JSObject*>, JS::Handle<JSObject*>) 	js/src/jscompartment.cpp
18 	XUL 	_ZL14firstCharKinds 	
19 	XUL 	nsWeakReference::QueryInterface(nsID const&, void**) 	xpcom/glue/nsID.h
20 	XUL 	XPCWrappedNative::GetNewOrUsed(xpcObjectHelper&, XPCWrappedNativeScope*, XPCNativeInterface*, XPCWrappedNative**) 	js/xpconnect/src/XPCWrappedNative.cpp
21 	CoreFoundation 	CoreFoundation@0x1f283 	
22 	XUL 	js::jit::ICStub::trace(JSTracer*) 	js/src/jit/BaselineIC.cpp
23 	CoreFoundation 	CoreFoundation@0x6da2 	
24 	CoreFoundation 	CoreFoundation@0x19599 	
25 	XUL 	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
26 	XUL 	JSCompartment::wrap(JSContext*, JS::MutableHandle<JSObject*>, JS::Handle<JSObject*>) 	js/src/jscompartment.cpp
27 	XUL 	_ZL14firstCharKinds 	
28 	XUL 	XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, XPCNativeInterface**, bool, tag_nsresult*) 	obj-firefox/x86_64/dist/include/nsAutoPtr.h
29 	XUL 	JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp
30 	XUL 	mozilla::dom::Function::Call(JSContext*, JS::Handle<JS::Value>, nsTArray<JS::Value> const&, mozilla::ErrorResult&) 	obj-firefox/x86_64/dom/bindings/FunctionBinding.cpp
31 	XUL 	mozilla::dom::NativeInterface2JSObjectAndThrowIfFailed(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>, xpcObjectHelper&, nsID const*, bool) 	dom/bindings/BindingUtils.cpp
32 	XUL 	JS::Value mozilla::dom::Function::Call<nsCOMPtr<nsISupports> >(nsCOMPtr<nsISupports> const&, nsTArray<JS::Value> const&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) 	obj-firefox/x86_64/dist/include/mozilla/dom/FunctionBinding.h
33 	XUL 	mozilla::dom::exceptions::JSStackFrame::ToString(nsACString_internal&)::format 	
34 	XUL 	nsDocShell::QueryInterface(nsID const&, void**) 	docshell/base/nsDocShell.cpp
35 	XUL 	nsDocShell::GetSameTypeParentIgnoreBrowserAndAppBoundaries(nsIDocShell**) 	obj-firefox/x86_64/dist/include/nsCOMPtr.h
36 	XUL 	nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) 	dom/base/nsGlobalWindow.cpp
37 	XUL 	XUL@0xfd2020 	
38 	XUL 	nsCOMPtr_base::assign_with_AddRef(nsISupports*) 	xpcom/glue/nsCOMPtr.cpp
39 	XUL 	GetTopImpl 	obj-firefox/x86_64/dist/include/nsCOMPtr.h
40 	libmozglue.dylib 	arena_malloc 	memory/mozjemalloc/jemalloc.c
41 	libmozglue.dylib 	je_malloc 	memory/mozjemalloc/jemalloc.c
42 	libsystem_malloc.dylib 	libsystem_malloc.dylib@0x10868 	
43 	XUL 	NS_CycleCollectorSuspect3 	obj-firefox/x86_64/dist/include/mozilla/ThreadLocal.h
44 	XUL 	nsGlobalWindow::RunTimeout(nsTimeout*) 	dom/base/nsGlobalWindow.cpp
45 	XUL 	nsGlobalWindow::TimerCallback(nsITimer*, void*) 	dom/base/nsGlobalWindow.cpp
46 	XUL 	XUL@0xfeb680 	
47 	XUL 	nsTimerImpl::Fire() 	xpcom/threads/nsTimerImpl.cpp
48 	XUL 	nsTimerEvent::Run() 	xpcom/threads/nsTimerImpl.cpp
49 	XUL 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
50 	CoreFoundation 	CoreFoundation@0x6da2 	
51 	CoreFoundation 	CoreFoundation@0x71077 	
52 	XUL 	NS_ProcessPendingEvents(nsIThread*, unsigned int) 	xpcom/glue/nsThreadUtils.cpp
53 	XUL 	nsBaseAppShell::NativeEventCallback() 	widget/xpwidgets/nsBaseAppShell.cpp
54 	XUL 	nsAppShell::ProcessGeckoEvents(void*) 	widget/cocoa/nsAppShell.mm
55 	CoreFoundation 	CoreFoundation@0x7f731 	
56 	CoreFoundation 	CoreFoundation@0x70ea2 	
57 	CoreFoundation 	CoreFoundation@0x7062f


Shu disassembled the stack trace and recommended including this JIT code:

(lldb) bt
* thread #1: tid = 0x201a6d, 0x00000111000004d0, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x111000004d0)
    frame #0: 0x00000111000004d0
  * frame #1: 0x00000001236ea47f
(lldb) di -s 0x1236ea47f-128 -c 40
   0x1236ea3ff:  adcb   %al, (%rax)
   0x1236ea401:  addb   %al, (%rax)
   0x1236ea403:  movabsq $0x10933a100, %rdx
   0x1236ea40d:  movq   (%rdx), %rdx
   0x1236ea410:  xorl   %r8d, %r8d
   0x1236ea413:  movq   (%rbp), %rax
   0x1236ea417:  testl  $0x100, -0x8(%rax)
   0x1236ea41e:  je     0x1236ea47d
   0x1236ea424:  movabsq $0x1078135d8, %r11
   0x1236ea42e:  cmpl   $0x0, (%r11)
   0x1236ea432:  je     0x1236ea47d
   0x1236ea438:  movl   0x40(%rdi), %ecx
   0x1236ea43b:  movabsq $0x1078135c8, %r11
   0x1236ea445:  movq   (%r11), %rax
   0x1236ea448:  movl   (%rax), %eax
   0x1236ea44a:  addl   $-0x1, %eax
   0x1236ea44d:  movabsq $0x1078135d0, %r11
   0x1236ea457:  cmpl   %eax, (%r11)
   0x1236ea45a:  jle    0x1236ea47d
   0x1236ea460:  shlq   $0x5, %rax
   0x1236ea464:  pushq  %rax
   0x1236ea465:  movabsq $0x1078135c0, %r11
   0x1236ea46f:  movq   (%r11), %rax
   0x1236ea472:  addq   (%rsp), %rax
   0x1236ea476:  addq   $0x8, %rsp
   0x1236ea47a:  movl   %ecx, 0x18(%rax)
   0x1236ea47d:  callq  *%rdx
-> 0x1236ea47f:  popq   %r11
   0x1236ea481:  shrq   $0x4, %r11
   0x1236ea485:  addq   %r11, %rsp
   0x1236ea488:  popq   %rbp
   0x1236ea489:  popq   %rdi
   0x1236ea48a:  popq   %rsi
   0x1236ea48b:  movq   %rsi, (%rsp)
   0x1236ea48f:  movq   0x18(%rdi), %rdi
   0x1236ea493:  jmpq   *(%rdi)
   0x1236ea495:  movq   %rbp, %rsp
   0x1236ea498:  popq   %rbp
   0x1236ea499:  popq   %rdi
   0x1236ea49a:  popq   %rsi
(lldb)

Comment 1

[Shu-yu Guo [:shu]]

The asm above seems to be coming from CallGeneric, and we're crashing when trying to call into the arguments rectifier. GC related?

   0x1236ea3ff:  adcb   %al, (%rax)
   0x1236ea401:  addb   %al, (%rax)
   0x1236ea403:  movabsq $0x10933a100, %rdx      <- load the arguments rectifier
   0x1236ea40d:  movq   (%rdx), %rdx
   0x1236ea410:  xorl   %r8d, %r8d

Comment 2

[Shu-yu Guo [:shu]]

Marty suggested that it might be related to bug 977810.

Comment 3

[Shu-yu Guo [:shu]]

Marty dug around some more. I didn't disassemble far enough back to catch that the arguments rectifier is conditionally loaded (duh), and in the crash that he caught in gdb, the JitCode * of argumentsRectifier_ is alive and well.

So it's the callee itself that has a bogus code pointer.

Comment 4

[Shu-yu Guo [:shu]]

Some more progress from debugging with Marty:

The bogus callee address comes from the script->baselineOrIonRaw. This value is supposed to be == script->method()->raw() when we have an IonScript, which we dereferenced and found out to be not the case.

The next step should probably be asserting that baselineOrIonRaw == method()->raw() at various places and figure out when they first get out of sync.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #4)
> The next step should probably be asserting that baselineOrIonRaw ==
> method()->raw() at various places and figure out when they first get out of
> sync.

When I added baselineOrIonRaw, I made JSScript::ion and JSScript::baseline private, and added setIonScript and setBaselineScript methods that always call updateBaselineOrIonRaw(). So it should be pretty hard for these to go out of sync... I also don't see any other assignments to baseline/ion.

But yeah somebody should debug this, Marty can you take it?

Comment 6

[Marty Rosenberg [:mjrosenb]]

Yeah, Hopefully, I'll get to this later today, definitely by tomorrow.

Comment 7

[Marty Rosenberg [:mjrosenb]]

Attachment: fixCallCrash-r0.patch

I think I caught all of the uses in that file.

Comment 8

[Marty Rosenberg [:mjrosenb]]

Attachment: fixCallCrash-r2.patch

looks like I managed to split the fix the problem in two patches, then attached the wrong patch.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8392254 [details] [diff] [review]
fixCallCrash-r2.patch

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

::: js/src/jit/BaselineIC.cpp
@@ +6674,5 @@
>          callee = regs.takeAny();
>      }
>      Register code = regs.takeAny();
>      masm.loadPtr(Address(BaselineStubReg, ICGetProp_CallScripted::offsetOfGetter()), callee);
> +    masm.branchIfFunctionHasNoScript(callee, &failureLeaveStubFrame);

ICGetElemNativeCompiler::emitCallScripted needs the same fix.

            // Load function in scratchReg and ensure that it has a jit script.
            masm.loadPtr(Address(BaselineStubReg, ICGetElemNativeGetterStub::offsetOfGetter()),
                         scratchReg);
            masm.branchIfFunctionHasNoScript(scratchReg, popR1 ? &failurePopR1 : &failure);

@@ +6679,4 @@
>      masm.loadPtr(Address(callee, JSFunction::offsetOfNativeOrScript()), code);
>      masm.loadBaselineOrIonRaw(code, code, SequentialExecution, &failureLeaveStubFrame);
>  
> +

Nit: remove this line.

::: js/src/jsscript.h
@@ +1226,5 @@
>      }
>      static size_t offsetOfBaselineOrIonRaw() {
>          return offsetof(JSScript, baselineOrIonRaw);
>      }
> +

Nit: remove this line.

Comment 10

[Jan de Mooij [:jandem]]

Likely a regression from bug 886193, so we should backport this to beta and aurora.

Comment 11

[Marty Rosenberg [:mjrosenb]]

Attachment: fixCallCrash-r3.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
not impossible, but it is beyond my abilities.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
the code change itself does...
Which older supported branches are affected by this flaw?
beta, aurora
If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
this patch should apply cleanly everywhere.  the code hasn't been touched since march 2013
How likely is this patch to cause regressions; how much testing does it need?
nearly impossible


fixed minor nits, carrying over r+.  After speaking with efaust, it sounds like we don't need to check the other path mentioned, because the calling function always inserts a check (not exactly efficiently, but it gets the job done)

Comment 12

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

We need a rating in order to approve this. Is the crash exploitable?

Comment 13

[Marty Rosenberg [:mjrosenb]]

My conservative guess would be yes, since we're explicitly calling a value that we shouldn't be, moreso because the offset of the field that we're accessing is 0x78, which is beyond the size of the struct that we /know/ is there (js::LazyScript is 64 bytes on amd64).

Comment 14

[Jan de Mooij [:jandem]]

What about the ICGetElemNativeCompiler::emitCallScripted case I mentioned in comment 9?

Comment 15

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

Comment on attachment 8392623 [details] [diff] [review]
fixCallCrash-r3.patch

sec-approval+ for checkin after 3/29/14 (to reduce window of exposure). Please make Aurora and Beta patches to be nominated and checked in after it is on trunk.

Comment 16

[Chris Peterson [:cpeterson]]

I tested Marty's patch locally and it seems to have fixed this intermittent crash.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Marty, it is possible to land this patch in m-c and request for uplift? We are going to build 29 b6 today.
Thanks

Comment 18

[Marty Rosenberg [:mjrosenb]]

landed on mi: https://hg.mozilla.org/integration/mozilla-inbound/rev/231095210ed3

Comment 19

[Jan de Mooij [:jandem]]

You still have to fix the GetElemNative case, see comment 9 and comment 14.

Comment 20

[Carsten Book [:Tomcat]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #18)
> landed on mi:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/231095210ed3

landed on central https://hg.mozilla.org/mozilla-central/rev/231095210ed3

Comment 21

[Marty Rosenberg [:mjrosenb]]

Attachment: bug982398-r0.patch

So I thought this check already existed on this path, but it definitely isn't there, so I'm not too sure what it was that I saw last time.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8404579 [details] [diff] [review]
bug982398-r0.patch

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

Thanks!

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

I take it from comment 10 that this issue was introduced in Firefox 29.

Jan/Al - Can you confirm that this issue does not impact ESR24?

Comment 24

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

Comment 11 says that ESR24 was not affected (by implication).

FYI, you don't need to needinfo? me on bugs to get my attention. I read my bugmail if I'm cc'd on a bug and addressed by name, I will respond.

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks Al both for the clarification on the issue (missed the implication in comment 11) and the clarification about how to communicate with you in Bugzilla.

Comment 26

[Jan de Mooij [:jandem]]

(What Al said. Bug 886193 (Firefox 29) regressed this.)

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Marty, can we get the uplift request for beta & aurora? (FYI, beta8 gtb is today around 12 PDT and it would be nice to have this patch included). Thanks

Comment 28

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8404579 [details] [diff] [review]
bug982398-r0.patch

[Security approval request comment]
Answers to all question should be the same as previous patch.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
886193 
User impact if declined: 
crashes
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Could you evaluate the risk please?

Comment 30

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

Comment on attachment 8404579 [details] [diff] [review]
bug982398-r0.patch

This already received sec-approval+, which only applies to trunk. I think Release Management wants an assessment of the risk to take this on Aurora and Beta before approving it for those branches though.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Exactly! Thanks Al for the precision

Comment 32

[Marty Rosenberg [:mjrosenb]]

The risk of this patch breaking things is lower than the original patch (which was already basically zero), and the chance of it getting exploited is lower (imo), since this code path seems to be harder to get us to execute.

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8404579 [details] [diff] [review]
bug982398-r0.patch

OK. Thanks Marty.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/436ee1fbffb7
https://hg.mozilla.org/releases/mozilla-beta/rev/d5a00d84b8d6

Comment 37

[Bogdan Maris, Desktop Test Engineering]

Reproduced the crash using a old Nightly build. Verified as fixed using Firefox 29 beta 9, latest Nightly and latest Aurora on Mac OS X 10.9.2.