Skip to content

Commit cf415a9

Browse files
rniwarniwa
committed
[alpha.webkit.UnretainedLambdaCapturesChecker] Add the support for protectedSelf
This PR adds the support for treating capturing of "self" as safe if the lambda simultaneously captures "protectedSelf", which is a RetainPtr of "self". This PR also fixes a bug that the checker wasn't generating a warning when "self" is implicitly captured. Note when "self" is implicitly captured, we use the lambda's getBeginLoc as a fallback source location. In addition, this PR also fixes a bug that the checker wasn't generating warnings for lambda caotures when ARC is enabled. While ARC protects variables in the stack, it does not automatically extend the lifetime of a lambda capture. So we now call RetainTypeChecker's isUnretained with ignoreARC set to true.
1 parent fe6bced commit cf415a9

File tree

4 files changed

+73
-11
lines changed

4 files changed

+73
-11
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
147147
bool isCtorOfRetainPtr(const clang::FunctionDecl *F) {
148148
const std::string &FunctionName = safeGetName(F);
149149
return FunctionName == "RetainPtr" || FunctionName == "adoptNS" ||
150-
FunctionName == "adoptCF";
150+
FunctionName == "adoptCF" || FunctionName == "retainPtr";
151151
}
152152

153153
bool isCtorOfSafePtr(const clang::FunctionDecl *F) {

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

Lines changed: 48 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ class RawPtrRefLambdaCapturesChecker
3636
: Bug(this, description, "WebKit coding guidelines") {}
3737

3838
virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
39+
virtual bool isPtrType(const std::string &) const = 0;
3940
virtual const char *ptrKind(QualType QT) const = 0;
4041

4142
void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -68,6 +69,15 @@ class RawPtrRefLambdaCapturesChecker
6869
return DynamicRecursiveASTVisitor::TraverseCXXMethodDecl(CXXMD);
6970
}
7071

72+
bool TraverseObjCMethodDecl(ObjCMethodDecl *OCMD) override {
73+
llvm::SaveAndRestore SavedDecl(ClsType);
74+
if (OCMD && OCMD->isInstanceMethod()) {
75+
if (auto *ImplParamDecl = OCMD->getSelfDecl())
76+
ClsType = ImplParamDecl->getType();
77+
}
78+
return DynamicRecursiveASTVisitor::TraverseObjCMethodDecl(OCMD);
79+
}
80+
7181
bool VisitTypedefDecl(TypedefDecl *TD) override {
7282
if (Checker->RTC)
7383
Checker->RTC->visitTypedef(TD);
@@ -287,7 +297,7 @@ class RawPtrRefLambdaCapturesChecker
287297
if (!Ctor)
288298
return false;
289299
auto clsName = safeGetName(Ctor->getParent());
290-
if (isRefType(clsName) && CE->getNumArgs()) {
300+
if (Checker->isPtrType(clsName) && CE->getNumArgs()) {
291301
Arg = CE->getArg(0)->IgnoreParenCasts();
292302
continue;
293303
}
@@ -307,6 +317,12 @@ class RawPtrRefLambdaCapturesChecker
307317
Arg = CE->getArg(0)->IgnoreParenCasts();
308318
continue;
309319
}
320+
if (auto *Callee = CE->getDirectCallee()) {
321+
if (isCtorOfSafePtr(Callee) && CE->getNumArgs() == 1) {
322+
Arg = CE->getArg(0)->IgnoreParenCasts();
323+
continue;
324+
}
325+
}
310326
}
311327
if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
312328
auto OpCode = OpCE->getOperator();
@@ -315,7 +331,7 @@ class RawPtrRefLambdaCapturesChecker
315331
if (!Callee)
316332
return false;
317333
auto clsName = safeGetName(Callee->getParent());
318-
if (!isRefType(clsName) || !OpCE->getNumArgs())
334+
if (!Checker->isPtrType(clsName) || !OpCE->getNumArgs())
319335
return false;
320336
Arg = OpCE->getArg(0)->IgnoreParenCasts();
321337
continue;
@@ -330,8 +346,15 @@ class RawPtrRefLambdaCapturesChecker
330346
}
331347
break;
332348
} while (Arg);
333-
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
334-
return ProtectedThisDecls.contains(DRE->getDecl());
349+
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
350+
auto *Decl = DRE->getDecl();
351+
if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(Decl)) {
352+
auto kind = ImplicitParam->getParameterKind();
353+
return kind == ImplicitParamKind::ObjCSelf ||
354+
kind == ImplicitParamKind::CXXThis;
355+
}
356+
return ProtectedThisDecls.contains(Decl);
357+
}
335358
return isa<CXXThisExpr>(Arg);
336359
}
337360
};
@@ -351,10 +374,16 @@ class RawPtrRefLambdaCapturesChecker
351374
ValueDecl *CapturedVar = C.getCapturedVar();
352375
if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
353376
continue;
377+
if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(CapturedVar)) {
378+
auto kind = ImplicitParam->getParameterKind();
379+
if ((kind == ImplicitParamKind::ObjCSelf ||
380+
kind == ImplicitParamKind::CXXThis) && !shouldCheckThis)
381+
continue;
382+
}
354383
QualType CapturedVarQualType = CapturedVar->getType();
355384
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
356385
if (IsUncountedPtr && *IsUncountedPtr)
357-
reportBug(C, CapturedVar, CapturedVarQualType);
386+
reportBug(C, CapturedVar, CapturedVarQualType, L);
358387
} else if (C.capturesThis() && shouldCheckThis) {
359388
if (ignoreParamVarDecl) // this is always a parameter to this function.
360389
continue;
@@ -364,11 +393,12 @@ class RawPtrRefLambdaCapturesChecker
364393
}
365394

