[clang-tidy] Add new check bugprone-capture-this-by-field (#130297)

Finds lambda captures that capture the ``this`` pointer and store it as
class
members without handle the copy and move constructors and the
assignments.

Capture this in a lambda and store it as a class member is dangerous
because the
lambda can outlive the object it captures. Especially when the object is
copied
or moved, the captured ``this`` pointer will be implicitly propagated to
the
new object. Most of the time, people will believe that the captured
``this``
pointer points to the new object, which will lead to bugs.

Fixes: #120863

---------

Co-authored-by: Baranov Victor <[email protected]>
Co-authored-by: Baranov Victor <[email protected]>

Author: HerrCai0907

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

@@ -16,6 +16,7 @@
 #include "BitwisePointerCastCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "BranchCloneCheck.h"
+#include "CapturingThisInMemberVariableCheck.h"
 #include "CastingThroughVoidCheck.h"
 #include "ChainedComparisonCheck.h"
 #include "ComparePointerToMemberVirtualFunctionCheck.h"
@@ -118,6 +119,8 @@ class BugproneModule : public ClangTidyModule {
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "bugprone-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<BranchCloneCheck>("bugprone-branch-clone");
+    CheckFactories.registerCheck<CapturingThisInMemberVariableCheck>(
+        "bugprone-capturing-this-in-member-variable");
     CheckFactories.registerCheck<CastingThroughVoidCheck>(
         "bugprone-casting-through-void");
     CheckFactories.registerCheck<ChainedComparisonCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -12,6 +12,7 @@ add_clang_library(clangTidyBugproneModule STATIC
   BoolPointerImplicitConversionCheck.cpp
   BranchCloneCheck.cpp
   BugproneTidyModule.cpp
+  CapturingThisInMemberVariableCheck.cpp
   CastingThroughVoidCheck.cpp
   ChainedComparisonCheck.cpp
   ComparePointerToMemberVirtualFunctionCheck.cpp

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

@@ -0,0 +1,123 @@
+//===--- CapturingThisInMemberVariableCheck.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 "CapturingThisInMemberVariableCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+
+AST_MATCHER(CXXRecordDecl, correctHandleCaptureThisLambda) {
+  // unresolved
+  if (Node.needsOverloadResolutionForCopyConstructor() &&
+      Node.needsImplicitCopyConstructor())
+    return false;
+  if (Node.needsOverloadResolutionForMoveConstructor() &&
+      Node.needsImplicitMoveConstructor())
+    return false;
+  if (Node.needsOverloadResolutionForCopyAssignment() &&
+      Node.needsImplicitCopyAssignment())
+    return false;
+  if (Node.needsOverloadResolutionForMoveAssignment() &&
+      Node.needsImplicitMoveAssignment())
+    return false;
+  // default but not deleted
+  if (Node.hasSimpleCopyConstructor())
+    return false;
+  if (Node.hasSimpleMoveConstructor())
+    return false;
+  if (Node.hasSimpleCopyAssignment())
+    return false;
+  if (Node.hasSimpleMoveAssignment())
+    return false;
+
+  for (CXXConstructorDecl const *C : Node.ctors()) {
+    if (C->isCopyOrMoveConstructor() && C->isDefaulted() && !C->isDeleted())
+      return false;
+  }
+  for (CXXMethodDecl const *M : Node.methods()) {
+    if (M->isCopyAssignmentOperator())
+      llvm::errs() << M->isDeleted() << "\n";
+    if (M->isCopyAssignmentOperator() && M->isDefaulted() && !M->isDeleted())
+      return false;
+    if (M->isMoveAssignmentOperator() && M->isDefaulted() && !M->isDeleted())
+      return false;
+  }
+  // FIXME: find ways to identifier correct handle capture this lambda
+  return true;
+}
+
+} // namespace
+
+constexpr const char *DefaultFunctionWrapperTypes =
+    "::std::function;::std::move_only_function;::boost::function";
+
+CapturingThisInMemberVariableCheck::CapturingThisInMemberVariableCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      FunctionWrapperTypes(utils::options::parseStringList(
+          Options.get("FunctionWrapperTypes", DefaultFunctionWrapperTypes))) {}
+void CapturingThisInMemberVariableCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "FunctionWrapperTypes",
+                utils::options::serializeStringList(FunctionWrapperTypes));
+}
+
+void CapturingThisInMemberVariableCheck::registerMatchers(MatchFinder *Finder) {
+  auto IsStdFunctionField =
+      fieldDecl(hasType(cxxRecordDecl(
+                    matchers::matchesAnyListedName(FunctionWrapperTypes))))
+          .bind("field");
+  auto CaptureThis = lambdaCapture(anyOf(
+      // [this]
+      capturesThis(),
+      // [self = this]
+      capturesVar(varDecl(hasInitializer(cxxThisExpr())))));
+  auto IsLambdaCapturingThis =
+      lambdaExpr(hasAnyCapture(CaptureThis.bind("capture"))).bind("lambda");
+  auto IsInitWithLambda =
+      anyOf(IsLambdaCapturingThis,
+            cxxConstructExpr(hasArgument(0, IsLambdaCapturingThis)));
+  Finder->addMatcher(
+      cxxRecordDecl(
+          anyOf(has(cxxConstructorDecl(
+                    unless(isCopyConstructor()), unless(isMoveConstructor()),
+                    hasAnyConstructorInitializer(cxxCtorInitializer(
+                        isMemberInitializer(), forField(IsStdFunctionField),
+                        withInitializer(IsInitWithLambda))))),
+                has(fieldDecl(IsStdFunctionField,
+                              hasInClassInitializer(IsInitWithLambda)))),
+          unless(correctHandleCaptureThisLambda())),
+      this);
+}
+
+void CapturingThisInMemberVariableCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Capture = Result.Nodes.getNodeAs<LambdaCapture>("capture");
+  const auto *Lambda = Result.Nodes.getNodeAs<LambdaExpr>("lambda");
+  const auto *Field = Result.Nodes.getNodeAs<FieldDecl>("field");
+  diag(Lambda->getBeginLoc(),
+       "'this' captured by a lambda and stored in a class member variable; "
+       "disable implicit class copying/moving to prevent potential "
+       "use-after-free")
+      << Capture->getLocation();
+  diag(Field->getLocation(),
+       "class member of type '%0' that stores captured 'this'",
+       DiagnosticIDs::Note)
+      << Field->getType().getAsString();
+}
+
+} // namespace clang::tidy::bugprone

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

