[clang] [alpha.webkit.UncountedLocalVarsChecker] Warn the use of a raw pointer/reference when the guardian variable gets mutated. (PR #113859)

Ryosuke Niwa via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 28 08:37:18 PDT 2024


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 <rniwa at webkit.org>
Date: Sun, 27 Oct 2024 21:43:24 -0700
Subject: [PATCH 1/2] [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 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();
+  }
+}
+
+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();
+  }
+}
+
 } // namespace guardian_scopes
 
 namespace auto_keyword {

>From 075f5677dd68ed4e5bf29138bf6202dd788621f9 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Mon, 28 Oct 2024 08:36:56 -0700
Subject: [PATCH 2/2] 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)) {



More information about the cfe-commits mailing list