https://github.com/rniwa updated
https://github.com/llvm/llvm-project/pull/113859
>From 8fce7f69eb5e28f6ec648ee0dc4cc23c793f64c0 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@webkit.org>
Date: Sun, 27 Oct 2024 21:43:24 -0700
Subject: [PATCH 1/3] [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.
---
.../WebKit/UncountedLocalVarsChecker.cpp | 72 +++++++++++++++++--
.../Analysis/Checkers/WebKit/mock-types.h | 34 ++++++++-
.../Checkers/WebKit/uncounted-local-vars.cpp | 59 +++++++++++++++
3 files changed, 159 insertions(+), 6 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 5cdf047738abcb..5f5fb6e2bf1f87 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() && 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..7028d7bb0ba160 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -83,6 +83,65 @@ void foo7(RefCountable* obj) {
bar.obj->method();
}
+void foo8(RefCountable* obj) {
+ RefPtr<RefCountable> foo;
+ {
+ RefCountable *bar = foo.get();
+ // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe
[alpha.webkit.UncountedLocalVarsChecker]}}
+ foo = nullptr;
+ bar->method();
+ }
+ RefPtr<RefCountable> baz;
+ {
+ RefCountable *bar = baz.get();
+ // expected-warning@-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@-1{{Local variable 'bar' is uncounted and unsafe
[alpha.webkit.UncountedLocalVarsChecker]}}
+ foo.releaseNonNull();
+ bar->method();
+ }
+}
+
+void foo9(RefCountable& o) {
+ Ref<RefCountable> guardian(o);
+ {
+ RefCountable &bar = guardian.get();
+ // expected-warning@-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@-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@-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@-1{{Local variable 'bar' is uncounted and unsafe
[alpha.webkit.UncountedLocalVarsChecker]}}
+ guardian.leakRef();
+ bar->method();
+ }
+}
+
} // namespace guardian_scopes
namespace auto_keyword {
>From 075f5677dd68ed4e5bf29138bf6202dd788621f9 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@webkit.org>
Date: Mon, 28 Oct 2024 08:36:56 -0700
Subject: [PATCH 2/3] Fix formatting.
---
.../Checkers/WebKit/UncountedLocalVarsChecker.cpp | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 5f5fb6e2bf1f87..5978b23760650f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -51,11 +51,10 @@ bool isRefcountedStringsHack(const VarDecl *V) {
struct GuardianVisitor : public RecursiveASTVisitor<GuardianVisitor> {
using Base = RecursiveASTVisitor<GuardianVisitor>;
- const VarDecl *Guardian { nullptr };
+ const VarDecl *Guardian{nullptr};
public:
- explicit GuardianVisitor(const VarDecl *Guardian)
- : Guardian(Guardian) {
+ explicit GuardianVisitor(const VarDecl *Guardian) : Guardian(Guardian) {
assert(Guardian);
}
@@ -69,7 +68,7 @@ struct GuardianVisitor : public
RecursiveASTVisitor<GuardianVisitor> {
return true;
}
- bool VisitCXXConstructExpr(const CXXConstructExpr* CE) {
+ bool VisitCXXConstructExpr(const CXXConstructExpr *CE) {
if (auto *Ctor = CE->getConstructor()) {
if (Ctor->isMoveConstructor() && CE->getNumArgs() == 1) {
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
@@ -82,7 +81,7 @@ struct GuardianVisitor : public
RecursiveASTVisitor<GuardianVisitor> {
return true;
}
- bool VisitCXXMemberCallExpr(const CXXMemberCallExpr* MCE) {
+ bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
auto MethodName = safeGetName(MCE->getMethodDecl());
if (MethodName == "swap" || MethodName == "leakRef" ||
MethodName == "releaseNonNull") {
@@ -95,7 +94,7 @@ struct GuardianVisitor : public
RecursiveASTVisitor<GuardianVisitor> {
return true;
}
- bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr* OCE) {
+ bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE) {
if (OCE->isAssignmentOp() && OCE->getNumArgs() == 2) {
auto *ThisArg = OCE->getArg(0)->IgnoreParenCasts();
if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) {
>From a9c8d853e8b2b69d2c96c781ec9bdca35c19a17f Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@webkit.org>
Date: Mon, 28 Oct 2024 16:46:14 -0700
Subject: [PATCH 3/3] Removed the redundant OCE->getNumArgs() == 2 check in
VisitCXXOperatorCallExpr when it's an assignment, and added tests for ternary
expresssion per review comments.
---
.../Checkers/WebKit/UncountedLocalVarsChecker.cpp | 3 ++-
.../Checkers/WebKit/uncounted-local-vars.cpp | 12 ++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 5978b23760650f..76a4599cc8d788 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -95,7 +95,8 @@ struct GuardianVisitor : public
RecursiveASTVisitor<GuardianVisitor> {
}
bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE) {
- if (OCE->isAssignmentOp() && OCE->getNumArgs() == 2) {
+ if (OCE->isAssignmentOp()) {
+ assert(OCE->getNumArgs() == 2);
auto *ThisArg = OCE->getArg(0)->IgnoreParenCasts();
if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) {
if (VarRef->getDecl() == Guardian)
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 7028d7bb0ba160..408956de2c36cb 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -111,6 +111,12 @@ void foo8(RefCountable* obj) {
foo.releaseNonNull();
bar->method();
}
+ {
+ RefCountable *bar = foo.get();
+ // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe
[alpha.webkit.UncountedLocalVarsChecker]}}
+ foo = obj ? obj : nullptr;
+ bar->method();
+ }
}
void foo9(RefCountable& o) {
@@ -140,6 +146,12 @@ void foo9(RefCountable& o) {
guardian.leakRef();
bar->method();
}
+ {
+ RefCountable *bar = guardian.ptr();
+ // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe
[alpha.webkit.UncountedLocalVarsChecker]}}
+ guardian = o.trivial() ? o : *bar;
+ bar->method();
+ }
}
} // namespace guardian_scopes
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
