267311 - netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect") in a XBL constructor make mozilla crash. [@ JS_FrameIterator]

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[José Antonio Sánchez Sánchez]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041028

xbl constructor, make mozilla crash. 

the files on the test are
test.xul
widgets.css
widgets.xml


Reproducible: Always
Steps to Reproduce:
1. create a simple widget with a constructor containing the line:
netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect")
2. create a xul with a binding to that widget. 
3. run the xul.
Actual Results:  
Mozilla crash, and the application is closed. 

Expected Results:  
Mozilla crash, and the application is closed.

js3250.dll

Comment 1

[Gary van der Merwe]

*** Bug 267305 has been marked as a duplicate of this bug. ***

Comment 2

[Gary van der Merwe]

*** Bug 267306 has been marked as a duplicate of this bug. ***

Comment 3

[Gary van der Merwe]

*** Bug 267308 has been marked as a duplicate of this bug. ***

Comment 4

[Gary van der Merwe]

*** Bug 267315 has been marked as a duplicate of this bug. ***

Comment 5

[Gary van der Merwe]

*** Bug 267316 has been marked as a duplicate of this bug. ***

Comment 6

[José Antonio Sánchez Sánchez]

while I was trying tu submit this bug, an internal server error was reported as
a result of the submit, so I try to submit the bug again a few times. 

The duplicated bugs are for this reason. 

Sorry. 

Comment 7

[timeless]

first bug is caps, so to caps it goes

 	js3250.dll!JS_FrameIterator(JSContext * cx=0x00000000, JSStackFrame * *
iteratorp=0x0012f228)  Line 652 + 0xe	C
 	caps.dll!nsScriptSecurityManager::GetPrincipalAndFrame(JSContext *
cx=0x00000000, nsIPrincipal * * result=0x0012f3cc, JSStackFrame * *
frameResult=0x0012f3c8)  Line 1900 + 0x15	C++
>	caps.dll!nsScriptSecurityManager::EnableCapability(const char *
capability=0x03b1d1e0)  Line 2314 + 0x1b	C++
 	caps.dll!netscape_security_enablePrivilege(JSContext * cx=0x0317da40, JSObject
* obj=0x03298038, unsigned int argc=0x00000001, long * argv=0x039c128c, long *
rval=0x0012f44c)  Line 143 + 0xf	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x0012f35c, unsigned int argc=0x00000000,
unsigned int flags=0x77f58a3a)  Line 1286 + 0x11	C
 	js3250.dll!js_Interpret(JSContext * cx=0x00000000, long * result=0x77f58a3a) 
Line 3500	C
 	js3250.dll!js_Invoke(JSContext * cx=0x0012f35c, unsigned int argc=0x00000000,
unsigned int flags=0x77f58a3a)  Line 1306 + 0xa	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x0317da40, JSObject *
obj=0x02f538d8, long fval=0x02f53c98, unsigned int flags=0x00000000, unsigned
int argc=0x0317da6c, long * argv=0x00000000, long * rval=0x0012f788)  Line
1428 + 0x13	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x0317da40, JSObject *
obj=0x02f538d8, long fval=0x02f53c98, unsigned int argc=0x00000000, long *
argv=0x00000000, long * rval=0x0012f788)  Line 3783 + 0x1c	C
 	gklayout.dll!nsXBLProtoImplAnonymousMethod::Execute(nsIContent *
aBoundElement=0x03bec0b8)  Line 283 + 0x11	C++
 	gklayout.dll!nsXBLBinding::ExecuteAttachedHandler()  Line 849	C++
 	gklayout.dll!nsBindingManager::ProcessAttachedQueue()  Line 932	C++
 	gklayout.dll!nsCSSFrameConstructor::ContentInserted(nsPresContext *
aPresContext=0x03903de0, nsIContent * aContainer=0x01845508, nsIFrame *
aContainerFrame=0x038a0a4c, nsIContent * aChild=0x03bec0b8, int
aIndexInContainer=0x01845508, nsILayoutHistoryState * aFrameState=0x00000000,
int aInReinsertContent=0x00000000)  Line 9329	C++
 	gklayout.dll!PresShell::ContentInserted(nsIDocument * aDocument=0x030e9418,
nsIContent * aContainer=0x01845508, nsIContent * aChild=0x03bec0b8, int
aIndexInContainer=0x00000000)  Line 5141	C++
 	gklayout.dll!nsXBLBindingRequest::DocumentLoaded(nsIDocument *
aBindingDoc=0x038f7048)  Line 175	C++
 	gklayout.dll!nsXBLStreamListener::Load(nsIDOMEvent * aEvent=0x02f893f0)  Line
