Author: Ryosuke Niwa Date: 2025-02-14T16:33:34-08:00 New Revision: 77041da98932f77896d48366703d956ae7a82036
URL: https://github.com/llvm/llvm-project/commit/77041da98932f77896d48366703d956ae7a82036 DIFF: https://github.com/llvm/llvm-project/commit/77041da98932f77896d48366703d956ae7a82036.diff LOG: [webkit.UncountedLambdaCapturesChecker] Recognize nested protectedThis pattern (#126443) In WebKit, it's pretty common to capture "this" and "protectedThis" where "protectedThis" is a guardian variable of type Ref or RefPtr for "this". Furthermore, it's common for this "protectedThis" variable from being passed to an inner lambda by std::move. Recognize this pattern so that we don't emit warnings for nested inner lambdas. To recognize this pattern, we introduce a new DenseSet, ProtectedThisDecls, which contains every "protectedThis" we've recognized to our subclass of DynamicRecursiveASTVisitor. This set is now populated in "hasProtectedThis" and "declProtectsThis" uses this DenseSet to determine a given value declaration constitutes a "protectedThis" pattern or not. Because hasProtectedThis and declProtectsThis had to be moved from the checker class to the visitor class, it's now a responsibility of each caller of visitLambdaExpr to check whether a given lambda captures "this" without a "protectedThis" or not. Finally, this PR improves the code to recognize "protectedThis" pattern by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and UnaryOperator expressions. Added: Modified: clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp Removed: ################################################################################ diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp index 972364a855eb4..4ffdac5ca4873 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp @@ -41,7 +41,9 @@ class UncountedLambdaCapturesChecker const UncountedLambdaCapturesChecker *Checker; llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore; llvm::DenseSet<const LambdaExpr *> LambdasToIgnore; + llvm::DenseSet<const ValueDecl *> ProtectedThisDecls; llvm::DenseSet<const CXXConstructExpr *> ConstructToIgnore; + QualType ClsType; explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker) @@ -66,7 +68,7 @@ class UncountedLambdaCapturesChecker bool VisitLambdaExpr(LambdaExpr *L) override { if (LambdasToIgnore.contains(L)) return true; - Checker->visitLambdaExpr(L, shouldCheckThis()); + Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L)); return true; } @@ -94,7 +96,7 @@ class UncountedLambdaCapturesChecker if (!L) return true; LambdasToIgnore.insert(L); - Checker->visitLambdaExpr(L, shouldCheckThis()); + Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L)); return true; } @@ -119,7 +121,8 @@ class UncountedLambdaCapturesChecker if (auto *L = findLambdaInArg(Arg)) { LambdasToIgnore.insert(L); if (!Param->hasAttr<NoEscapeAttr>()) - Checker->visitLambdaExpr(L, shouldCheckThis()); + Checker->visitLambdaExpr(L, shouldCheckThis() && + !hasProtectedThis(L)); } ++ArgIndex; } @@ -139,7 +142,8 @@ class UncountedLambdaCapturesChecker if (auto *L = findLambdaInArg(Arg)) { LambdasToIgnore.insert(L); if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape) - Checker->visitLambdaExpr(L, shouldCheckThis()); + Checker->visitLambdaExpr(L, shouldCheckThis() && + !hasProtectedThis(L)); } ++ArgIndex; } @@ -168,6 +172,13 @@ class UncountedLambdaCapturesChecker ConstructToIgnore.insert(CE); return Lambda; } + if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) { + E = TempExpr->getSubExpr()->IgnoreParenCasts(); + if (auto *Lambda = dyn_cast<LambdaExpr>(E)) { + ConstructToIgnore.insert(CE); + return Lambda; + } + } auto *DRE = dyn_cast<DeclRefExpr>(CtorArg); if (!DRE) return nullptr; @@ -213,9 +224,68 @@ class UncountedLambdaCapturesChecker return; DeclRefExprsToIgnore.insert(ArgRef); LambdasToIgnore.insert(L); - Checker->visitLambdaExpr(L, shouldCheckThis(), + Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L), /* ignoreParamVarDecl */ true); } + + bool hasProtectedThis(LambdaExpr *L) { + for (const LambdaCapture &OtherCapture : L->captures()) { + if (!OtherCapture.capturesVariable()) + continue; + if (auto *ValueDecl = OtherCapture.getCapturedVar()) { + if (declProtectsThis(ValueDecl)) { + ProtectedThisDecls.insert(ValueDecl); + return true; + } + } + } + return false; + } + + bool declProtectsThis(const ValueDecl *ValueDecl) const { + auto *VD = dyn_cast<VarDecl>(ValueDecl); + if (!VD) + return false; + auto *Init = VD->getInit()->IgnoreParenCasts(); + if (!Init) + return false; + const Expr *Arg = Init; + do { + if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg)) + Arg = BTE->getSubExpr()->IgnoreParenCasts(); + if (auto *CE = dyn_cast_or_null<CXXConstructExpr>(Arg)) { + auto *Ctor = CE->getConstructor(); + if (!Ctor) + return false; + auto clsName = safeGetName(Ctor->getParent()); + if (!isRefType(clsName) || !CE->getNumArgs()) + return false; + Arg = CE->getArg(0)->IgnoreParenCasts(); + continue; + } + if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) { + auto OpCode = OpCE->getOperator(); + if (OpCode == OO_Star || OpCode == OO_Amp) { + auto *Callee = OpCE->getDirectCallee(); + auto clsName = safeGetName(Callee->getParent()); + if (!isRefType(clsName) || !OpCE->getNumArgs()) + return false; + Arg = OpCE->getArg(0)->IgnoreParenCasts(); + continue; + } + } + if (auto *UO = dyn_cast<UnaryOperator>(Arg)) { + auto OpCode = UO->getOpcode(); + if (OpCode == UO_Deref || OpCode == UO_AddrOf) + Arg = UO->getSubExpr()->IgnoreParenCasts(); + continue; + } + break; + } while (Arg); + if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) + return ProtectedThisDecls.contains(DRE->getDecl()); + return isa<CXXThisExpr>(Arg); + } }; LocalVisitor visitor(this); @@ -238,53 +308,11 @@ class UncountedLambdaCapturesChecker } else if (C.capturesThis() && shouldCheckThis) { if (ignoreParamVarDecl) // this is always a parameter to this function. continue; - bool hasProtectThis = false; - for (const LambdaCapture &OtherCapture : L->captures()) { - if (!OtherCapture.capturesVariable()) - continue; - if (auto *ValueDecl = OtherCapture.getCapturedVar()) { - if (protectThis(ValueDecl)) { - hasProtectThis = true; - break; - } - } - } - if (!hasProtectThis) - reportBugOnThisPtr(C); + reportBugOnThisPtr(C); } } } - bool protectThis(const ValueDecl *ValueDecl) const { - auto *VD = dyn_cast<VarDecl>(ValueDecl); - if (!VD) - return false; - auto *Init = VD->getInit()->IgnoreParenCasts(); - if (!Init) - return false; - auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Init); - if (!BTE) - return false; - auto *CE = dyn_cast_or_null<CXXConstructExpr>(BTE->getSubExpr()); - if (!CE) - return false; - auto *Ctor = CE->getConstructor(); - if (!Ctor) - return false; - auto clsName = safeGetName(Ctor->getParent()); - if (!isRefType(clsName) || !CE->getNumArgs()) - return false; - auto *Arg = CE->getArg(0)->IgnoreParenCasts(); - while (auto *UO = dyn_cast<UnaryOperator>(Arg)) { - auto OpCode = UO->getOpcode(); - if (OpCode == UO_Deref || OpCode == UO_AddrOf) - Arg = UO->getSubExpr(); - else - break; - } - return isa<CXXThisExpr>(Arg); - } - void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar, const QualType T) const { assert(CapturedVar); diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp index 62a8245db99b9..82058bf13d137 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp @@ -89,6 +89,7 @@ template <typename Callback> void call(Callback callback) { someFunction(); callback(); } +void callAsync(const WTF::Function<void()>&); void raw_ptr() { RefCountable* ref_countable = make_obj(); @@ -303,6 +304,37 @@ struct RefCountableWithLambdaCapturingThis { }); } + void callAsyncNoescape([[clang::noescape]] WTF::Function<bool(RefCountable&)>&&); + void method_temp_lambda(RefCountable* obj) { + callAsyncNoescape([this, otherObj = RefPtr { obj }](auto& obj) { + return otherObj == &obj; + }); + } + + void method_nested_lambda() { + callAsync([this, protectedThis = Ref { *this }] { + callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(protectedThis)] { + nonTrivial(); + }); + }); + } + + void method_nested_lambda2() { + callAsync([this, protectedThis = RefPtr { this }] { + callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(*protectedThis)] { + nonTrivial(); + }); + }); + } + + void method_nested_lambda3() { + callAsync([this, protectedThis = RefPtr { this }] { + callAsync([this] { + // expected-warning@-1{{Captured raw-pointer 'this' to ref-counted type or CheckedPtr-capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + nonTrivial(); + }); + }); + } }; struct NonRefCountableWithLambdaCapturingThis { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
