Skip to content

Commit 7314aa9

Browse files
rniwaRyosuke Niwa
rniwa
authored and
Ryosuke Niwa
committed
[alpha.webkit.UnretainedLambdaCapturesChecker] Add a WebKit checker for lambda capturing NS or CF types. (llvm#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 e31b229 commit 7314aa9

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
@@ -3479,6 +3479,18 @@ Raw pointers and references to an object which supports CheckedPtr or CheckedRef
34793479
34803480
See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
34813481
3482+
alpha.webkit.UnretainedLambdaCapturesChecker
3483+
""""""""""""""""""""""""""""""""""""""""""""
3484+
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.
3485+
3486+
.. code-block:: cpp
3487+
3488+
void foo(NSObject *a, NSObject *b) {
3489+
[&, a](){ // warn about 'a'
3490+
do_something(b); // warn about 'b'
3491+
};
3492+
};
3493+
34823494
.. _alpha-webkit-UncountedCallArgsChecker:
34833495
34843496
alpha.webkit.UncountedCallArgsChecker

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

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

1798+
def UnretainedLambdaCapturesChecker : Checker<"UnretainedLambdaCapturesChecker">,
1799+
HelpText<"Check unretained lambda captures.">,
1800+
Documentation<HasDocumentation>;
1801+
17981802
def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
17991803
HelpText<"Check uncounted call arguments.">,
18001804
Documentation<HasDocumentation>;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,14 +133,14 @@ add_clang_library(clangStaticAnalyzerCheckers
133133
VLASizeChecker.cpp
134134
ValistChecker.cpp
135135
VirtualCallChecker.cpp
136-
WebKit/RawPtrRefMemberChecker.cpp
137136
WebKit/ASTUtils.cpp
138137
WebKit/MemoryUnsafeCastChecker.cpp
139138
WebKit/PtrTypesSemantics.cpp
140139
WebKit/RefCntblBaseVirtualDtorChecker.cpp
141140
WebKit/RawPtrRefCallArgsChecker.cpp
142-
WebKit/UncountedLambdaCapturesChecker.cpp
141+
WebKit/RawPtrRefLambdaCapturesChecker.cpp
143142
WebKit/RawPtrRefLocalVarsChecker.cpp
143+
WebKit/RawPtrRefMemberChecker.cpp
144144

145145
LINK_LIBS
146146
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,7 +46,7 @@ class UncountedLambdaCapturesChecker
3846
// visit template instantiations or lambda classes. We
3947
// want to visit those, so we make our own RecursiveASTVisitor.
4048
struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
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;
@@ -48,7 +56,7 @@ class UncountedLambdaCapturesChecker
4856

4957
using Base = RecursiveASTVisitor<LocalVisitor>;
5058

51-
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
59+
explicit LocalVisitor(const RawPtrRefLambdaCapturesChecker *Checker)
5260
: Checker(Checker) {
5361
assert(Checker);
5462
}
@@ -63,16 +71,23 @@ class UncountedLambdaCapturesChecker
6371
return Base::TraverseCXXMethodDecl(CXXMD);
6472
}
6573

74+
bool VisitTypedefDecl(TypedefDecl *TD) {
75+
if (Checker->RTC)
76+
Checker->RTC->visitTypedef(TD);
77+
return true;
78+
}
79+
6680
bool shouldCheckThis() {
6781
auto result =
68-
!ClsType.isNull() ? isUnsafePtr(ClsType, false) : std::nullopt;
82+
!ClsType.isNull() ? Checker->isUnsafePtr(ClsType) : std::nullopt;
6983
return result && *result;
7084
}
7185

7286
bool VisitLambdaExpr(LambdaExpr *L) {
7387
if (LambdasToIgnore.contains(L))
7488
return true;
75-
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
89+
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
90+
ClsType);
7691
return true;
7792
}
7893

@@ -100,7 +115,8 @@ class UncountedLambdaCapturesChecker
100115
if (!L)
101116
return true;
102117
LambdasToIgnore.insert(L);
103-
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
118+
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
119+
ClsType);
104120
return true;
105121
}
106122

@@ -125,8 +141,8 @@ class UncountedLambdaCapturesChecker
125141
if (auto *L = findLambdaInArg(Arg)) {
126142
LambdasToIgnore.insert(L);
127143
if (!Param->hasAttr<NoEscapeAttr>())
128-
Checker->visitLambdaExpr(L, shouldCheckThis() &&
129-
!hasProtectedThis(L));
144+
Checker->visitLambdaExpr(
145+
L, shouldCheckThis() && !hasProtectedThis(L), ClsType);
130146
}
131147
++ArgIndex;
132148
}
@@ -146,8 +162,8 @@ class UncountedLambdaCapturesChecker
146162
if (auto *L = findLambdaInArg(Arg)) {
147163
LambdasToIgnore.insert(L);
148164
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
149-
Checker->visitLambdaExpr(L, shouldCheckThis() &&
150-
!hasProtectedThis(L));
165+
Checker->visitLambdaExpr(
166+
L, shouldCheckThis() && !hasProtectedThis(L), ClsType);
151167
}
152168
++ArgIndex;
153169
}
@@ -172,14 +188,22 @@ class UncountedLambdaCapturesChecker
172188
auto *CtorArg = CE->getArg(0)->IgnoreParenCasts();
173189
if (!CtorArg)
174190
return nullptr;
175-
if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg)) {
191+
auto *InnerCE = dyn_cast_or_null<CXXConstructExpr>(CtorArg);
192+
if (InnerCE && InnerCE->getNumArgs())
193+
CtorArg = InnerCE->getArg(0)->IgnoreParenCasts();
194+
auto updateIgnoreList = [&] {
176195
ConstructToIgnore.insert(CE);
196+
if (InnerCE)
197+
ConstructToIgnore.insert(InnerCE);
198+
};
199+
if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg)) {
200+
updateIgnoreList();
177201
return Lambda;
178202
}
179203
if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
180204
E = TempExpr->getSubExpr()->IgnoreParenCasts();
181205
if (auto *Lambda = dyn_cast<LambdaExpr>(E)) {
182-
ConstructToIgnore.insert(CE);
206+
updateIgnoreList();
183207
return Lambda;
184208
}
185209
}
@@ -192,10 +216,14 @@ class UncountedLambdaCapturesChecker
192216
auto *Init = VD->getInit();
193217
if (!Init)
194218
return nullptr;
219+
if (auto *Lambda = dyn_cast<LambdaExpr>(Init)) {
220+
updateIgnoreList();
221+
return Lambda;
222+
}
195223
TempExpr = dyn_cast<CXXBindTemporaryExpr>(Init->IgnoreParenCasts());
196224
if (!TempExpr)
197225
return nullptr;
198-
ConstructToIgnore.insert(CE);
226+
updateIgnoreList();
199227
return dyn_cast_or_null<LambdaExpr>(TempExpr->getSubExpr());
200228
}
201229

