[clang-tidy] Add `bugprone-pointer-arithmetic-on-polymorphic-object` check #91951

Discookie · 2024-05-13T12:13:15Z

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.

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. 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.

Finds pointer arithmetic on classes that declare a virtual function.

Useful for reports within template instantiations.

llvmbot · 2024-05-13T12:13:49Z

@llvm/pr-subscribers-clang-tidy

Author: Discookie (Discookie)

Changes

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.

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. 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 --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

llvmbot · 2024-05-13T12:13:49Z

@llvm/pr-subscribers-clang-tools-extra

Author: Discookie (Discookie)

Changes

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.

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. 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 --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

Discookie · 2024-05-13T12:33:30Z

(Finding classes with virtual member fns declared is a good heuristic for finding polymorphic objects in general, as previously discussed in #82977 (comment).)

EugeneZelenko

Please mention new check in Release Notes.

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.h

clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst

5chmidti

Needs some small changes before landing. It's a good check to have, thanks.

5chmidti · 2024-05-13T13:08:49Z

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

+  diag(PointerExpr->getBeginLoc(),
+       "pointer arithmetic on class '%0' that declares a virtual function, "
+       "undefined behavior if the pointee is a different class")
+      << PointeeType->getName();


Please stream the SourceRange of PointerExpr into the diagnostic, so that it is fully underlined in the diagnostic/editor

I think the comma should be replaced with (can) result in, wdyt?

You don't have to escape the %0 with ' when you pass PointeeType without the getName instead.

In Tidy we can only use a single SourceLocation and not a range, right? I don't know of a way to feed ranges into it.

The diag member function only accepts a SourceLocation as the first parameter, that is correct.
When you stream a SourceRange into the diagnostic, that range will be underlined by the diagnostic engine (and in clangd). The same goes for attached fix-it hints, which highlight the ranges they change.

E.g.

125 | base += 1; | ^

vs

125 | base += 1; | ^~~~

when using

diag(PointerExpr->getBeginLoc(), "pointer arithmetic on polymorphic class '%0', which can result in " "undefined behavior if the pointee is a different class") << PointeeType->getName() << PointerExpr->getSourceRange(); // adding this

Tidy supports source ranges like that!? I never knew, thank you! Added.
I don't think I've seen support for it in CodeChecker, that's why I was surprised. Will have to follow up on that.

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.h

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst

clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp

PiotrZSL

Few nits.
My main concern is an check name, had to open PR to find out what it does.
maybe should be more a "bugprone-arithmetic-on-polymorphic-pointer" or something like that.

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst

PiotrZSL · 2024-05-13T15:10:01Z

clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp

+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class 'Base' that declares a virtual function
+
+  templateWarning(b);
+}


Add test for bugfree scenario:

void test() { Object table[10]; Object* ptr = table[5]; }

Is this snippet meant to be Object &ref? Array subscript can't return a pointer AFAIK.
Other than that the case is not too distinct from Derived d[10]; d += 1; for which I have a test.

Object* ptr = &table[5];

carlosgalvezp · 2024-05-13T15:36:20Z

I'm not very happy about the name, there's no such thing as "virtual arithmetic".

What about "bugprone-pointer-arithmetic-polymorphic-object"?

I rather have a long descriptive name than a short ambiguous name.

carlosgalvezp · 2024-05-13T15:37:30Z

Otherwise since this is a problem also on non-polymorphic objects, maybe "bugprone-pointer-arithmetic-base-class"?

PiotrZSL · 2024-05-13T15:42:32Z

bugprone-pointer-arithmetic-polymorphic-object or bugprone-pointer-arithmetic-on-polymorphic-object could be fine.
limiting check to polymorphic objects it's fine, someone can always enable an cppguidelines check to cover all cases.

Also check-alias should be added to cert.

carlosgalvezp · 2024-05-13T15:51:44Z

I realize it's probably very hard (if not impossible) to detect whether a class is a base class unless it has virtual functions.

That's why the corresponding AUTOSAR rule bans only non-final classes. But that is a bit overkill and leads to some pain.

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.h

Discookie

Renamed the check as requested, and fixed what I reasonably could regarding the rest.

Added the check option MatchInheritedVirtualFunctions to differentiate between inherited and newly-declared virtual functions. Inherited has many more false positives on the projects I tested.

Discookie · 2024-05-29T06:35:25Z

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

