Skip to content

Commit b47e231

Browse files
rniwarniwa
authored
[alpha.webkit.UncountedLocalVarsChecker] Warn the use of a raw pointer/reference when the guardian variable gets mutated. (#113859)
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 df0d249 commit b47e231

File tree

3 files changed

+177
-6
lines changed

3 files changed

+177
-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) : Guardian(Guardian) {
58+
assert(Guardian);
59+
}
60+
61+
bool VisitBinaryOperator(const BinaryOperator *BO) {
62+
if (BO->isAssignmentOp()) {
63+
if (auto *VarRef = dyn_cast<DeclRefExpr>(BO->getLHS())) {
64+
if (VarRef->getDecl() == Guardian)
65+
return false;
66+
}
67+
}
68+
return true;
69+
}
70+
71+
bool VisitCXXConstructExpr(const CXXConstructExpr *CE) {
72+
if (auto *Ctor = CE->getConstructor()) {
73+
if (Ctor->isMoveConstructor() && CE->getNumArgs() == 1) {
74+
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
75+
if (auto *VarRef = dyn_cast<DeclRefExpr>(Arg)) {
76+
if (VarRef->getDecl() == Guardian)
77+
return false;
78+
}
79+
}
80+
}
81+
return true;
82+
}
83+
84+
bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
85+
auto MethodName = safeGetName(MCE->getMethodDecl());
86+
if (MethodName == "swap" || MethodName == "leakRef" ||
87+
MethodName == "releaseNonNull") {
88+
auto *ThisArg = MCE->getImplicitObjectArgument()->IgnoreParenCasts();
89+
if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) {
90+
if (VarRef->getDecl() == Guardian)
91+
return false;
92+
}
93+
}
94+
return true;
95+
}
96+
97+
bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE) {
98+
if (OCE->isAssignmentOp()) {
99+
assert(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: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,83 @@ 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+
RefCountable *bar = foo.get();
116+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
117+
foo = obj ? obj : nullptr;
118+
bar->method();
119+
}
120+
{
121+
RefCountable *bar = foo->trivial() ? foo.get() : nullptr;
122+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
123+
foo = nullptr;
124+
bar->method();
125+
}
126+
}
127+
128+
void foo9(RefCountable& o) {
129+
Ref<RefCountable> guardian(o);
130+
{
131+
RefCountable &bar = guardian.get();
132+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
133+
guardian = o; // We don't detect that we're setting it to the same value.
134+
bar.method();
135+
}
136+
{
137+
RefCountable *bar = guardian.ptr();
138+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
139+
Ref<RefCountable> other(*bar); // We don't detect other has the same value as guardian.
140+
guardian.swap(other);
141+
bar->method();
142+
}
143+
{
144+
RefCountable *bar = guardian.ptr();
145+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
146+
Ref<RefCountable> other(static_cast<Ref<RefCountable>&&>(guardian));
147+
bar->method();
148+
}
149+
{
150+
RefCountable *bar = guardian.ptr();
151+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
152+
guardian.leakRef();
153+
bar->method();
154+
}
155+
{
156+
RefCountable *bar = guardian.ptr();
157+
// expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
158+
guardian = o.trivial() ? o : *bar;
159+
bar->method();
160+
}
161+
}
162+
86163
} // namespace guardian_scopes
87164

88165
namespace auto_keyword {

0 commit comments

Comments
 (0)