Open
Bug 685266
Opened 13 years ago
Updated 2 years ago
Add a static analysis to find the cases where a base class pointer without a virtual destructor is deleted
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Tracking
(Not tracked)
NEW
People
(Reporter: ehsan.akhgari, Unassigned)
References
Details
It is almost always an error to delete a base class pointer which does not have a virtual destructor: class Base { virtual foo(); // in order for Base to have a vtable }; class Derived : public Base { public: ~Derived(); // will never be called in f below }; void f() { Base * obj = new Derived(); delete obj; // oops } Having a static analysis to catch this is potentially going to catch things like bug 605732.
Comment 1•13 years ago
|
||
This is going to be very hard to do without a whole-program analysis: you aren't guaranteed in any particular translation unit to know both parts of the question: * I'm deleting a class without a virtual dtor * That class has subclasses And even within a translation unit this can be tricky depending on the order you receive type/method information.
Comment 3•13 years ago
|
||
One thing we can do more easily is warn/error if: 1. Base has a virtual function, 2. Base does not have a virtual destructor, 3. delete <expr of type Base> is called from a function f, 4. and f is not a virtual member of Base marked NS_MUST_OVERRIDE. Note that XPCOM relies on having a vtable for methods but not having a virtual destructor, so we would have to mark all methods in NS_DECL_ISUPPORTS NS_MUST_OVERRIDE to guarantee this. [NS_MUST_OVERRIDE methods must be overridden in all subclasses, and this is already statically enforced, or would be, if we had a static-checking buildbot already].
Comment 4•13 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #2) > Doesn't GCC warn about this? Can we just turn that warning into an error? There is -Wnon-virtual-dtor, but enabling that as an error would cause all of XPCOM to fail to compile.
Comment 5•13 years ago
|
||
If we find a way to do this, something to look out for is that sometimes in IDL (such as nsIXPCScriptable.idl) you will have something like: > %{ C++ > ... > class NS_NO_VTABLE nsXPCClassInfo : public nsIClassInfo, > public nsIXPCScriptable > {
Updated•13 years ago
|
Blocks: static_analyses
Reporter | ||
Comment 6•13 years ago
|
||
I think comment 3 provides a perfect solution here. It will result in some false positives, but I think we might want to fix those anyways, so that they wouldn't be real problems in the future.
Updated•7 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•