[clang] [webkit.UncountedLambdaCapturesChecker] Detect protectedThis pattern. (PR #120528)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 19 13:12:35 PST 2024
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/120528
>From 97a721c8358d48333e0f8ab4177906135a2e2364 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Fri, 13 Dec 2024 14:34:39 -0800
Subject: [PATCH 1/4] [webkit.UncountedLambdaCapturesChecker] Detect
protectedThis pattern.
In WebKit, we often capture this as Ref or RefPtr in addition to this itself so that
the object lives as long as a capturing lambda stays alive.
Detect this pattern and treat it as safe. This PR also makes the check for a lambda
being passed as a function argument more robust by handling CXXBindTemporaryExpr,
CXXConstructExpr, and DeclRefExpr referring to the lambda.
---
.../WebKit/UncountedLambdaCapturesChecker.cpp | 76 ++++++++++++++++++-
.../WebKit/uncounted-lambda-captures.cpp | 36 +++++++++
2 files changed, 110 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index d786b02e2d7f3a..5c35a9a738c5ed 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -115,7 +115,7 @@ class UncountedLambdaCapturesChecker
if (ArgIndex >= CE->getNumArgs())
return true;
auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
- if (auto *L = dyn_cast_or_null<LambdaExpr>(Arg)) {
+ if (auto *L = findLambdaInArg(Arg)) {
LambdasToIgnore.insert(L);
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
Checker->visitLambdaExpr(L, shouldCheckThis());
@@ -126,6 +126,38 @@ class UncountedLambdaCapturesChecker
return true;
}
+ LambdaExpr *findLambdaInArg(Expr *E) {
+ if (auto *Lambda = dyn_cast_or_null<LambdaExpr>(E))
+ return Lambda;
+ auto *TempExpr = dyn_cast_or_null<CXXBindTemporaryExpr>(E);
+ if (!TempExpr)
+ return nullptr;
+ E = TempExpr->getSubExpr()->IgnoreParenCasts();
+ if (!E)
+ return nullptr;
+ if (auto *Lambda = dyn_cast<LambdaExpr>(E))
+ return Lambda;
+ auto *CE = dyn_cast_or_null<CXXConstructExpr>(E);
+ if (!CE || !CE->getNumArgs())
+ return nullptr;
+ auto *CtorArg = CE->getArg(0)->IgnoreParenCasts();
+ if (!CtorArg)
+ return nullptr;
+ if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg))
+ return Lambda;
+ auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
+ if (!DRE)
+ return nullptr;
+ auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl());
+ if (!VD)
+ return nullptr;
+ auto *Init = VD->getInit();
+ if (!Init)
+ return nullptr;
+ TempExpr = dyn_cast<CXXBindTemporaryExpr>(Init->IgnoreParenCasts());
+ return dyn_cast_or_null<LambdaExpr>(TempExpr->getSubExpr());
+ }
+
void checkCalleeLambda(CallExpr *CE) {
auto *Callee = CE->getCallee();
if (!Callee)
@@ -180,11 +212,51 @@ class UncountedLambdaCapturesChecker
} else if (C.capturesThis() && shouldCheckThis) {
if (ignoreParamVarDecl) // this is always a parameter to this function.
continue;
- reportBugOnThisPtr(C);
+ bool hasProtectThis = false;
+ for (const LambdaCapture &OtherCapture : L->captures()) {
+ if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
+ if (protectThis(ValueDecl)) {
+ hasProtectThis = true;
+ break;
+ }
+ }
+ }
+ if (!hasProtectThis)
+ 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 daff32e9940c83..b2cadbe50786e3 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -207,6 +207,42 @@ struct RefCountableWithLambdaCapturingThis {
};
call(lambda);
}
+
+ void method_captures_this_unsafe_capture_local_var_explicitly() {
+ RefCountable* x = make_obj();
+ call([this, protectedThis = RefPtr { this }, x]() {
+ // expected-warning at -1{{Captured raw-pointer 'x' to ref-counted type or CheckedPtr-capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ nonTrivial();
+ x->method();
+ });
+ }
+
+ void method_captures_this_unsafe_capture_local_var_explicitly_with_deref() {
+ RefCountable* x = make_obj();
+ call([this, protectedThis = Ref { *this }, x]() {
+ // expected-warning at -1{{Captured raw-pointer 'x' to ref-counted type or CheckedPtr-capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ nonTrivial();
+ x->method();
+ });
+ }
+
+ void method_captures_this_unsafe_local_var_via_vardecl() {
+ RefCountable* x = make_obj();
+ auto lambda = [this, protectedThis = Ref { *this }, x]() {
+ // expected-warning at -1{{Captured raw-pointer 'x' to ref-counted type or CheckedPtr-capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ nonTrivial();
+ x->method();
+ };
+ call(lambda);
+ }
+
+ void method_captures_this_with_guardian() {
+ auto lambda = [this, protectedThis = Ref { *this }]() {
+ nonTrivial();
+ };
+ call(lambda);
+ }
+
};
struct NonRefCountableWithLambdaCapturingThis {
>From 3b5d22fc688f22c35edee3cd6109f88a9d31d38c Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Wed, 18 Dec 2024 23:56:30 -0800
Subject: [PATCH 2/4] Added more test cases
---
.../WebKit/uncounted-lambda-captures.cpp | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
index b2cadbe50786e3..2173245bc7af3e 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -217,6 +217,15 @@ struct RefCountableWithLambdaCapturingThis {
});
}
+ void method_captures_this_with_other_protected_var() {
+ RefCountable* x = make_obj();
+ call([this, protectedX = RefPtr { x }]() {
+ // expected-warning at -1{{Captured raw-pointer 'this' to ref-counted type or CheckedPtr-capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ nonTrivial();
+ protectedX->method();
+ });
+ }
+
void method_captures_this_unsafe_capture_local_var_explicitly_with_deref() {
RefCountable* x = make_obj();
call([this, protectedThis = Ref { *this }, x]() {
@@ -243,6 +252,13 @@ struct RefCountableWithLambdaCapturingThis {
call(lambda);
}
+ void method_captures_this_with_guardian_refPtr() {
+ auto lambda = [this, protectedThis = RefPtr { &*this }]() {
+ nonTrivial();
+ };
+ call(lambda);
+ }
+
};
struct NonRefCountableWithLambdaCapturingThis {
>From 6d56aea468a785da81495e8aa8ddea31f9a7f880 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Thu, 19 Dec 2024 12:52:00 -0800
Subject: [PATCH 3/4] Fix debug assertion in visitLambdaExpr
---
.../Checkers/WebKit/UncountedLambdaCapturesChecker.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index 5c35a9a738c5ed..83165b512c77d6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -214,6 +214,8 @@ class UncountedLambdaCapturesChecker
continue;
bool hasProtectThis = false;
for (const LambdaCapture &OtherCapture : L->captures()) {
+ if (!OtherCapture.capturesVariable())
+ continue;
if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
if (protectThis(ValueDecl)) {
hasProtectThis = true;
>From 91dd080cd2563110c44cab3f456450c180aebc9d Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Thu, 19 Dec 2024 13:12:17 -0800
Subject: [PATCH 4/4] Fix formatting
---
.../Checkers/WebKit/UncountedLambdaCapturesChecker.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index 83165b512c77d6..da9698e327562e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -215,7 +215,7 @@ class UncountedLambdaCapturesChecker
bool hasProtectThis = false;
for (const LambdaCapture &OtherCapture : L->captures()) {
if (!OtherCapture.capturesVariable())
- continue;
+ continue;
if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
if (protectThis(ValueDecl)) {
hasProtectThis = true;
More information about the cfe-commits
mailing list