426 + 0xa	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext *
aPresContext=0x03aa49b4, nsEvent * aEvent=0x0012fadc, nsIDOMEvent * *
aDOMEvent=0x0012faac, nsIDOMEventTarget * aCurrentTarget=0x038f70f0, unsigned
int aFlags=0x00000007, nsEventStatus * aEventStatus=0x0012fb1c)  Line 1607 +
0x2c	C++
 	gklayout.dll!nsDocument::HandleDOMEvent(nsPresContext *
aPresContext=0x00000000, nsEvent * aEvent=0x0012fadc, nsIDOMEvent * *
aDOMEvent=0x0012faac, unsigned int aFlags=0x00000001, nsEventStatus *
aEventStatus=0x0012fb1c)  Line 3828	C++
 	gklayout.dll!nsXMLDocument::EndLoad()  Line 643	C++
 	gklayout.dll!nsXMLContentSink::DidBuildModel()  Line 295	C++
 	gkparser.dll!nsExpatDriver::DidBuildModel(unsigned int anErrorCode=0x00000000,
int aNotifySink=0x00000001, nsIParser * aParser=0x038dab88, nsIContentSink *
aSink=0x03be9ec0)  Line 1062	C++
 	gkparser.dll!nsParser::DidBuildModel(unsigned int anErrorCode=0x00000000) 
Line 1248 + 0xe	C++
 	gkparser.dll!nsParser::ResumeParse(int allowIteration=0x00000001, int
aIsFinalChunk=0x00000001, int aCanInterrupt=0x00000001)  Line 1832	C++
 	gkparser.dll!nsParser::OnStopRequest(nsIRequest * request=0x03a1d890,
nsISupports * aContext=0x00000000, unsigned int status=0x00000000)  Line 2495	C++
 	gklayout.dll!nsXBLStreamListener::OnStopRequest(nsIRequest *
request=0x03a1d890, nsISupports * aCtxt=0x00000000, unsigned int
aStatus=0x00000000)  Line 319 + 0x14	C++
 	necko.dll!nsStreamListenerTee::OnStopRequest(nsIRequest * request=0x03a1d890,
nsISupports * context=0x00000000, unsigned int status=0x00000000)  Line 65 +
0x19	C++
 	necko.dll!nsHttpChannel::OnStopRequest(nsIRequest * request=0x03746cf0,
nsISupports * ctxt=0x00000000, unsigned int status=0x00000000)  Line 3723	C++
 	necko.dll!nsInputStreamPump::OnStateStop()  Line 505	C++
 	necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *
