[alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef

clang/docs/analyzer/checkers.rst

@@ -3440,6 +3440,27 @@ Check for non-determinism caused by sorting of pointers.
 alpha.WebKit
 ^^^^^^^^^^^^
 
+.. _alpha-webkit-NoUncheckedPtrMemberChecker:
+
+alpha.webkit.NoUncheckedPtrMemberChecker
+""""""""""""""""""""""""""""""""""""""""
+Raw pointers and references to an object which supports CheckedPtr or CheckedRef can't be used as class members. Only CheckedPtr, CheckedRef, RefPtr, or Ref are allowed.
+
+.. code-block:: cpp
+
+ struct CheckableObj {
+   void incrementPtrCount() {}
+   void decrementPtrCount() {}
+ };
+
+ struct Foo {
+   CheckableObj* ptr; // warn
+   CheckableObj& ptr; // warn
+   // ...
+ };
+
+See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
+
 .. _alpha-webkit-UncountedCallArgsChecker:
 
 alpha.webkit.UncountedCallArgsChecker

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

@@ -1771,6 +1771,10 @@ def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">,
 
 let ParentPackage = WebKitAlpha in {
 
+def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">,
+  HelpText<"Check for no unchecked member variables.">,
+  Documentation<HasDocumentation>;
+
 def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
   HelpText<"Check uncounted call arguments.">,
   Documentation<HasDocumentation>;

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

@@ -132,7 +132,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   VLASizeChecker.cpp
   ValistChecker.cpp
   VirtualCallChecker.cpp
-  WebKit/NoUncountedMembersChecker.cpp
+  WebKit/RawPtrRefMemberChecker.cpp
   WebKit/ASTUtils.cpp
   WebKit/PtrTypesSemantics.cpp
   WebKit/RefCntblBaseVirtualDtorChecker.cpp

clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp

@@ -19,8 +19,7 @@ using namespace clang;
 
 namespace {
 
-bool hasPublicMethodInBaseClass(const CXXRecordDecl *R,
-                                const char *NameToMatch) {
+bool hasPublicMethodInBaseClass(const CXXRecordDecl *R, StringRef NameToMatch) {
   assert(R);
   assert(R->hasDefinition());
 
@@ -37,7 +36,7 @@ bool hasPublicMethodInBaseClass(const CXXRecordDecl *R,
 namespace clang {
 
 std::optional<const clang::CXXRecordDecl *>
-hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) {
+hasPublicMethodInBase(const CXXBaseSpecifier *Base, StringRef NameToMatch) {
   assert(Base);
 
   const Type *T = Base->getType().getTypePtrOrNull();
@@ -53,48 +52,49 @@ hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) {
   return hasPublicMethodInBaseClass(R, NameToMatch) ? R : nullptr;
 }
 
-std::optional<bool> isRefCountable(const CXXRecordDecl* R)
-{
+std::optional<bool> isSmartPtrCompatible(const CXXRecordDecl *R,
+                                         StringRef IncMethodName,
+                                         StringRef DecMethodName) {
   assert(R);
 
   R = R->getDefinition();
   if (!R)
     return std::nullopt;
 
-  bool hasRef = hasPublicMethodInBaseClass(R, "ref");
-  bool hasDeref = hasPublicMethodInBaseClass(R, "deref");
+  bool hasRef = hasPublicMethodInBaseClass(R, IncMethodName);
+  bool hasDeref = hasPublicMethodInBaseClass(R, DecMethodName);
   if (hasRef && hasDeref)
     return true;
 
   CXXBasePaths Paths;
   Paths.setOrigin(const_cast<CXXRecordDecl *>(R));
 
   bool AnyInconclusiveBase = false;
-  const auto hasPublicRefInBase =
-      [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
-        auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
-        if (!hasRefInBase) {
-          AnyInconclusiveBase = true;
-          return false;
-        }
-        return (*hasRefInBase) != nullptr;
-      };
+  const auto hasPublicRefInBase = [&](const CXXBaseSpecifier *Base,
+                                      CXXBasePath &) {
+    auto hasRefInBase = clang::hasPublicMethodInBase(Base, IncMethodName);
+    if (!hasRefInBase) {
+      AnyInconclusiveBase = true;
+      return false;
+    }
+    return (*hasRefInBase) != nullptr;
+  };
 
   hasRef = hasRef || R->lookupInBases(hasPublicRefInBase, Paths,
                                       /*LookupInDependent =*/true);
   if (AnyInconclusiveBase)
     return std::nullopt;
 
   Paths.clear();
-  const auto hasPublicDerefInBase =
-      [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
-        auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
-        if (!hasDerefInBase) {
-          AnyInconclusiveBase = true;
-          return false;
-        }
-        return (*hasDerefInBase) != nullptr;
-      };
+  const auto hasPublicDerefInBase = [&](const CXXBaseSpecifier *Base,
+                                        CXXBasePath &) {
+    auto hasDerefInBase = clang::hasPublicMethodInBase(Base, DecMethodName);
+    if (!hasDerefInBase) {
+      AnyInconclusiveBase = true;
+      return false;
+    }
+    return (*hasDerefInBase) != nullptr;
+  };
   hasDeref = hasDeref || R->lookupInBases(hasPublicDerefInBase, Paths,
                                           /*LookupInDependent =*/true);
   if (AnyInconclusiveBase)
@@ -103,11 +103,23 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
   return hasRef && hasDeref;
 }
 
+std::optional<bool> isRefCountable(const clang::CXXRecordDecl *R) {
+  return isSmartPtrCompatible(R, "ref", "deref");
+}
+
+std::optional<bool> isCheckedPtrCapable(const clang::CXXRecordDecl *R) {
+  return isSmartPtrCompatible(R, "incrementPtrCount", "decrementPtrCount");
+}
+
 bool isRefType(const std::string &Name) {
   return Name == "Ref" || Name == "RefAllowingPartiallyDestroyed" ||
          Name == "RefPtr" || Name == "RefPtrAllowingPartiallyDestroyed";
 }
 
+bool isCheckedPtr(const std::string &Name) {
+  return Name == "CheckedPtr" || Name == "CheckedRef";
+}
+
 bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
   assert(F);
   const std::string &FunctionName = safeGetName(F);
@@ -218,6 +230,15 @@ bool isRefCounted(const CXXRecordDecl *R) {
   return false;
 }
 
+bool isCheckedPtr(const CXXRecordDecl *R) {
+  assert(R);
+  if (auto *TmplR = R->getTemplateInstantiationPattern()) {
+    const auto &ClassName = safeGetName(TmplR);
+    return isCheckedPtr(ClassName);
+  }
+  return false;
+}
+
 bool isPtrConversion(const FunctionDecl *F) {
   assert(F);
   if (isCtorOfRefCounted(F))

clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h

@@ -34,15 +34,23 @@ class Type;
 /// \returns CXXRecordDecl of the base if the type has ref as a public method,
 /// nullptr if not, std::nullopt if inconclusive.
 std::optional<const clang::CXXRecordDecl *>
-hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch);
+hasPublicMethodInBase(const CXXBaseSpecifier *Base,
+                      llvm::StringRef NameToMatch);
 
 /// \returns true if \p Class is ref-countable, false if not, std::nullopt if
 /// inconclusive.
-std::optional<bool> isRefCountable(const clang::CXXRecordDecl* Class);
+std::optional<bool> isRefCountable(const clang::CXXRecordDecl *Class);
+
+/// \returns true if \p Class is checked-pointer compatible, false if not,
+/// std::nullopt if inconclusive.
+std::optional<bool> isCheckedPtrCapable(const clang::CXXRecordDecl *Class);
 
 /// \returns true if \p Class is ref-counted, false if not.
 bool isRefCounted(const clang::CXXRecordDecl *Class);
 
+/// \returns true if \p Class is a CheckedPtr / CheckedRef, false if not.
+bool isCheckedPtr(const clang::CXXRecordDecl *Class);
+
 /// \returns true if \p Class is ref-countable AND not ref-counted, false if
 /// not, std::nullopt if inconclusive.
 std::optional<bool> isUncounted(const clang::QualType T);

clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp → clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp

@@ -1,4 +1,4 @@
-//=======- NoUncountedMembersChecker.cpp -------------------------*- C++ -*-==//
+//=======- RawPtrRefMemberChecker.cpp ----------------------------*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -25,18 +25,21 @@ using namespace ento;
 
 namespace {
 
-class NoUncountedMemberChecker
+class RawPtrRefMemberChecker
     : public Checker<check::ASTDecl<TranslationUnitDecl>> {
 private:
   BugType Bug;
   mutable BugReporter *BR;
 
 public:
-  NoUncountedMemberChecker()
-      : Bug(this,
-            "Member variable is a raw-pointer/reference to reference-countable "
-            "type",
-            "WebKit coding guidelines") {}
+  RawPtrRefMemberChecker(const char *description)
+      : Bug(this, description, "WebKit coding guidelines") {}
+
+  virtual std::optional<bool>
+  isPtrCompatible(const clang::CXXRecordDecl *) const = 0;
+  virtual bool isPtrCls(const clang::CXXRecordDecl *) const = 0;
+  virtual const char *typeName() const = 0;
+  virtual const char *invariant() const = 0;
 
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
                     BugReporter &BRArg) const {
@@ -46,8 +49,8 @@ class NoUncountedMemberChecker
     // visit template instantiations or lambda classes. We
     // want to visit those, so we make our own RecursiveASTVisitor.
     struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
-      const NoUncountedMemberChecker *Checker;
-      explicit LocalVisitor(const NoUncountedMemberChecker *Checker)
+      const RawPtrRefMemberChecker *Checker;
+      explicit LocalVisitor(const RawPtrRefMemberChecker *Checker)
           : Checker(Checker) {
         assert(Checker);
       }
@@ -77,9 +80,9 @@ class NoUncountedMemberChecker
       if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
         // If we don't see the definition we just don't know.
         if (MemberCXXRD->hasDefinition()) {
-            std::optional<bool> isRCAble = isRefCountable(MemberCXXRD);
-            if (isRCAble && *isRCAble)
-                reportBug(Member, MemberType, MemberCXXRD, RD);
+          std::optional<bool> isRCAble = isPtrCompatible(MemberCXXRD);
+          if (isRCAble && *isRCAble)
+            reportBug(Member, MemberType, MemberCXXRD, RD);
         }
       }
     }
@@ -114,7 +117,7 @@ class NoUncountedMemberChecker
     // a member but we trust them to handle it correctly.
     auto CXXRD = llvm::dyn_cast_or_null<CXXRecordDecl>(RD);
     if (CXXRD)
-      return isRefCounted(CXXRD);
+      return isPtrCls(CXXRD);
 
     return false;
   }
@@ -134,10 +137,10 @@ class NoUncountedMemberChecker
     Os << " in ";
     printQuotedQualifiedName(Os, ClassCXXRD);
     Os << " is a "
-       << (isa<PointerType>(MemberType) ? "raw pointer" : "reference")
-       << " to ref-countable type ";
+       << (isa<PointerType>(MemberType) ? "raw pointer" : "reference") << " to "
+       << typeName() << " ";
     printQuotedQualifiedName(Os, MemberCXXRD);
-    Os << "; member variables must be ref-counted.";
+    Os << "; " << invariant() << ".";
 
     PathDiagnosticLocation BSLoc(Member->getSourceRange().getBegin(),
                                  BR->getSourceManager());
@@ -146,13 +149,67 @@ class NoUncountedMemberChecker
     BR->emitReport(std::move(Report));
   }
 };
+
+class NoUncountedMemberChecker final : public RawPtrRefMemberChecker {
+public:
+  NoUncountedMemberChecker()
+      : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
+                               "reference-countable type") {}
+
+  std::optional<bool>
+  isPtrCompatible(const clang::CXXRecordDecl *R) const final {
+    return isRefCountable(R);
+  }
+
+  bool isPtrCls(const clang::CXXRecordDecl *R) const final {
+    return isRefCounted(R);
+  }
+
+  const char *typeName() const final { return "ref-countable type"; }
+
+  const char *invariant() const final {
+    return "member variables must be Ref, RefPtr, WeakRef, or WeakPtr";
+  }
+};
+
+class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker {
+public:
+  NoUncheckedPtrMemberChecker()
+      : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
+                               "checked-pointer capable type") {}
+
+  std::optional<bool>
+  isPtrCompatible(const clang::CXXRecordDecl *R) const final {
+    return isCheckedPtrCapable(R);
+  }
+
+  bool isPtrCls(const clang::CXXRecordDecl *R) const final {
+    return isCheckedPtr(R);
+  }
+
+  const char *typeName() const final { return "CheckedPtr capable type"; }
+
+  const char *invariant() const final {
+    return "member variables must be a CheckedPtr, CheckedRef, WeakRef, or "
+           "WeakPtr";
+  }
+};
+
 } // namespace
 
 void ento::registerNoUncountedMemberChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<NoUncountedMemberChecker>();
 }
 
-bool ento::shouldRegisterNoUncountedMemberChecker(
+bool ento::shouldRegisterNoUncountedMemberChecker(const CheckerManager &Mgr) {
+  return true;
+}
+
+void ento::registerNoUncheckedPtrMemberChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<NoUncheckedPtrMemberChecker>();
+}
+
+bool ento::shouldRegisterNoUncheckedPtrMemberChecker(
     const CheckerManager &Mgr) {
   return true;
 }

clang/test/Analysis/Checkers/WebKit/mock-types.h

@@ -108,4 +108,52 @@ struct RefCountable {
 
 template <typename T> T *downcast(T *t) { return t; }
 
+template <typename T> struct CheckedRef {
+private:
+  T *t;
+
+public:
+  CheckedRef() : t{} {};
+  CheckedRef(T &t) : t(t) { t->incrementPtrCount(); }
+  CheckedRef(const CheckedRef& o) : t(o.t) { if (t) t->incrementPtrCount(); }
+  ~CheckedRef() { if (t) t->decrementPtrCount(); }
+  T &get() { return *t; }
+  T *ptr() { return t; }
+  T *operator->() { return t; }
+  operator const T &() const { return *t; }
+  operator T &() { return *t; }
+};
+
+template <typename T> struct CheckedPtr {
+private:
+  T *t;
+
+public:
+  CheckedPtr() : t(nullptr) {}
+  CheckedPtr(T *t)
+    : t(t) {
+    if (t)
+      t->incrementPtrCount();
+  }
+  CheckedPtr(Ref<T>&& o)
+    : t(o.leakRef())
+  { }
+  ~CheckedPtr() {
+    if (t)
+      t->decrementPtrCount();
+  }
+  T *get() { return t; }
+  T *operator->() { return t; }
+  const T *operator->() const { return t; }
+  T &operator*() { return *t; }
+  CheckedPtr &operator=(T *) { return *this; }
+  operator bool() const { return t; }
+};
+
+class CheckedObj {
+public:
+  void incrementPtrCount();
+  void decrementPtrCount();
+};
+
 #endif

clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp

@@ -0,0 +1,52 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUncheckedPtrMemberChecker -verify %s
+
+#include "mock-types.h"
+
+namespace members {
+
+  struct Foo {
+  private:
+    CheckedObj* a = nullptr;
+// expected-warning@-1{{Member variable 'a' in 'members::Foo' is a raw pointer to CheckedPtr capable type 'CheckedObj'}}
+    CheckedObj& b;
+// expected-warning@-1{{Member variable 'b' in 'members::Foo' is a reference to CheckedPtr capable type 'CheckedObj'}}
+
+    [[clang::suppress]]
+    CheckedObj* a_suppressed = nullptr;
+
+    [[clang::suppress]]
+    CheckedObj& b_suppressed;
+
+    CheckedPtr<CheckedObj> c;
+    CheckedRef<CheckedObj> d;
+
+  public:
+    Foo();
+  };
+
+  template <typename S>
+  struct FooTmpl {
+    S* e;
+// expected-warning@-1{{Member variable 'e' in 'members::FooTmpl<CheckedObj>' is a raw pointer to CheckedPtr capable type 'CheckedObj'}}
+  };
+
+  void forceTmplToInstantiate(FooTmpl<CheckedObj>) { }
+
+} // namespace members
+
+namespace ignore_unions {
+
+  union Foo {
+    CheckedObj* a;
+    CheckedPtr<CheckedObj> c;
+    CheckedRef<CheckedObj> d;
+  };
+
+  template<class T>
+  union FooTmpl {
+    T* a;
+  };
+
+  void forceTmplToInstantiate(FooTmpl<CheckedObj>) { }
+
+} // namespace ignore_unions

llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn

@@ -141,7 +141,7 @@ static_library("Checkers") {
     "VforkChecker.cpp",
     "VirtualCallChecker.cpp",
     "WebKit/ASTUtils.cpp",
-    "WebKit/NoUncountedMembersChecker.cpp",
+    "WebKit/RawPtrRefMemberChecker.cpp",
     "WebKit/PtrTypesSemantics.cpp",
     "WebKit/RefCntblBaseVirtualDtorChecker.cpp",
     "WebKit/UncountedCallArgsChecker.cpp",