Skip to content

Commit dc53e06

Browse files
rniwarniwa
committed
[alpha.webkit.UnretainedLambdaCapturesChecker] Add a WebKit checker for lambda capturing NS or CF types.
Add a new WebKit checker for checking that lambda captures of CF types use RetainPtr either when ARC is disabled or enabled, and those of NS types use RetainPtr when ARC is disabled.
1 parent b335d5a commit dc53e06

File tree

10 files changed

+851
-60
lines changed

10 files changed

+851
-60
lines changed

‎clang/docs/analyzer/checkers.rst‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3487,6 +3487,18 @@ Raw pointers and references to an object which supports CheckedPtr or CheckedRef
34873487
34883488
See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
34893489
3490+
alpha.webkit.UnretainedLambdaCapturesChecker
3491+
""""""""""""""""""""""""""""""""""""""""""""
3492+
Raw pointers and references to unretained types can't be captured in lambdas. Only RetainPtr is allowed.
3493+
3494+
.. code-block:: cpp
3495+
3496+
void foo(NSObject *a, NSObject *b) {
3497+
[&, a](){ // warn about 'a'
3498+
do_something(b); // warn about 'b'
3499+
};
3500+
};
3501+
34903502
.. _alpha-webkit-UncountedCallArgsChecker:
34913503
34923504
alpha.webkit.UncountedCallArgsChecker

‎clang/include/clang/StaticAnalyzer/Checkers/Checkers.td‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1766,6 +1766,10 @@ def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">,
17661766
HelpText<"Check for no unchecked member variables.">,
17671767
Documentation<HasDocumentation>;
17681768

1769+
def UnretainedLambdaCapturesChecker : Checker<"UnretainedLambdaCapturesChecker">,
1770+
HelpText<"Check unretained lambda captures.">,
1771+
Documentation<HasDocumentation>;
1772+
17691773
def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
17701774
HelpText<"Check uncounted call arguments.">,
17711775
Documentation<HasDocumentation>;

