Skip to content

Commit c07d305

Browse files
rniwarniwa
authored
[webkit.UncountedLambdaCapturesChecker] Add the support for WTF::ScopeExit and WTF::makeVisitor (#161926)
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 7c441b2 commit c07d305

File tree

2 files changed

+250
-8
lines changed

2 files changed

+250
-8
lines changed

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

Lines changed: 99 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,10 +225,14 @@ 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 (isVisitFunction(CE, Callee))
233+
return true;
172234
checkParameters(CE, Callee);
173-
else if (auto *CalleeE = CE->getCallee()) {
235+
} else if (auto *CalleeE = CE->getCallee()) {
174236
if (auto *DRE = dyn_cast<DeclRefExpr>(CalleeE->IgnoreParenCasts())) {
175237
if (auto *Callee = dyn_cast_or_null<FunctionDecl>(DRE->getDecl()))
176238
checkParameters(CE, Callee);
@@ -179,6 +241,34 @@ class RawPtrRefLambdaCapturesChecker
179241
return true;
180242
}
181243

244+
bool isVisitFunction(CallExpr *CallExpr, FunctionDecl *FnDecl) {
245+
bool IsVisitFn = safeGetName(FnDecl) == "visit";
246+
if (!IsVisitFn)
247+
return false;
248+
bool ArgCnt = CallExpr->getNumArgs();
249+
if (!ArgCnt)
250+
return false;
251+
auto *Ns = FnDecl->getParent();
252+
if (!Ns)
253+
return false;
254+
auto NsName = safeGetName(Ns);
255+
if (NsName != "WTF" && NsName != "std")
256+
return false;
257+
auto *Arg = CallExpr->getArg(0);
258+
if (!Arg)
259+
return false;
260+
auto *DRE = dyn_cast<DeclRefExpr>(Arg->IgnoreParenCasts());
261+
if (!DRE)
262+
return false;
263+
auto *VD = dyn_cast<VarDecl>(DRE->getDecl());
264+
if (!VD)
265+
return false;
266+
if (!LambdaOwnerMap.contains(VD))
267+
return false;
268+
DeclRefExprsToIgnore.insert(DRE);
269+
return true;
270+
}
271+
182272
void checkParameters(CallExpr *CE, FunctionDecl *Callee) {
183273
unsigned ArgIndex = isa<CXXOperatorCallExpr>(CE);
184274
bool TreatAllArgsAsNoEscape = shouldTreatAllArgAsNoEscape(Callee);
@@ -280,7 +370,7 @@ class RawPtrRefLambdaCapturesChecker
280370
LambdasToIgnore.insert(L);
281371
}
282372

283-
bool hasProtectedThis(LambdaExpr *L) {
373+
bool hasProtectedThis(const LambdaExpr *L) {
284374
for (const LambdaCapture &OtherCapture : L->captures()) {
285375
if (!OtherCapture.capturesVariable())
286376
continue;
@@ -378,7 +468,8 @@ class RawPtrRefLambdaCapturesChecker
378468
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
379469
}
380470

381-
void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis, const QualType T,
471+
void visitLambdaExpr(const LambdaExpr *L, bool shouldCheckThis,
472+
const QualType T,
382473
bool ignoreParamVarDecl = false) const {
383474
if (TFA.isTrivial(L->getBody()))
384475
return;
@@ -410,7 +501,7 @@ class RawPtrRefLambdaCapturesChecker
410501
}
411502

412503
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
413-
const QualType T, LambdaExpr *L) const {
504+
const QualType T, const LambdaExpr *L) const {
414505
assert(CapturedVar);
415506

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