Skip to content

Commit 2c31403

Browse files
rniwarniwa
authored
[alpha.webkit.UnretainedLambdaCapturesChecker] Add the support for protectedSelf (#132518)
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.
1 parent ef1088f commit 2c31403

File tree

4 files changed

+95
-14
lines changed

4 files changed

+95
-14
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: 50 additions & 11 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);
@@ -275,10 +285,10 @@ class RawPtrRefLambdaCapturesChecker
275285
auto *VD = dyn_cast<VarDecl>(ValueDecl);
276286
if (!VD)
277287
return false;
278-
auto *Init = VD->getInit()->IgnoreParenCasts();
288+
auto *Init = VD->getInit();
279289
if (!Init)
280290
return false;
281-
const Expr *Arg = Init;
291+
const Expr *Arg = Init->IgnoreParenCasts();
282292
do {
283293
if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
284294
Arg = BTE->getSubExpr()->IgnoreParenCasts();
@@ -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,17 @@ 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) &&
381+
!shouldCheckThis)
382+
continue;
383+
}
354384
QualType CapturedVarQualType = CapturedVar->getType();
355385
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
356386
if (IsUncountedPtr && *IsUncountedPtr)
357-
reportBug(C, CapturedVar, CapturedVarQualType);
387+
reportBug(C, CapturedVar, CapturedVarQualType, L);
358388
} else if (C.capturesThis() && shouldCheckThis) {
359389
if (ignoreParamVarDecl) // this is always a parameter to this function.
360390
continue;
@@ -364,11 +394,12 @@ class RawPtrRefLambdaCapturesChecker
364394
}
365395

366396
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
367-
const QualType T) const {
397+
const QualType T, LambdaExpr *L) const {
368398
assert(CapturedVar);
369399

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

373404
SmallString<100> Buf;
374405
llvm::raw_svector_ostream Os(Buf);
@@ -387,7 +418,7 @@ class RawPtrRefLambdaCapturesChecker
387418
printQuotedQualifiedName(Os, CapturedVar);
388419
Os << " to " << ptrKind(T) << " type is unsafe.";
389420

390-
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
421+
PathDiagnosticLocation BSLoc(Location, BR->getSourceManager());
391422
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
392423
BR->emitReport(std::move(Report));
393424
}
@@ -429,6 +460,10 @@ class UncountedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
429460
return result2;
430461
}
431462

463+
virtual bool isPtrType(const std::string &Name) const final {
464+
return isRefType(Name) || isCheckedPtr(Name);
465+
}
466+
432467
const char *ptrKind(QualType QT) const final {
433468
if (isUncounted(QT))
434469
return "uncounted";
@@ -448,6 +483,10 @@ class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
448483
return RTC->isUnretained(QT);
449484
}
450485

486+
virtual bool isPtrType(const std::string &Name) const final {
487+
return isRetainPtr(Name);
488+
}
489+
451490
const char *ptrKind(QualType QT) const final { return "unretained"; }
452491
};
453492

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,7 @@ @interface ObjWithSelf : NSObject {
256256
RetainPtr<id> delegate;
257257
}
258258
-(void)doWork;
259+
-(void)doMoreWork;
259260
-(void)run;
260261
@end
261262

@@ -265,9 +266,28 @@ -(void)doWork {
265266
someFunction();
266267
[delegate doWork];
267268
};
269+
auto doMoreWork = [=] {
270+
someFunction();
271+
[delegate doWork];
272+
};
273+
auto doExtraWork = [&, protectedSelf = retainPtr(self)] {
274+
someFunction();
275+
[delegate doWork];
276+
};
268277
callFunctionOpaque(doWork);
278+
callFunctionOpaque(doMoreWork);
279+
callFunctionOpaque(doExtraWork);
269280
}
281+
282+
-(void)doMoreWork {
283+
auto doWork = [self, protectedSelf = retainPtr(self)] {
284+
someFunction();
285+
[self doWork];
286+
};
287+
callFunctionOpaque(doWork);
288+
}
289+
270290
-(void)run {
271291
someFunction();
272292
}
273-
@end
293+
@end

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,18 +279,40 @@ @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 doMoreWork = [=] {
294+
// expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
295+
someFunction();
296+
[delegate doWork];
297+
};
298+
auto doExtraWork = [&, protectedSelf = retainPtr(self)] {
288299
someFunction();
289300
[delegate doWork];
290301
};
291302
callFunctionOpaque(doWork);
303+
callFunctionOpaque(doMoreWork);
304+
callFunctionOpaque(doExtraWork);
292305
}
306+
307+
-(void)doMoreWork {
308+
auto doWork = [self, protectedSelf = retainPtr(self)] {
309+
someFunction();
310+
[self doWork];
311+
};
312+
callFunctionOpaque(doWork);
313+
}
314+
293315
-(void)run {
294316
someFunction();
295317
}
296-
@end
318+
@end

0 commit comments

Comments
 (0)