bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

Crash [@ NS_GetInnermostURI] from NS_EnsureSafeToReturn with broken nsIURI.clone

RESOLVED FIXED in mozilla1.9.3a5



9 years ago
7 years ago


(Reporter: morac, Assigned: timeless)



Windows XP
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [ccbr], crash signature)


(1 attachment)



9 years ago
When trying to update the nsSessionStore component to allow restoring of referrer URIs, I incorrectly overwrote a tab's nsISHEntry referrerURI with an object instead of an instance of a nsiURI.  See bug 491168 comment #3.

This resulted in a crash when session store tried to restore the browser history with the following command:

Here's the crash report:

NS_GetInnermostURI (or NS_SecurityHashURI) should probably check to see if the uri is valid before trying to address it.

Comment 1

9 years ago
That's about what happens, even though it crashes somewhere else for me (evaluate the following line as a whole in the Error Console):

const Cc = Components.classes, Ci = Components.interfaces; var shEntry = Cc["@mozilla.org/browser/session-history-entry;1"].createInstance(Ci.nsISHEntry); var ioService = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService); shEntry.setURI(ioService.newURI("about:", null, null)); shEntry.setTitle("about:"); shEntry.loadType = Ci.nsIDocShellLoadInfo.loadHistory; shEntry.referrerURI = { clone: function() { } }; var wn = Application.windows[0]._window.getWebNavigation(); var sh = wn.sessionHistory; if (sh.count) sh.PurgeHistory(sh.count); sh.QueryInterface(Ci.nsISHistoryInternal); sh.addEntry(shEntry, true); wn.gotoIndex(0);

Comment 2

9 years ago
so, I *believe* the bug belongs in nsPrincipal::GetHashValue.

NS_GetInnermostURI does *not* want to do a null check for performance reasons. 

NS_SecurityHashURI can't return anything useful for null, so it shouldn't be asked to handle null either.

That leaves:
nsScriptSecurityManager::HashPrincipalByOrigin and
Assignee: nobody → dveditz
Component: Networking → Security: CAPS
Keywords: crash
QA Contact: networking → caps
Summary: Crash [@ NS_GetInnermostURI(nsIURI*) ] with invalid nsiURI → Crash [@ NS_GetInnermostURI - NS_SecurityHashURI - nsScriptSecurityManager::HashPrincipalByOrigin - nsPrincipal::GetHashValue] with unusual nsIURI
Uh...  Isn't the bug in whoever creates a bogus nsPrincipal object here?  There is no way to create an nsPrincipal with a null URI via our public APIs, to my knowledge.  If that's what we have here, how did we end up with one?

Or put another way, I think this might well be a session restore bug.  Or _possibly_ an nsSHEntry one.

Comment 4

9 years ago
(In reply to comment #3)
Or even an XPCOM bug for letting us pass a JS object instead of the nsIURI required by the IDL?
Any scriptable interface can be implemented by arbitrary JS objects.  XPConnect synthesizes an nsIFoo pointer as needed.  If you've ever implemented an interface in JS... this is why.
Trying to reproduce using Michael's broken patch from bug 491168 and a 1.9.1 branch build doesn't result in a crash for me, but I do hit a bunch of failures:

[Exception... "'JavaScript component does not have a method named: "clone"' when calling method: [nsIURI::clone]"  nsresult: "0x80570030 (NS_ERROR_XPC
_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: file:///C:/moz/mozilla-1.9.1/ff-dbg/dist/bin/components/nsSessionStore.js :: sss_restoreHist
ory :: line 1909"  data: no]

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80570030: file c:/moz/mozilla-1.9.1/netwerk/base/src/nsIOService.cpp, line 925
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80570030: file c:\moz\mozilla-1.9.1\ff-dbg\dist\include\content\nsContentPolicyUtils.h, line 219
WARNING: NS_ENSURE_TRUE(mMutable) failed: file c:/moz/mozilla-1.9.1/netwerk/base/src/nsSimpleURI.cpp, line 224

The test case in comment 1 crashes in nsPrincipal::Init() under nsScriptSecurityManager::GetCodebasePrincipal() called from InternalLoad(), because NS_TryToMakeImmutable(aCodebase); returns null (since the object's clone() returns undefined, presumably).

I suppose the nsISHEntry referrerURI setter chould check for a bogus nsIURI object somehow, but that seems like it would be error-prone and not actually useful in practice, and it seems like most of the rest of the code involved doesn't want to check for perf reasons. WONTFIX?
Uh..  NS_TryToMakeImmutable is guaranteed to never return null on non-null input.  How did it manage to do that?  For the case when clone() throws, I'd expect NS_EnsureSafeToReturn to return an error nsresult, so nsIOService::ToImmutableURI returns an error nsresult, so NS_TryToMakeImmutable returns the passed-in URI.

Or did a null pointer get passed to NS_TryToMakeImmutable?
clone in that example doesn't throw, though, it just returns undefined. Doesn't that get converted into NS_OK-with-null-return?
Oh, that example.  That's violating the clone() contract for nsIURI; you'd get the same thing with a buggy non-JS implementation.  I don't think we want to be guarding against that.


9 years ago
Assignee: dveditz → nobody
Component: Security: CAPS → Networking
QA Contact: caps → networking


9 years ago
Summary: Crash [@ NS_GetInnermostURI - NS_SecurityHashURI - nsScriptSecurityManager::HashPrincipalByOrigin - nsPrincipal::GetHashValue] with unusual nsIURI → Crash [@ NS_GetInnermostURI] from NS_EnsureSafeToReturn with broken nsIURI.clone

Comment 10

9 years ago
Created attachment 375714 [details] [diff] [review]
this is about the only fix that would make sense

i'm not saying i agree with this or that we should do it.

However, I think that a function which says "EnsureSafeToReturn" should seriously consider providing only safe output for what a caller thought was safe input.

Currently the fact that we crashed so *very* far from where the cause was bothers me.

I don't mind a null pointer deref 3 lines away from a culprit, but this isn't even in the same ballpark.
Attachment #375714 - Flags: review?(cbiesinger)
Maybe only an assertion in debug builds if this will hurt perf? Just my .02 :)
Whiteboard: [ccbr]
Comment on attachment 375714 [details] [diff] [review]
this is about the only fix that would make sense

I'm OK with this if you really want to, but since this bug report requires fairly unusual steps to reproduce I don't really see the point.
Attachment #375714 - Flags: review?(cbiesinger) → review+
Assignee: nobody → timeless
Last Resolved: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Crash Signature: [@ NS_GetInnermostURI]
You need to log in before you can comment on or make changes to this bug.