llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Peiqi Li (voyager-jhk) <details> <summary>Changes</summary> This patch introduces a new check to detect lambdas that capture local variables by reference and subsequently escape the local scope, leading to use-after-free. It identifies two primary escape mechanisms: concurrency sinks, where lambdas are passed to asynchronous execution APIs (e.g., `std::thread`), and storage sinks, where they are stored in containers (e.g., `std::vector`) that have global or field storage duration. The AST matcher explicitly unwraps the argument conversion spine to accurately target the escaping lambdas. All escape sinks are configurable via `CheckOptions`. --- Patch is 28.26 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/203757.diff 10 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/LambdaCaptureLifetimeCheck.cpp (+153) - (added) clang-tools-extra/clang-tidy/bugprone/LambdaCaptureLifetimeCheck.h (+42) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/lambda-capture-lifetime.rst (+59) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/lambda-capture-lifetime.cpp (+66) - (modified) llvm/lib/Target/RISCV/RISCVMoveMerger.cpp (+136-15) - (modified) llvm/test/CodeGen/RISCV/rv32-move-merge.ll (+94) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 3aa39d10ceb5d..f10ec5dae5bbb 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -45,6 +45,7 @@ #include "InfiniteLoopCheck.h" #include "IntegerDivisionCheck.h" #include "InvalidEnumDefaultInitializationCheck.h" +#include "LambdaCaptureLifetimeCheck.h" #include "LambdaFunctionNameCheck.h" #include "MacroParenthesesCheck.h" #include "MacroRepeatedSideEffectsCheck.h" @@ -184,6 +185,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-incorrect-enable-if"); CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>( "bugprone-incorrect-enable-shared-from-this"); + CheckFactories.registerCheck<LambdaCaptureLifetimeCheck>( + "bugprone-lambda-capture-lifetime"); CheckFactories.registerCheck<UnintendedCharOstreamOutputCheck>( "bugprone-unintended-char-ostream-output"); CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 43e85b1407f21..33a07e3243aa0 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -38,6 +38,7 @@ add_clang_library(clangTidyBugproneModule STATIC IncorrectEnableIfCheck.cpp IncorrectEnableSharedFromThisCheck.cpp InvalidEnumDefaultInitializationCheck.cpp + LambdaCaptureLifetimeCheck.cpp MissingEndComparisonCheck.cpp UnintendedCharOstreamOutputCheck.cpp ReturnConstRefFromParameterCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/LambdaCaptureLifetimeCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/LambdaCaptureLifetimeCheck.cpp new file mode 100644 index 0000000000000..bcc03d5120491 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/LambdaCaptureLifetimeCheck.cpp @@ -0,0 +1,153 @@ +//===----------------------------------------------------------------------===// +// +// 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 "LambdaCaptureLifetimeCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { +namespace { + +const LambdaExpr *getEscapingLambdaFromArgument(const Expr *E) { + if (!E) + return nullptr; + + E = E->IgnoreParenImpCasts(); + + if (const auto *Lambda = dyn_cast<LambdaExpr>(E)) + return Lambda; + + if (const auto *Cleanups = dyn_cast<ExprWithCleanups>(E)) + return getEscapingLambdaFromArgument(Cleanups->getSubExpr()); + + if (const auto *Temporary = dyn_cast<MaterializeTemporaryExpr>(E)) + return getEscapingLambdaFromArgument(Temporary->getSubExpr()); + + if (const auto *Temporary = dyn_cast<CXXBindTemporaryExpr>(E)) + return getEscapingLambdaFromArgument(Temporary->getSubExpr()); + + if (const auto *Cast = dyn_cast<CastExpr>(E)) + return getEscapingLambdaFromArgument(Cast->getSubExpr()); + + if (const auto *Construct = dyn_cast<CXXConstructExpr>(E)) { + if (Construct->getNumArgs() == 1) + return getEscapingLambdaFromArgument(Construct->getArg(0)); + return nullptr; + } + + if (const auto *InitList = dyn_cast<InitListExpr>(E)) { + if (InitList->getNumInits() == 1) + return getEscapingLambdaFromArgument(InitList->getInit(0)); + return nullptr; + } + + return nullptr; +} + +const LambdaExpr *getEscapingLambda(const CXXConstructExpr *Construct) { + for (const Expr *Arg : Construct->arguments()) + if (const LambdaExpr *Lambda = getEscapingLambdaFromArgument(Arg)) + return Lambda; + return nullptr; +} + +const LambdaExpr *getEscapingLambda(const CallExpr *Call) { + for (const Expr *Arg : Call->arguments()) + if (const LambdaExpr *Lambda = getEscapingLambdaFromArgument(Arg)) + return Lambda; + return nullptr; +} + +bool capturesLocalVariableByReference(const LambdaExpr *Lambda) { + for (const LambdaCapture &Capture : Lambda->captures()) { + if (!Capture.capturesVariable() || Capture.getCaptureKind() != LCK_ByRef) + continue; + + if (const auto *Var = dyn_cast<VarDecl>(Capture.getCapturedVar())) { + if (Var->hasLocalStorage()) + return true; + } + } + return false; +} + +} // namespace + +LambdaCaptureLifetimeCheck::LambdaCaptureLifetimeCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AsyncClasses(utils::options::parseStringList( + Options.get("AsyncClasses", "::std::thread;::std::jthread"))), + AsyncFunctions(utils::options::parseStringList( + Options.get("AsyncFunctions", "::std::async"))), + StorageClasses(utils::options::parseStringList( + Options.get("StorageClasses", "::std::vector"))), + StorageFunctions(utils::options::parseStringList(Options.get( + "StorageFunctions", "push_back;emplace_back;insert;assign"))) {} + +void LambdaCaptureLifetimeCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AsyncClasses", + utils::options::serializeStringList(AsyncClasses)); + Options.store(Opts, "AsyncFunctions", + utils::options::serializeStringList(AsyncFunctions)); + Options.store(Opts, "StorageClasses", + utils::options::serializeStringList(StorageClasses)); + Options.store(Opts, "StorageFunctions", + utils::options::serializeStringList(StorageFunctions)); +} + +void LambdaCaptureLifetimeCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(cxxConstructExpr(hasDeclaration(cxxConstructorDecl( + ofClass(hasAnyName(AsyncClasses)))), + hasAnyArgument(expr())) + .bind("escape-point"), + this); + + Finder->addMatcher(callExpr(callee(functionDecl(hasAnyName(AsyncFunctions))), + hasAnyArgument(expr())) + .bind("escape-point"), + this); + + auto LongLivedStorage = anyOf(varDecl(hasGlobalStorage()), fieldDecl()); + + auto IsDirectRef = declRefExpr(to(LongLivedStorage)); + auto IsMemberRef = memberExpr(member(LongLivedStorage)); + auto StorageClass = cxxRecordDecl( + anyOf(hasAnyName(StorageClasses), + classTemplateSpecializationDecl(hasAnyName(StorageClasses)))); + + Finder->addMatcher( + cxxMemberCallExpr( + callee(cxxMethodDecl(hasAnyName(StorageFunctions), + ofClass(StorageClass))), + on(expr(ignoringParenImpCasts(anyOf(IsDirectRef, IsMemberRef)))), + hasAnyArgument(expr())) + .bind("escape-point"), + this); +} + +void LambdaCaptureLifetimeCheck::check(const MatchFinder::MatchResult &Result) { + const LambdaExpr *Lambda = nullptr; + if (const auto *Construct = + Result.Nodes.getNodeAs<CXXConstructExpr>("escape-point")) + Lambda = getEscapingLambda(Construct); + else if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("escape-point")) + Lambda = getEscapingLambda(Call); + + if (Lambda && capturesLocalVariableByReference(Lambda)) { + diag(Lambda->getBeginLoc(), + "lambda captures local variables by reference, but escapes the local " + "scope, potentially causing a use-after-free"); + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/LambdaCaptureLifetimeCheck.h b/clang-tools-extra/clang-tidy/bugprone/LambdaCaptureLifetimeCheck.h new file mode 100644 index 0000000000000..99a455172c2f1 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/LambdaCaptureLifetimeCheck.h @@ -0,0 +1,42 @@ +//===----------------------------------------------------------------------===// +// +// 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_LAMBDACAPTURELIFETIMECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_LAMBDACAPTURELIFETIMECHECK_H + +#include "../ClangTidyCheck.h" +#include <vector> + +namespace clang::tidy::bugprone { + +/// Finds lambdas that capture local variables by reference and escape their +/// local scope by being passed to asynchronous sinks or out-of-scope +/// containers. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/lambda-capture-lifetime.html +class LambdaCaptureLifetimeCheck : public ClangTidyCheck { +public: + LambdaCaptureLifetimeCheck(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; + } + +private: + const std::vector<StringRef> AsyncClasses; + const std::vector<StringRef> AsyncFunctions; + const std::vector<StringRef> StorageClasses; + const std::vector<StringRef> StorageFunctions; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_LAMBDACAPTURELIFETIMECHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 99fe37f4145dd..079d73f688ad0 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -219,6 +219,12 @@ New checks Finds assignments within selection statements. +- New :doc:`bugprone-lambda-capture-lifetime + <clang-tidy/checks/bugprone/lambda-capture-lifetime>` check. + + Finds lambdas that capture local variables by reference and escape their + local scope by being passed to asynchronous sinks or out-of-scope containers. + - New :doc:`bugprone-missing-end-comparison <clang-tidy/checks/bugprone/missing-end-comparison>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/lambda-capture-lifetime.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/lambda-capture-lifetime.rst new file mode 100644 index 0000000000000..6c3666b41cad1 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/lambda-capture-lifetime.rst @@ -0,0 +1,59 @@ +.. title:: clang-tidy - bugprone-lambda-capture-lifetime + +bugprone-lambda-capture-lifetime +================================ + +Finds lambdas that capture local variables by reference and escape their +local scope by being passed to asynchronous sinks or out-of-scope containers, +potentially causing use-after-free bugs. + +Examples: + +.. code-block:: c++ + + #include <thread> + #include <vector> + #include <functional> + + void thread_escape() { + int local_var = 42; + // WARNING: 'local_var' is captured by reference but escapes to a thread + std::thread t([&local_var]() { + local_var++; + }); + t.detach(); + } // 'local_var' is destroyed here, causing a use-after-free in the thread. + + std::vector<std::function<void()>> GlobalActions; + + void container_escape() { + int local_var = 42; + // WARNING: 'local_var' is captured by reference but escapes to a global container + GlobalActions.push_back([&local_var]() { + local_var++; + }); + } // 'local_var' is destroyed here, but the lambda lives on in GlobalActions. + +Options +------- + +.. option:: AsyncClasses + + Semicolon-separated list of names of asynchronous classes whose constructors + are considered escape sinks. Default is ``::std::thread;::std::jthread``. + +.. option:: AsyncFunctions + + Semicolon-separated list of names of asynchronous functions that are + considered escape sinks. Default is ``::std::async``. + +.. option:: StorageClasses + + Semicolon-separated list of names of classes that act as long-lived + storage containers. Default is ``::std::vector``. + +.. option:: StorageFunctions + + Semicolon-separated list of names of member functions that store a + callable into a long-lived container. Default is + ``push_back;emplace_back;insert;assign``. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 0eb9e8a243081..aacec4aaa3116 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -114,6 +114,7 @@ Clang-Tidy Checks :doc:`bugprone-infinite-loop <bugprone/infinite-loop>`, :doc:`bugprone-integer-division <bugprone/integer-division>`, :doc:`bugprone-invalid-enum-default-initialization <bugprone/invalid-enum-default-initialization>`, + :doc:`bugprone-lambda-capture-lifetime <bugprone/lambda-capture-lifetime>`, "Yes" :doc:`bugprone-lambda-function-name <bugprone/lambda-function-name>`, :doc:`bugprone-macro-parentheses <bugprone/macro-parentheses>`, "Yes" :doc:`bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/lambda-capture-lifetime.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/lambda-capture-lifetime.cpp new file mode 100644 index 0000000000000..aa53a7fc02b48 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/lambda-capture-lifetime.cpp @@ -0,0 +1,66 @@ +// RUN: %check_clang_tidy %s bugprone-lambda-capture-lifetime %t + +namespace std { +class thread { +public: + template <typename Callable> thread(Callable&& f) {} +}; + +template <typename T> class function { +public: + function() = default; + template <typename Callable> function(Callable&& f) {} +}; + +template <typename T> +class vector { +public: + void emplace_back(T t) {} + void push_back(T t) {} +}; + +template <typename Callable> +void async(Callable&& f) {} +} // namespace std + +std::vector<std::function<int()>> GlobalFns; +std::function<int()> make_function(int); + +void test_thread() { + int x = 0; + + std::thread t1([&x]() { x = 1; }); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: lambda captures local variables by reference, but escapes the local scope + + std::thread t2([x]() { int y = x; }); +} + +void test_async_function() { + int x = 0; + + std::async([&x]() { x = 1; }); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: lambda captures local variables by reference, but escapes the local scope +} + +void test_vector_escape() { + int y = 0; + + GlobalFns.emplace_back([&y]() -> int { return y; }); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: lambda captures local variables by reference, but escapes the local scope + + GlobalFns.push_back(std::function<int()>([&y]() -> int { return y; })); + // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: lambda captures local variables by reference, but escapes the local scope +} + +void test_safe_local_vector() { + int z = 0; + std::vector<std::function<int()>> LocalFns; + + LocalFns.emplace_back([&z]() -> int { return z; }); +} + +void test_nested_lambda_inside_argument_does_not_escape() { + int q = 0; + + GlobalFns.emplace_back(make_function(([&q]() -> int { return q; })())); +} diff --git a/llvm/lib/Target/RISCV/RISCVMoveMerger.cpp b/llvm/lib/Target/RISCV/RISCVMoveMerger.cpp index b193dd3280478..a242ec3594bd8 100644 --- a/llvm/lib/Target/RISCV/RISCVMoveMerger.cpp +++ b/llvm/lib/Target/RISCV/RISCVMoveMerger.cpp @@ -37,6 +37,9 @@ struct RISCVMoveMerge : public MachineFunctionPass { bool isCandidateToMergeMVA01S(const DestSourcePair &RegPair); bool isCandidateToMergeMVSA01(const DestSourcePair &RegPair); + + bool isPLIPairCandidate(const MachineInstr &MI, bool EvenRegPair); + // Merge the two instructions indicated into a single pair instruction. MachineBasicBlock::iterator mergeGPRPairInsns(MachineBasicBlock::iterator I, @@ -44,17 +47,22 @@ struct RISCVMoveMerge : public MachineFunctionPass { MachineBasicBlock::iterator mergePairedInsns(MachineBasicBlock::iterator I, MachineBasicBlock::iterator Paired, bool MoveFromSToA); + MachineBasicBlock::iterator mergePLIPair(MachineBasicBlock::iterator I, + MachineBasicBlock::iterator Paired, + bool RegPairIsEven); MachineBasicBlock::iterator - findMatchingInstPair(MachineBasicBlock::iterator &MBBI, bool EvenRegPair, - const DestSourcePair &RegPair); + findMatchingGPRPairCopy(MachineBasicBlock::iterator &MBBI, bool EvenRegPair, + const DestSourcePair &RegPair); // Look for C.MV instruction that can be combined with // the given instruction into CM.MVA01S or CM.MVSA01. Return the matching // instruction if one exists. MachineBasicBlock::iterator - findMatchingInst(MachineBasicBlock::iterator &MBBI, bool MoveFromSToA, - const DestSourcePair &RegPair); - bool mergeMoveSARegPair(MachineBasicBlock &MBB); + findMatchingSACopy(MachineBasicBlock::iterator &MBBI, bool MoveFromSToA, + const DestSourcePair &RegPair); + MachineBasicBlock::iterator findMatchingPLI(MachineBasicBlock::iterator &MBBI, + bool EvenRegPair); + bool mergeMovePairs(MachineBasicBlock &MBB); bool runOnMachineFunction(MachineFunction &Fn) override; StringRef getPassName() const override { return RISCV_MOVE_MERGE_NAME; } @@ -87,6 +95,20 @@ static unsigned getCM_MVOpcode(const RISCVSubtarget &ST, bool MoveFromSToA) { llvm_unreachable("Unhandled subtarget with paired move."); } +// Returns 0 if Opc has no paired form. +static unsigned getPairedPLIOpcode(unsigned Opc) { + switch (Opc) { + case RISCV::PLI_B: + return RISCV::PLI_DB; + case RISCV::PLI_H: + return RISCV::PLI_DH; + case RISCV::PLUI_H: + return RISCV::PLUI_DH; + default: + return 0; + } +} + bool RISCVMoveMerge::isGPRPairCopyCandidate(const DestSourcePair &RegPair, bool EvenRegPair) { Register Destination = RegPair.Destination->getReg(); @@ -132,6 +154,21 @@ bool RISCVMoveMerge::isCandidateToMergeMVSA01(const DestSourcePair &RegPair) { return false; } +// Check if MI is a single-reg pli/plui whose destination is a half of a +// GPRPair. +bool RISCVMoveMerge::isPLIPairCandidate(const MachineInstr &MI, + bool EvenRegPair) { + if (!ST->hasStdExtP() || ST->is64Bit()) + return false; + if (!getPairedPLIOpcode(MI.getOpcode())) + return false; + unsigned SubIdx = EvenRegPair ? RISCV::sub_gpr_even : RISCV::sub_gpr_odd; + return TRI + ->getMatchingSuperReg(MI.getOperand(0).getReg(), SubIdx, + &RISCV::GPRPairRegClass) + .isValid(); +} + MachineBasicBlock::iterator RISCVMoveMerge::mergeGPRPairInsns(MachineBasicBlock::iterator I, MachineBasicBlock::iterator Paired, @@ -229,9 +266,34 @@ RISCVMoveMerge::mergePairedInsns(MachineBasicBlock::iterator I, } MachineBasicBlock::iterator -RISCVMoveMerge::findMatchingInstPair(MachineBasicBlock::iterator &MBBI, - bool EvenRegPair, - const DestSourcePair &RegPair) { +RISCVMoveMerge::mergePLIPair(MachineBasicBlock::iterator I, + MachineBasicBlock::iterator Paired, + bool RegPairIsEven) { + MachineBasicBlock::iterator E = I->getParent()->end(); + MachineBasicBlock::iterator NextI = next_nodbg(I, E); + + if (NextI == Paired) + NextI = next_nodbg(NextI, E); + DebugLoc DL = I->getDebugLoc(); + + unsigned Opcode = getPairedPLIOpcode(I->getOpcode()); + unsigned GPRPairIdx = + RegPairIsEven ? RISCV::sub_gpr_even : RISCV::sub_gpr_odd; + Register DestReg = TRI->getMatchingSuperReg( + I->getOperand(0).getReg(), GPRP... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/203757 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
