410486 - nsScriptSecurityManager::CheckLoadURIFromScript() causes assertion failure: (cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread in JS_GetReservedSlot

Status: VERIFIED FIXED

(Core :: Security: CAPS, defect)

Description

[georgi - hopefully not receiving bugspam]

Attachment: neterr1b.html

Assertion failure: (cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread
in JS_GetReservedSlot

similar to Bug 407053
while digging a about:neterr strangeness, JS_Assert popped up.

*debug* trunk is needed.

<a href="javascript:try {setTimeout('location.replace(\'about:config\')',1000);} catch(e) {};var a=new Error();a.toString=function() {return {toString: 'Components.classes' }};a;">click me (probably several times)</a>

clicking on above link causes:
JS_Assert (
    s=0xb7f16e50 "(cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread", file=0xb7f16e20 "/opt/joro/firefox-cvs/mozilla/js/src/jsapi.c", ln=4063)
    at /opt/joro/firefox-cvs/mozilla/js/src/jsutil.c:63
63          abort();
Current language:  auto; currently c
(gdb) bt
#0  JS_Assert (
    s=0xb7f16e50 "(cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread", file=0xb7f16e20 "/opt/joro/firefox-cvs/mozilla/js/src/jsapi.c", ln=4063)
    at /opt/joro/firefox-cvs/mozilla/js/src/jsutil.c:63
#1  0xb7e411c4 in JS_GetReservedSlot (cx=0x887fbc8, obj=0xaf80c520, index=17, 
    vp=0xbf81b6ec) at /opt/joro/firefox-cvs/mozilla/js/src/jsapi.c:4063
#2  0xb7eb0a64 in js_GetClassObject (cx=0x887fbc8, obj=0xaf80c520, 
    key=JSProto_Error, objp=0xbf81b754)
    at /opt/joro/firefox-cvs/mozilla/js/src/jsobj.c:2672
#3  0xb7eb0f5d in js_FindClassObject (cx=0x887fbc8, start=0x0, id=35, 
    vp=0xbf81b7a8) at /opt/joro/firefox-cvs/mozilla/js/src/jsobj.c:2748

Comment 1

[Brian Crowder]

Here's the portion of the stack that I think matters:

#21 0x001e379c in JS_EvaluateUCScriptForPrincipals (cx=0x1f1c7ae0, obj=0x182870a0, principals=0x1b983384, chars=0x1f3e89c0, length=32, filename=0x1f1a0de8 "javascript:try%20{setTimeout('location.replace(\\'about:config\\')',1000);}%20catch(e)%20{};var%20a=new%20Error();a.toString=function()%20{return%20{toString:%20'Components.classes'%20}};a;", lineno=1, rval=0xbfffde78) at /Users/crowder/mozilla/js/src/jsapi.c:4872
#22 0x130edd49 in nsJSContext::EvaluateString (this=0x1f1cc940, aScript=@0xbfffdff4, aScopeObject=0x182870a0, aPrincipal=0x1b983380, aURL=0x1f1a0de8 "javascript:try%20{setTimeout('location.replace(\\'about:config\\')',1000);}%20catch(e)%20{};var%20a=new%20Error();a.toString=function()%20{return%20{toString:%20'Components.classes'%20}};a;", aLineNo=1, aVersion=0, aRetValue=0x0, aIsUndefined=0xbfffdfd8) at /Users/crowder/mozilla/dom/src/base/nsJSEnvironment.cpp:1518
#23 0x1311beba in nsGlobalWindow::RunTimeout (this=0x1f379130, aTimeout=0x1f1a0ef0) at /Users/crowder/mozilla/dom/src/base/nsGlobalWindow.cpp:7355
#24 0x1311c562 in nsGlobalWindow::TimerCallback (aTimer=0x1f380280, aClosure=0x1f1a0ef0) at /Users/crowder/mozilla/dom/src/base/nsGlobalWindow.cpp:7705
#25 0x0039a37c in nsTimerImpl::Fire (this=0x1f380280) at /Users/crowder/mozilla/xpcom/threads/nsTimerImpl.cpp:400
#26 0x0039a59c in nsTimerEvent::Run (this=0x1f362870) at /Users/crowder/mozilla/xpcom/threads/nsTimerImpl.cpp:487
#27 0x00393e3a in nsThread::ProcessNextEvent (this=0x82b860, mayWait=0, result=0xbfffe234) at /Users/crowder/mozilla/xpcom/threads/nsThread.cpp:510
#28 0x003201c0 in NS_ProcessPendingEvents_P (thread=0x82b860, timeout=20) at nsThreadUtils.cpp:180
#29 0x1276c061 in nsBaseAppShell::NativeEventCallback (this=0x8526b0) at /Users/crowder/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:112
#30 0x12746752 in nsAppShell::ProcessGeckoEvents (aInfo=0x8526b0) at /Users/crowder/mozilla/widget/src/cocoa/nsAppShell.mm:294