‎clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,14 +128,14 @@ add_clang_library(clangStaticAnalyzerCheckers
128128
VLASizeChecker.cpp
129129
ValistChecker.cpp
130130
VirtualCallChecker.cpp
131-
WebKit/RawPtrRefMemberChecker.cpp
132131
WebKit/ASTUtils.cpp
133132
WebKit/MemoryUnsafeCastChecker.cpp
134133
WebKit/PtrTypesSemantics.cpp
135134
WebKit/RefCntblBaseVirtualDtorChecker.cpp
136135
WebKit/RawPtrRefCallArgsChecker.cpp
137-
WebKit/UncountedLambdaCapturesChecker.cpp
136+
WebKit/RawPtrRefLambdaCapturesChecker.cpp
138137
WebKit/RawPtrRefLocalVarsChecker.cpp
138+
WebKit/RawPtrRefMemberChecker.cpp
139139

140140
LINK_LIBS
141141
clangAST

‎clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp‎ renamed to ‎clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp‎

Lines changed: 112 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,23 @@ using namespace clang;
2121
using namespace ento;
2222

2323
namespace {
24-
class UncountedLambdaCapturesChecker
24+
class RawPtrRefLambdaCapturesChecker
2525
: public Checker<check::ASTDecl<TranslationUnitDecl>> {
2626
private:
27-
BugType Bug{this, "Lambda capture of uncounted variable",
28-
"WebKit coding guidelines"};
27+
BugType Bug;
2928
mutable BugReporter *BR = nullptr;
3029
TrivialFunctionAnalysis TFA;
3130

31+
protected:
32+
mutable std::optional<RetainTypeChecker> RTC;
33+
3234
public:
35+
RawPtrRefLambdaCapturesChecker(const char *description)
36+
: Bug(this, description, "WebKit coding guidelines") {}
37+
38+
virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
39+
virtual const char *ptrKind(QualType QT) const = 0;
40+
3341
void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
3442
BugReporter &BRArg) const {
3543
BR = &BRArg;
@@ -38,15 +46,15 @@ class UncountedLambdaCapturesChecker
3846
// visit template instantiations or lambda classes. We
3947
// want to visit those, so we make our own RecursiveASTVisitor.
4048
struct LocalVisitor : DynamicRecursiveASTVisitor {
41-
const UncountedLambdaCapturesChecker *Checker;
49+
const RawPtrRefLambdaCapturesChecker *Checker;
4250
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
4351
llvm::DenseSet<const LambdaExpr *> LambdasToIgnore;
4452
llvm::DenseSet<const ValueDecl *> ProtectedThisDecls;
4553
llvm::DenseSet<const CXXConstructExpr *> ConstructToIgnore;
4654

4755
QualType ClsType;
4856

49-
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
57+
explicit LocalVisitor(const RawPtrRefLambdaCapturesChecker *Checker)
5058
: Checker(Checker) {
5159
assert(Checker);
5260
ShouldVisitTemplateInstantiations = true;
@@ -60,16 +68,23 @@ class UncountedLambdaCapturesChecker
6068
return DynamicRecursiveASTVisitor::TraverseCXXMethodDecl(CXXMD);
6169
}
6270

71+
bool VisitTypedefDecl(TypedefDecl *TD) override {
72+
if (Checker->RTC)
73+
Checker->RTC->visitTypedef(TD);
74+
return true;
75+
}
76+
6377
bool shouldCheckThis() {
64-
auto result =
65-
!ClsType.isNull() ? isUnsafePtr(ClsType, false) : std::nullopt;
78+
auto result = !ClsType.isNull() ?
79+
Checker->isUnsafePtr(ClsType) : std::nullopt;
6680
return result && *result;
6781
}
6882

6983
bool VisitLambdaExpr(LambdaExpr *L) override {
7084
if (LambdasToIgnore.contains(L))
7185
return true;
72-
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
86+
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
87+
ClsType);
7388
return true;
7489
}
7590

@@ -97,7 +112,8 @@ class UncountedLambdaCapturesChecker
97112
if (!L)
98113
return true;
99114
LambdasToIgnore.insert(L);
100-
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
115+
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
116+
ClsType);
101117
return true;
102118
}
103119

@@ -123,7 +139,8 @@ class UncountedLambdaCapturesChecker
123139
LambdasToIgnore.insert(L);
124140
if (!Param->hasAttr<NoEscapeAttr>())
125141
Checker->visitLambdaExpr(L, shouldCheckThis() &&
126-
!hasProtectedThis(L));
142+
!hasProtectedThis(L),
143+
ClsType);
127144
}
128145
++ArgIndex;
129146
}
@@ -144,7 +161,8 @@ class UncountedLambdaCapturesChecker
144161
LambdasToIgnore.insert(L);
145162
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
146163
Checker->visitLambdaExpr(L, shouldCheckThis() &&
147-
!hasProtectedThis(L));
164+
!hasProtectedThis(L),
165+
ClsType);
148166
}
149167
++ArgIndex;
150168
}
@@ -169,14 +187,22 @@ class UncountedLambdaCapturesChecker
169187
auto *CtorArg = CE->getArg(0)->IgnoreParenCasts();
170188
if (!CtorArg)
171189
return nullptr;
172-
if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg)) {
190+
auto *InnerCE = dyn_cast_or_null<CXXConstructExpr>(CtorArg);
191+
if (InnerCE && InnerCE->getNumArgs())
192+
CtorArg = InnerCE->getArg(0)->IgnoreParenCasts();
193+
auto updateIgnoreList = [&] {
173194
ConstructToIgnore.insert(CE);
195+
if (InnerCE)
196+
ConstructToIgnore.insert(InnerCE);
197+
};
198+
if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg)) {
199+
updateIgnoreList();
174200
return Lambda;
175201
}
176202
if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
177203
E = TempExpr->getSubExpr()->IgnoreParenCasts();
178204
if (auto *Lambda = dyn_cast<LambdaExpr>(E)) {
179-
ConstructToIgnore.insert(CE);
205+
updateIgnoreList();
180206
return Lambda;
181207
}
182208
}
@@ -189,10 +215,14 @@ class UncountedLambdaCapturesChecker
189215
auto *Init = VD->getInit();
190216
if (!Init)
191217
return nullptr;
218+
if (auto *Lambda = dyn_cast<LambdaExpr>(Init)) {
219+
updateIgnoreList();
220+
return Lambda;
221+
}
192222
TempExpr = dyn_cast<CXXBindTemporaryExpr>(Init->IgnoreParenCasts());
193223
if (!TempExpr)
194224
return nullptr;
195-
ConstructToIgnore.insert(CE);
225+
updateIgnoreList();
196226
return dyn_cast_or_null<LambdaExpr>(TempExpr->getSubExpr());
197227
}
198228

@@ -226,7 +256,7 @@ class UncountedLambdaCapturesChecker
226256
DeclRefExprsToIgnore.insert(ArgRef);
227257
LambdasToIgnore.insert(L);
228258
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
229-
/* ignoreParamVarDecl */ true);
259+
ClsType, /* ignoreParamVarDecl */ true);
230260
}
231261

232262
bool hasProtectedThis(LambdaExpr *L) {
@@ -293,10 +323,13 @@ class UncountedLambdaCapturesChecker
293323
};
294324

295325
LocalVisitor visitor(this);
326+
if (RTC)
327+
RTC->visitTranslationUnitDecl(TUD);
296328
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
297329
}
298330

