Skip to content

Commit 6c6dd96

Browse files
rniwarniwa
committed
[webkit.UncountedLambdaCapturesChecker] Add the support for WTF::ScopeExit and WTF::makeVisitor
Lambda passed to WTF::ScopeExit / WTF::makeScopeExit and WTF::makeVisitor should be ignored by the lambda captures checker so long as its resulting object doesn't escape the current scope. Unfortunately, recognizing this pattern generally is too hard to do so directly hard-code these two function names to the checker.
1 parent 77a3d43 commit 6c6dd96

File tree

2 files changed

+239
-8
lines changed

2 files changed

+239
-8
lines changed

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

Lines changed: 88 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,9 @@ class RawPtrRefLambdaCapturesChecker
5050
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
5151
llvm::DenseSet<const LambdaExpr *> LambdasToIgnore;
5252
llvm::DenseSet<const ValueDecl *> ProtectedThisDecls;
53+
llvm::DenseSet<const CallExpr *> CallToIgnore;
5354
llvm::DenseSet<const CXXConstructExpr *> ConstructToIgnore;
55+
llvm::DenseMap<const VarDecl*, const LambdaExpr*> LambdaOwnerMap;
5456

5557
QualType ClsType;
5658

@@ -101,10 +103,60 @@ class RawPtrRefLambdaCapturesChecker
101103
auto *Init = VD->getInit();
102104
if (!Init)
103105
return true;
104-
auto *L = dyn_cast_or_null<LambdaExpr>(Init->IgnoreParenCasts());
105-
if (!L)
106+
if (auto *L = dyn_cast_or_null<LambdaExpr>(Init->IgnoreParenCasts())) {
107+
LambdasToIgnore.insert(L); // Evaluate lambdas in VisitDeclRefExpr.
108+
return true;
109+
}
110+
if (!VD->hasLocalStorage())
106111
return true;
107-
LambdasToIgnore.insert(L); // Evaluate lambdas in VisitDeclRefExpr.
112+
if (auto *E = dyn_cast<ExprWithCleanups>(Init))
113+
Init = E->getSubExpr();
114+
if (auto *E = dyn_cast<CXXBindTemporaryExpr>(Init))
115+
Init = E->getSubExpr();
116+
if (auto *CE = dyn_cast<CallExpr>(Init)) {
117+
if (auto *Callee = CE->getDirectCallee()) {
118+
auto FnName = safeGetName(Callee);
119+
unsigned ArgCnt = CE->getNumArgs();
120+
if (FnName == "makeScopeExit" && ArgCnt == 1) {
121+
auto *Arg = CE->getArg(0);
122+
if (auto *E = dyn_cast<MaterializeTemporaryExpr>(Arg))
123+
Arg = E->getSubExpr();
124+
if (auto *L = dyn_cast<LambdaExpr>(Arg)) {
125+
LambdaOwnerMap.insert(std::make_pair(VD, L));
126+
CallToIgnore.insert(CE);
127+
LambdasToIgnore.insert(L);
128+
}
129+
} else if (FnName == "makeVisitor") {
130+
for (unsigned ArgIndex = 0; ArgIndex < ArgCnt; ++ArgIndex) {
131+
auto *Arg = CE->getArg(ArgIndex);
132+
if (auto *E = dyn_cast<MaterializeTemporaryExpr>(Arg))
133+
Arg = E->getSubExpr();
134+
if (auto *L = dyn_cast<LambdaExpr>(Arg)) {
135+
LambdaOwnerMap.insert(std::make_pair(VD, L));
136+
CallToIgnore.insert(CE);
137+
LambdasToIgnore.insert(L);
138+
}
139+
}
140+
}
141+
}
142+
} else if (auto *CE = dyn_cast<CXXConstructExpr>(Init)) {
143+
if (auto *Ctor = CE->getConstructor()) {
144+
if (auto *Cls = Ctor->getParent()) {
145+
auto FnName = safeGetName(Cls);
146+
unsigned ArgCnt = CE->getNumArgs();
147+
if (FnName == "ScopeExit" && ArgCnt == 1) {
148+
auto *Arg = CE->getArg(0);
149+
if (auto *E = dyn_cast<MaterializeTemporaryExpr>(Arg))
150+
Arg = E->getSubExpr();
151+
if (auto *L = dyn_cast<LambdaExpr>(Arg)) {
152+
LambdaOwnerMap.insert(std::make_pair(VD, L));
153+
ConstructToIgnore.insert(CE);
154+
LambdasToIgnore.insert(L);
155+
}
156+
}
157+
}
158+
}
159+
}
108160
return true;
109161
}
110162

