Make a bunch of classes with NS_INLINE_DECL_REFCOUNTING/NS_INLINE_DECL_THREADSAFE_REFCOUNTING less footgun-ish (w/ MOZ_FINAL & non-public destructor)

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Depends on 1 bug, {sec-audit})

Trunk
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox-esr31 wontfix)

Details

Attachments

(8 attachments)

Refcounted classes should generally
 (a) not be subclassed (unless we know what we're doing & have a virtual destructor, to make sure Release() reliably cleans things up)

 (b) be declared with a private destructor (so that it's a compile error to declare an instance of that variable, e.g. on the stack or as a member-var)  

(Read "private" as "protected" for classes who know what they're doing w.r.t. inheritance, per (a).)

From a quick MXR search, it looks like generally we don't bother with either (a) nor (b). This is Bad. We should fix this.

Filing this bug on doing that. Starting this out as security-sensitive to be on the safe side, since it seems possible this might turn up something dangerous, but probably/hopefully not.
Group: core-security
(Per bug summary, I'm scoping this just to classes that use NS_INLINE_DECL_REFCOUNTING for now, since there are lots of them. But really, this all should be true of any class with AddRef/Release, I think.)
Component: General → XPCOM
Keywords: sec-audit
I think for XPCOM classes these days we usually either make them final or give them a virtual dtor if they have subclasses, perhaps with a few exceptions.

Making the dtors private does more than just prevent creating objects of that type on the stack though, it will also make it impossible to delete them (which I think is fine...)
Willy-nilly deleting refcounted objects is a bad idea.
Fwiw I've started requiring private destructors for refcounted objects in my reviews because people misuse them and assign to nsAutoPtr, manually delete, etc.
Err, comment 2 is missing another paragraph which I thought I put in, but apparently not.  I meant to add, it's not exactly clear to me what Daniel is proposing to do here, so it's hard for me to say either yay or nay.  So comment 2 is basically a bunch of random thoughts on the issue.

(+1 to both comment 3 and 4, FWIW)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) (PTO on 3/20) from comment #5)
> it's not exactly clear to me what Daniel is
> proposing to do here,

I'm adding MOZ_FINAL and marking the destructor as private in a bunch of refcounted classes, to prevent problems like comment 4.

(You're right that this will make it impossible to delete them. If any such class really needs to be deleted by something other than ::Release, then it's worth forcing the person writing the code to think twice about that, and if they *really* need it, then they can make the destructor public or add a friend class or whatever)
(In reply to Daniel Holbert [:dholbert] from comment #6)
> I'm adding MOZ_FINAL and marking the destructor as private in a bunch of
> refcounted classes, to prevent problems like comment 4.

[...and also to increase the likelihood that future refcounted classes will be written with MOZ_FINAL & have a private destructor, due to their authors cribbing from other code with those features.]
That sounds like a good idea to me.  Thanks for doing this.
(I don't think this belongs in XPCOM -- moving it back to "General". I'm not intending to touch XPCOM code specifically; just miscellaneous refcounted classes (probably none that inherit from nsISupports) in various directories.)
Component: XPCOM → General
This patch fixes up all of the NS_INLINE_DECL_REFCOUNTING classes in layout, except for URLValue/ImageValue, which have special problems. I filed bug 984780 on those.
Attachment #8393988 - Flags: review?(dbaron)
Comment on attachment 8393988 [details] [diff] [review]
part 1: in layout, add private destructor and MOZ_FINAL to classes that use NS_INLINE_DECL_REFCOUNTING

r=dbaron
Attachment #8393988 - Flags: review?(dbaron) → review+
(private dtor of course doesn't ensure that the object is deleted only via Release.
 One can still call delete this; in some other method. So the comments in the patch are a bit misleading.)
Yeah, "ensure" might be too strong of a word.

How about:
   // Private destructor, to prevent accidental deletion outside of Release():
well, private: doesn't prevent that in the base class which has the refcounting defined.

So, to prevent accidental deletion outside the class which defined refcounting, or some such.
I think it's worth emphasizing that Release() is the only thing that we're expecting to do the deletion. (I also want to keep it short so that it fits on one line.)

Per IRC, I'll go with:
   // Private destructor, to discourage deletion outside of Release():

Try run: https://tbpl.mozilla.org/?tree=Try&rev=ac11ed17f840

(Patches for other areas of code likely coming over the next few days, particularly on my flight over the weekend.)
Landed first patch:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/cb183f7c1687

Also un-hiding -- I initially hid this (as mentioned @ end of comment 1) in case I ran across any classes where this fix wouldn't compile and revealed that something maybe-dangerous might be happening. But if I find anything like that, I'll just spin off specific maybe-hidden bugs to address them.
Whiteboard: [leave open]
Group: core-security
Keywords: leave-open
Whiteboard: [leave open]
There are 2 classes with NS_INLINE_DECL_REFCOUNTING in a11y.

The first (Notification) has subclasses, so this patch moves its existing virtual destructor to its "protected" section to prevent accidental deletion.

The second (SelData) doesn't have subclasses, so this patch marks it as MOZ_FINAL and gives it a trivial private destructor (again, to prevent accidental deletion/instantiation-on-the-stack).
Attachment #8400277 - Flags: review?(trev.saunders)
Comment on attachment 8400277 [details] [diff] [review]
part 2: in a11y, make destructor private/protected destructor to 2 classes, & MOZ_FINAL to one

I'm not super thrilled about adding comments all over just to help people who might copy stuff, but whatever.
Attachment #8400277 - Flags: review?(trev.saunders) → review+
Summary: Make classes with NS_INLINE_DECL_REFCOUNTING less footgun-ish → Make classes with NS_INLINE_DECL_REFCOUNTING/NS_INLINE_DECL_THREADSAFE_REFCOUNTING less footgun-ish (w/ MOZ_FINAL & private destructor)
Attachment #8400358 - Flags: review?(bent.mozilla)
Posted patch part 4: /domSplinter Review
Here's the patch for /dom.

Notes:
 - For the classes that I'm making MOZ_FINAL, I also convert any existing 'protected' to 'private', to make it more explicit that there are no subclasses. (e.g. in one case, the class used to have both "private" and "protected", which makes no sense given the lack of subclasses.)

 - Some of these classes already have MOZ_FINAL and/or a private destructor, which is why the patch only adds one or the other in a few places. (And some classes have subclasses, which means they don't get MOZ_FINAL -- and I made sure they have a virtual destructor, which makes refcounting them w/ subclasses OK.)

 - In cases where the class already has a "private"/"protected" block and a trivial-ish destructor, I'm just moving the destructor to the existing private/protected block. When there's no existing block, I generally just wrap the destructor in 'private: [...] public:' since that seemed cleanest to me Happy to adjust if you like.
Attachment #8400370 - Flags: review?(bugs)
Attachment #8400389 - Flags: review?(kinetik)
Attachment #8400391 - Flags: review?(ehsan)
Attachment #8400389 - Flags: review?(kinetik) → review+
Attachment #8400398 - Flags: review?(hurley)
Attachment #8400358 - Flags: review?(bent.mozilla) → review+
Attachment #8393988 - Flags: checkin+
Attachment #8400277 - Flags: checkin+
Comment on attachment 8400358 [details] [diff] [review]
part 3: fix 2 classes in /content subdirs

Landed part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/90b50be3a6e1
Attachment #8400358 - Flags: checkin+
Attachment #8400370 - Flags: review?(bugs) → review+
Attachment #8400391 - Flags: review?(ehsan) → review+
Attachment #8400370 - Flags: checkin+
Attachment #8400389 - Flags: checkin+
Attachment #8400391 - Flags: checkin+
Attachment #8400398 - Flags: review?(hurley) → review+
Attachment #8401027 - Flags: review?(bjacob)
Comment on attachment 8401027 [details] [diff] [review]
part 8: /content/canvas and /gfx

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

This is soooooo r+. Thanks for doing it!

Is there a c++ wizardry trick that would allow to guard that with an assertion or something?
Attachment #8401027 - Flags: review?(bjacob) → review+
Thanks for the review! And, I'm not sure what you're suggesting about an assertion. This bug's patches should already make it a compile error for anyone to do "MyRefcountedClass foo;" on the stack*, per comment 0 point (b) -- so, no assertion needed, because we catch it at compile time. :)

* (except in code with 'private' privileges for the class, e.g. methods / friend classes)
I meant an assertion ensuring that you won't have to go over and over again fix all of mozilla-central. That is, RefCounted<T> would static_assert that T is final and that its destructor is not public. I'm told offline that the former is type_traits is_final in C++11, but the latter is more distant on the horizon unfortunately, and that it wouldn't be possible to implement it with sfinae at all.
Depends on: 1016795
I'm going to close this out, since I'm not actively working on adding more patches at the moment, and nobody likes bugs w/ patches that land across large spans of time.

Subsequent patches to fix up issues of this sort can land in new bug(s).
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite-
Keywords: leave-open
Resolution: --- → FIXED
Summary: Make classes with NS_INLINE_DECL_REFCOUNTING/NS_INLINE_DECL_THREADSAFE_REFCOUNTING less footgun-ish (w/ MOZ_FINAL & private destructor) → Make a bunch of classes with NS_INLINE_DECL_REFCOUNTING/NS_INLINE_DECL_THREADSAFE_REFCOUNTING less footgun-ish (w/ MOZ_FINAL & private destructor)
Target Milestone: --- → mozilla31
Summary: Make a bunch of classes with NS_INLINE_DECL_REFCOUNTING/NS_INLINE_DECL_THREADSAFE_REFCOUNTING less footgun-ish (w/ MOZ_FINAL & private destructor) → Make a bunch of classes with NS_INLINE_DECL_REFCOUNTING/NS_INLINE_DECL_THREADSAFE_REFCOUNTING less footgun-ish (w/ MOZ_FINAL & non-public destructor)
I posted a PSA to dev.platform, to increase the likelihood that new code aligns with the changes that I've made here: https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/jMDAQtBzdCA
Bug 1027251 continues this work by addressing remaining cases outside of a finite, explicit whitelist. After that's landed, we'll be able to file bugs about those remaining explicit bad destructors and fix them one by one.
Depends on: 1028427
You need to log in before you can comment on or make changes to this bug.