[alpha.webkit.UncountedLocalVarsChecker] Warn the use of a raw pointer/reference when the guardian variable gets mutated.

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);
+        }
       }
     }
   }

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; }
 };
 

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@-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();
+  }
+  {
+    RefCountable *bar = foo.get();
+    // expected-warning@-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@-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@-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();
+  }
+  {
+    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
 
 namespace auto_keyword {