Comment 2

[Brian Crowder]

Moving to what I hope is the right component.

Comment 3

[Brian Crowder]

Attachment: the full stack, in case I'm mistaken.

Comment 4

[Brendan Eich [:brendan]]

Re: comment 1: there is a JSAutoRequest local around the call from nsJSContext::EvaluateString to JS_EvaluateUCScriptForPrincipals, so that's not the problem. That request is suspended by XPCWrappedNative::CallMethod around the NS_InvokeByIndex_P as usual, so the bug is that nsScriptSecurityManager::CheckLoadURIFromScript fails to use a JSAutoRequest.

/be

Comment 5

[georgi - hopefully not receiving bugspam]

/offtopic

i am not receiving bug mail since Wed Dec 19 10:44:13 2007 probably due to bugzilla bug. email me if i have missed important comment.

Comment 6

[georgi - hopefully not receiving bugspam]

probably [sg:nse?]

Comment 7

[Martin Mokrejs]

Attachment: gdb full stacktrace

I get the same with TRUNK as of now. I tried to render an .html file containing javascript from the disk.

Comment 8

[Martin Mokrejs]

Attachment: testcase.html

actually created as a testcase for bug #266789

Comment 9

[Martin Mokrejs]

Isn't this a dupe of bug #397043 and #397192 ? The jsapic.c contains:

3520 JS_PUBLIC_API(JSBool)
3521 JS_SetProperty(JSContext *cx, JSObject *obj, const char *name, jsval *vp)
3522 {
3523     JSAtom *atom;
3524 
3525     CHECK_REQUEST(cx);
3526     atom = js_Atomize(cx, name, strlen(name), 0);
3527     if (!atom)
3528         return JS_FALSE;
3529     return OBJ_SET_PROPERTY(cx, obj, ATOM_TO_JSID(atom), vp);
3530 }

Comment 10

[timeless]

Attachment: request

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 314350 [details] [diff] [review]
request

OK, but I think I'd rather have the JS_ReportError() turned into a SetPendingException() like everywhere else in nsScriptSecurityManager (I guess that'd require a nsPrintfCString, too).

Comment 12

[timeless]

Attachment: printf

yeah, i just didn't have the energy to think about which classes

Comment 13

[Martin Mokrejs]

timeless,
  I tried your patch from comment #12 but it did not help:

