Skip to content

Commit 5c3b059

Browse files
rniwarniwa
authored
[alpha.webkit.UnretainedLambdaCapturesChecker] Add a WebKit checker for lambda capturing NS or CF types. (#128651)
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 2e7a509 commit 5c3b059

File tree

10 files changed

+847
-62
lines changed

10 files changed

+847
-62
lines changed

‎clang/docs/analyzer/checkers.rst‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3522,6 +3522,18 @@ Raw pointers and references to an object which supports CheckedPtr or CheckedRef
35223522
35233523
See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
35243524
3525+
alpha.webkit.UnretainedLambdaCapturesChecker
3526+
""""""""""""""""""""""""""""""""""""""""""""
3527+
Raw pointers and references to NS or CF types can't be captured in lambdas. Only RetainPtr is allowed for CF types regardless of whether ARC is enabled or disabled, and only RetainPtr is allowed for NS types when ARC is disabled.
3528+
3529+
.. code-block:: cpp
3530+
3531+
void foo(NSObject *a, NSObject *b) {
3532+
[&, a](){ // warn about 'a'
3533+
do_something(b); // warn about 'b'
3534+
};
3535+
};
3536+
35253537
.. _alpha-webkit-UncountedCallArgsChecker:
35263538
35273539
alpha.webkit.UncountedCallArgsChecker

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

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

1768+
def UnretainedLambdaCapturesChecker : Checker<"UnretainedLambdaCapturesChecker">,
1769+
HelpText<"Check unretained lambda captures.">,
1770+
Documentation<HasDocumentation>;
1771+
17681772
def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
17691773
HelpText<"Check uncounted call arguments.">,
17701774
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: 108 additions & 26 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() {
6478
auto result =
65-
!ClsType.isNull() ? isUnsafePtr(ClsType, false) : std::nullopt;
79+
!ClsType.isNull() ? 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

@@ -122,8 +138,8 @@ class UncountedLambdaCapturesChecker
122138
if (auto *L = findLambdaInArg(Arg)) {
123139
LambdasToIgnore.insert(L);
124140
if (!Param->hasAttr<NoEscapeAttr>())
125-
Checker->visitLambdaExpr(L, shouldCheckThis() &&
126-
!hasProtectedThis(L));
141+
Checker->visitLambdaExpr(
142+
L, shouldCheckThis() && !hasProtectedThis(L), ClsType);
127143
}
128144
++ArgIndex;
129145
}
@@ -143,8 +159,8 @@ class UncountedLambdaCapturesChecker
143159
if (auto *L = findLambdaInArg(Arg)) {
144160
LambdasToIgnore.insert(L);
145161
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
146-
Checker->visitLambdaExpr(L, shouldCheckThis() &&
147-
!hasProtectedThis(L));
162+
Checker->visitLambdaExpr(
163+
L, shouldCheckThis() && !hasProtectedThis(L), ClsType);
148164
}
149165
++ArgIndex;
150166
}
@@ -169,14 +185,22 @@ class UncountedLambdaCapturesChecker
169185
auto *CtorArg = CE->getArg(0)->IgnoreParenCasts();
170186
if (!CtorArg)
171187
return nullptr;
172-
if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg)) {
188+
auto *InnerCE = dyn_cast_or_null<CXXConstructExpr>(CtorArg);
189+
if (InnerCE && InnerCE->getNumArgs())
190+
CtorArg = InnerCE->getArg(0)->IgnoreParenCasts();
191+
auto updateIgnoreList = [&] {
173192
ConstructToIgnore.insert(CE);
193+
if (InnerCE)
194+
ConstructToIgnore.insert(InnerCE);
195+
};
196+
if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg)) {
197+
updateIgnoreList();
174198
return Lambda;
175199
}
176200
if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
177201
E = TempExpr->getSubExpr()->IgnoreParenCasts();
178202
if (auto *Lambda = dyn_cast<LambdaExpr>(E)) {
179-
ConstructToIgnore.insert(CE);
203+
updateIgnoreList();
180204
return Lambda;
181205
}
182206
}
@@ -189,10 +213,14 @@ class UncountedLambdaCapturesChecker
189213
auto *Init = VD->getInit();
190214
if (!Init)
191215
return nullptr;
216+
if (auto *Lambda = dyn_cast<LambdaExpr>(Init)) {
217+
updateIgnoreList();
218+
return Lambda;
219+
}
192220
TempExpr = dyn_cast<CXXBindTemporaryExpr>(Init->IgnoreParenCasts());
193221
if (!TempExpr)
194222
return nullptr;
195-
ConstructToIgnore.insert(CE);
223+
updateIgnoreList();
196224
return dyn_cast_or_null<LambdaExpr>(TempExpr->getSubExpr());
197225
}
198226

