758224 - bug in nsPluginStreamListenerPeer dtor, pushing nearly-invalid address to an outside reference

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

Attachment: fix v1.0

294 nsPluginStreamListenerPeer::~nsPluginStreamListenerPeer()
295 {
296 #ifdef PLUGIN_LOGGING
297   PR_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_NORMAL,
298          ("nsPluginStreamListenerPeer::dtor this=%p, url=%s\n",this, mURLSpec.get()));
299 #endif
300 
301   if (mPStreamListener) {
302     mPStreamListener->SetStreamListenerPeer(this);
303   }

We should be doing 'SetStreamListenerPeer(nsnull)', not setting it to 'this' from the dtor. Found this while writing another patch that exposes this bug via double-free.

Comment 1

[Josh Aas]

pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/3a519ed51c90

Comment 2

[Josh Aas]

Comment on attachment 626807 [details] [diff] [review]
fix v1.0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 475991
User impact if declined: Possible cause of crashes.
Testing completed (on m-c, etc.): Should land on m-c today.
Risk to taking this patch (and alternatives if risky): Not risky.
String or UUID changes made by this patch: None

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/3a519ed51c90

Comment 4

[Alex Keybl [:akeybl]]

(In reply to Josh Aas (Mozilla Corporation) from comment #2)
> Comment on attachment 626807 [details] [diff] [review]
> fix v1.0
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Bug 475991
> User impact if declined: Possible cause of crashes.
> Testing completed (on m-c, etc.): Should land on m-c today.
> Risk to taking this patch (and alternatives if risky): Not risky.
> String or UUID changes made by this patch: None

Do we have an idea of speculative volume of crashes this would fix? If thought to be low, I think this can ride the trains given how old the regressing Bug 475991 is.

Comment 5

[Josh Aas]

(In reply to Alex Keybl [:akeybl] from comment #4)

> Do we have an idea of speculative volume of crashes this would fix? If
> thought to be low, I think this can ride the trains given how old the
> regressing Bug 475991 is.

I don't know for sure because it's not clear what the crash signature might be. I think the crash volume is probably low, possibly non-existent.

The patch is very low risk though, and it's the type of bug that concerns me quite a bit (promoting usage of a pointer to a deleted object).

The crash would probably happen here if it happened at all:

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#541

Comment 6

[Alex Keybl [:akeybl]]

Comment on attachment 626807 [details] [diff] [review]
fix v1.0

[Triage Comment]
Let's let this ride the trains if it's unclear how much impact this patch will have.