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

RESOLVED FIXED in mozilla15

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 626807 [details] [diff] [review]
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.
Attachment #626807 - Flags: review?(benjamin)
Attachment #626807 - Flags: review?(benjamin) → review+
(Assignee)

Comment 1

5 years ago
pushed to mozilla-inbound

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

Updated

5 years ago
Blocks: 758227
(Assignee)

Comment 2

5 years ago
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
Attachment #626807 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3a519ed51c90
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
(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

5 years ago
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.
Attachment #626807 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.