stream=0x03991dc8)  Line 342	C++
 	xpcom_core.dll!nsInputStreamReadyEvent::EventHandler(PLEvent *
plevent=0x03992544)  Line 119	C++
 	xpcom_core.dll!PL_HandleEvent(PLEvent * self=0x03992544)  Line 693	C
 	xpcom_core.dll!PL_ProcessPendingEvents(PLEventQueue * self=0x01063ec8)  Line 628	C
 	xpcom_core.dll!_md_EventReceiverProc(HWND__ * hwnd=0x000d2c70, unsigned int
uMsg=0x0000c14e, unsigned int wParam=0x00000000, long lParam=0x01063ec8)  Line
1434	C
 	user32.dll!77d43a50() 	
 	user32.dll!77d43b1f() 	
 	user32.dll!GetMessageW()  + 0x125	
 	user32.dll!DispatchMessageW()  + 0xb	
 	appshell.dll!nsAppShellService::Run()  Line 484	C++
 	mozilla.exe!main1(int argc=0x0012f35c, char * * argv=0x00000000, nsISupports *
nativeApp=0x77f58a3a)  Line 1336	C++
 	mozilla.exe!main(int argc=0x00000001, char * * argv=0x003f7b88)  Line 1827 +
0x16	C++
 	mozilla.exe!mainCRTStartup()  Line 400 + 0x11	C
 	kernel32.dll!TermsrvAppInstallMode()  + 0x269	

Comment 8

[timeless]

Attachment: don't call jsapis with invalid params (null cx), and null check GetPrincipalAndFrame's principal out since null seems to be very legal (see tail of that function)

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 164301 [details] [diff] [review]
don't call jsapis with invalid params (null cx), and null check GetPrincipalAndFrame's principal out since null seems to be very legal (see tail of that function)

Seems to me that the problem here is not that we're calling caps with bogus
arguments, rather that the arguments shouldn't be what they apparently are
here.

Looks like nsXBLProtoImplAnonymousMethod::Execute() needs to push the context
on which it is executing JS onto the context stack, not doing that can have
security problems etc.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 164301 [details] [diff] [review]
don't call jsapis with invalid params (null cx), and null check GetPrincipalAndFrame's principal out since null seems to be very legal (see tail of that function)

On second thought, this is a step in the right direction nonetheless. sr=jst,
but in addition to this we should fix the XBL code to push the right context
onto the stack, and more importantly, this code needs to throw JS exceptions on
failure, right now any failures will result in silent errors (unless I missed
something).

Comment 11

[timeless]

Attachment: xbl changes

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 164331 [details] [diff] [review]
xbl changes

r=jst

Comment 13

[Boris Zbarsky [:bzbarsky]]

So why do we need beginrequest/endrequest here?

Comment 14

[timeless]

i think the general idea is that gecko doesn't abide by the jsapi and we need to
start adding JS_BeginRequest/JS_EndRequest blocks at times. Someone else will
have to correct me if i misplaced them.

shaver volunteered to explain it.

Comment 15

[Brendan Eich [:brendan]]

Initial knee-jerk answer: No, we do not need requests in the main thread.  The
request model is for symmetric multi-threading of JS scripts, which run to
completion or block in native methods that know to suspend their requests. 
Mozilla is asymmetric: the only place GC can run is on the main thread, where it
single-threads with DOM-based scripts, including XBL scripts.

Or so the story went, before the request model was extended to do more than
interlock the GC with N concurrent script executions.  When I made locking
almost zero-cost (bug 54743), I extended the request model to subsume all
single-threaded objects' (scopes') locks under the request interlock mechanism
(which uses the per-runtime GC lock).  That means that any objects shared with
background threads in Gecko embeddings can suffer deadlocks if the main thread
fails to use the begin- and end-request APIs.

But that's a bug for another day.  I do not think this bug's patch should start
introducing request-model API usage in the main thread.

/be

Comment 16

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Comment on attachment 164301 [details] [diff] [review]
don't call jsapis with invalid params (null cx), and null check GetPrincipalAndFrame's principal out since null seems to be very legal (see tail of that function)

r=dveditz

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 164331 [details] [diff] [review]
xbl changes

sr=bzbarsky if you remove the beginrequest/endrequest stuff.

Comment 18

[timeless]

mozilla/content/xbl/src/nsXBLProtoImplMethod.cpp 	1.16
mozilla/caps/src/nsScriptSecurityManager.cpp 	1.241