[clang] b47e231 - [alpha.webkit.UncountedLocalVarsChecker] Warn the use of a raw pointer/reference when the guardian variable gets mutated. (#113859)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 29 23:13:27 PDT 2024
Author: Ryosuke Niwa
Date: 2024-10-29T23:13:23-07:00
New Revision: b47e2316bf083cd2e0e5ac2ef1e9c913f839a51b
URL: https://github.com/llvm/llvm-project/commit/b47e2316bf083cd2e0e5ac2ef1e9c913f839a51b
DIFF: https://github.com/llvm/llvm-project/commit/b47e2316bf083cd2e0e5ac2ef1e9c913f839a51b.diff
LOG: [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.
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
clang/test/Analysis/Checkers/WebKit/mock-types.h
clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 5cdf047738abcb..76a4599cc8d788 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -48,6 +48,65 @@ bool isRefcountedStringsHack(const VarDecl *V) {
return false;
}
+struct GuardianVisitor : public RecursiveASTVisitor<GuardianVisitor> {
+ using Base = RecursiveASTVisitor<GuardianVisitor>;
+
+ const VarDecl *Guardian{nullptr};
+
+public:
+ explicit GuardianVisitor(const VarDecl *Guardian) : Guardian(Guardian) {
+ assert(Guardian);
+ }
+
+ bool VisitBinaryOperator(const BinaryOperator *BO) {
+ if (BO->isAssignmentOp()) {
+ if (auto *VarRef = dyn_cast<DeclRefExpr>(BO->getLHS())) {
+ if (VarRef->getDecl() == Guardian)
+ return false;
+ }
+ }
+ return true;
+ }
+
+ bool VisitCXXConstructExpr(const CXXConstructExpr *CE) {
+ if (auto *Ctor = CE->getConstructor()) {
+ if (Ctor->isMoveConstructor() && CE->getNumArgs() == 1) {
+ auto *Arg = CE->getArg(0)->IgnoreParenCasts();
+ if (auto *VarRef = dyn_cast<DeclRefExpr>(Arg)) {
+ if (VarRef->getDecl() == Guardian)
+ return false;
+ }
+ }
+ }
+ return true;
+ }
+
+ bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
+ auto MethodName = safeGetName(MCE->getMethodDecl());
+ if (MethodName == "swap" || MethodName == "leakRef" ||
+ MethodName == "releaseNonNull") {
+ auto *ThisArg = MCE->getImplicitObjectArgument()->IgnoreParenCasts();
+ if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) {
+ if (VarRef->getDecl() == Guardian)
+ return false;
+ }
+ }
+ return true;
+ }
+
+ bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE) {
+ if (OCE->isAssignmentOp()) {
+ assert(OCE->getNumArgs() == 2);
+ auto *ThisArg = OCE->getArg(0)->IgnoreParenCasts();
+ if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) {
+ if (VarRef->getDecl() == Guardian)
+ return false;
+ }
+ }
+ return true;
+ }
+};
+
bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
const VarDecl *MaybeGuardian) {
assert(Guarded);
@@ -81,7 +140,7 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
// We need to skip the first CompoundStmt to avoid situation when guardian is
// defined in the same scope as guarded variable.
- bool HaveSkippedFirstCompoundStmt = false;
+ const CompoundStmt *FirstCompondStmt = nullptr;
for (DynTypedNodeList guardedVarAncestors = ctx.getParents(*Guarded);
!guardedVarAncestors.empty();
guardedVarAncestors = ctx.getParents(
@@ -90,12 +149,15 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
) {
for (auto &guardedVarAncestor : guardedVarAncestors) {
if (auto *CStmtAncestor = guardedVarAncestor.get<CompoundStmt>()) {
- if (!HaveSkippedFirstCompoundStmt) {
- HaveSkippedFirstCompoundStmt = true;
+ if (!FirstCompondStmt) {
+ FirstCompondStmt = CStmtAncestor;
continue;
}
- if (CStmtAncestor == guardiansClosestCompStmtAncestor)
- return true;
+ if (CStmtAncestor == guardiansClosestCompStmtAncestor) {
+ GuardianVisitor guardianVisitor(MaybeGuardian);
+ auto *GuardedScope = const_cast<CompoundStmt *>(FirstCompondStmt);
+ return guardianVisitor.TraverseCompoundStmt(GuardedScope);
+ }
}
}
}
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index 8d8a90f0afae0e..82c79c97a83de6 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -49,7 +49,23 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
Ref() : t{} {};
Ref(T &t) : t(&RefDerefTraits::ref(t)) { }
Ref(const Ref& o) : t(RefDerefTraits::refIfNotNull(PtrTraits::unwrap(o.t))) { }
+ Ref(Ref&& o) : t(o.leakRef()) { }
~Ref() { RefDerefTraits::derefIfNotNull(PtrTraits::exchange(t, nullptr)); }
+ Ref& operator=(T &t) {
+ Ref o(t);
+ swap(o);
+ return *this;
+ }
+ Ref& operator=(Ref &&o) {
+ Ref m(o);
+ swap(m);
+ return *this;
+ }
+ void swap(Ref& o) {
+ typename PtrTraits::StorageType tmp = t;
+ t = o.t;
+ o.t = tmp;
+ }
T &get() { return *PtrTraits::unwrap(t); }
T *ptr() { return PtrTraits::unwrap(t); }
T *operator->() { return PtrTraits::unwrap(t); }
@@ -74,11 +90,27 @@ template <typename T> struct RefPtr {
if (t)
t->deref();
}
+ Ref<T> releaseNonNull() {
+ Ref<T> tmp(*t);
+ if (t)
+ t->deref();
+ t = nullptr;
+ return tmp;
+ }
+ void swap(RefPtr& o) {
+ T* tmp = t;
+ t = o.t;
+ o.t = tmp;
+ }
T *get() { return t; }
T *operator->() { return t; }
const T *operator->() const { return t; }
T &operator*() { return *t; }
- RefPtr &operator=(T *) { return *this; }
+ RefPtr &operator=(T *t) {
+ RefPtr o(t);
+ swap(o);
+ return *this;
+ }
operator bool() const { return t; }
};
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 1c0df42cdda663..d7fb689557a6fc 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -83,6 +83,83 @@ void foo7(RefCountable* obj) {
bar.obj->method();
}
+void foo8(RefCountable* obj) {
+ RefPtr<RefCountable> foo;
+ {
+ RefCountable *bar = foo.get();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ foo = nullptr;
+ bar->method();
+ }
+ RefPtr<RefCountable> baz;
+ {
+ RefCountable *bar = baz.get();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ baz = obj;
+ bar->method();
+ }
+ foo = nullptr;
+ {
+ RefCountable *bar = foo.get();
+ // No warning. It's okay to mutate RefPtr in an outer scope.
+ bar->method();
+ }
+ foo = obj;
+ {
+ RefCountable *bar = foo.get();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ foo.releaseNonNull();
+ bar->method();
+ }
+ {
+ RefCountable *bar = foo.get();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ foo = obj ? obj : nullptr;
+ bar->method();
+ }
+ {
+ RefCountable *bar = foo->trivial() ? foo.get() : nullptr;
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ foo = nullptr;
+ bar->method();
+ }
+}
+
+void foo9(RefCountable& o) {
+ Ref<RefCountable> guardian(o);
+ {
+ RefCountable &bar = guardian.get();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ guardian = o; // We don't detect that we're setting it to the same value.
+ bar.method();
+ }
+ {
+ RefCountable *bar = guardian.ptr();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ Ref<RefCountable> other(*bar); // We don't detect other has the same value as guardian.
+ guardian.swap(other);
+ bar->method();
+ }
+ {
+ RefCountable *bar = guardian.ptr();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ Ref<RefCountable> other(static_cast<Ref<RefCountable>&&>(guardian));
+ bar->method();
+ }
+ {
+ RefCountable *bar = guardian.ptr();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ guardian.leakRef();
+ bar->method();
+ }
+ {
+ RefCountable *bar = guardian.ptr();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ guardian = o.trivial() ? o : *bar;
+ bar->method();
+ }
+}
+
} // namespace guardian_scopes
namespace auto_keyword {
More information about the cfe-commits
mailing list