danmar
Added check for deletion of this
Thanks for your contribution. Please add a unit test to test/testother.cpp.
The example in the original ticket had use-after-delete. delete this itself seems not that uncommon: https://github.com/search?q=%22delete+this%3B%22&type=code
Indeed, there are some popular libraries which use 'delete this'; I think a better check would be to ensure that no fields are used and that no other non-static methods get called after the deletion; if any of the above is not true, emit the warning. However, I am not sure on how to proceed to implement that check.
We don't seem to handle any (de)allocation in subfunctions even for normal variables (in CheckLeakAutoVar):
void d(int* q) {
delete q;
}
int* f() {
int* p = new int (1);
d(p);
*p = 8; // use after delete
return p;
}
int* a() {
int* r = new int (1);
return r;
}
void g() {
int* x = a();
if (*x) {} // leak
}
Perhaps a different warning message could be given, then... something like "deletion of this is highly unusual and can lead to problems if not handled correctly"?
Perhaps a different warning message could be given, then... something like "deletion of this is highly unusual and can lead to problems if not handled correctly"?
No. Looking at how common this construct seems to be.. I would assume that for most delete this; found there is no real undefined behavior. If there is no undefined behavior it is noise and then I fear the noise ratio would be very high.
One possible way to move forward here is to simplify the code example and construct an easier example that has UB. i.e.:
class Fred {
// ....
private:
void dostuff();
void release();
int x;
};
void Fred::release() {
delete this;
dostuff(); // <- BAD: calling method after delete.
x=0; // <- BAD: access member after delete
}
No "cross function" analysis is needed.
If you write a checker for this.. I feel it won't be controversial. However a bit of flow analysis is needed here.. I spontanously have the feeling that you could tweak FwdAnalysis in fwdanalysis.h.
Another possibility that might be acceptable, however it would be more controversial, would be to warn if "delete this" happens in the public interface:
class Fred {
public:
void release() {
delete this;
}
};
The public interface of this class is unsafe, wrong usage of the class can cause UB. We have some cppcheck options to check if the public interface of classes are unsafe. I believe the checking would be much simpler but we need to investigate if there are some use cases that we should not warn about.
