Closed Bug 1028132 Opened 7 years ago Closed 6 years ago

Fix all the temporarily whitelisted dangerous public destructors of refcounted classes

Categories

(Core :: General, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bjacob, Assigned: nika)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In bug 1027251 we removed all dangerous public destructors of XPCOM-refcounted classes outside of a finite whitelist, see HasDangerousPublicDestructor. Now we are going over the entries in this whitelist.

See blockers list.
Depends on: 1028134
Depends on: 1028136
Depends on: 1028139
Depends on: 1028140
Depends on: 1028141
Depends on: 1028142
Depends on: 1028143
Depends on: 1028144
Depends on: 1028146
Depends on: 1028147
Depends on: 1028148
Depends on: 1028588
Depends on: 1029137
Depends on: 1029138
Depends on: 1029139
Depends on: 1029140
Depends on: 1029141
Depends on: 1029150
Depends on: 1029151
Depends on: 1029478
Depends on: 1034906
Depends on: 1034907
Depends on: 1034909
Depends on: 1034910
Depends on: 1034911
Depends on: 1034912
Depends on: 1034913
Depends on: 1034914
Depends on: 1034915
Depends on: 1034916
Depends on: 1034917
Depends on: 1034918
Depends on: 1034919
Depends on: 1034920
Depends on: 1034921
Depends on: 1034922
Depends on: 1034923
Me or somebody else should do a little writeup here about how to fix these bugs.  The basic thing is to delete the whitelist block and make it compile, but it should also have a little bit of an explanation about what we're fixing, and common patterns that will need to be fixed.
Summary: Fix all the temporarily whitelisted dangerous public destructors of XPCOM-refcounted classes → Fix all the temporarily whitelisted dangerous public destructors of refcounted classes
Andrew: +1! Please do!
Depends on: 1036052
Depends on: 1036055
Depends on: 1036056
Depends on: 1036070
Here's an explanation of what the goal is of this bug I gave in bug 1034910:

> What are Reference-counted classes and why can't they have public destructors?

A reference counted class does not really use the normal fully manually memory management of C++.

Normally in C++, you'd say something like
Foo* x = new Foo();
// use x
// later, you know for sure you are done with x, so you destroy it.
delete x;

With a reference counted object, you aren't not as sure when it will go away, so you just have various places claiming that they hold the object alive.  They do this by calling an Addref() method (often hidden inside the nsRefPtr<> class) when they want to hold the object alive (this increases the reference count), and calling Release() method when they are done with it (this decreases the reference count).  If the reference count becomes 0 in Release(), then we delete the object.

The problem is that if the destructor is public, then any code can call delete on the object, even if the refcount is greater than 0.  This will cause any pointers to the object to end up pointing to dead objects, which can cause security problems when another object is allocated in the same space.

The purpose of this bug, and the other bugs blocking bug 1028132, is to make it less likely that some random code will delete an object with a nonzero reference count, by making the destructor protected.  There is some fancy C++ template stuff I don't know much about that enforces that, and that HasDangerousPublicDestructor<nsJSArgArray> class you deleted tells it to not produce an error for that.
The basic approach to fixing a class Foo is:
1. Remove the HasDangerousPublicDestructor<Foo> declaration.
2. Make the destructor of Foo either private or protected.  If there isn't one, then it is using the default one, so just create a new empty destructor and make it private.
3. Fix any compilation errors that come up from places that are trying to manually destroy an instance of Foo.

Some common patterns are:

a) raw pointer delete:
Foo* x = new Foo();
...
if (something failed) {
  delete x; // under some circumstance
  return NS_ERROR_FAILURE;
}
NS_ADDREF(*aReturn = x);

In this case, you want to turn the Foo* into an nsRefPtr<>.  That will addref x when it is created, and release x when it leaves the scope, so you can get rid of the delete.  Then you can change the NS_ADDREF line to x.forget(aReturn), and it will transfer x into the return value.  This requires a little bit of knowledge of how the Firefox ref counting works.

b) stack-allocated object
In this case, the object is allocated directly on the stack:
Foo x(a,b);
When x goes out of scope, it will have its destructor called directly, which is not good.
To fix this, change it to
nsRefPtr<Foo> x = new Foo(a, b);
x will still be destroyed when it goes out of scope (probably), it will just be allocated on the heap.
x is now a pointer, not a direct reference, so you'll have to add pointer indirection to the uses of x, which the compiler should tell you about.

For some examples, check out the bugs blocking this one that have a line through them, as they have had fixes landed.
This is great, thanks.
Another issue: if you convert a field mField of an object from Foo to nsRefPtr<Foo>, you need to make sure that mField is initialized to a new instance. So you have to add something like:
  mFoo(new Foo())
to the constructor.
Depends on: 1073210
Depends on: 1073219
We're down to only a handful of these in the tree.  I filed two more just now, but they are not new instances of it, just a few cases that slipped between the cracks in the initial bugstorm.
Thanks a lot to all the contributors who have been helping and Andrew for mentoring them and fixing particularly nontrivial ones yourself!
When my patches for bug 1034922 and bug 1073219 land, HasDangerousPublicDestructor<T> will no longer be necessary.

This patch removes HasDangerousPublicDestructor<T>.
Attachment #8630829 - Flags: review?(continuation)
Comment on attachment 8630829 [details] [diff] [review]
Remove mozilla::HasDangerousPublicDestructor<T>

Review of attachment 8630829 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  Thanks for finishing this off.
Attachment #8630829 - Flags: review?(continuation) → review+
Please land after bug 1073219
Keywords: checkin-needed
Assignee: nobody → michael
https://hg.mozilla.org/mozilla-central/rev/576d817e46ba
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1183729
You need to log in before you can comment on or make changes to this bug.