Skip to content

Commit 287781c

Browse files
rniwarniwa
authored
[webkit.UncountedLambdaCapturesChecker] Ignore trivial functions and [[clang::noescape]]. (#113845)
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 fdc7812 commit 287781c

File tree

4 files changed

+240
-33
lines changed

4 files changed

+240
-33
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: 99 additions & 9 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,32 +49,100 @@ class UncountedLambdaCapturesChecker
4549
bool shouldVisitTemplateInstantiations() const { return true; }
4650
bool shouldVisitImplicitCode() const { return false; }
4751

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

54120
LocalVisitor visitor(this);
55121
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
56122
}
57123

58-
void visitLambdaExpr(LambdaExpr *L) const {
124+
void visitLambdaExpr(LambdaExpr *L, bool ignoreParamVarDecl = false) const {
125+
if (TFA.isTrivial(L->getBody()))
126+
return;
59127
for (const LambdaCapture &C : L->captures()) {
60128
if (C.capturesVariable()) {
61129
ValueDecl *CapturedVar = C.getCapturedVar();
130+
if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
131+
continue;
62132
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-
}
133+
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
134+
if (IsUncountedPtr && *IsUncountedPtr)
135+
reportBug(C, CapturedVar, CapturedVarQualType);
136+
} else if (C.capturesThis()) {
137+
if (ignoreParamVarDecl) // this is always a parameter to this function.
138+
continue;
139+
reportBugOnThisPtr(C);
68140
}
69141
}
70142
}
71143

72144
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
73-
const Type *T) const {
145+
const QualType T) const {
74146
assert(CapturedVar);
75147

76148
SmallString<100> Buf;
@@ -89,7 +161,25 @@ class UncountedLambdaCapturesChecker
89161
}
90162

91163
printQuotedQualifiedName(Os, Capture.getCapturedVar());
92-
Os << " to uncounted type is unsafe.";
164+
Os << " to ref-counted / CheckedPtr capable type is unsafe.";
165+
166+
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
167+
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
168+
BR->emitReport(std::move(Report));
169+
}
170+
171+
void reportBugOnThisPtr(const LambdaCapture &Capture) const {
172+
SmallString<100> Buf;
173+
llvm::raw_svector_ostream Os(Buf);
174+
175+
if (Capture.isExplicit()) {
176+
Os << "Captured ";
177+
} else {
178+
Os << "Implicitly captured ";
179+
}
180+
181+
Os << "raw-pointer 'this' to ref-counted / CheckedPtr capable type is "
182+
"unsafe.";
93183

94184
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
95185
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
@@ -135,7 +135,9 @@ struct RefCountable {
135135
void ref() {}
136136
void deref() {}
137137
void method();
138+
void constMethod() const;
138139
int trivial() { return 123; }
140+
RefCountable* next();
139141
};
140142

141143
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)