+  diag(PointerExpr->getBeginLoc(),
+       "pointer arithmetic on class '%0' that declares a virtual function, "
+       "undefined behavior if the pointee is a different class")
+      << PointeeType->getName();


In Tidy we can only use a single SourceLocation and not a range, right? I don't know of a way to feed ranges into it.

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp

clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.h

Discookie · 2024-05-29T07:06:42Z

clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp

+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class 'Base' that declares a virtual function
+
+  templateWarning(b);
+}


Is this snippet meant to be Object &ref? Array subscript can't return a pointer AFAIK.
Other than that the case is not too distinct from Derived d[10]; d += 1; for which I have a test.

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.h

clang-tools-extra/docs/clang-tidy/checks/bugprone/pointer-arithmetic-on-polymorphic-object.rst

5chmidti

LGTM minus the stored option value. (please wait for others to finish their review)

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp

PiotrZSL

Few nits:

storeOptions needs fix
Consider excluding implicit code with TK_IgnoreUnlessSpelledInSource (not this may exclude also template instances, so I leave it up to you)
Documentation require small work
Make check more strict by default, and add options to relax it (as thats what CTR56-CPP require). I undestand that on your project this may cause some false-positives, but still thats your project problem. Consider adding options to "Allow classes where only virtual function is destructor", "Classes where all virtual functions are marked final", "Classes that do not add new virtual functions or members". (Just check what suit your project most).

Note that next release branch out is in a ~month. Would be nice if this check could fit-in.

clang-tools-extra/docs/clang-tidy/checks/bugprone/pointer-arithmetic-on-polymorphic-object.rst

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.h

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/bugprone/pointer-arithmetic-on-polymorphic-object.rst

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst

PiotrZSL · 2024-06-10T19:39:18Z

clang-tools-extra/test/clang-tidy/checkers/bugprone/virtual-arithmetic.cpp

+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: pointer arithmetic on class 'Base' that declares a virtual function
+
+  templateWarning(b);
+}


Object* ptr = &table[5];

clang-tools-extra/docs/ReleaseNotes.rst

* Renamed IgnoreInheritedVirtualFunctions * Documented cert-ctr56-cpp * IgnoreInheritedVirtualFunctions is now false when using cert-ctr56-cpp

Discookie

I found a curious report when browsing through the results for llvm-project: MemoryBuffer.cpp
I have no idea where these spurious fixits could be coming from. My check doesn't emit any. Could this be because of the source range I'm piping into my diagnostic, or is it just some side-effect of other checks, or maybe the Tidy module? I'll investigate and fix this before merging.

Other than this case, fixed the reviews. I'm debating adding a flag for virtual destructor only as well, but I haven't seen a case in the wild where it was the only inherited function.

Made IgnoreInheritedVirtualFunctions false by default - doesn't have too many meaningful differences, but it does have more reports. Results on projects

clang-tools-extra/docs/ReleaseNotes.rst

Discookie · 2024-06-18T08:21:05Z

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp

+  const auto PointerExprWithVirtualMethod =
+      expr(hasType(hasCanonicalType(pointerType(
+               pointee(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+                   unless(isFinal()),
+                   anyOf(hasMethod(isVirtualAsWritten()), isAbstract())))))))))
+          .bind("pointer");


Indeed, vtable-wise it works out, but the size could still be different. In fact, as noted in the previous CSA checker's PR, the size very rarely stays the same for subclasses, so this is still a good heuristic for bug-prone cases.

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/bugprone/pointer-arithmetic-on-polymorphic-object.rst

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst

clang-tools-extra/docs/clang-tidy/checks/bugprone/pointer-arithmetic-on-polymorphic-object.rst

Discookie · 2024-06-20T13:51:18Z

Seems to be a CodeChecker-specific issue, doesn't block upstreaming this review.

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp

PiotrZSL

Some small issues with diagnostic left.
Except that, looks good enough.

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp

