llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: CarvedCoder (CarvedCoder) <details> <summary>Changes</summary> Adds a clang-tidy check that warns when a class inherits from a base class with a non-virtual destructor while introducing data members. Deleting such objects through a base pointer can lead to undefined behavior because the derived portion of the object will not be destroyed. The check skips cases where the base class has a virtual destructor, a protected non-virtual destructor, or when the derived class introduces no data members. Includes tests and documentation. Fixes #<!-- -->183101 --- Full diff: https://github.com/llvm/llvm-project/pull/183384.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/misc/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/misc/ForbidNonVirtualBaseDtorCheck.cpp (+52) - (added) clang-tools-extra/clang-tidy/misc/ForbidNonVirtualBaseDtorCheck.h (+35) - (modified) clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp (+3) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+7) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/docs/clang-tidy/checks/misc/forbid-non-virtual-base-dtor.rst (+52) - (added) clang-tools-extra/test/clang-tidy/checkers/misc/forbid-non-virtual-base-dtor.cpp (+28) ``````````diff diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index e34b0cf687be3..4552b2a60ce80 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -23,6 +23,7 @@ add_clang_library(clangTidyMiscModule STATIC CoroutineHostileRAIICheck.cpp DefinitionsInHeadersCheck.cpp ConfusableIdentifierCheck.cpp + ForbidNonVirtualBaseDtorCheck.cpp HeaderIncludeCycleCheck.cpp IncludeCleanerCheck.cpp MiscTidyModule.cpp diff --git a/clang-tools-extra/clang-tidy/misc/ForbidNonVirtualBaseDtorCheck.cpp b/clang-tools-extra/clang-tidy/misc/ForbidNonVirtualBaseDtorCheck.cpp new file mode 100644 index 0000000000000..d26d412a4da6c --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/ForbidNonVirtualBaseDtorCheck.cpp @@ -0,0 +1,52 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "ForbidNonVirtualBaseDtorCheck.h" +#include "clang/AST/DeclCXX.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/Specifiers.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::misc { + +void ForbidNonVirtualBaseDtorCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + cxxRecordDecl(isDefinition(), + hasAnyBase(cxxBaseSpecifier().bind("BaseSpecifier"))) + .bind("derived"), + this); +} + +void ForbidNonVirtualBaseDtorCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Derived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived"); + const auto *BaseSpecifier = + Result.Nodes.getNodeAs<CXXBaseSpecifier>("BaseSpecifier"); + if (!Derived || !BaseSpecifier) + return; + if (BaseSpecifier->getAccessSpecifier() != AS_public) + return; + const auto *BaseType = BaseSpecifier->getType()->getAsCXXRecordDecl(); + if (!BaseType || !BaseType->hasDefinition()) + return; + const auto *Dtor = BaseType->getDestructor(); + if (Dtor && Dtor->isVirtual()) + return; + if (Dtor && Dtor->getAccess() == AS_protected && !Dtor->isVirtual()) + return; + if (Derived->isEmpty()) + return; + + diag(Derived->getLocation(), + "class '%0' inherits from '%1' which has a non-virtual destructor") + << Derived->getName() << BaseType->getName(); +} + +} // namespace clang::tidy::misc diff --git a/clang-tools-extra/clang-tidy/misc/ForbidNonVirtualBaseDtorCheck.h b/clang-tools-extra/clang-tidy/misc/ForbidNonVirtualBaseDtorCheck.h new file mode 100644 index 0000000000000..6c2286b1dfc84 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/ForbidNonVirtualBaseDtorCheck.h @@ -0,0 +1,35 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORBIDNONVIRTUALBASEDTORCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORBIDNONVIRTUALBASEDTORCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::misc { + +/// Warns when a class or struct publicly inherits from a base class or struct +/// whose destructor is neither virtual nor protected, and the derived class +/// adds data members +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/misc/forbid-non-virtual-base-dtor.html +class ForbidNonVirtualBaseDtorCheck : public ClangTidyCheck { +public: + ForbidNonVirtualBaseDtorCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } +}; + +} // namespace clang::tidy::misc + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORBIDNONVIRTUALBASEDTORCHECK_H diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index f8550b30b9789..671415f2648ab 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -13,6 +13,7 @@ #include "ConstCorrectnessCheck.h" #include "CoroutineHostileRAIICheck.h" #include "DefinitionsInHeadersCheck.h" +#include "ForbidNonVirtualBaseDtorCheck.h" #include "HeaderIncludeCycleCheck.h" #include "IncludeCleanerCheck.h" #include "MisleadingBidirectionalCheck.h" @@ -53,6 +54,8 @@ class MiscModule : public ClangTidyModule { "misc-coroutine-hostile-raii"); CheckFactories.registerCheck<DefinitionsInHeadersCheck>( "misc-definitions-in-headers"); + CheckFactories.registerCheck<ForbidNonVirtualBaseDtorCheck>( + "misc-forbid-non-virtual-base-dtor"); CheckFactories.registerCheck<HeaderIncludeCycleCheck>( "misc-header-include-cycle"); CheckFactories.registerCheck<IncludeCleanerCheck>("misc-include-cleaner"); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 64c8fbbe2f07a..518eba0935943 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -124,6 +124,13 @@ New checks ``llvm::to_vector(llvm::make_filter_range(...))`` that can be replaced with ``llvm::map_to_vector`` and ``llvm::filter_to_vector``. +- New :doc:`misc-forbid-non-virtual-base-dtor + <clang-tidy/checks/misc/forbid-non-virtual-base-dtor>` check. + + Warns when a class or struct publicly inherits from a base whose destructor + is neither virtual nor protected, and the derived type adds data members. + This pattern causes resource leaks when deleting through a base pointer. + - New :doc:`modernize-use-string-view <clang-tidy/checks/modernize/use-string-view>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index c475870ed7b31..8045a5e518c97 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -266,6 +266,7 @@ Clang-Tidy Checks :doc:`misc-const-correctness <misc/const-correctness>`, "Yes" :doc:`misc-coroutine-hostile-raii <misc/coroutine-hostile-raii>`, :doc:`misc-definitions-in-headers <misc/definitions-in-headers>`, "Yes" + :doc:`misc-forbid-non-virtual-base-dtor <misc/forbid-non-virtual-base-dtor>`, "Yes" :doc:`misc-header-include-cycle <misc/header-include-cycle>`, :doc:`misc-include-cleaner <misc/include-cleaner>`, "Yes" :doc:`misc-misleading-bidirectional <misc/misleading-bidirectional>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/forbid-non-virtual-base-dtor.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/forbid-non-virtual-base-dtor.rst new file mode 100644 index 0000000000000..1c446adf224b2 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc/forbid-non-virtual-base-dtor.rst @@ -0,0 +1,52 @@ +.. title:: clang-tidy - misc-forbid-non-virtual-base-dtor + +misc-forbid-non-virtual-base-dtor +================================= + +Warns when a class or struct publicly inherits from a base class or struct +whose destructor is neither virtual nor protected, and the derived class adds +data members. This pattern causes resource leaks when the derived object is +deleted through a base class pointer, because the derived destructor is never +called. + +Examples +-------- + +The following code will trigger a warning: + +.. code-block:: c++ + + class Base {}; // non-virtual destructor + + class Derived : public Base { // warning: class 'Derived' inherits from + int data; // 'Base' which has a non-virtual destructor + }; + + Base *b = new Derived(); + delete b; // leaks Derived::data —> Base::~Base() is called, not ~Derived() + +The following patterns are safe and will **not** trigger a warning: + +.. code-block:: c++ + + // safe: base has a virtual destructor + class Base1 { + public: + virtual ~Base1() {} + }; + class Derived1 : public Base1 { int data; }; + + // safe: base has a protected destructor (prevents delete-through-base) + class Base2 { + protected: + ~Base2() {} + }; + class Derived2 : public Base2 { int data; }; + + // safe: derived adds no data members + class Base3 {}; + class Derived3 : public Base3 {}; // OK + + // safe: private/protected inheritance (base pointer not accessible) + class Base4 {}; + class Derived4 : private Base4 { int data; }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/forbid-non-virtual-base-dtor.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/forbid-non-virtual-base-dtor.cpp new file mode 100644 index 0000000000000..0b7024dfd0249 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/forbid-non-virtual-base-dtor.cpp @@ -0,0 +1,28 @@ +// RUN: %check_clang_tidy %s misc-forbid-non-virtual-base-dtor %t + +// should warn -> non-virtual base + derived has data +class A {}; +class B: public A{ + int x; +}; +// CHECK-MESSAGES: warning: class 'B' inherits from 'A' which has a non-virtual destructor [misc-forbid-non-virtual-base-dtor] + +//shouldn't warn -> derived has no data +class C : public A{}; + +// shouldn't warn -> base has virtual destructor +class D { + public: + virtual ~D(){}; +}; +class E : public D{ + int y; +}; + +//shouldn't crash -> incomplete base +class F; +class F {}; +class G: public F{ + int z; +}; +// CHECK-MESSAGES: warning: class 'G' inherits from 'F' which has a non-virtual destructor [misc-forbid-non-virtual-base-dtor] `````````` </details> https://github.com/llvm/llvm-project/pull/183384 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
