Skip to content

Commit 77041da

Browse files
rniwarniwa
authored
[webkit.UncountedLambdaCapturesChecker] Recognize nested protectedThis pattern (llvm#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.
1 parent 60af835 commit 77041da

File tree

2 files changed

+108
-48
lines changed

2 files changed

+108
-48
lines changed

‎clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp‎

Lines changed: 76 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ class UncountedLambdaCapturesChecker
4141
const UncountedLambdaCapturesChecker *Checker;
4242
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
4343
llvm::DenseSet<const LambdaExpr *> LambdasToIgnore;
44+
llvm::DenseSet<const ValueDecl *> ProtectedThisDecls;
4445
llvm::DenseSet<const CXXConstructExpr *> ConstructToIgnore;
46+
4547
QualType ClsType;
4648

4749
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
@@ -66,7 +68,7 @@ class UncountedLambdaCapturesChecker
6668
bool VisitLambdaExpr(LambdaExpr *L) override {
6769
if (LambdasToIgnore.contains(L))
6870
return true;
69-
Checker->visitLambdaExpr(L, shouldCheckThis());
71+
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
7072
return true;
7173
}
7274

@@ -94,7 +96,7 @@ class UncountedLambdaCapturesChecker
9496
if (!L)
9597
return true;
9698
LambdasToIgnore.insert(L);
97-
Checker->visitLambdaExpr(L, shouldCheckThis());
99+
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
98100
return true;
99101
}
100102

@@ -119,7 +121,8 @@ class UncountedLambdaCapturesChecker
119121
if (auto *L = findLambdaInArg(Arg)) {
120122
LambdasToIgnore.insert(L);
121123
if (!Param->hasAttr<NoEscapeAttr>())
122-
Checker->visitLambdaExpr(L, shouldCheckThis());
124+
Checker->visitLambdaExpr(L, shouldCheckThis() &&
125+
!hasProtectedThis(L));
123126
}
124127
++ArgIndex;
125128
}
@@ -139,7 +142,8 @@ class UncountedLambdaCapturesChecker
139142
if (auto *L = findLambdaInArg(Arg)) {
140143
LambdasToIgnore.insert(L);
141144
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
142-
Checker->visitLambdaExpr(L, shouldCheckThis());
145+
Checker->visitLambdaExpr(L, shouldCheckThis() &&
146+
!hasProtectedThis(L));
143147
}
144148
++ArgIndex;
145149
}
@@ -168,6 +172,13 @@ class UncountedLambdaCapturesChecker
168172
ConstructToIgnore.insert(CE);
169173
return Lambda;
170174
}
175+
if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
176+
E = TempExpr->getSubExpr()->IgnoreParenCasts();
177+
if (auto *Lambda = dyn_cast<LambdaExpr>(E)) {
178+
ConstructToIgnore.insert(CE);
179+
return Lambda;
180+
}
181+
}
171182
auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
172183
if (!DRE)
173184
return nullptr;
@@ -213,9 +224,68 @@ class UncountedLambdaCapturesChecker
213224
return;
214225
DeclRefExprsToIgnore.insert(ArgRef);
215226
LambdasToIgnore.insert(L);
216-
Checker->visitLambdaExpr(L, shouldCheckThis(),
227+
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
217228
/* ignoreParamVarDecl */ true);
218229
}
230+
231+
bool hasProtectedThis(LambdaExpr *L) {
232+
for (const LambdaCapture &OtherCapture : L->captures()) {
233+
if (!OtherCapture.capturesVariable())
234+
continue;
235+
if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
236+
if (declProtectsThis(ValueDecl)) {
237+
ProtectedThisDecls.insert(ValueDecl);
238+
return true;
239+
}
240+
}
241+
}
242+
return false;
243+
}
244+
245+
bool declProtectsThis(const ValueDecl *ValueDecl) const {
246+
auto *VD = dyn_cast<VarDecl>(ValueDecl);
247+
if (!VD)
248+
return false;
249+
auto *Init = VD->getInit()->IgnoreParenCasts();
250+
if (!Init)
251+
return false;
252+
const Expr *Arg = Init;
253+
do {
254+
if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
255+
Arg = BTE->getSubExpr()->IgnoreParenCasts();
256+
if (auto *CE = dyn_cast_or_null<CXXConstructExpr>(Arg)) {
257+
auto *Ctor = CE->getConstructor();
258+
if (!Ctor)
259+
return false;
260+
auto clsName = safeGetName(Ctor->getParent());
261+
if (!isRefType(clsName) || !CE->getNumArgs())
262+
return false;
263+
Arg = CE->getArg(0)->IgnoreParenCasts();
264+
continue;
265+
}
266+
if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
267+
auto OpCode = OpCE->getOperator();
268+
if (OpCode == OO_Star || OpCode == OO_Amp) {
269+
auto *Callee = OpCE->getDirectCallee();
270+
auto clsName = safeGetName(Callee->getParent());
271+
if (!isRefType(clsName) || !OpCE->getNumArgs())
272+
return false;
273+
Arg = OpCE->getArg(0)->IgnoreParenCasts();
274+
continue;
275+
}
276+
}
277+
if (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
278+
auto OpCode = UO->getOpcode();
279+
if (OpCode == UO_Deref || OpCode == UO_AddrOf)
280+
Arg = UO->getSubExpr()->IgnoreParenCasts();
281+
continue;
282+
}
283+
break;
284+
} while (Arg);
285+
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
286+
return ProtectedThisDecls.contains(DRE->getDecl());
287+
return isa<CXXThisExpr>(Arg);
288+
}
219289
};
220290

