592199 - Crash [@ js_fun_apply ] with embedded CloudMade / OpenStreetMap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

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).

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase

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

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Luke Wagner [:luke]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Luke Wagner [:luke]]

Attachment: 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.

Comment 8

[chris hofmann]

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.

Comment 9

[Luke Wagner [:luke]]

Attachment: 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)).

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Luke Wagner [:luke]]

> 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.

Comment 12

[Johnny Stenback (:jst)]

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?

Comment 13

[David Mandelin [:dmandelin]]

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.

Comment 14

[Daniel Holbert [:dholbert]]

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

Comment 15

[Luke Wagner [:luke]]

(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

Comment 16

[Daniel Veditz [:dveditz]]

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.

Comment 17

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/9f2641871ce8

(In reply to comment #16)
The problem originated in bug 509599.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

(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

[chris hofmann]

if we respin lets pick this up, otherwise land for b6

Comment 20

[Johnny Stenback (:jst)]

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.

Comment 21

[Daniel Veditz [:dveditz]]

(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.

Comment 22

[Daniel Holbert [:dholbert]]

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?

Comment 23

[Luke Wagner [:luke]]

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.