https://github.com/Discookie created https://github.com/llvm/llvm-project/pull/91951
Finds pointer arithmetic on classes that declare a virtual function. This check corresponds to the SEI Cert rule [CTR56-CPP: Do not use pointer arithmetic on polymorphic objects](https://wiki.sei.cmu.edu/confluence/display/cplusplus/CTR56-CPP.+Do+not+use+pointer+arithmetic+on+polymorphic+objects). ```cpp struct Base { virtual void ~Base(); }; struct Derived : public Base {}; void foo(Base *b) { b += 1; // passing `Derived` to `foo()` results in UB } ``` [Results on open-source projects](https://codechecker-demo.eastus.cloudapp.azure.com/Default/runs?run=Discookie-ctr56-with-classnames). Most of the Qtbase reports are from having a `virtual override` declaration, and the LLVM reports are true positives, as far as I can tell. >From 69cbd3da19eb0f8eb6758782b46daf99b5b79ea4 Mon Sep 17 00:00:00 2001 From: Viktor <[email protected]> Date: Mon, 6 May 2024 06:11:58 +0000 Subject: [PATCH 1/2] Add `bugprone-virtual-arithmetic` check Finds pointer arithmetic on classes that declare a virtual function. --- .../bugprone/BugproneTidyModule.cpp | 3 ++ .../clang-tidy/bugprone/CMakeLists.txt | 1 + .../bugprone/VirtualArithmeticCheck.cpp | 46 +++++++++++++++++ .../bugprone/VirtualArithmeticCheck.h | 30 +++++++++++ .../checks/bugprone/virtual-arithmetic.rst | 50 +++++++++++++++++++ .../docs/clang-tidy/checks/list.rst | 1 + .../checkers/bugprone/virtual-arithmetic.cpp | 45 +++++++++++++++++ 7 files changed, 176 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 1b92d2e60cc17..813f6720074ae 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -91,6 +91,7 @@ #include "UnusedRaiiCheck.h" #include "UnusedReturnValueCheck.h" #include "UseAfterMoveCheck.h" +#include "VirtualArithmeticCheck.h" #include "VirtualNearMissCheck.h" namespace clang::tidy { @@ -254,6 +255,8 @@ class BugproneModule : public ClangTidyModule { CheckFactories.registerCheck<UnusedReturnValueCheck>( "bugprone-unused-return-value"); CheckFactories.registerCheck<UseAfterMoveCheck>("bugprone-use-after-move"); + CheckFactories.registerCheck<VirtualArithmeticCheck>( + "bugprone-virtual-arithmetic"); CheckFactories.registerCheck<VirtualNearMissCheck>( "bugprone-virtual-near-miss"); } diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 2d303191f8865..ec1f3231e7990 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -87,6 +87,7 @@ add_clang_library(clangTidyBugproneModule UnusedRaiiCheck.cpp UnusedReturnValueCheck.cpp UseAfterMoveCheck.cpp + VirtualArithmeticCheck.cpp VirtualNearMissCheck.cpp LINK_LIBS diff --git a/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp new file mode 100644 index 0000000000000..57347af2b5881 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp @@ -0,0 +1,46 @@ +//===--- VirtualArithmeticCheck.cpp - clang-tidy---------------------------===// +// +// 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 "VirtualArithmeticCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +void VirtualArithmeticCheck::registerMatchers(MatchFinder *Finder) { + const auto PointerExprWithVirtualMethod = + expr(hasType(pointerType(pointee(hasDeclaration( + cxxRecordDecl(hasMethod(isVirtualAsWritten()))))))) + .bind("pointer"); + + const auto ArraySubscript = + arraySubscriptExpr(hasBase(PointerExprWithVirtualMethod)); + + const auto BinaryOperators = + binaryOperator(hasAnyOperatorName("+", "-", "+=", "-="), + hasEitherOperand(PointerExprWithVirtualMethod)); + + const auto UnaryOperators = + unaryOperator(hasAnyOperatorName("++", "--"), + hasUnaryOperand(PointerExprWithVirtualMethod)); + + Finder->addMatcher( + expr(anyOf(ArraySubscript, BinaryOperators, UnaryOperators)), this); +} + +void VirtualArithmeticCheck::check(const MatchFinder::MatchResult &Result) { + const auto *PointerExpr = Result.Nodes.getNodeAs<Expr>("pointer"); + + diag(PointerExpr->getBeginLoc(), + "pointer arithmetic on class that declares a virtual function, " + "undefined behavior if the pointee is a different class"); +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.h b/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.h new file mode 100644 index 0000000000000..6a5f86a391747 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.h @@ -0,0 +1,30 @@ +//===--- VirtualArithmeticCheck.h - clang-tidy-------------------*- C++ -*-===// +// +// 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_BUGPRONE_VIRTUAL_ARITHMETIC_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_VIRTUAL_ARITHMETIC_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Finds pointer arithmetic on classes that declare a virtual function. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/virtual-arithmetic.html +class VirtualArithmeticCheck : public ClangTidyCheck { +public: + VirtualArithmeticCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_VIRTUAL_ARITHMETIC_H diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst new file mode 100644 index 0000000000000..69d43e13392be --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst @@ -0,0 +1,50 @@ +.. title:: clang-tidy - bugprone-virtual-arithmetic + +bugprone-virtual-arithmetic +=========================== + +Warn if pointer arithmetic is performed on a class that declares a +virtual function. + +Pointer arithmetic on polymorphic objects where the pointer's static type is +different from its dynamic type is undefined behavior. +Finding pointers where the static type contains a virtual member function is a +good heuristic, as the pointer is likely to point to a different, derived class. + +This check corresponds to the SEI Cert rule `CTR56-CPP: Do not use pointer arithmetic on polymorphic objects <https://wiki.sei.cmu.edu/confluence/display/cplusplus/CTR56-CPP.+Do+not+use+pointer+arithmetic+on+polymorphic+objects>`_. + +Example: + +.. code-block:: c++ + + struct Base { + virtual void ~Base(); + }; + + struct Derived : public Base {}; + + void foo() { + Base *b = new Derived[10]; + + b += 1; + // warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class + + delete[] static_cast<Derived*>(b); + } + +Classes that do not declare a new virtual function are excluded, +as they make up the majority of false positives. + +.. code-block:: c++ + + void bar() { + Base *b = new Base[10]; + b += 1; // warning, as Base has a virtual destructor + + delete[] b; + + Derived *d = new Derived[10]; + d += 1; // no warning, as Derived overrides the destructor + + delete[] d; + } diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 5cdaf9e35b6ac..ba1e5cafa6e47 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -157,6 +157,7 @@ Clang-Tidy Checks :doc:`bugprone-unused-raii <bugprone/unused-raii>`, "Yes" :doc:`bugprone-unused-return-value <bugprone/unused-return-value>`, :doc:`bugprone-use-after-move <bugprone/use-after-move>`, + :doc:`bugprone-virtual-arithmetic <bugprone/virtual-arithmetic>`, :doc:`bugprone-virtual-near-miss <bugprone/virtual-near-miss>`, "Yes" :doc:`cert-dcl50-cpp <cert/dcl50-cpp>`, :doc:`cert-dcl58-cpp <cert/dcl58-cpp>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp new file mode 100644 index 0000000000000..d772112293cbc --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp @@ -0,0 +1,45 @@ +// RUN: %check_clang_tidy %s bugprone-virtual-arithmetic %t + +class Base { +public: + virtual ~Base() {} +}; + +class Derived : public Base {}; + +void operators() { + Base *b = new Derived[10]; + + b += 1; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + + b = b + 1; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + + b++; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + + b[1]; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + + delete[] static_cast<Derived*>(b); +} + +void subclassWarnings() { + Base *b = new Base[10]; + + // False positive that's impossible to distinguish without + // path-sensitive analysis, but the code is bug-prone regardless. + b += 1; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + + delete[] b; + + // Common false positive is a class that overrides all parent functions. + Derived *d = new Derived[10]; + + d += 1; + // no-warning + + delete[] d; +} \ No newline at end of file >From 47068bd748cfb6f1b74b006d0f9918103f936a34 Mon Sep 17 00:00:00 2001 From: Viktor <[email protected]> Date: Mon, 13 May 2024 09:43:46 +0000 Subject: [PATCH 2/2] Display the classname that has the virtual function Useful for reports within template instantiations. --- .../bugprone/VirtualArithmeticCheck.cpp | 7 ++++-- .../checkers/bugprone/virtual-arithmetic.cpp | 24 +++++++++++++++---- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp index 57347af2b5881..dec43ae9bd8ca 100644 --- a/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp @@ -37,10 +37,13 @@ void VirtualArithmeticCheck::registerMatchers(MatchFinder *Finder) { void VirtualArithmeticCheck::check(const MatchFinder::MatchResult &Result) { const auto *PointerExpr = Result.Nodes.getNodeAs<Expr>("pointer"); + const CXXRecordDecl *PointeeType = + PointerExpr->getType()->getPointeeType()->getAsCXXRecordDecl(); diag(PointerExpr->getBeginLoc(), - "pointer arithmetic on class that declares a virtual function, " - "undefined behavior if the pointee is a different class"); + "pointer arithmetic on class '%0' that declares a virtual function, " + "undefined behavior if the pointee is a different class") + << PointeeType->getName(); } } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp index d772112293cbc..d89e75186c60f 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp @@ -11,16 +11,16 @@ void operators() { Base *b = new Derived[10]; b += 1; - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class 'Base' that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] b = b + 1; - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer arithmetic on class 'Base' that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] b++; - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class 'Base' that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] b[1]; - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class 'Base' that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] delete[] static_cast<Derived*>(b); } @@ -31,7 +31,7 @@ void subclassWarnings() { // False positive that's impossible to distinguish without // path-sensitive analysis, but the code is bug-prone regardless. b += 1; - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class that declares a virtual function, undefined behavior if the pointee is a different class [bugprone-virtual-arithmetic] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class 'Base' that declares a virtual function delete[] b; @@ -42,4 +42,18 @@ void subclassWarnings() { // no-warning delete[] d; +} + +template <typename T> +void templateWarning(T *t) { + // FIXME: Show the location of the template instantiation in diagnostic. + t += 1; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class 'Base' that declares a virtual function +} + +void functionArgument(Base *b) { + b += 1; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class 'Base' that declares a virtual function + + templateWarning(b); } \ No newline at end of file _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