299331
void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis,
332+
const QualType T,
300333
bool ignoreParamVarDecl = false) const {
301334
if (TFA.isTrivial(L->getBody()))
302335
return;
@@ -306,20 +339,23 @@ class UncountedLambdaCapturesChecker
306339
if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
307340
continue;
308341
QualType CapturedVarQualType = CapturedVar->getType();
309-
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType(), false);
342+
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
310343
if (IsUncountedPtr && *IsUncountedPtr)
311344
reportBug(C, CapturedVar, CapturedVarQualType);
312345
} else if (C.capturesThis() && shouldCheckThis) {
313346
if (ignoreParamVarDecl) // this is always a parameter to this function.
314347
continue;
315-
reportBugOnThisPtr(C);
348+
reportBugOnThisPtr(C, T);
316349
}
317350
}
318351
}
319352

320353
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
321354
const QualType T) const {
322355
assert(CapturedVar);
356+
357+
if (isa<ImplicitParamDecl>(CapturedVar) && !Capture.getLocation().isValid())
358+
return; // Ignore implicit captruing of self.
323359

324360
SmallString<100> Buf;
325361
llvm::raw_svector_ostream Os(Buf);
@@ -329,22 +365,22 @@ class UncountedLambdaCapturesChecker
329365
} else {
330366
Os << "Implicitly captured ";
331367
}
332-
if (T->isPointerType()) {
368+
if (isa<PointerType>(T) || isa<ObjCObjectPointerType>(T)) {
333369
Os << "raw-pointer ";
334370
} else {
335-
assert(T->isReferenceType());
336371
Os << "reference ";
337372
}
338373

339-
printQuotedQualifiedName(Os, Capture.getCapturedVar());
340-
Os << " to ref-counted type or CheckedPtr-capable type is unsafe.";
374+
printQuotedQualifiedName(Os, CapturedVar);
375+
Os << " to " << ptrKind(T) << " type is unsafe.";
341376

342377
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
343378
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
344379
BR->emitReport(std::move(Report));
345380
}
346381

347-
void reportBugOnThisPtr(const LambdaCapture &Capture) const {
382+
void reportBugOnThisPtr(const LambdaCapture &Capture,
383+
const QualType T) const {
348384
SmallString<100> Buf;
349385
llvm::raw_svector_ostream Os(Buf);
350386

@@ -354,14 +390,57 @@ class UncountedLambdaCapturesChecker
354390
Os << "Implicitly captured ";
355391
}
356392

357-
Os << "raw-pointer 'this' to ref-counted type or CheckedPtr-capable type "
358-
"is unsafe.";
393+
Os << "raw-pointer 'this' to " << ptrKind(T) << " type is unsafe.";
359394

360395
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
361396
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
362397
BR->emitReport(std::move(Report));
363398
}
364399
};
400+
401+
class UncountedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
402+
public:
403+
UncountedLambdaCapturesChecker()
404+
: RawPtrRefLambdaCapturesChecker("Lambda capture of uncounted or "
405+
"unchecked variable") {}
406+
407+
std::optional<bool> isUnsafePtr(QualType QT) const final {
408+
auto result1 = isUncountedPtr(QT);
409+
auto result2 = isUncheckedPtr(QT);
410+
if (result1 && *result1)
411+
return true;
412+
if (result2 && *result2)
413+
return true;
414+
if (result1)
415+
return *result1;
416+
return result2;
417+
}
418+
419+
const char *ptrKind(QualType QT) const final {
420+
if (isUncounted(QT))
421+
return "uncounted";
422+
return "unchecked";
423+
}
424+
425+
};
426+
427+
class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
428+
public:
429+
UnretainedLambdaCapturesChecker()
430+
: RawPtrRefLambdaCapturesChecker("Lambda capture of unretained "
431+
"variables") {
432+
RTC = RetainTypeChecker();
433+
}
434+
435+
std::optional<bool> isUnsafePtr(QualType QT) const final {
436+
return RTC->isUnretained(QT);
437+
}
438+
439+
const char *ptrKind(QualType QT) const final {
440+
return "unretained";
441+
}
442+
};
443+
365444
} // namespace
366445

367446
void ento::registerUncountedLambdaCapturesChecker(CheckerManager &Mgr) {
@@ -372,3 +451,12 @@ bool ento::shouldRegisterUncountedLambdaCapturesChecker(
372451
const CheckerManager &mgr) {
373452
return true;
374453
}
454+
455+
void ento::registerUnretainedLambdaCapturesChecker(CheckerManager &Mgr) {
456+
Mgr.registerChecker<UnretainedLambdaCapturesChecker>();
457+
}
458+
459+
bool ento::shouldRegisterUnretainedLambdaCapturesChecker(
460+
const CheckerManager &mgr) {
461+
return true;
462+
}

0 commit comments

Comments
 (0)