Page MenuHomePhabricator

[analyzer] Delete with non-virtual destructor check
ClosedPublic

Authored by rnkovacs on Jul 24 2017, 5:53 AM.

Details

Summary

This check warns if a derived type object is deleted through a base pointer with a non-virtual destructor in its base class. It also places a note at the last point where the conversion from derived to base happened.

Corresponding CERT rule: OOP52-CPP: Do not delete a polymorphic object without a virtual destructor.

Diff Detail

Repository
rL LLVM

Event Timeline

rnkovacs created this revision.Jul 24 2017, 5:53 AM
aaron.ballman edited edge metadata.Jul 25 2017, 5:29 AM

How does this check differ from the -Wdelete-non-virtual-dtor warning class that comes out of the frontend?

NoQ edited edge metadata.Jul 25 2017, 5:41 AM

It seems that this check is more powerful because it works by knowing the dynamic type of the object. However, i still suspect that -Wnon-virtual-dtor (the other one, without delete-, that simply asks to make the destructor of polymorphic classes virtual) covers most practical cases. The only thing i see not covered by -Wnon-virtual-dtor but covered by this checker is the situation when the destructor is private. Reka, would you confirm our understanding?

rnkovacs updated this revision to Diff 108226.Jul 26 2017, 2:08 AM
In D35796#819965, @NoQ wrote:

It seems that this check is more powerful because it works by knowing the dynamic type of the object. However, i still suspect that -Wnon-virtual-dtor (the other one, without delete-, that simply asks to make the destructor of polymorphic classes virtual) covers most practical cases. The only thing i see not covered by -Wnon-virtual-dtor but covered by this checker is the situation when the destructor is private. Reka, would you confirm our understanding?

You're right, the flag covers many cases, but there are some common exceptions. E.g. deleting a polymorphic object without a virtual destructor might not be a problem if it's referenced by its precise type. The checker is aware of this. It looks for object deletions, checks their dynamic and static types, and only warns if an object is deleted through a base pointer (with no virtual destructor).

I added a slightly more sophisticated test case for the private destructor situation, I'm wondering whether you meant something like that.

dcoughlin edited edge metadata.Aug 7 2017, 5:30 PM

This looks like a useful checker! Have you run this on large codebases yet? Does it find bugs? What kind of false positives do you see? Do you have a sense of what additional work would it take to bring this out of alpha and have it turned on by default?

Other than some super tiny comments in-line, this looks good to me.

include/clang/StaticAnalyzer/Checkers/Checkers.td
296 ↗(On Diff #108226)

I think users would find it helpful if this had a more specific name. There are a lot of ways to misuse polymorphic objects, and this checker won't check all of them.

What do you think about "DeleteWithNonVirtualDestructor"?

lib/StaticAnalyzer/Checkers/MisusedPolymorphicObjectChecker.cpp
16 ↗(On Diff #108226)

I think it would be helpful to future maintainers to provide a high-level description of how this differs from -Wnon-virtual-dtor and -Wdelete-non-virtual-dtor.

137 ↗(On Diff #108226)

A minor style suggestion to avoid the stacked noun phrase: "Conversion from derived to base happened here".

rnkovacs marked 3 inline comments as done.Sep 14 2017, 4:54 AM
rnkovacs added inline comments.
include/clang/StaticAnalyzer/Checkers/Checkers.td
296 ↗(On Diff #108226)

I settled with the slightly abbreviated "DeleteWithNonVirtualDtor" version to shorten it a little.

rnkovacs updated this revision to Diff 115198.Sep 14 2017, 4:54 AM
rnkovacs marked an inline comment as done.
rnkovacs retitled this revision from [analyzer] Misused polymorphic object checker to [analyzer] Delete with non-virtual destructor check.
rnkovacs edited the summary of this revision. (Show Details)

Sorry for the late reply. I did run it on a few open-source projects as well as LLVM/Clang and honestly it didn't find anything. As the test cases seem to work fine it might already be in a state ready to bring out of alpha.

rnkovacs updated this revision to Diff 116060.Sep 20 2017, 1:20 PM
  • Accidentally left-in comment removed.
  • Checker file clang-formatted.
dcoughlin accepted this revision.Sep 21 2017, 2:42 PM

This looks good to me! Do you have commit access, or do you need someone to commit it for you?

This revision is now accepted and ready to land.Sep 21 2017, 2:42 PM

This looks good to me! Do you have commit access, or do you need someone to commit it for you?

Thanks! I don't, so it would be nice if someone committed it for me.

This revision was automatically updated to reflect the committed changes.