@@ -229,7 +257,7 @@ class UncountedLambdaCapturesChecker
229257
DeclRefExprsToIgnore.insert(ArgRef);
230258
LambdasToIgnore.insert(L);
231259
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
232-
/* ignoreParamVarDecl */ true);
260+
ClsType, /* ignoreParamVarDecl */ true);
233261
}
234262

235263
bool hasProtectedThis(LambdaExpr *L) {
@@ -296,10 +324,12 @@ class UncountedLambdaCapturesChecker
296324
};
297325

298326
LocalVisitor visitor(this);
327+
if (RTC)
328+
RTC->visitTranslationUnitDecl(TUD);
299329
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
300330
}
301331

302-
void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis,
332+
void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis, const QualType T,
303333
bool ignoreParamVarDecl = false) const {
304334
if (TFA.isTrivial(L->getBody()))
305335
return;
@@ -309,13 +339,13 @@ class UncountedLambdaCapturesChecker
309339
if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
310340
continue;
311341
QualType CapturedVarQualType = CapturedVar->getType();
312-
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType(), false);
342+
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
313343
if (IsUncountedPtr && *IsUncountedPtr)
314344
reportBug(C, CapturedVar, CapturedVarQualType);
315345
} else if (C.capturesThis() && shouldCheckThis) {
316346
if (ignoreParamVarDecl) // this is always a parameter to this function.
317347
continue;
318-
reportBugOnThisPtr(C);
348+
reportBugOnThisPtr(C, T);
319349
}
320350
}
321351
}
@@ -324,6 +354,9 @@ class UncountedLambdaCapturesChecker
324354
const QualType T) const {
325355
assert(CapturedVar);
326356

357+
if (isa<ImplicitParamDecl>(CapturedVar) && !Capture.getLocation().isValid())
358+
return; // Ignore implicit captruing of self.
359+
327360
SmallString<100> Buf;
328361
llvm::raw_svector_ostream Os(Buf);
329362

@@ -332,22 +365,22 @@ class UncountedLambdaCapturesChecker
332365
} else {
333366
Os << "Implicitly captured ";
334367
}
335-
if (T->isPointerType()) {
368+
if (isa<PointerType>(T) || isa<ObjCObjectPointerType>(T)) {
336369
Os << "raw-pointer ";
337370
} else {
338-
assert(T->isReferenceType());
339371
Os << "reference ";
340372
}
341373

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

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

350-
void reportBugOnThisPtr(const LambdaCapture &Capture) const {
382+
void reportBugOnThisPtr(const LambdaCapture &Capture,
383+
const QualType T) const {
351384
SmallString<100> Buf;
352385
llvm::raw_svector_ostream Os(Buf);
353386

@@ -357,14 +390,54 @@ class UncountedLambdaCapturesChecker
357390
Os << "Implicitly captured ";
358391
}
359392

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

363395
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
364396
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
365397
BR->emitReport(std::move(Report));
366398
}
367399
};
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+
class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
427+
public:
428+
UnretainedLambdaCapturesChecker()
429+
: RawPtrRefLambdaCapturesChecker("Lambda capture of unretained "
430+
"variables") {
431+
RTC = RetainTypeChecker();
432+
}
433+
434+
std::optional<bool> isUnsafePtr(QualType QT) const final {
435+
return RTC->isUnretained(QT);
436+
}
437+
438+
const char *ptrKind(QualType QT) const final { return "unretained"; }
439+
};
440+
368441
} // namespace
369442

370443
void ento::registerUncountedLambdaCapturesChecker(CheckerManager &Mgr) {
@@ -375,3 +448,12 @@ bool ento::shouldRegisterUncountedLambdaCapturesChecker(
375448
const CheckerManager &mgr) {
376449
return true;
377450
}
451+
452+
void ento::registerUnretainedLambdaCapturesChecker(CheckerManager &Mgr) {
453+
Mgr.registerChecker<UnretainedLambdaCapturesChecker>();
454+
}
455+
456+
bool ento::shouldRegisterUnretainedLambdaCapturesChecker(
457+
const CheckerManager &mgr) {
458+
return true;
459+
}

0 commit comments

Comments
 (0)