Closed
Bug 984786
Opened 11 years ago
Closed 10 years ago
Make a bunch of classes with NS_INLINE_DECL_REFCOUNTING/NS_INLINE_DECL_THREADSAFE_REFCOUNTING less footgun-ish (w/ MOZ_FINAL & non-public destructor)
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla31
Tracking | Status | |
---|---|---|
firefox-esr31 | --- | wontfix |
People
(Reporter: dholbert, Assigned: dholbert)
References
(Depends on 1 open bug)
Details
(Keywords: sec-audit)
Attachments
(8 files)
(deleted),
patch
|
dbaron
:
review+
dholbert
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tbsaunde
:
review+
dholbert
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bent.mozilla
:
review+
dholbert
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
dholbert
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kinetik
:
review+
dholbert
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
dholbert
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
u408661
:
review+
dholbert
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bjacob
:
review+
dholbert
:
checkin+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Group: core-security
Assignee | ||
Comment 1•11 years ago
|
||
(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.)
Comment 2•11 years ago
|
||
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...)
Fwiw I've started requiring private destructors for refcounted objects in my reviews because people misuse them and assign to nsAutoPtr, manually delete, etc.
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
(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)
Assignee | ||
Comment 7•11 years ago
|
||
(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.]
Assignee | ||
Comment 9•11 years ago
|
||
(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
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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+
Comment 12•11 years ago
|
||
(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.)
Assignee | ||
Comment 13•11 years ago
|
||
Yeah, "ensure" might be too strong of a word. How about: // Private destructor, to prevent accidental deletion outside of Release():
Comment 14•11 years ago
|
||
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.
Assignee | ||
Comment 15•11 years ago
|
||
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.)
Assignee | ||
Comment 16•11 years ago
|
||
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]
Assignee | ||
Updated•11 years ago
|
Group: core-security
Updated•11 years ago
|
Keywords: leave-open
Whiteboard: [leave open]
Assignee | ||
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
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+
Assignee | ||
Comment 20•11 years ago
|
||
Landed part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/82ed5fd863e8
Assignee | ||
Updated•11 years ago
|
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)
Assignee | ||
Comment 22•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8400389 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 25•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8400398 -
Flags: review?(hurley)
Updated•11 years ago
|
Attachment #8400358 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8393988 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #8400277 -
Flags: checkin+
Assignee | ||
Comment 26•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8400370 -
Flags: review?(bugs) → review+
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/82ed5fd863e8 https://hg.mozilla.org/mozilla-central/rev/90b50be3a6e1
Updated•11 years ago
|
Attachment #8400391 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 28•11 years ago
|
||
Landed part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed66b2c115a5 and part 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/91a6fdfb2ad1 and part 6: https://hg.mozilla.org/integration/mozilla-inbound/rev/793324f7c684
Assignee | ||
Updated•11 years ago
|
Attachment #8400370 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #8400389 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #8400391 -
Flags: checkin+
Attachment #8400398 -
Flags: review?(hurley) → review+
Assignee | ||
Comment 29•11 years ago
|
||
Comment on attachment 8400398 [details] [diff] [review] part 7: /netwerk Landed part 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/e52d5c70955e
Attachment #8400398 -
Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/ed66b2c115a5 https://hg.mozilla.org/mozilla-central/rev/91a6fdfb2ad1 https://hg.mozilla.org/mozilla-central/rev/793324f7c684 https://hg.mozilla.org/mozilla-central/rev/e52d5c70955e
Comment 32•11 years ago
|
||
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+
Assignee | ||
Comment 33•11 years ago
|
||
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)
Comment 34•11 years ago
|
||
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.
Assignee | ||
Comment 35•11 years ago
|
||
Comment on attachment 8401027 [details] [diff] [review] part 8: /content/canvas and /gfx Landed part 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/50e3d7132fa9
Attachment #8401027 -
Flags: checkin+
Assignee | ||
Comment 37•10 years ago
|
||
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: 10 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
Assignee | ||
Updated•10 years ago
|
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)
Assignee | ||
Comment 38•10 years ago
|
||
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
Comment 39•10 years ago
|
||
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.
Updated•10 years ago
|
status-firefox-esr31:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•