@@ -0,0 +1,44 @@
+//===--- CapturingThisInMemberVariableCheck.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_CAPTURINGTHISINMEMBERVARIABLECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CAPTURINGTHISINMEMBERVARIABLECHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include <optional>
+
+namespace clang::tidy::bugprone {
+
+/// Finds lambda captures that capture the ``this`` pointer and store it as
+/// class members without handle the copy and move constructors and the
+/// assignments.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/capturing-this-in-member-variable.html
+class CapturingThisInMemberVariableCheck : public ClangTidyCheck {
+public:
+  CapturingThisInMemberVariableCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  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.CPlusPlus11;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TraversalKind::TK_IgnoreUnlessSpelledInSource;
+  }
+
+private:
+  ///< store the function wrapper types
+  const std::vector<StringRef> FunctionWrapperTypes;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CAPTURINGTHISINMEMBERVARIABLECHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -100,6 +100,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-capturing-this-in-member-variable
+  <clang-tidy/checks/bugprone/capturing-this-in-member-variable>` check.
+
+  Finds lambda captures that capture the ``this`` pointer and store it as class
+  members without handle the copy and move constructors and the assignments.
+
 - New :doc:`bugprone-unintended-char-ostream-output
   <clang-tidy/checks/bugprone/unintended-char-ostream-output>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/capturing-this-in-member-variable.rst

@@ -0,0 +1,39 @@
+.. title:: clang-tidy - bugprone-capturing-this-in-member-variable
+
+bugprone-capturing-this-in-member-variable
+==========================================
+
+Finds lambda captures that capture the ``this`` pointer and store it as class
+members without handle the copy and move constructors and the assignments.
+
+Capture this in a lambda and store it as a class member is dangerous because the
+lambda can outlive the object it captures. Especially when the object is copied
+or moved, the captured ``this`` pointer will be implicitly propagated to the
+new object. Most of the time, people will believe that the captured ``this``
+pointer points to the new object, which will lead to bugs.
+
+.. code-block:: c++
+
+  struct C {
+    C() : Captured([this]() -> C const * { return this; }) {}
+    std::function<C const *()> Captured;
+  };
+
+  void foo() {
+    C v1{};
+    C v2 = v1; // v2.Captured capture v1's 'this' pointer
+    assert(v2.Captured() == v1.Captured()); // v2.Captured capture v1's 'this' pointer
+    assert(v2.Captured() == &v2); // assertion failed.
+  }
+
+Possible fixes:
+  - marking copy and move constructors and assignment operators deleted.
+  - using class member method instead of class member variable with function
+    object types.
+  - passing ``this`` pointer as parameter 
+
+.. option:: FunctionWrapperTypes
+
+  A semicolon-separated list of names of types. Used to specify function
+  wrapper that can hold lambda expressions.
+  Default is `::std::function;::std::move_only_function;::boost::function`.

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -84,6 +84,7 @@ Clang-Tidy Checks
    :doc:`bugprone-bitwise-pointer-cast <bugprone/bitwise-pointer-cast>`,
    :doc:`bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion>`, "Yes"
    :doc:`bugprone-branch-clone <bugprone/branch-clone>`,
+   :doc:`bugprone-capturing-this-in-member-variable <bugprone/capturing-this-in-member-variable>`,
    :doc:`bugprone-casting-through-void <bugprone/casting-through-void>`,
    :doc:`bugprone-chained-comparison <bugprone/chained-comparison>`,
    :doc:`bugprone-compare-pointer-to-member-virtual-function <bugprone/compare-pointer-to-member-virtual-function>`,