343850 - Crash [@ nsHTMLSharedObjectElement::StartObjectLoad] [@ nsObjectLoadingContent::LoadObject]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jesse Ruderman]

Loading the testcase makes Firefox crash.  Based on some stack traces having random addresses on top, this is [sg:critical].

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Attachment: stack trace (mac debug in gdb) for a related testcase

This is why I said this is [sg:critical].  The testcase I attached is simpler and usually makes it look like a null deref.

Comment 3

[Daniel Veditz [:dveditz] out until Nov 10]

Wow, first time I've ever tripped WinXP's Data Execution Protection from one of these memory corruption crashes. I was beginning to think DEP didn't work :-)

If you're getting different crashes maybe the "related testcase" should be attached as well? Getting stopped by DEP is plenty evidence that *this* testcase is exploitable, I just worry maybe there's a second bug hiding in your other testcase.

Didn't crash in Bon Echo, is this a 1.9-only regression?

Comment 4

[Mats Palmgren (inactive)]

Attachment: trace

Comment 5

[Mats Palmgren (inactive)]

We save a raw pointer to the channel here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/src/nsObjectLoadingContent.cpp&rev=1.33&root=/cvsroot&mark=977#926

This doesn't work well with nsJSChannel:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp&rev=1.128&root=/cvsroot&mark=534,546,598#529

which only temporarily is part of the load group, but later "delegates" the
request to its |mStreamChannel|. The problem with this is that there is no
strong ref on the nsJSChannel object itself so when |chan| goes out of
scope in nsObjectLoadingContent::LoadObject it is destroyed and |mChannel|
is a stray pointer.

Comment 6

[Mats Palmgren (inactive)]

Attachment: wip

I don't know this code very well, but this fixes the crash.
Instead of saving |mChannel| in LoadObject() we do it in
OnStartRequest() where the |aRequest| is the |mStreamChannel|
that nsJSChannel delegated the request to.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Get rid of 'mChannel', this should make the code more robust I think.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

I really don't like this fix... what's wrong with the existing code?

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, missed comment 5. Isn't that a bug in the channel implementation?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, this is a bug in the channel impl, as I understand the channel API.  There should be an OnChannelRedirect notification sent here or something...

That said, nsIChannel doesn't explicitly say that the channel itself is passed to the On*Request methods of the nsIStreamListener, does it?  It probably should.

Comment 11

[Darin Fisher]

nsJSChannel does lots of things to violate the nsIChannel API, like calling nsILoadGroup::RemoveRequest from AsyncOpen :-(

Comment 12

[Boris Zbarsky [:bzbarsky]]

Right.  I'm saying we should fix those.  And document more clearly that these _are_ violations of the API.

Comment 13

[Darin Fisher]

> Right.  I'm saying we should fix those.  And document more clearly that these
> _are_ violations of the API.

I tried once, and it caused loads of regressions.  The problem is that javascript: evaluation can cause nsDocShell::LoadURI to be called, and evaluation has to happen during AsyncOpen for some reason.  There's really not much hope of implementing a sane nsJSChannel IMO :-/  It almost seems like javascript: shouldn't be implemented as a nsIChannel.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Evaluation "has" to happen during AsyncOpen because script expects setting something's URI to javascript: to synchronously run the script.  :(

If we can make it not be an nsIChannel, that would be great, but the problem is that it can return data that should then be loaded....

All that said, maybe we can make this suck a little less in a slightly less painful way.  Lemme give it a shot.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Attempted fix to nsJSChannel

Better document the requirements on nsIChannel and make nsJSChannel closer to those by having it be the listener for the stream channel.  The one issue I see here is mention of the unfrozen nsIChannelEventSink interface in nsIChannel.  I'm really not sure how to document the expected behavior without that, though.  :(

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 228601 [details] [diff] [review]
Patch rev. 2

I'll mark this r- for now because I don't think this is the way to go.

Comment 17

[Martijn Wargers (dead)]

I don't crash on the testcase with a recent 1.8.0.5 build nor with a recent 1.8.1 build.

Comment 18

[Darin Fisher]

Comment on attachment 229388 [details] [diff] [review]
Attempted fix to nsJSChannel

"If the notification callbacks do not provide an nsIChannelEventSink" ... you should probably also mention the nsILoadGroup's notification callbacks to be clear.

This patch makes sense to me.  r=darin

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 229388 [details] [diff] [review]
Attempted fix to nsJSChannel

sr=jst

Comment 20

[Boris Zbarsky [:bzbarsky]]

Changed that paragraph to:

     * If the channel's and loadgroup's notification callbacks do not provide
     * an nsIChannelEventSink when onChannelRedirect would be called, that's
     * equivalent to having called onChannelRedirect.

and fixed on trunk.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

sorry for commenting late, but:
+    // Make sure to drop our ref to mListener
+    nsCOMPtr<nsIStreamListener> listener;
+    listener.swap(mListener);
+    
+    NS_ENSURE_TRUE(aRequest == mStreamChannel, NS_ERROR_UNEXPECTED);

shouldn't the NS_ENSURE_TRUE be above the listener swapping?

Comment 22

[Boris Zbarsky [:bzbarsky]]

No, that's where the "Make sure to drop our ref to mListener" thing comes in...  Though we probably _should_ null-check |listener| before calling things on it.  I'll check in that change.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Actually, I guess if we want this to work in the face of someone maliciously calling random methods on the nsJSChannel while it's loading we do need to change the order...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Made the order change.

Comment 25

[Jesse Ruderman]

Crashtest checked in.