(gdb) 
++DOMWINDOW == 11 (0x82120fc) [serial = 12] [Outer = 0x8211500]
Assertion failure: (cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread, at jsapi.c:3531

Program received signal SIGTRAP, Trace/breakpoint trap.
JS_Assert (s=0xb7eecd48 "(cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread", file=0xb7eecd40 "jsapi.c", ln=3531) at jsutil.c:63
/scratch/mozilla/js/src/jsutil.c:63:2308:beg:0xb7ed1132
Current language:  auto; currently c
(gdb) where
#0  JS_Assert (s=0xb7eecd48 "(cx)->requestDepth || (cx)->thread == (cx)->runtime->gcThread", file=0xb7eecd40 "jsapi.c", ln=3531) at jsutil.c:63
#1  0xb7e0f3e0 in JS_SetProperty (cx=0x89f2720, obj=0x8b50700, name=0xb62c2e9f "HTTPIndex", vp=0xbfea2e54) at jsapi.c:3531
#2  0xb628dfb5 in nsHTTPIndex::OnStartRequest (this=0x8c52700, request=0x87e3090, aContext=0x0) at nsDirectoryViewer.cpp:309
#3  0xb53baa0a in nsDocumentOpenInfo::OnStartRequest (this=0x8f4e790, request=0x87e3090, aCtxt=0x0) at nsURILoader.cpp:290
#4  0xb4e0c3ed in nsBaseChannel::OnStartRequest (this=0x87e3060, request=0x8f73da0, ctxt=0x0) at nsBaseChannel.cpp:607
#5  0xb4e1ef3f in nsInputStreamPump::OnStateStart (this=0x8f73da0) at nsInputStreamPump.cpp:439
#6  0xb4e1fd13 in nsInputStreamPump::OnInputStreamReady (this=0x8f73da0, stream=0x87e324c) at nsInputStreamPump.cpp:395
#7  0xb7ce784e in nsInputStreamReadyEvent::Run (this=0x8f4d200) at nsStreamUtils.cpp:111
#8  0xb7d146df in nsThread::ProcessNextEvent (this=0x8135c90, mayWait=1, result=0xbfea30a0) at nsThread.cpp:510
#9  0xb7ca3cc1 in NS_ProcessNextEvent_P (thread=0x8135c90, mayWait=1) at nsThreadUtils.cpp:227
#10 0xb639a56e in nsBaseAppShell::Run (this=0x84ffb00) at nsBaseAppShell.cpp:170
#11 0xb66d0e89 in nsAppStartup::Run (this=0x85a2370) at nsAppStartup.cpp:181
#12 0xb7da88b0 in XRE_main (argc=1, argv=0xbfea3794, aAppData=0x8111380) at nsAppRunner.cpp:3170
#13 0x08048b32 in main (argc=1, argv=0xbfea3794) at nsSuiteApp.cpp:103
#14 0xb732a41f in __libc_start_main () from /lib/libc.so.6
(gdb)



mozilla/caps/src/nsScriptSecurityManager.cpp:

1186     // See if we're attempting to load a file: URI. If so, let a
1187     // UniversalFileRead capability trump the above check.
1188     PRBool isFile = PR_FALSE;
1189     PRBool isRes = PR_FALSE;
1190     if (NS_FAILED(aURI->SchemeIs("file", &isFile)) ||
1191         NS_FAILED(aURI->SchemeIs("resource", &isRes)))
1192         return NS_ERROR_FAILURE;
1193     if (isFile || isRes)
1194     {
1195         PRBool enabled;
1196         if (NS_FAILED(IsCapabilityEnabled("UniversalFileRead", &enabled)))
1197             return NS_ERROR_FAILURE;
1198         if (enabled)
1199             return NS_OK;
1200     }
1201 
1202     // Report error.
1203     nsCAutoString spec;
1204     if (NS_FAILED(aURI->GetAsciiSpec(spec)))
1205         return NS_ERROR_FAILURE;
1206     SetPendingException(cx, nsPrintfCString("Access to '%s' from script denied", spec.get()).get());
1207     return NS_ERROR_DOM_BAD_URI;
1208 }

Comment 14

[Martin Mokrejs]

Attachment: current cvs HEAD stacktrace

Comment 15

[Mike Shaver (:shaver emeritus)]

Looks like nsHTTPIndex::OnStartRequest needs a JSAutoRequest around its JS_SetProperty.

Comment 16

[timeless]

i should have mentioned that, it's its own bug... bug 429123 

Comment 17

[Martin Mokrejs]

The patch in comment #12 however is still helpfull:

(gdb) c
--WEBSHELL 0x813c8a0 == 6
Document file:///home/mmokrejs/public_html/tmp/testcase-bug266789.html loaded successfully
--DOMWINDOW == 12 (0x821260c) [serial = 14] [Outer = 0x8212430]
--DOMWINDOW == 11 (0x821245c) [serial = 13] [Outer = (nil)]
--DOMWINDOW == 10 (0x82122ac) [serial = 12] [Outer = 0x8211500]
JavaScript error: file:///home/mmokrejs/public_html/tmp/testcase-bug266789.html, line 8: Permission denied to get property XPCComponents.classes
[Thread 0xb1b7eb90 (LWP 15191) exited]

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 314876 [details] [diff] [review]
printf

r/sr dveditz

Comment 19

[timeless]

Comment on attachment 314876 [details] [diff] [review]
printf

not using the request model is asking for scary crashes

Comment 20

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

Can I get a risk/reward statement for approval decision, please?

Comment 21

[Brian Crowder]