@@ -226,7 +254,7 @@ class UncountedLambdaCapturesChecker
226254
DeclRefExprsToIgnore.insert(ArgRef);
227255
LambdasToIgnore.insert(L);
228256
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
229-
/* ignoreParamVarDecl */ true);
257+
ClsType, /* ignoreParamVarDecl */ true);
230258
}
231259

232260
bool hasProtectedThis(LambdaExpr *L) {
@@ -293,10 +321,12 @@ class UncountedLambdaCapturesChecker
293321
};
294322

295323
LocalVisitor visitor(this);
324+
if (RTC)
325+
RTC->visitTranslationUnitDecl(TUD);
296326
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
297327
}
298328

299-
void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis,
329+
void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis, const QualType T,
300330
bool ignoreParamVarDecl = false) const {
301331
if (TFA.isTrivial(L->getBody()))
302332
return;
@@ -306,13 +336,13 @@ class UncountedLambdaCapturesChecker
306336
if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
307337
continue;
308338
QualType CapturedVarQualType = CapturedVar->getType();
309-
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType(), false);
339+
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
310340
if (IsUncountedPtr && *IsUncountedPtr)
311341
reportBug(C, CapturedVar, CapturedVarQualType);
312342
} else if (C.capturesThis() && shouldCheckThis) {
313343
if (ignoreParamVarDecl) // this is always a parameter to this function.
314344
continue;
315-
reportBugOnThisPtr(C);
345+
reportBugOnThisPtr(C, T);
316346
}
317347
}
318348
}
@@ -321,6 +351,9 @@ class UncountedLambdaCapturesChecker
321351
const QualType T) const {
322352
assert(CapturedVar);
323353

354+
if (isa<ImplicitParamDecl>(CapturedVar) && !Capture.getLocation().isValid())
355+
return; // Ignore implicit captruing of self.
356+
324357
SmallString<100> Buf;
325358
llvm::raw_svector_ostream Os(Buf);
326359

@@ -329,22 +362,22 @@ class UncountedLambdaCapturesChecker
329362
} else {
330363
Os << "Implicitly captured ";
331364
}
332-
if (T->isPointerType()) {
365+
if (isa<PointerType>(T) || isa<ObjCObjectPointerType>(T)) {
333366
Os << "raw-pointer ";
334367
} else {
335-
assert(T->isReferenceType());
336368
Os << "reference ";
337369
}
338370

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

342374
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
343375
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
344376
BR->emitReport(std::move(Report));
345377
}
346378

347-
void reportBugOnThisPtr(const LambdaCapture &Capture) const {
379+
void reportBugOnThisPtr(const LambdaCapture &Capture,
380+
const QualType T) const {
348381
SmallString<100> Buf;
349382
llvm::raw_svector_ostream Os(Buf);
350383

@@ -354,14 +387,54 @@ class UncountedLambdaCapturesChecker
354387
Os << "Implicitly captured ";
355388
}
356389

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

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

367440
void ento::registerUncountedLambdaCapturesChecker(CheckerManager &Mgr) {
@@ -372,3 +445,12 @@ bool ento::shouldRegisterUncountedLambdaCapturesChecker(
372445
const CheckerManager &mgr) {
373446
return true;
374447
}
448+
449+
void ento::registerUnretainedLambdaCapturesChecker(CheckerManager &Mgr) {
450+
Mgr.registerChecker<UnretainedLambdaCapturesChecker>();
451+
}
452+
453+
bool ento::shouldRegisterUnretainedLambdaCapturesChecker(
454+
const CheckerManager &mgr) {
455+
return true;
456+
}

0 commit comments

Comments
 (0)