[clang-tools-extra] [clang-tidy] Add `bugprone-virtual-arithmetic` check (PR #91951)

[via cfe-commits]

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