366395
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
367-
const QualType T) const {
396+
const QualType T, LambdaExpr *L) const {
368397
assert(CapturedVar);
369398

370-
if (isa<ImplicitParamDecl>(CapturedVar) && !Capture.getLocation().isValid())
371-
return; // Ignore implicit captruing of self.
399+
auto Location = Capture.getLocation();
400+
if (isa<ImplicitParamDecl>(CapturedVar) && !Location.isValid())
401+
Location = L->getBeginLoc();
372402

373403
SmallString<100> Buf;
374404
llvm::raw_svector_ostream Os(Buf);
@@ -387,7 +417,7 @@ class RawPtrRefLambdaCapturesChecker
387417
printQuotedQualifiedName(Os, CapturedVar);
388418
Os << " to " << ptrKind(T) << " type is unsafe.";
389419

390-
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
420+
PathDiagnosticLocation BSLoc(Location, BR->getSourceManager());
391421
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
392422
BR->emitReport(std::move(Report));
393423
}
@@ -429,6 +459,10 @@ class UncountedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
429459
return result2;
430460
}
431461

462+
virtual bool isPtrType(const std::string &Name) const final {
463+
return isRefType(Name) || isCheckedPtr(Name);
464+
}
465+
432466
const char *ptrKind(QualType QT) const final {
433467
if (isUncounted(QT))
434468
return "uncounted";
@@ -445,7 +479,11 @@ class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
445479
}
446480

447481
std::optional<bool> isUnsafePtr(QualType QT) const final {
448-
return RTC->isUnretained(QT);
482+
return RTC->isUnretained(QT, /* ignoreARC */ true);
483+
}
484+
485+
virtual bool isPtrType(const std::string &Name) const final {
486+
return isRetainPtr(Name);
449487
}
450488

451489
const char *ptrKind(QualType QT) const final { return "unretained"; }

‎clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures-arc.mm‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,19 +123,23 @@ explicit CallableWrapper(CallableType& callable)
123123
void raw_ptr() {
124124
SomeObj* obj = make_obj();
125125
auto foo1 = [obj](){
126+
// expected-warning@-1{{Captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
126127
[obj doWork];
127128
};
128129
call(foo1);
129130

130131
auto foo2 = [&obj](){
132+
// expected-warning@-1{{Captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
131133
[obj doWork];
132134
};
133135
auto foo3 = [&](){
134136
[obj doWork];
137+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
135138
obj = nullptr;
136139
};
137140
auto foo4 = [=](){
138141
[obj doWork];
142+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
139143
};
140144

141145
auto cf = make_cf();
@@ -218,6 +222,7 @@ void noescape_lambda() {
218222
[otherObj doWork];
219223
}, [&](SomeObj *obj) {
220224
[otherObj doWork];
225+
// expected-warning@-1{{Implicitly captured raw-pointer 'otherObj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
221226
});
222227
([&] {
223228
[someObj doWork];
@@ -242,11 +247,13 @@ void lambda_converted_to_function(SomeObj* obj, CFMutableArrayRef cf)
242247
{
243248
callFunction([&]() {
244249
[obj doWork];
250+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
245251
CFArrayAppendValue(cf, nullptr);
246252
// expected-warning@-1{{Implicitly captured reference 'cf' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
247253
});
248254
callFunctionOpaque([&]() {
249255
[obj doWork];
256+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
250257
CFArrayAppendValue(cf, nullptr);
251258
// expected-warning@-1{{Implicitly captured reference 'cf' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
252259
});
@@ -262,6 +269,7 @@ -(void)run;
262269
@implementation ObjWithSelf
263270
-(void)doWork {
264271
auto doWork = [&] {
272+
// expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
265273
someFunction();
266274
[delegate doWork];
267275
};

‎clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,17 +279,33 @@ @interface ObjWithSelf : NSObject {
279279
RetainPtr<id> delegate;
280280
}
281281
-(void)doWork;
282+
-(void)doMoreWork;
282283
-(void)run;
283284
@end
284285

285286
@implementation ObjWithSelf
286287
-(void)doWork {
287288
auto doWork = [&] {
289+
// expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
290+
someFunction();
291+
[delegate doWork];
292+
};
293+
auto doExtraWork = [&, protectedSelf = retainPtr(self)] {
288294
someFunction();
289295
[delegate doWork];
290296
};
291297
callFunctionOpaque(doWork);
298+
callFunctionOpaque(doExtraWork);
292299
}
300+
301+
-(void)doMoreWork {
302+
auto doWork = [self, protectedSelf = retainPtr(self)] {
303+
someFunction();
304+
[self doWork];
305+
};
306+
callFunctionOpaque(doWork);
307+
}
308+
293309
-(void)run {
294310
someFunction();
295311
}

0 commit comments

Comments
 (0)