@@ -114,6 +166,12 @@ class RawPtrRefLambdaCapturesChecker
114166
auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl());
115167
if (!VD)
116168
return true;
169+
if (auto It = LambdaOwnerMap.find(VD); It != LambdaOwnerMap.end()) {
170+
auto *L = It->second;
171+
Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
172+
ClsType);
173+
return true;
174+
}
117175
auto *Init = VD->getInit();
118176
if (!Init)
119177
return true;
@@ -167,9 +225,30 @@ class RawPtrRefLambdaCapturesChecker
167225
}
168226

169227
bool VisitCallExpr(CallExpr *CE) override {
228+
if (CallToIgnore.contains(CE))
229+
return true;
170230
checkCalleeLambda(CE);
171-
if (auto *Callee = CE->getDirectCallee())
231+
if (auto *Callee = CE->getDirectCallee()) {
232+
if (auto *Ns = Callee->getParent()) {
233+
auto NsName = safeGetName(Ns);
234+
bool IsVisitFn = safeGetName(Callee) == "visit";
235+
bool ArgCnt = CE->getNumArgs();
236+
if (IsVisitFn && ArgCnt && (NsName == "WTF" || NsName == "std")) {
237+
if (auto *Arg = CE->getArg(0)) {
238+
Arg = Arg->IgnoreParenCasts();
239+
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
240+
if (auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
241+
if (LambdaOwnerMap.contains(VD)) {
242+
DeclRefExprsToIgnore.insert(DRE);
243+
return true;
244+
}
245+
}
246+
}
247+
}
248+
}
249+
}
172250
checkParameters(CE, Callee);
251+
}
173252
else if (auto *CalleeE = CE->getCallee()) {
174253
if (auto *DRE = dyn_cast<DeclRefExpr>(CalleeE->IgnoreParenCasts())) {
175254
if (auto *Callee = dyn_cast_or_null<FunctionDecl>(DRE->getDecl()))
@@ -280,7 +359,7 @@ class RawPtrRefLambdaCapturesChecker
280359
LambdasToIgnore.insert(L);
281360
}
282361

283-
bool hasProtectedThis(LambdaExpr *L) {
362+
bool hasProtectedThis(const LambdaExpr *L) {
284363
for (const LambdaCapture &OtherCapture : L->captures()) {
285364
if (!OtherCapture.capturesVariable())
286365
continue;
@@ -378,8 +457,9 @@ class RawPtrRefLambdaCapturesChecker
378457
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
379458
}
380459

381-
void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis, const QualType T,
382-
bool ignoreParamVarDecl = false) const {
460+
void visitLambdaExpr(const LambdaExpr *L, bool shouldCheckThis,
461+
const QualType T,
462+
bool ignoreParamVarDecl = false) const {
383463
if (TFA.isTrivial(L->getBody()))
384464
return;
385465
for (const LambdaCapture &C : L->captures()) {
@@ -410,7 +490,7 @@ class RawPtrRefLambdaCapturesChecker
410490
}
411491

412492
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
413-
const QualType T, LambdaExpr *L) const {
493+
const QualType T, const LambdaExpr *L) const {
414494
assert(CapturedVar);
415495

416496
auto Location = Capture.getLocation();

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

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,79 @@ class HashMap {
107107
ValueType* m_table { nullptr };
108108
};
109109

110+
class ScopeExit final {
111+
public:
112+
template<typename ExitFunctionParameter>
113+
explicit ScopeExit(ExitFunctionParameter&& exitFunction)
114+
: m_exitFunction(std::move(exitFunction)) {
115+
}
116+
117+
ScopeExit(ScopeExit&& other)
118+
: m_exitFunction(std::move(other.m_exitFunction))
119+
, m_execute(std::move(other.m_execute)) {
120+
}
121+
122+
~ScopeExit() {
123+
if (m_execute)
124+
m_exitFunction();
125+
}
126+
127+
WTF::Function<void()> take() {
128+
m_execute = false;
129+
return std::move(m_exitFunction);
130+
}
131+
132+
void release() { m_execute = false; }
133+
134+
ScopeExit(const ScopeExit&) = delete;
135+
ScopeExit& operator=(const ScopeExit&) = delete;
136+
ScopeExit& operator=(ScopeExit&&) = delete;
137+
138+
private:
139+
WTF::Function<void()> m_exitFunction;
140+
bool m_execute { true };
141+
};
142+
143+
template<typename ExitFunction> ScopeExit makeScopeExit(ExitFunction&&);
144+
template<typename ExitFunction>
145+
ScopeExit makeScopeExit(ExitFunction&& exitFunction)
146+
{
147+
return ScopeExit(std::move(exitFunction));
148+
}
149+
150+
// Visitor adapted from http://stackoverflow.com/questions/25338795/is-there-a-name-for-this-tuple-creation-idiom
151+
152+
template<class A, class... B> struct Visitor : Visitor<A>, Visitor<B...> {
153+
Visitor(A a, B... b)
154+
: Visitor<A>(a)
155+
, Visitor<B...>(b...)
156+
{
157+
}
158+
159+
using Visitor<A>::operator ();
160+
using Visitor<B...>::operator ();
161+
};
162+
163+
template<class A> struct Visitor<A> : A {
164+
Visitor(A a)
165+
: A(a)
166+
{
167+
}
168+
169+
using A::operator();
170+
};
171+
172+
template<class... F> Visitor<F...> makeVisitor(F... f)
173+
{
174+
return Visitor<F...>(f...);
175+
}
176+
177+
void opaqueFunction();
178+
template <typename Visitor, typename... Variants> void visit(Visitor&& v, Variants&&... values)
179+
{
180+
opaqueFunction();
181+
}
182+
110183
} // namespace WTF
111184

112185
struct A {
@@ -501,3 +574,81 @@ void RefCountedObj::call() const
501574
};
502575
callLambda(lambda);
503576
}
577+
578+
void scope_exit(RefCountable* obj) {
579+
auto scope = WTF::makeScopeExit([&] {
580+
obj->method();
581+
});
582+
someFunction();
583+
WTF::ScopeExit scope2([&] {
584+
obj->method();
585+
});
586+
someFunction();
587+
}
588+
589+
void doWhateverWith(WTF::ScopeExit& obj);
590+
591+
void scope_exit_with_side_effect(RefCountable* obj) {
592+
auto scope = WTF::makeScopeExit([&] {
593+
obj->method();
594+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
595+
});
596+
doWhateverWith(scope);
597+
}
598+
599+
void scope_exit_static(RefCountable* obj) {
600+
static auto scope = WTF::makeScopeExit([&] {
601+
obj->method();
602+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
603+
});
604+
}
605+
606+
WTF::Function<void()> scope_exit_take_lambda(RefCountable* obj) {
607+
auto scope = WTF::makeScopeExit([&] {
608+
obj->method();
609+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
610+
});
611+
return scope.take();
612+
}
613+
614+
// FIXME: Ideally, we treat release() as a trivial function.
615+
void scope_exit_release(RefCountable* obj) {
616+
auto scope = WTF::makeScopeExit([&] {
617+
obj->method();
618+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
619+
});
620+
scope.release();
621+
}
622+
623+
void make_visitor(RefCountable* obj) {
624+
auto visitor = WTF::makeVisitor([&] {
625+
obj->method();
626+
});
627+
}
628+
629+
void use_visitor(RefCountable* obj) {
630+
auto visitor = WTF::makeVisitor([&] {
631+
obj->method();
632+
});
633+
WTF::visit(visitor, obj);
634+
}
635+
636+
template <typename Visitor, typename ObjectType>
637+
void bad_visit(Visitor&, ObjectType*) {
638+
someFunction();
639+
}
640+
641+
void static_visitor(RefCountable* obj) {
642+
static auto visitor = WTF::makeVisitor([&] {
643+
obj->method();
644+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
645+
});
646+
}
647+
648+
void bad_use_visitor(RefCountable* obj) {
649+
auto visitor = WTF::makeVisitor([&] {
650+
obj->method();
651+
// expected-warning@-1{{Implicitly captured raw-pointer 'obj' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
652+
});
653+
bad_visit(visitor, obj);
654+
}

0 commit comments

Comments
 (0)