Skip to content

Commit 8fce7f6

Browse files
rniwarniwa
committed
[alpha.webkit.UncountedLocalVarsChecker] Warn the use of a raw pointer/reference when the guardian variable gets mutated.
This checker has a notion of a guardian variable which is a variable and keeps the object pointed to by a raw pointer / reference in an inner scope alive long enough to "guard" it from use-after-free. But such a guardian variable fails to flawed to keep the object alive if it ever gets mutated within the scope of a raw pointer / reference. This PR fixes this bug by introducing a new AST visitor class, GuardianVisitor, which traverses the compound statements of a guarded variable (raw pointer / reference) and looks for any operator=, move constructor, or calls to "swap", "leakRef", or "releaseNonNull" functions.
1 parent fb33af0 commit 8fce7f6

File tree

3 files changed

+159
-6
lines changed

3 files changed

+159
-6
lines changed

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

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,65 @@ bool isRefcountedStringsHack(const VarDecl *V) {
4848
return false;
4949
}
5050

51+
struct GuardianVisitor : public RecursiveASTVisitor<GuardianVisitor> {
52+
using Base = RecursiveASTVisitor<GuardianVisitor>;
53+
54+
const VarDecl *Guardian { nullptr };
55+
56+
public:
57+
explicit GuardianVisitor(const VarDecl *Guardian)
58+
: Guardian(Guardian) {
59+
assert(Guardian);
60+
}
61+
62+
bool VisitBinaryOperator(const BinaryOperator *BO) {
63+
if (BO->isAssignmentOp()) {
64+
if (auto *VarRef = dyn_cast<DeclRefExpr>(BO->getLHS())) {
65+
if (VarRef->getDecl() == Guardian)
66+
return false;
67+
}
68+
}
69+
return true;
70+
}
71+
72+
bool VisitCXXConstructExpr(const CXXConstructExpr* CE) {
73+
if (auto *Ctor = CE->getConstructor()) {
74+
if (Ctor->isMoveConstructor() && CE->getNumArgs() == 1) {
75+
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
76+
if (auto *VarRef = dyn_cast<DeclRefExpr>(Arg)) {
77+
if (VarRef->getDecl() == Guardian)
78+
return false;
79+
}
80+
}
81+
}
82+
return true;
83+
}
84+
85+
bool VisitCXXMemberCallExpr(const CXXMemberCallExpr* MCE) {
86+
auto MethodName = safeGetName(MCE->getMethodDecl());
87+
if (MethodName == "swap" || MethodName == "leakRef" ||
88+
MethodName == "releaseNonNull") {
89+
auto *ThisArg = MCE->getImplicitObjectArgument()->IgnoreParenCasts();
90+
if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) {
91+
if (VarRef->getDecl() == Guardian)
92+
return false;
93+
}
94+
}
95+
return true;
96+
}
97+
98+
bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr* OCE) {
99+
if (OCE->isAssignmentOp() && OCE->getNumArgs() == 2) {
100+
auto *ThisArg = OCE->getArg(0)->IgnoreParenCasts();
101+
if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) {
102+
if (VarRef->getDecl() == Guardian)
103+
return false;
104+
}
105+
}
106+
return true;
107+
}
108+
};
109+
51110
bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
52111
const VarDecl *MaybeGuardian) {
53112
assert(Guarded);
@@ -81,7 +140,7 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
81140

82141
// We need to skip the first CompoundStmt to avoid situation when guardian is
83142
// defined in the same scope as guarded variable.
84-
bool HaveSkippedFirstCompoundStmt = false;
143+
const CompoundStmt *FirstCompondStmt = nullptr;
85144
for (DynTypedNodeList guardedVarAncestors = ctx.getParents(*Guarded);
86145
!guardedVarAncestors.empty();
87146
guardedVarAncestors = ctx.getParents(
@@ -90,12 +149,15 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
90149
) {
91150
for (auto &guardedVarAncestor : guardedVarAncestors) {
92151
if (auto *CStmtAncestor = guardedVarAncestor.get<CompoundStmt>()) {
93-
if (!HaveSkippedFirstCompoundStmt) {
94-
HaveSkippedFirstCompoundStmt = true;
152+
if (!FirstCompondStmt) {
153+
FirstCompondStmt = CStmtAncestor;
95154
continue;
96155
}
97-
if (CStmtAncestor == guardiansClosestCompStmtAncestor)
98-
return true;
156+
if (CStmtAncestor == guardiansClosestCompStmtAncestor) {
157+
GuardianVisitor guardianVisitor(MaybeGuardian);
158+
auto *GuardedScope = const_cast<CompoundStmt *>(FirstCompondStmt);
159+
return guardianVisitor.TraverseCompoundStmt(GuardedScope);
160+
}
99161
}
100162
}
101163
}

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

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,23 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
4949
Ref() : t{} {};
5050
Ref(T &t) : t(&RefDerefTraits::ref(t)) { }
5151
Ref(const Ref& o) : t(RefDerefTraits::refIfNotNull(PtrTraits::unwrap(o.t))) { }
52+
Ref(Ref&& o) : t(o.leakRef()) { }
5253
~Ref() { RefDerefTraits::derefIfNotNull(PtrTraits::exchange(t, nullptr)); }
54+
Ref& operator=(T &t) {
55+
Ref o(t);
56+
swap(o);
57+
return *this;
58+
}
59+
Ref& operator=(Ref &&o) {
60+
Ref m(o);
61+
swap(m);
62+
return *this;
63+
}
64+
void swap(Ref& o) {
65+
typename PtrTraits::StorageType tmp = t;
66+
t = o.t;
67+
o.t = tmp;
68+
}
5369
T &get() { return *PtrTraits::unwrap(t); }
5470
T *ptr() { return PtrTraits::unwrap(t); }
5571
T *operator->() { return PtrTraits::unwrap(t); }
@@ -74,11 +90,27 @@ template <typename T> struct RefPtr {
7490
if (t)
7591
t->deref();
7692
}
93+
Ref<T> releaseNonNull() {
94+
Ref<T> tmp(*t);
95+
if (t)
96+
t->deref();
97+
t = nullptr;
98+
return tmp;
99+
}
100+
void swap(RefPtr& o) {
101+
T* tmp = t;
102+
t = o.t;
103+
o.t = tmp;
104+
}
77105
T *get() { return t; }
78106
T *operator->() { return t; }
79107
const T *operator->() const { return t; }
80108
T &operator*() { return *t; }
81-
RefPtr &operator=(T *) { return *this; }
109+
RefPtr &operator=(T *t) {
110+
RefPtr o(t);
111+
swap(o);
112+
return *this;
113+
}
82114
operator bool() const { return t; }
83115
};
84116

‎clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,65 @@ void foo7(RefCountable* obj) {
8383
bar.obj->method();
8484
}
8585

86+
void foo8(RefCountable* obj) {
87+
RefPtr<RefCountable> foo;
88+
{
89+
RefCountable *bar = foo.get();
90+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
91+
foo = nullptr;
92+
bar->method();
93+
}
94+
RefPtr<RefCountable> baz;
95+
{
96+
RefCountable *bar = baz.get();
97+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
98+
baz = obj;
99+
bar->method();
100+
}
101+
foo = nullptr;
102+
{
103+
RefCountable *bar = foo.get();
104+
// No warning. It's okay to mutate RefPtr in an outer scope.
105+
bar->method();
106+
}
107+
foo = obj;
108+
{
109+
RefCountable *bar = foo.get();
110+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
111+
foo.releaseNonNull();
112+
bar->method();
113+
}
114+
}
115+
116+
void foo9(RefCountable& o) {
117+
Ref<RefCountable> guardian(o);
118+
{
119+
RefCountable &bar = guardian.get();
120+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
121+
guardian = o; // We don't detect that we're setting it to the same value.
122+
bar.method();
123+
}
124+
{
125+
RefCountable *bar = guardian.ptr();
126+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
127+
Ref<RefCountable> other(*bar); // We don't detect other has the same value as guardian.
128+
guardian.swap(other);
129+
bar->method();
130+
}
131+
{
132+
RefCountable *bar = guardian.ptr();
133+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
134+
Ref<RefCountable> other(static_cast<Ref<RefCountable>&&>(guardian));
135+
bar->method();
136+
}
137+
{
138+
RefCountable *bar = guardian.ptr();
139+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
140+
guardian.leakRef();
141+
bar->method();
142+
}
143+
}
144+
86145
} // namespace guardian_scopes
87146

88147
namespace auto_keyword {

0 commit comments

Comments
 (0)