The printf patch looks very low-risk to me -- but I don't really understand timeless remark about "request models" there.  Perhaps that was intended for the "request" attachment (which perhaps has moved to another bug?)

Comment 22

[Brendan Eich [:brendan]]

Timeless: nsScriptSecurityManager.cpp's SetPendingException does use a request:

inline void SetPendingException(JSContext *cx, const char *aMsg)
{
    JSAutoRequest ar(cx);
    JS_ReportError(cx, "%s", aMsg);
}

What did you mean?

Risk looks very low, reward is clear diagnostics (see comment 17) for devs. Worth it in my book, although timeless should comment to remove any FUD sown by his last comment :-/.

/be

Comment 23

[Damon Sicore (:damons)]

Timeless, can you please address your comment 17 .  Seems to be scaring people (like me). 

Re-request approval once addressed.

Comment 24

[timeless]

Comment on attachment 314876 [details] [diff] [review]
printf

the *current* code calls JS_ReportError which expected to be called within a request, and is not. The change is simply putting the api *required* request model into use.

Comment 25

[Mike Schroepfer]

Comment on attachment 314876 [details] [diff] [review]
printf

only taking zero risk (e.g. css) or showstopper issues at this point.  If this
is one of those please re-nom and let me know.

Comment 26

[Johnny Stenback (:jst)]

Attachment: Fix for nsDirectoryViewer.cpp

I'm not sure this fixes *the* problem here, but it certainly should fix the problem mentioned in comment #13.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 320252 [details] [diff] [review]
Fix for nsDirectoryViewer.cpp

Shouldn't this be for bug 429123 ?

Comment 28

[Johnny Stenback (:jst)]

Yeah... Should we dupe this, or is there more to this bug than fixing nsDirectoryViewer.cpp?

Comment 29

[timeless]

Comment on attachment 320252 [details] [diff] [review]
Fix for nsDirectoryViewer.cpp

one bug per problem. don't hijack my bugs. do get my patches approved.

Comment 30

[Johnny Stenback (:jst)]

Then do tell us what this bug is about, and why it matters, and how to trigger it.

Comment 31

[georgi - hopefully not receiving bugspam]

this bug was filed by me.
the testcase is attachment neterr1b.html

i don't know if the bug matters, i thought it was usual to file bugs on assertions like the one in comment #0

Comment 32

[Phil Ringnalda (:philor)]

timeless: you have the most confusing stable of bugs ever. For some reason you have r+r+ here, is that r+sr, and this is ready to land and only didn't land at the time because it was during the period when there was no trunk, only a trunk pretending to be a branch that required approval?

Comment 33

[timeless]

Comment on attachment 314876 [details] [diff] [review]
printf

i'm not sure how/why dveditz managed r+r+ instead of r+sr.

but yes, this still needs fixing. and since sr is not technically equivalent to r, i don't believe in carrying it over :).

and as to how it got lost, between the strange review state i described here and the strange tree state you described above, yep, that about sums it up :).

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 314876 [details] [diff] [review]
printf

Let's get this in sooner rather than later.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/401c215ae146.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 314876 [details] [diff] [review]
printf

I think we should take this on both branches.

Comment 37

[Boris Zbarsky [:bzbarsky]]

This patch failed tests, which I should have realized would happen.  nsPrintfCString by default only creates strings of length at most 15; this patch ends up truncating exception messages.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/091e0fa0fa7a to fix that.

Comment 39

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

Comment on attachment 314876 [details] [diff] [review]
printf

a191=beltzner

Comment 40

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

(and make sure to take the followup test!)

Comment 41

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/82073d327d4c (which includes the test fix).

Comment 42

[Daniel Veditz [:dveditz]]

Comment on attachment 314876 [details] [diff] [review]
printf

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 43

[Boris Zbarsky [:bzbarsky]]

Checking in caps/src/nsScriptSecurityManager.cpp;
/cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v  <--  nsScriptSecurityManager.cpp
new revision: 1.363; previous revision: 1.362
done

Comment 44

[Aakash Desai [:aakashd]]

verified FIXED on debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090629 Minefield/3.6a1pre ID:20090629082126

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre)
Gecko/20090629 Shiretoko/3.5pre ID:20090629082025

Comment 45

[Martin Stránský [:stransky] (ni? me)]

Does not seem to affect 1.8.