Nice! So it sounds like we could use std::is_destructible to harden our refcounting-impl macros (like NS_INLINE_DECL_REFCOUNTING), by having those macros include a static_assert which enforces that they're only invoked by classes with private/protected destructors.
(I'll bet that this static_assert wouldn't transfer to subclasses - e.g. if class Foo has NS_INLINE_DECL_REFCOUNTING and Foo has a subclass Bar, I'll bet the static_assert would only check Foo, and not Bar. Fortunately, it's rare to have concrete refcounted classes with subclasses, so this shouldn't be a huge source of trouble.) For now, our code isn't clean enough for this sort of static_assert to be doable. :-/ And we have at least one instance of a refcounted class that's semi-intentionally (albeit carefully) declared on the stack: gfxContext, per https://bugzilla.mozilla.org/show_bug.cgi?id=742100 Still, the static_assert could be a good way of finding (in a local build) all the existing refcounted classes that want a non-public destructor, I suppose. ~Daniel On 05/28/2014 01:51 PM, Benoit Jacob wrote: > Awesome work! > > By the way, I just figured a way that you could static_assert so that at > least on supporting C++11 compilers, we would automatically catch this. > > The basic C++11 tool here is std::is_destructible from <type_traits>, > but it has a problem: it only returns false if the destructor is > deleted, it doesn't return false if the destructor is private. However, > the example below shows how we can still achieve what we want by using > wrapping the class that we are interested in as a member of a helper > templated struct: > > > > #include <type_traits> > #include <iostream> > > class ClassWithDeletedDtor { > ~ClassWithDeletedDtor() = delete; > }; > > class ClassWithPrivateDtor { > ~ClassWithPrivateDtor() {} > }; > > class ClassWithPublicDtor { > public: > ~ClassWithPublicDtor() {} > }; > > template <typename T> > class IsDestructorPrivateOrDeletedHelper { > T x; > }; > > template <typename T> > struct IsDestructorPrivateOrDeleted > { > static const bool value = > !std::is_destructible<IsDestructorPrivateOrDeletedHelper<T>>::value; > }; > > int main() { > #define PRINT(x) std::cerr << #x " = " << (x) << std::endl; > > PRINT(std::is_destructible<ClassWithDeletedDtor>::value); > PRINT(std::is_destructible<ClassWithPrivateDtor>::value); > PRINT(std::is_destructible<ClassWithPublicDtor>::value); > > std::cerr << std::endl; > > PRINT(IsDestructorPrivateOrDeleted<ClassWithDeletedDtor>::value); > PRINT(IsDestructorPrivateOrDeleted<ClassWithPrivateDtor>::value); > PRINT(IsDestructorPrivateOrDeleted<ClassWithPublicDtor>::value); > } > > > Output: > > > std::is_destructible<ClassWithDeletedDtor>::value = 0 > std::is_destructible<ClassWithPrivateDtor>::value = 0 > std::is_destructible<ClassWithPublicDtor>::value = 1 > > IsDestructorPrivateOrDeleted<ClassWithDeletedDtor>::value = 1 > IsDestructorPrivateOrDeleted<ClassWithPrivateDtor>::value = 1 > IsDestructorPrivateOrDeleted<ClassWithPublicDtor>::value = 0 > > > If you also want to require classes to be final, C++11 <type_traits> > also has std::is_final for that. > > Cheers, > Benoit > > > 2014-05-28 16:24 GMT-04:00 Daniel Holbert <dholb...@mozilla.com > <mailto:dholb...@mozilla.com>>: > > Hi dev-platform, > > PSA: if you are adding a concrete class with AddRef/Release > implementations (e.g. via NS_INLINE_DECL_REFCOUNTING), please be aware > of the following best-practices: > > (a) Your class should have an explicitly-declared non-public > destructor. (should be 'private' or 'protected') > > (b) Your class should be labeled as MOZ_FINAL (or, see below). > > > WHY THIS IS A GOOD IDEA > ======================= > We'd like to ensure that refcounted objects are *only* deleted via their > ::Release() methods. Otherwise, we're potentially susceptible to > double-free bugs. > > We can go a long way towards enforcing this rule at compile-time by > giving these classes non-public destructors. This prevents a whole > category of double-free bugs. > > In particular: if your class has a public destructor (the default), then > it's easy for you or someone else to accidentally declare an instance on > the stack or as a member-variable in another class, like so: > MyClass foo; > This is *extremely* dangerous. If any code wraps 'foo' in a nsRefPtr > (say, if some function that we pass 'foo' or '&foo' into declares a > nsRefPtr to it for some reason), then we'll get a double-free. The > object will be freed when the nsRefPtr goes out of scope, and then again > when the MyClass instance goes out of scope. But if we give MyClass a > non-public destructor, then it'll make it a compile error (in most code) > to declare a MyClass instance on the stack or as a member-variable. So > we'd catch this bug immediately, at compile-time. > > So, that explains why a non-public destructor is a good idea. But why > MOZ_FINAL? If your class isn't MOZ_FINAL, then that opens up another > route to trigger the same sort of bug -- someone can come along and add > a subclass, perhaps not realizing that they're subclassing a refcounted > class, and the subclass will (by default) have a public destructor, > which means then that anyone can declare > MySubclass foo; > and run into the exact same problem with the subclass. A MOZ_FINAL > annotation will prevent that by keeping people from naively adding > subclasses. > > BUT WHAT IF I NEED SUBCLASSES > ============================= > First, if your class is abstract, then it shouldn't have AddRef/Release > implementations to begin with. Those belong on the concrete subclasses > -- not on your abstract base class. > > But if your class is concrete and refcounted and needs to have > subclasses, then: > - Your base class *and each of its subclasses* should have virtual, > protected destructors, to prevent the "MySubclass foo;" problem > mentioned above. > - Your subclasses themselves should also probably be declared as > "MOZ_FINAL", to keep someone from naively adding another subclass > without recognizing the above. > - Your subclasses should definitely *not* declare their own > AddRef/Release methods. (They should share the base class's methods & > refcount.) > > For more information, see > https://bugzilla.mozilla.org/show_bug.cgi?id=984786 , where I've fixed > this sort of thing in a bunch of existing classes. I definitely didn't > catch everything there, so please feel encouraged to continue this work > in other bugs. (And if you catch any cases that look like potential > double-frees, mark them as security-sensitive.) > > Thanks! > ~Daniel > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org <mailto:dev-platform@lists.mozilla.org> > https://lists.mozilla.org/listinfo/dev-platform > > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