221291
LocalVisitor visitor(this);
@@ -238,53 +308,11 @@ class UncountedLambdaCapturesChecker
238308
} else if (C.capturesThis() && shouldCheckThis) {
239309
if (ignoreParamVarDecl) // this is always a parameter to this function.
240310
continue;
241-
bool hasProtectThis = false;
242-
for (const LambdaCapture &OtherCapture : L->captures()) {
243-
if (!OtherCapture.capturesVariable())
244-
continue;
245-
if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
246-
if (protectThis(ValueDecl)) {
247-
hasProtectThis = true;
248-
break;
249-
}
250-
}
251-
}
252-
if (!hasProtectThis)
253-
reportBugOnThisPtr(C);
311+
reportBugOnThisPtr(C);
254312
}
255313
}
256314
}
257315

258-
bool protectThis(const ValueDecl *ValueDecl) const {
259-
auto *VD = dyn_cast<VarDecl>(ValueDecl);
260-
if (!VD)
261-
return false;
262-
auto *Init = VD->getInit()->IgnoreParenCasts();
263-
if (!Init)
264-
return false;
265-
auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Init);
266-
if (!BTE)
267-
return false;
268-
auto *CE = dyn_cast_or_null<CXXConstructExpr>(BTE->getSubExpr());
269-
if (!CE)
270-
return false;
271-
auto *Ctor = CE->getConstructor();
272-
if (!Ctor)
273-
return false;
274-
auto clsName = safeGetName(Ctor->getParent());
275-
if (!isRefType(clsName) || !CE->getNumArgs())
276-
return false;
277-
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
278-
while (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
279-
auto OpCode = UO->getOpcode();
280-
if (OpCode == UO_Deref || OpCode == UO_AddrOf)
281-
Arg = UO->getSubExpr();
282-
else
283-
break;
284-
}
285-
return isa<CXXThisExpr>(Arg);
286-
}
287-
288316
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
289317
const QualType T) const {
290318
assert(CapturedVar);

‎clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ template <typename Callback> void call(Callback callback) {
8989
someFunction();
9090
callback();
9191
}
92+
void callAsync(const WTF::Function<void()>&);
9293

9394
void raw_ptr() {
9495
RefCountable* ref_countable = make_obj();
@@ -303,6 +304,37 @@ struct RefCountableWithLambdaCapturingThis {
303304
});
304305
}
305306

307+
void callAsyncNoescape([[clang::noescape]] WTF::Function<bool(RefCountable&)>&&);
308+
void method_temp_lambda(RefCountable* obj) {
309+
callAsyncNoescape([this, otherObj = RefPtr { obj }](auto& obj) {
310+
return otherObj == &obj;
311+
});
312+
}
313+
314+
void method_nested_lambda() {
315+
callAsync([this, protectedThis = Ref { *this }] {
316+
callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(protectedThis)] {
317+
nonTrivial();
318+
});
319+
});
320+
}
321+
322+
void method_nested_lambda2() {
323+
callAsync([this, protectedThis = RefPtr { this }] {
324+
callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(*protectedThis)] {
325+
nonTrivial();
326+
});
327+
});
328+
}
329+
330+
void method_nested_lambda3() {
331+
callAsync([this, protectedThis = RefPtr { this }] {
332+
callAsync([this] {
333+
// expected-warning@-1{{Captured raw-pointer 'this' to ref-counted type or CheckedPtr-capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
334+
nonTrivial();
335+
});
336+
});
337+
}
306338
};
307339

308340
struct NonRefCountableWithLambdaCapturingThis {

0 commit comments

Comments
 (0)