Closed
Bug 470804
Opened 16 years ago
Closed 15 years ago
crash [@ NS_GetInnermostURI - nsScriptSecurityManager::CheckLoadURIWithPrincipal]
Categories
(Core :: Security: CAPS, defect)
Core
Security: CAPS
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b4
People
(Reporter: wsmwk, Assigned: timeless)
References
Details
(Keywords: crash, fixed1.9.1, topcrash, Whiteboard: [tb3needs])
Crash Data
Attachments
(2 files)
638 bytes,
patch
|
bzbarsky
:
review+
dveditz
:
superreview+
benjamin
:
approval1.9.1+
|
Details | Diff | Splinter Review |
2.11 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
topcrash 10 crasher for Thunderbird 3.0b1 security:caps? Doesn't appear much in nightlies, but does in the a1-3 and b1 releases. none have comments check the graph at http://crash-stats.mozilla.com/report/list?product=Thunderbird&version=Thunderbird%3A3.0a1&version=Thunderbird%3A3.0a2&version=Thunderbird%3A3.0a3&version=Thunderbird%3A3.0b1&platform=windows&query_search=signature&query_type=contains&query=NS_GetInnermostURI&date=&range_value=4&range_unit=weeks&do_query=1&signature=NS_GetInnermostURI%28nsIURI*%29 bp-d298ebd7-630a-46bb-9cce-0487b2081220 NS_GetInnermostURI nsNetUtil.h:1412 nsScriptSecurityManager::CheckLoadURIWithPrincipal caps/src/nsScriptSecurityManager.cpp:1321 NS_InvokeByIndex_P xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101 XPCWrappedNative::CallMethod js/src/xpconnect/src/xpcwrappednative.cpp:2263 XPC_WN_CallMethod js/src/xpconnect/src/xpcwrappednativejsops.cpp:1477 js_Invoke js/src/jsinterp.cpp:1313
Comment 1•15 years ago
|
||
I'm wondering if there are any extensions installed - from what I can tell, no one calls CheckLoadURIWithPrincipal from js.
Comment 2•15 years ago
|
||
Content context menu does, http://mxr.mozilla.org/comm-central/source/mail/base/content/nsContextMenu.js#291 (as does contentAreaUtils.js which viewSourceUtils.js includes, but I don't see it hitting that). It's the sort of thing I often miss, but I don't quite see what you could pass to CheckLoadURIWithPrincipal to make it crash getting the innermost URI.
Component: Account Manager → General
QA Contact: account-manager → general
Comment 3•15 years ago
|
||
ah, I was just looking in the comm-central mxr, which doesn't include toolkit, I guess. I think passing null would make it crash. I think NS_PRECONDITION does nothing in release builds.
this is a bug in caps. the method is scriptable it needs to handle null
Assignee: nobody → dveditz
Component: General → Security: CAPS
OS: Windows Vista → All
Product: Thunderbird → Core
QA Contact: general → caps
Hardware: x86 → All
Summary: crash [@ NS_GetInnermostURI(nsIURI*)] → crash [@ NS_GetInnermostURI - nsScriptSecurityManager::CheckLoadURIWithPrincipal]
Comment 6•15 years ago
|
||
Comment on attachment 354912 [details] [diff] [review] throw early sr=dveditz leaving r?bz on in case he wants to look, but this is pretty self-obvious. But maybe handling null in NS_GetInnermostURI more gracefully would be worth the extra compare if (!nestedURI && uri) before addref-ing. You'd still need this change, of course, because if you survived the NS_GetInnermostURI call you'd crash on targetBaseURI->GetScheme(). This patch is both necessary and sufficient to fix this bug, additional changes to NS_GetInnermostURI can be a separate issue. As long as you're in nsScriptSecurityManager, are there any other scriptable APIs that aren't sanity checking? checkLoadURI() doesn't check target, but falls through to this one. checkLoadURIStr/checkLoadURIStrWithPrincipal look OK, as does getCodebasePrincipal. getChannelPrincipal() is scriptable and will crash on a null channel. "Don't do that" springs to mind, how robust do we want to be in the face of stupid XUL code? isSystemPrincipal() can't handle a null return param, but that can't happen from script.
Attachment #354912 -
Flags: superreview+
Comment 7•15 years ago
|
||
Comment on attachment 354912 [details] [diff] [review] throw early Looks good. As far as NS_GetInnermostURI, I think we want to leave it as-is. It's used in some performance-critical cases (and _does_ assert if passed unexpected input).
Attachment #354912 -
Flags: review?(bzbarsky) → review+
Comment 8•15 years ago
|
||
This does a nice job of crashing without the patch and not with, but I'm not really sure I'm not making bad assumptions about things like any exception being fine, or what principal I should pass to get as close as possible without crashing.
Attachment #355011 -
Flags: review?(bzbarsky)
Comment 9•15 years ago
|
||
Comment on attachment 355011 [details] [diff] [review] Crashtest Any non-system principal will do, so just do a check that the principal is not the system principal? r=bzbarsky with that change.
Attachment #355011 -
Flags: review?(bzbarsky) → review+
Comment 10•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/eb870a41e5cb http://hg.mozilla.org/mozilla-central/rev/59c1cdb2cf7e
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Comment 11•15 years ago
|
||
Comment on attachment 354912 [details] [diff] [review] throw early wsm says this is a top crash on 1.9.1, it has a test and is tiny and correct, could we push to the branch?
Attachment #354912 -
Flags: approval1.9.1?
Reporter | ||
Comment 12•15 years ago
|
||
(In reply to comment #11) > (From update of attachment 354912 [details] [diff] [review]) > wsm says this is a top crash on 1.9.1, it has a test and is tiny and correct, > could we push to the branch? indeed, this needs to go out with the next beta, 3.0b3. #9 crasher for 3.0b2+3.0b3pre
Whiteboard: [tb3needs]
Comment 13•15 years ago
|
||
(In reply to comment #11) > (From update of attachment 354912 [details] [diff] [review]) > wsm says this is a top crash on 1.9.1, it has a test and is tiny and correct, > could we push to the branch? Yes.
Updated•15 years ago
|
Attachment #354912 -
Flags: approval1.9.1? → approval1.9.1+
Comment 15•15 years ago
|
||
Comment on attachment 354912 [details] [diff] [review] throw early a=me ... please land the test on the branch as well
Comment 16•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/79b969c3d4fa http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3b6c1a8033a4
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b4
Reporter | ||
Comment 17•15 years ago
|
||
v.fixed - don't see any crashes after 3.0b2 20090408014204
Status: RESOLVED → VERIFIED
Updated•13 years ago
|
Crash Signature: [@ NS_GetInnermostURI - nsScriptSecurityManager::CheckLoadURIWithPrincipal]
You need to log in
before you can comment on or make changes to this bug.
Description
•