…check (llvm#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.

Discookie added 2 commits May 6, 2024 06:11

Add bugprone-virtual-arithmetic check

69cbd3d

Finds pointer arithmetic on classes that declare a virtual function.

Display the classname that has the virtual function

47068bd

Useful for reports within template instantiations.

Discookie requested review from PiotrZSL, LegalizeAdulthood and NagyDonat May 13, 2024 12:13

llvmbot added clang-tools-extra clang-tidy labels May 13, 2024

Discookie requested a review from gamesh411 May 13, 2024 12:30

EugeneZelenko reviewed May 13, 2024

View reviewed changes

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.h Outdated Show resolved Hide resolved

clang-tools-extra/docs/clang-tidy/checks/bugprone/virtual-arithmetic.rst Outdated Show resolved Hide resolved

5chmidti requested changes May 13, 2024

View reviewed changes

PiotrZSL reviewed May 13, 2024

View reviewed changes

Rename check to bugprone-pointer-arithmetic-on-polymorphic-object

df2e3f1

EugeneZelenko reviewed May 28, 2024

View reviewed changes

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.h Show resolved Hide resolved

Discookie added 8 commits May 29, 2024 04:08

Add MatchInheritedVirtualFunctions option

fe0d5a7

Restrict to C++

f373922

Update check message and comments

a29c9cd

Add support for type aliases

914f349

Add tests for prefix ++

801d7ec

Add comment about the UB's cause

e5dc6b2

Exclude final classes

dc7e63c

Add CERT alias

6415b14

Discookie commented May 29, 2024

View reviewed changes

Discookie changed the title ~~[clang-tidy] Add bugprone-virtual-arithmetic check~~ [clang-tidy] Add bugprone-pointer-arithmetic-on-polymorphic-object check May 29, 2024

Discookie removed request for gamesh411 and NagyDonat May 29, 2024 07:34

EugeneZelenko reviewed May 29, 2024

View reviewed changes

Discookie added 3 commits June 3, 2024 05:46

Use isAbstract matcher for inherited pure functions

6a4f75a

Pipe source range into diag message

d0e5bc2

Docs fixes

3c2508c

Discookie requested review from PiotrZSL and 5chmidti June 3, 2024 08:39

Discookie mentioned this pull request Jun 3, 2024

Save source ranges in Clang-Tidy diagnostics Ericsson/codechecker#4252

Open

5chmidti approved these changes Jun 7, 2024

View reviewed changes

clang-tools-extra/clang-tidy/bugprone/VirtualArithmeticCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp Outdated Show resolved Hide resolved

PiotrZSL requested changes Jun 10, 2024

View reviewed changes

PiotrZSL reviewed Jun 10, 2024

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

Discookie added 6 commits June 18, 2024 08:03

Minor fixes

225804a

* Renamed IgnoreInheritedVirtualFunctions * Documented cert-ctr56-cpp * IgnoreInheritedVirtualFunctions is now false when using cert-ctr56-cpp

Merge branch 'main' into ctr56-tidy

7b87e77

Order typo

11d838e

Make check respect TK_IgnoreUnlessSpelledInSource

c7f2391

Make check description consistent

34370b1

Make IgnoreInheritedVirtualFunctions default false

411f815

Discookie commented Jun 20, 2024

View reviewed changes

Fix side effects of TK_IgnoreUnlessSpelledInSource

07a4e9b

5chmidti reviewed Jun 23, 2024

View reviewed changes

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp Outdated Show resolved Hide resolved

Better warning text

4dac982

PiotrZSL approved these changes Jul 3, 2024

View reviewed changes

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/bugprone/PointerArithmeticOnPolymorphicObjectCheck.cpp Outdated Show resolved Hide resolved

Fix diag message typos

7cd17bb

Discookie merged commit f329e3e into llvm:main Jul 4, 2024
8 checks passed

[clang-tidy] Add bugprone-pointer-arithmetic-on-polymorphic-object check #91951

[clang-tidy] Add bugprone-pointer-arithmetic-on-polymorphic-object check #91951

Uh oh!

Conversation

Discookie commented May 13, 2024

Uh oh!

llvmbot commented May 13, 2024

Uh oh!

llvmbot commented May 13, 2024

Uh oh!

Discookie commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EugeneZelenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosgalvezp commented May 13, 2024

Uh oh!

carlosgalvezp commented May 13, 2024

Uh oh!

PiotrZSL commented May 13, 2024

Uh oh!

carlosgalvezp commented May 13, 2024

Uh oh!

Uh oh!

Discookie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

[clang-tidy] Add `bugprone-pointer-arithmetic-on-polymorphic-object` check #91951

[clang-tidy] Add `bugprone-pointer-arithmetic-on-polymorphic-object` check #91951

Discookie commented May 13, 2024 •

edited

Loading