GIRP game crashes in EnterBaseline

VERIFIED FIXED in Firefox 29

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: mjrosenb)

Tracking

(Blocks 1 bug, 4 keywords)

unspecified
mozilla31
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 unaffected, firefox29+ verified, firefox30+ verified, firefox31+ verified, firefox-esr24- unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [js:p1])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
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

5 years ago
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

5 years ago
Marty suggested that it might be related to bug 977810.

Comment 3

5 years ago
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

5 years ago
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.
(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?
Whiteboard: [js:p1]
Assignee

Comment 6

5 years ago
Yeah, Hopefully, I'll get to this later today, definitely by tomorrow.
Assignee

Comment 7

5 years ago
Posted patch fixCallCrash-r0.patch (obsolete) — Splinter Review
I think I caught all of the uses in that file.
Attachment #8392210 - Flags: review?(jdemooij)
Assignee

Comment 8

5 years ago
Posted patch fixCallCrash-r2.patch (obsolete) — Splinter Review
looks like I managed to split the fix the problem in two patches, then attached the wrong patch.
Attachment #8392210 - Attachment is obsolete: true
Attachment #8392210 - Flags: review?(jdemooij)
Assignee

Updated

5 years ago
Attachment #8392254 - Flags: review?(jdemooij)
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.
Attachment #8392254 - Flags: review?(jdemooij) → review+
Likely a regression from bug 886193, so we should backport this to beta and aurora.
Assignee

Comment 11

5 years ago
[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)
Attachment #8392254 - Attachment is obsolete: true
Attachment #8392623 - Flags: sec-approval?
Attachment #8392623 - Flags: review+
We need a rating in order to approve this. Is the crash exploitable?
Assignee

Comment 13

5 years ago
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).
What about the ICGetElemNativeCompiler::emitCallScripted case I mentioned in comment 9?
Blocks: 886193
Keywords: regression, sec-high
Whiteboard: [js:p1] → [js:p1][checkin after 3/29/14]
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.
Attachment #8392623 - Flags: sec-approval? → sec-approval+
Reporter

Comment 16

5 years ago
I tested Marty's patch locally and it seems to have fixed this intermittent crash.
Marty, it is possible to land this patch in m-c and request for uplift? We are going to build 29 b6 today.
Thanks
Flags: needinfo?(mrosenberg)
Assignee

Updated

5 years ago
Keywords: checkin-needed
You still have to fix the GetElemNative case, see comment 9 and comment 14.
Flags: needinfo?(mrosenberg)
(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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee: nobody → mrosenberg
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [js:p1][checkin after 3/29/14] → [js:p1]
Target Milestone: mozilla31 → ---
Assignee

Comment 21

5 years ago
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.
Attachment #8404579 - Flags: review?(jdemooij)
Flags: needinfo?(mrosenberg)
Comment on attachment 8404579 [details] [diff] [review]
bug982398-r0.patch

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

Thanks!
Attachment #8404579 - Flags: review?(jdemooij) → review+
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?
Flags: needinfo?(jdemooij)
Flags: needinfo?(abillings)
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.
Flags: needinfo?(abillings)
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.
(What Al said. Bug 886193 (Firefox 29) regressed this.)
Flags: needinfo?(jdemooij)
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
Flags: needinfo?(mrosenberg)
Assignee

Comment 28

5 years ago
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:
Attachment #8404579 - Flags: sec-approval?
Attachment #8404579 - Flags: approval-mozilla-beta?
Attachment #8404579 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mrosenberg)
Could you evaluate the risk please?
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.
Attachment #8404579 - Flags: sec-approval?
Exactly! Thanks Al for the precision
Flags: needinfo?(mrosenberg)
Assignee

Comment 32

5 years ago
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.
Flags: needinfo?(mrosenberg)
Comment on attachment 8404579 [details] [diff] [review]
bug982398-r0.patch

OK. Thanks Marty.
Attachment #8404579 - Flags: approval-mozilla-beta?
Attachment #8404579 - Flags: approval-mozilla-beta+
Attachment #8404579 - Flags: approval-mozilla-aurora?
Attachment #8404579 - Flags: approval-mozilla-aurora+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea688f935389
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Keywords: verifyme
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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.