Skip to content

Commit 1524ca5

Browse files
rniwarniwa
committed
[webkit.UncountedLambdaCapturesChecker] Ignore trivial functions and [[clang::noescape]].
This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial functions as well as the one being passed to an argument with [[clang::noescape]] attribute. This dramatically reduces the false positive rate for this checker. To do this, this PR replaces VisitLambdaExpr in favor of checking lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a lambda is defined but never called or stored somewhere, then capturing whatever variable in such a lambda is harmless. VisitCallExpr explicitly looks for direct invocation of lambdas and registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda is being passed to a function, it checks whether its argument is annotated with [[clang::noescape]]. If it's not annotated such, it checks captures for their safety. Because WTF::switchOn could not be annotated with [[clang::noescape]] as function type parameters are variadic template function so we hard-code this function into the checker. Finally, this PR also converts the accompanying test to use -verify and adds a bunch of tests.
1 parent fb33af0 commit 1524ca5

File tree

4 files changed

+233
-34
lines changed

4 files changed

+233
-34
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,10 @@ std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
6363
/// class, false if not, std::nullopt if inconclusive.
6464
std::optional<bool> isUncountedPtr(const clang::QualType T);
6565

66+
/// \returns true if \p T is either a raw pointer or reference to an uncounted
67+
/// or unchecked class, false if not, std::nullopt if inconclusive.
68+
std::optional<bool> isUnsafePtr(const QualType T);
69+
6670
/// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
6771
/// variant, false if not.
6872
bool isSafePtrType(const clang::QualType T);

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

Lines changed: 92 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
//
77
//===----------------------------------------------------------------------===//
88

9+
#include "ASTUtils.h"
910
#include "DiagOutputUtils.h"
1011
#include "PtrTypesSemantics.h"
1112
#include "clang/AST/CXXInheritance.h"
@@ -26,6 +27,7 @@ class UncountedLambdaCapturesChecker
2627
BugType Bug{this, "Lambda capture of uncounted variable",
2728
"WebKit coding guidelines"};
2829
mutable BugReporter *BR = nullptr;
30+
TrivialFunctionAnalysis TFA;
2931

3032
public:
3133
void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -37,6 +39,8 @@ class UncountedLambdaCapturesChecker
3739
// want to visit those, so we make our own RecursiveASTVisitor.
3840
struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
3941
const UncountedLambdaCapturesChecker *Checker;
42+
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
43+
4044
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
4145
: Checker(Checker) {
4246
assert(Checker);
@@ -45,8 +49,61 @@ class UncountedLambdaCapturesChecker
4549
bool shouldVisitTemplateInstantiations() const { return true; }
4650
bool shouldVisitImplicitCode() const { return false; }
4751

48-
bool VisitLambdaExpr(LambdaExpr *L) {
49-
Checker->visitLambdaExpr(L);
52+
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
53+
if (DeclRefExprsToIgnore.contains(DRE))
54+
return true;
55+
if (auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) {
56+
auto *Init = VD->getInit()->IgnoreParenCasts();
57+
if (auto *L = dyn_cast_or_null<LambdaExpr>(Init)) {
58+
Checker->visitLambdaExpr(L);
59+
return true;
60+
}
61+
}
62+
return true;
63+
}
64+
65+
// WTF::switchOn(T, F... f) is a variadic template function and couldn't be annotated with NOESCAPE.
66+
// We hard code it here to workaround that.
67+
bool shouldTreatAllArgAsNoEscape(FunctionDecl *Decl) {
68+
auto *NsDecl = Decl->getParent();
69+
if (!NsDecl || !isa<NamespaceDecl>(NsDecl))
70+
return false;
71+
return safeGetName(NsDecl) == "WTF" && safeGetName(Decl) == "switchOn";
72+
}
73+
74+
bool VisitCallExpr(CallExpr *CE) {
75+
if (auto *Callee = CE->getCallee()) {
76+
if (auto *DRE = dyn_cast<DeclRefExpr>(Callee->IgnoreParenCasts())) {
77+
if (auto *MD = dyn_cast_or_null<CXXMethodDecl>(DRE->getDecl())) {
78+
if (CE->getNumArgs() == 1) {
79+
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
80+
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
81+
if (auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) {
82+
auto *Init = VD->getInit()->IgnoreParenCasts();
83+
if (auto *L = dyn_cast_or_null<LambdaExpr>(Init)) {
84+
DeclRefExprsToIgnore.insert(DRE);
85+
Checker->visitLambdaExpr(L, /* ignoreParamVarDecl */ true);
86+
}
87+
}
88+
}
89+
}
90+
}
91+
}
92+
}
93+
if (auto *Callee = CE->getDirectCallee()) {
94+
bool TreatAllArgsAsNoEscape = shouldTreatAllArgAsNoEscape(Callee);
95+
unsigned ArgIndex = 0;
96+
for (auto *Param : Callee->parameters()) {
97+
if (ArgIndex >= CE->getNumArgs())
98+
break;
99+
auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
100+
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape) {
101+
if (auto *L = dyn_cast_or_null<LambdaExpr>(Arg->IgnoreParenCasts()))
102+
Checker->visitLambdaExpr(L);
103+
}
104+
++ArgIndex;
105+
}
106+
}
50107
return true;
51108
}
52109
};
@@ -55,22 +112,28 @@ class UncountedLambdaCapturesChecker
55112
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
56113
}
57114

