199910 - should HTTP null-check prompt callbacks?

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just crashed due to http://www.w3.org/Style/Group/members.html , which has an
<object type="text/html"/> whose data requires HTTP authentication.  The
mCallbacks on the HTTP channel were null.

It seems like we should either:
 * assert that the callbacks are non-null somewhere so that we'll catch this
kind of thing even when there isn't HTTP authentication
 * make the necessary null-checks and noisily (but without crashing) fail when
they're null.

Crash stack:

#3  <signal handler called>
#4  0x41126d40 in nsHttpChannel::GetCallback(nsID const&, void**) (
    this=0x83d1fd0, aIID=@0x83a7ad8, aResult=0x50)
    at /builds/trunk/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:756
#5  0x4112c1ee in nsHttpChannel::PromptForIdentity(char const*, int, int, char
const*, char const*, unsigned, nsHttpAuthIdentity&) (this=0x83d1fd0,
    host=0x83a7ad8 "cgi.w3.org", port=80, proxyAuth=0,
    realm=0xbfffec30 "W3C-Member", scheme=0xbfffecb0 "basic", authFlags=13,
    ident=@0x83d20c4)
    at /builds/trunk/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:2036
#6  0x4112b878 in nsHttpChannel::GetCredentials(char const*, int,
nsAFlatCString&) (this=0x83d1fd0, challenges=0x83b68c0 "Basic realm=\"W3C-Member\"",
    proxyAuth=0, creds=@0xbfffed90)
    at /builds/trunk/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:1879
#7  0x4112b084 in nsHttpChannel::ProcessAuthentication(unsigned) (
    this=0x83d1fd0, httpStatus=401)
    at /builds/trunk/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:1748
#8  0x41126849 in nsHttpChannel::ProcessResponse() (this=0x83d1fd0)
    at /builds/trunk/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:684
#9  0x4112f981 in nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (
    this=0x83d1fd0, request=0x81fd1c8, ctxt=0x0)
    at /builds/trunk/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:2970
#10 0x410a2737 in nsInputStreamPump::OnStateStart() (this=0x81fd1c8)
    at /builds/trunk/mozilla/netwerk/base/src/nsInputStreamPump.cpp:350
#11 0x410a25d9 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (
    this=0x81fd1c8, stream=0x83a7a44)
    at /builds/trunk/mozilla/netwerk/base/src/nsInputStreamPump.cpp:318
#12 0x403f2b49 in nsInputStreamReadyEvent::EventHandler(PLEvent*) (
    plevent=0x82f4544) at /builds/trunk/mozilla/xpcom/io/nsStreamUtils.cpp:116
#13 0x40416a91 in PL_HandleEvent (self=0x82f4544)
    at /builds/trunk/mozilla/xpcom/threads/plevent.c:659
#14 0x404168c3 in PL_ProcessPendingEvents (self=0x8246280)
...

(gdb) frame 4
#4  0x41126d40 in nsHttpChannel::GetCallback(nsID const&, void**) (
    this=0x83d1fd0, aIID=@0x83a7ad8, aResult=0x50)
    at /builds/trunk/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:756
756         rv = mCallbacks->GetInterface(aIID, aResult);
(gdb) p mCallbacks
$1 = {mRawPtr = 0x0}


I'll file a separate bug on the fact that they're null.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Bug 199911 covers the fact that they're null in this case.)

Comment 2

[Darin Fisher]

sounds like a regression.  i think we used to null check mCallbacks.

-> suresh

Comment 3

[Suresh]

Attachment: patch.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 118985 [details] [diff] [review]
patch.

This doesn't do any good.  A real null-check here would fix the crash.	It also
might be good to assert, but somewhere else, so we catch the problem with any
testing in a DEBUG build rather than just testing that hits HTTP auth.

Comment 5

[Darin Fisher]

Comment on attachment 118985 [details] [diff] [review]
patch.

>+    NS_ASSERTION(mCallbacks, "Oops, no interface requestor!");

NS_ASSERTION is debug only.  this doesn't help release builds.
NS_ENSURE_TRUE would be better.

Comment 6

[Suresh]

Attachment: updated patch

Comment 7

[Darin Fisher]

Comment on attachment 118996 [details] [diff] [review]
updated patch

>+    NS_ENSURE_TRUE(mCallbacks, NS_ERROR_NOT_INITIALIZED);

suresh: this is fine, but i was just looking at the callers
of GetCallback to see how they handle errors.  it seems that
PromptTempRedirect() in particular will return NS_OK if
GetCallback returns NS_OK and a null nsIPrompt impl.  that
is a bit troublesome because it means that the user was
prompted.  from a security point of view, that is probably
bad behavior.  we shouldn't redirect unless the user was
prompted and said ok.  i think it would be good to make
GetCallback ensure a non-null return value.

in particular, we had a topcrash in N7.0 resulting from some
commercial add-on that incorrectly implemented GetInterface.
it would return NS_OK and a null result.  so, it would probably
be a good idea to add the extra null check in GetCallback (and
eliminate it from PromptTempRedirect).

if you could tack on that change to this patch it would be great!
thanks!

Comment 8

[Suresh]

Attachment: updated patch

Comment 9

[Darin Fisher]

Comment on attachment 119012 [details] [diff] [review]
updated patch

>Index: nsHttpChannel.cpp

>+    if (!*aResult)
>+        return NS_ERROR_NO_INTERFACE;
>+
>     return rv;
> }

hmm.. couple problems with this:

1- what if *aResult is passed into GetCallback uninitialized?  currently, that
isn't a problem because we always use nsCOMPtr with this function, but this
function shouldn't be so fragile.

2- suppose *aResult is null because some function failed.  we should return the
|rv| which explains that.

i think this would be better:

  // defend against bad nsIInterfaceRequestor implementations.
  if (NS_SUCCEEDED(rv) && !*aResult)
      rv = NS_ERROR_NO_INTERFACE;

Comment 10

[Suresh]

Attachment: updated patch

Comment 11

[Darin Fisher]

Comment on attachment 119095 [details] [diff] [review]
updated patch

sr=darin

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 119095 [details] [diff] [review]
updated patch

r=dougt

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 119095 [details] [diff] [review]
updated patch

fix up the spacing here:

@@ -786,11 +792,9 @@ nsHttpChannel::PromptTempRedirect()
	 nsCOMPtr<nsIPrompt> prompt;
	 rv = GetCallback(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt));
	 if (NS_FAILED(rv)) return rv;
-	 if (prompt) {
	     prompt->Confirm(nsnull, messageString, &repost);
	     if (!repost)
		 return NS_ERROR_FAILURE;
-	 }
     }

Comment 14

[Suresh]

fixed in trunk.

Comment 15

[benc]

Is opening the page enough to cause a crash? This behaved consistently for me w/
mozilla 1.3f & 1.4b (pre- and post- checkin milestones).

Comment 16

[Boris Zbarsky [:bzbarsky]]

This patch caused regression bug 208087 (because the early return keeps us from
asking the loadgroup for its notification callbacks).