If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash [@ js_fun_apply ] with embedded CloudMade / OpenStreetMap

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: dholbert, Assigned: luke)

Tracking

({crash, crashreportid, regression})

Trunk
x86
Linux
crash, crashreportid, regression
Points:
---

Firefox Tracking Flags

(blocking2.0 beta5+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: fixed-in-tracemonkey, crash signature, URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
STEPS TO REPRODUCE:
1. Load URL:
   http://pugio.net/2009/07/openstreetmap-geolocation-in-firefox-35.html
2. Click and pan around the embedded map.
  e.g. this reproduces it pretty reliably: Click the center of europe, drag it all the way to the left. Wait a second or two.

RESULTS: Crash [@ js_fun_apply ]

I've reproduced in m-c nightly:
Mozilla/5.0 (X11; Linux i686; rv:2.0b5pre) Gecko/20100830 Firefox/4.0b5pre
as well as 4.0b4:
Mozilla/5.0 (X11; Linux i686; rv:2.0b4) Gecko/20100818 Firefox/4.0b4

However, I couldn't reproduce in a Firefox 3.6 nightly build:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.8pre) Gecko/20100708 Namoroka/3.6.8pre
(Reporter)

Comment 1

7 years ago
Here are some crash report IDs:
bp-05f49cda-5973-44b0-9564-2a7332100831
bp-893e53b1-2c9e-4e9b-9208-331d82100831
bp-b035f1e4-4295-4b74-80d8-60df82100831
bp-0e8dda67-9b83-4f9e-aa3e-cf4d52100831

Backtrace:
0  	libxul.so  	js_fun_apply  	 js/src/jsfun.cpp:2409
1 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:4696
2 	libxul.so 	js::InvokeCommon<JSBool > 	js/src/jsinterp.cpp:577
3 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:696
4 	libxul.so 	js::InternalInvoke 	js/src/jsinterp.cpp:736
5 	libxul.so 	JS_CallFunctionValue 	js/src/jsinterp.h:651
6 	libxul.so 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:2248
7 	libxul.so 	nsGlobalWindow::RunTimeout 	dom/base/nsGlobalWindow.cpp:8585
8 	libxul.so 	nsGlobalWindow::TimerCallback 	dom/base/nsGlobalWindow.cpp:8930
9 	libxul.so 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:425
10 	libxul.so 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:517
11 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:547
12 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:250
13 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
14 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
15 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:202
16 	libxul.so 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:175
17 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:191
18 		@0xb75a3f4f 	
19 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3662
20 	firefox-bin 	main 	browser/app/nsBrowserApp.cpp:158
21 	libc-2.11.1.so 	libc-2.11.1.so@0x16bd5 	
22 	firefox-bin 	firefox-bin@0x1390 	
23 	firefox-bin 	Output 	browser/app/nsBrowserApp.cpp:77
24 		@0x3 

In my crash reports, the Crash Address is always 0xa162. I'm not sure what the significance of that value is.

Tentatively marking security-sensitive.
Group: core-security
(Reporter)

Comment 2

7 years ago
Nightly regression range:
20100806031556 26ee1b556bd9
20100807025746 81c119fb86c7

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=26ee1b556bd9&tochange=81c119fb86c7

That range includes a fairly large tracemonkey merge (49 csets).
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
Keywords: crash, crashreportid
(Reporter)

Updated

7 years ago
Summary: Crash [@ js_fun_apply ] → Crash [@ js_fun_apply ] with openstreetmap demo
(Reporter)

Comment 3

7 years ago
Created attachment 470724 [details]
testcase

Here's a somewhat reduced testcase. ("somewhat" because it references an external script from cloudmade, which is some sort of openstreetmap-wrapping service)
(Reporter)

Updated

7 years ago
Summary: Crash [@ js_fun_apply ] with openstreetmap demo → Crash [@ js_fun_apply ] with embedded CloudMade / OpenStreetMap
(Reporter)

Comment 4

7 years ago
I just confirmed that this affects the latest tracemonkey build, too:
Mozilla/5.0 (X11; Linux i686; rv:2.0b5pre) Gecko/20100830 Firefox/4.0b5pre
Crash report from there is:
bp-0610a262-e03e-4f60-b544-b96c12100831
(Assignee)

Comment 5

7 years ago
Thanks a lot!  I think this is a reproducible testcase for the #2 crasher for FF4.0b4 (bug 586004).
(Reporter)

Comment 6

7 years ago
Hooray! You're very welcome.

FWIW, I tested tracemonkey nightlies, and the regression range there is actually the same date-range as on m-c:
20100806033340 c15ed7c71e27
20100807032731 bdd944b09a53
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=c15ed7c71e27&tochange=bdd944b09a53
(Assignee)

Comment 7

7 years ago
Created attachment 470739 [details] [diff] [review]
fixes crash

It looks like this is all just caused by an over-eager optimization: putActivationObjects doesn't call js_PutArguments on the arguments object created on trace if argc == 0, even though the object may be aliased elsewhere.  This bug goes way back (http://hg.mozilla.org/tracemonkey/rev/78d111c4ab84), although perhaps we didn't trace enough back then to let it bite us.  Now sleep.
(Assignee)

Updated

7 years ago
Attachment #470739 - Attachment description: maybe fix → fixes crash

Comment 8

7 years ago
did we confirm the security sensitivity of this bug?  if it is sg:crit? should we mark bug 586004 security sensitive as well, or just get this fixed soon and move on?

should be b5 blocker if respins are done based on high volume crasher that might also introduce a vulnerability.   definitely should try to pick this up for b6.
(Assignee)

Comment 9

7 years ago
Created attachment 470812 [details] [diff] [review]
fix

Oops, I forgot to qref before posting that last patch.

I don't see how this crash is exploitable: since the bug only happens for argc == 0, code shouldn't be doing anything with the trashed Arguments private-ptr.  Most sites guard on argc > 0 before even loading fp->argv, so this bug would have been innocuous, but js_fun_apply loads fp->argv in all cases (for a memset(fp->argv, ..., argc)).
Assignee: general → lw
Attachment #470739 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #470812 - Flags: review?(dmandelin)
Blocks: 586004
Is this really exploitable if it involves the evil fixed JS_ARGUMENTS_OBJECT_ON_TRACE address?

I think that hack is worth eliminating -- followup bug?

/be
(Assignee)

Comment 11

7 years ago
> I think that hack is worth eliminating -- followup bug?

I don't think its so much a hack as a debugging tool: argc == 0 case aside; if you are accessing the private-ptr of an Arguments object on trace, something is wrong.  OTOH, I think bug 590871 will make the whole on/off trace distinction go away by allowing activation objects to store the JSStackFrame* early.
Marking this blocking beta5 since this is likely to fix the #2 topcrash for beta4. David, can we get this reviewed?

And sounds like this should not be a security bug per comment #9, any objections to me opening this up?
blocking2.0: ? → beta5+
Comment on attachment 470812 [details] [diff] [review]
fix

A trace-test test case would be nice, but I know we need this patch urgently, so add it if you can easily. Otherwise r+. And thanks for the description of the problem.
Attachment #470812 - Flags: review?(dmandelin) → review+
(Reporter)

Comment 14

7 years ago
(In reply to comment #10)
> Is this really exploitable if it involves the evil fixed
> JS_ARGUMENTS_OBJECT_ON_TRACE address?

Ah, interesting - FWIW, the crashes are at addresses slightly offset from that value, and the amount of offset seems to vary (despite what I said at the end of comment 1).

That is, JS_ARGUMENT_OBJECT_ON_TRACE is #defined as 0xa126
vs. bp-0e8dda67-9b83-4f9e-aa3e-cf4d52100831 is at 0xa146
vs. bp-893e53b1-2c9e-4e9b-9208-331d82100831 is at 0xa162

(In reply to comment #12)
> And sounds like this should not be a security bug per comment #9, any
> objections to me opening this up?
None here, given comment 9.  (I originally hid it just to be on the safe side.)
(Assignee)

Comment 15

7 years ago
(In reply to comment #14)
> Ah, interesting - FWIW, the crashes are at addresses slightly offset from that
> value, and the amount of offset seems to vary (despite what I said at the end
> of comment 1).
> 
> That is, JS_ARGUMENT_OBJECT_ON_TRACE is #defined as 0xa126
> vs. bp-0e8dda67-9b83-4f9e-aa3e-cf4d52100831 is at 0xa146
> vs. bp-893e53b1-2c9e-4e9b-9208-331d82100831 is at 0xa162

Those changes reflect the ever-changing offsetof(JSStackFrame, argv):
http://hg.mozilla.org/projects/electrolysis/annotate/7652c4f2d60d/js/src/jsinterp.h#l86
http://hg.mozilla.org/mozilla-central/annotate/9d6448b6a677/js/src/jsinterp.h#l84
Is this a regression from bug 585231? This seems the most relevant in the regression range in comment 2
http://hg.mozilla.org/mozilla-central/rev/d695985dc913

Would like to pin down the regressing bug/changeset so we can link the bugs in case we ever want to take the regressing bug on branches. Seems unlikely if it really is bug 585231, but if it isn't it might be some other security-related fix.
(Assignee)

Comment 17

7 years ago
http://hg.mozilla.org/tracemonkey/rev/9f2641871ce8

(In reply to comment #16)
The problem originated in bug 509599.
Whiteboard: fixed-in-tracemonkey
(In reply to comment #12)
> Marking this blocking beta5 since this is likely to fix the #2 topcrash for
> beta4. David, can we get this reviewed?

Uhhhh, beta5 has been tagged and built already. Are you proposing that we respin for this bug? I don't think that's necessary, really, and would rather save it for beta6. Let me know ASAP.

Comment 19

7 years ago
if we respin lets pick this up, otherwise land for b6
I was informed that we're respinning for some focus related issues, and if we are, we should make sure this gets in too, if not, we'll do beta6 here.
(In reply to comment #17)
> The problem originated in bug 509599.

Which would imply 1.9.2 needs the fix, although dholbert couldn't reproduce the problem on that branch.
blocking1.9.2: --- → ?
status1.9.2: --- → ?
(Reporter)

Comment 22

7 years ago
See also comment 7. (It sounds like we've had this foot-gun for a while, but we only recently disabled the safety button.)

Luke, any thoughts on whether this patch would be useful/appropriate/risky on 1.9.2?
(Assignee)

Comment 23

7 years ago
http://hg.mozilla.org/mozilla-central/rev/c38ea6cd0ea2

I tried to exercise the bug on 1.9.2 but was unable to: the 1.9.2 version of js_fun_apply seems to guard on the arg count before touching fp->argv hence, while Arguments objects are escaping trace when argc == 0, no code (AFAICS) seems to care.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
blocking1.9.2: ? → ---
status1.9.1: --- → unaffected
status1.9.2: ? → unaffected

Updated

7 years ago
Duplicate of this bug: 586004
Group: core-security
Crash Signature: [@ js_fun_apply ]
You need to log in before you can comment on or make changes to this bug.