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>:
> 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
> https://lists.mozilla.org/listinfo/dev-platform
>
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