58-
void visitLambdaExpr(LambdaExpr *L) const {
115+
void visitLambdaExpr(LambdaExpr *L, bool ignoreParamVarDecl = false) const {
116+
if (TFA.isTrivial(L->getBody()))
117+
return;
59118
for (const LambdaCapture &C : L->captures()) {
60119
if (C.capturesVariable()) {
61120
ValueDecl *CapturedVar = C.getCapturedVar();
121+
if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
122+
continue;
62123
QualType CapturedVarQualType = CapturedVar->getType();
63-
if (auto *CapturedVarType = CapturedVarQualType.getTypePtrOrNull()) {
64-
auto IsUncountedPtr = isUncountedPtr(CapturedVarQualType);
65-
if (IsUncountedPtr && *IsUncountedPtr)
66-
reportBug(C, CapturedVar, CapturedVarType);
67-
}
124+
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
125+
if (IsUncountedPtr && *IsUncountedPtr)
126+
reportBug(C, CapturedVar, CapturedVarQualType);
127+
} else if (C.capturesThis()) {
128+
if (ignoreParamVarDecl) // this is always a parameter to this function.
129+
continue;
130+
reportBugOnThisPtr(C);
68131
}
69132
}
70133
}
71134

72135
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
73-
const Type *T) const {
136+
const QualType T) const {
74137
assert(CapturedVar);
75138

76139
SmallString<100> Buf;
@@ -89,7 +152,26 @@ class UncountedLambdaCapturesChecker
89152
}
90153

91154
printQuotedQualifiedName(Os, Capture.getCapturedVar());
92-
Os << " to uncounted type is unsafe.";
155+
Os << " to ref-counted / CheckedPtr capable type is unsafe.";
156+
157+
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
158+
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
159+
BR->emitReport(std::move(Report));
160+
}
161+
162+
void reportBugOnThisPtr(const LambdaCapture &Capture) const {
163+
assert(CapturedVar);
164+
165+
SmallString<100> Buf;
166+
llvm::raw_svector_ostream Os(Buf);
167+
168+
if (Capture.isExplicit()) {
169+
Os << "Captured ";
170+
} else {
171+
Os << "Implicitly captured ";
172+
}
173+
174+
Os << "raw-pointer 'this' to ref-counted / CheckedPtr capable type is unsafe.";
93175

94176
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
95177
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);

‎clang/test/Analysis/Checkers/WebKit/mock-types.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,9 @@ struct RefCountable {
103103
void ref() {}
104104
void deref() {}
105105
void method();
106+
void constMethod() const;
106107
int trivial() { return 123; }
108+
RefCountable* next();
107109
};
108110

109111
template <typename T> T *downcast(T *t) { return t; }

‎clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp‎

Lines changed: 135 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,73 @@
1-
// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker %s 2>&1 | FileCheck %s --strict-whitespace
2-
#include "mock-types.h"
1+
// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker -verify %s
2+
//#include "mock-types.h"
3+
4+
struct RefCountable {
5+
// static Ref<RefCountable> create();
6+
void ref() {}
7+
void deref() {}
8+
void method();
9+
void constMethod() const;
10+
int trivial() { return 123; }
11+
RefCountable* next();
12+
};
13+
14+
RefCountable* make_obj();
15+
16+
void someFunction();
17+
template <typename Callback> void call(Callback callback) {
18+
someFunction();
19+
callback();
20+
}
321

