llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Discookie (Discookie) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/91951.diff 7 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) - (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp (+49) - (added) clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.h (+30) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst (+50) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp (+59) ``````````diff 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..dec43ae9bd8ca --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp @@ -0,0 +1,49 @@ +//===--- 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"); + const CXXRecordDecl *PointeeType = + PointerExpr->getType()->getPointeeType()->getAsCXXRecordDecl(); + + diag(PointerExpr->getBeginLoc(), + "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/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..d89e75186c60f --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp @@ -0,0 +1,59 @@ +// 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 '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 '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 '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 'Base' 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 'Base' that declares a virtual function + + delete[] b; + + // Common false positive is a class that overrides all parent functions. + Derived *d = new Derived[10]; + + d += 1; + // 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 `````````` </details> https://github.com/llvm/llvm-project/pull/91951 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