422
void raw_ptr() {
5-
RefCountable* ref_countable = nullptr;
6-
auto foo1 = [ref_countable](){};
7-
// CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
8-
// CHECK-NEXT:{{^ 6 | }} auto foo1 = [ref_countable](){};
9-
// CHECK-NEXT:{{^ | }} ^
10-
auto foo2 = [&ref_countable](){};
11-
// CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
12-
auto foo3 = [&](){ ref_countable = nullptr; };
13-
// CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
14-
// CHECK-NEXT:{{^ 12 | }} auto foo3 = [&](){ ref_countable = nullptr; };
15-
// CHECK-NEXT:{{^ | }} ^
16-
auto foo4 = [=](){ (void) ref_countable; };
17-
// CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
23+
RefCountable* ref_countable = make_obj();
24+
auto foo1 = [ref_countable](){
25+
// expected-warning@-1{{Captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
26+
ref_countable->method();
27+
};
28+
auto foo2 = [&ref_countable](){
29+
// expected-warning@-1{{Captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
30+
ref_countable->method();
31+
};
32+
auto foo3 = [&](){
33+
ref_countable->method();
34+
// expected-warning@-1{{Implicitly captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
35+
ref_countable = nullptr;
36+
};
37+
38+
auto foo4 = [=](){
39+
ref_countable->method();
40+
// expected-warning@-1{{Implicitly captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
41+
};
42+
43+
call(foo1);
44+
call(foo2);
45+
call(foo3);
46+
call(foo4);
1847

1948
// Confirm that the checker respects [[clang::suppress]].
2049
RefCountable* suppressed_ref_countable = nullptr;
2150
[[clang::suppress]] auto foo5 = [suppressed_ref_countable](){};
22-
// CHECK-NOT: warning: Captured raw-pointer 'suppressed_ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
51+
// no warning.
52+
call(foo5);
2353
}
2454

2555
void references() {
2656
RefCountable automatic;
2757
RefCountable& ref_countable_ref = automatic;
58+
auto foo1 = [ref_countable_ref](){ ref_countable_ref.constMethod(); };
59+
// expected-warning@-1{{Captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
60+
auto foo2 = [&ref_countable_ref](){ ref_countable_ref.method(); };
61+
// expected-warning@-1{{Captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
62+
auto foo3 = [&](){ ref_countable_ref.method(); };
63+
// expected-warning@-1{{Implicitly captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
64+
auto foo4 = [=](){ ref_countable_ref.constMethod(); };
65+
// expected-warning@-1{{Implicitly captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
2866

29-
auto foo1 = [ref_countable_ref](){};
30-
// CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
31-
auto foo2 = [&ref_countable_ref](){};
32-
// CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
33-
auto foo3 = [&](){ (void) ref_countable_ref; };
34-
// CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
35-
auto foo4 = [=](){ (void) ref_countable_ref; };
36-
// CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
67+
call(foo1);
68+
call(foo2);
69+
call(foo3);
70+
call(foo4);
3771
}
3872

3973
void quiet() {
@@ -45,5 +79,82 @@ void quiet() {
4579

4680
auto foo3 = [&]() {};
4781
auto foo4 = [=]() {};
82+
83+
call(foo3);
84+
call(foo4);
85+
4886
RefCountable *ref_countable = nullptr;
4987
}
88+
89+
template <typename Callback>
90+
void map(RefCountable* start, [[clang::noescape]] Callback&& callback)
91+
{
92+
while (start) {
93+
callback(*start);
94+
start = start->next();
95+
}
96+
}
97+
98+
template <typename Callback1, typename Callback2>
99+
void doubleMap(RefCountable* start, [[clang::noescape]] Callback1&& callback1, Callback2&& callback2)
100+
{
101+
while (start) {
102+
callback1(*start);
103+
callback2(*start);
104+
start = start->next();
105+
}
106+
}
107+
108+
void noescape_lambda() {
109+
RefCountable* someObj = make_obj();
110+
RefCountable* otherObj = make_obj();
111+
map(make_obj(), [&](RefCountable& obj) {
112+
otherObj->method();
113+
});
114+
doubleMap(make_obj(), [&](RefCountable& obj) {
115+
otherObj->method();
116+
}, [&](RefCountable& obj) {
117+
otherObj->method();
118+
// expected-warning@-1{{Implicitly captured raw-pointer 'otherObj' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
119+
});
120+
([&] {
121+
someObj->method();
122+
})();
123+
}
124+
125+
void lambda_capture_param(RefCountable* obj) {
126+
auto someLambda = [&] {
127+
obj->method();
128+
};
129+
someLambda();
130+
someLambda();
131+
}
132+
133+
struct RefCountableWithLambdaCapturingThis {
134+
void ref() const;
135+
void deref() const;
136+
void nonTrivial();
137+
138+
void method_captures_this_safe() {
139+
auto lambda = [&]() {
140+
nonTrivial();
141+
};
142+
lambda();
143+
}
144+
145+
void method_captures_this_unsafe() {
146+
auto lambda = [&]() {
147+
nonTrivial();
148+
// expected-warning@-1{{Implicitly captured raw-pointer 'this' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
149+
};
150+
call(lambda);
151+
}
152+
};
153+
154+
void trivial_lambda() {
155+
RefCountable* ref_countable = make_obj();
156+
auto trivial_lambda = [&]() {
157+
return ref_countable->trivial();
158+
};
159+
trivial_lambda();
160+
}

0 commit comments

Comments
 (0)