Introduce a new WebKit checker for a unchecked local variable (llvm#113708)

This PR introduces alpha.webkit.UncheckedLocalVarsChecker which detects
a raw reference or a raw pointer local, static, or global variable to a
CheckedPtr capable object without a guardian variable in an outer scope.

Author: rniwa

clang/docs/analyzer/checkers.rst

@@ -3584,7 +3584,7 @@ These are examples of cases that we consider safe:
       RefCountable* uncounted = this; // ok
     }
 
-Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that an argument is safe or it's considered if not a bug then bug-prone.
+Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that a local variable is safe or it's considered unsafe.
 
   .. code-block:: cpp
 
@@ -3603,11 +3603,48 @@ Here are some examples of situations that we warn about as they *might* be poten
       RefCountable* uncounted = counted.get(); // warn
     }
 
-We don't warn about these cases - we don't consider them necessarily safe but since they are very common and usually safe we'd introduce a lot of false positives otherwise:
-- variable defined in condition part of an ```if``` statement
-- variable defined in init statement condition of a ```for``` statement
+alpha.webkit.UncheckedLocalVarsChecker
+""""""""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure that any unchecked local variable is backed by a CheckedPtr or CheckedRef with lifetime that is strictly larger than the scope of the unchecked local variable. To be on the safe side we require the scope of an unchecked variable to be embedded in the scope of CheckedPtr/CheckRef object that backs it.
+
+These are examples of cases that we consider safe:
+
+  .. code-block:: cpp
 
-For the time being we also don't warn about uninitialized uncounted local variables.
+    void foo1() {
+      CheckedPtr<RefCountable> counted;
+      // The scope of uncounted is EMBEDDED in the scope of counted.
+      {
+        RefCountable* uncounted = counted.get(); // ok
+      }
+    }
+
+    void foo2(CheckedPtr<RefCountable> counted_param) {
+      RefCountable* uncounted = counted_param.get(); // ok
+    }
+
+    void FooClass::foo_method() {
+      RefCountable* uncounted = this; // ok
+    }
+
+Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that a local variable is safe or it's considered unsafe.
+
+  .. code-block:: cpp
+
+    void foo1() {
+      RefCountable* uncounted = new RefCountable; // warn
+    }
+
+    RefCountable* global_uncounted;
+    void foo2() {
+      RefCountable* uncounted = global_uncounted; // warn
+    }
+
+    void foo3() {
+      RefPtr<RefCountable> counted;
+      // The scope of uncounted is not EMBEDDED in the scope of counted.
+      RefCountable* uncounted = counted.get(); // warn
+    }
 
 Debug Checkers
 ---------------

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

@@ -1764,4 +1764,8 @@ def UncountedLocalVarsChecker : Checker<"UncountedLocalVarsChecker">,
   HelpText<"Check uncounted local variables.">,
   Documentation<HasDocumentation>;
 
+def UncheckedLocalVarsChecker : Checker<"UncheckedLocalVarsChecker">,
+  HelpText<"Check unchecked local variables.">,
+  Documentation<HasDocumentation>;
+
 } // end alpha.webkit

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

@@ -136,7 +136,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   WebKit/RefCntblBaseVirtualDtorChecker.cpp
   WebKit/UncountedCallArgsChecker.cpp
   WebKit/UncountedLambdaCapturesChecker.cpp
-  WebKit/UncountedLocalVarsChecker.cpp
+  WebKit/RawPtrRefLocalVarsChecker.cpp
 
   LINK_LIBS
   clangAST

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

@@ -200,6 +200,14 @@ std::optional<bool> isUncountedPtr(const QualType T) {
   return false;
 }
 
+std::optional<bool> isUncheckedPtr(const QualType T) {
+  if (T->isPointerType() || T->isReferenceType()) {
+    if (auto *CXXRD = T->getPointeeCXXRecordDecl())
+      return isUnchecked(CXXRD);
+  }
+  return false;
+}
+
 std::optional<bool> isUnsafePtr(const QualType T) {
   if (T->isPointerType() || T->isReferenceType()) {
     if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {

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

@@ -63,6 +63,10 @@ std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
 /// class, false if not, std::nullopt if inconclusive.
 std::optional<bool> isUncountedPtr(const clang::QualType T);
 
+/// \returns true if \p T is either a raw pointer or reference to an unchecked
+/// class, false if not, std::nullopt if inconclusive.
+std::optional<bool> isUncheckedPtr(const clang::QualType T);
+
 /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
 /// variant, false if not.
 bool isSafePtrType(const clang::QualType T);

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp renamed to clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp

@@ -165,15 +165,18 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
   return false;
 }
 
-class UncountedLocalVarsChecker
+class RawPtrRefLocalVarsChecker
     : public Checker<check::ASTDecl<TranslationUnitDecl>> {
-  BugType Bug{this,
-              "Uncounted raw pointer or reference not provably backed by "
-              "ref-counted variable",
-              "WebKit coding guidelines"};
+  BugType Bug;
   mutable BugReporter *BR;
 
 public:
+  RawPtrRefLocalVarsChecker(const char *description)
+      : Bug(this, description, "WebKit coding guidelines") {}
+
+  virtual std::optional<bool> isUnsafePtr(const QualType T) const = 0;
+  virtual const char *ptrKind() const = 0;
+
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
                     BugReporter &BRArg) const {
     BR = &BRArg;
@@ -182,14 +185,14 @@ class UncountedLocalVarsChecker
     // visit template instantiations or lambda classes. We
     // want to visit those, so we make our own RecursiveASTVisitor.
     struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
-      const UncountedLocalVarsChecker *Checker;
+      const RawPtrRefLocalVarsChecker *Checker;
       Decl *DeclWithIssue{nullptr};
 
       TrivialFunctionAnalysis TFA;
 
       using Base = RecursiveASTVisitor<LocalVisitor>;
 
-      explicit LocalVisitor(const UncountedLocalVarsChecker *Checker)
+      explicit LocalVisitor(const RawPtrRefLocalVarsChecker *Checker)
           : Checker(Checker) {
         assert(Checker);
       }
@@ -261,7 +264,7 @@ class UncountedLocalVarsChecker
     if (shouldSkipVarDecl(V))
       return;
 
-    std::optional<bool> IsUncountedPtr = isUncountedPtr(V->getType());
+    std::optional<bool> IsUncountedPtr = isUnsafePtr(V->getType());
     if (IsUncountedPtr && *IsUncountedPtr) {
       if (tryToFindPtrOrigin(
               Value, /*StopAtFirstRefCountedObj=*/false,
@@ -324,7 +327,7 @@ class UncountedLocalVarsChecker
     llvm::raw_svector_ostream Os(Buf);
 
     if (dyn_cast<ParmVarDecl>(V)) {
-      Os << "Assignment to an uncounted parameter ";
+      Os << "Assignment to an " << ptrKind() << " parameter ";
       printQuotedQualifiedName(Os, V);
       Os << " is unsafe.";
 
@@ -342,7 +345,7 @@ class UncountedLocalVarsChecker
       else
         Os << "Variable ";
       printQuotedQualifiedName(Os, V);
-      Os << " is uncounted and unsafe.";
+      Os << " is " << ptrKind() << " and unsafe.";
 
       PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
       auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
@@ -352,6 +355,29 @@ class UncountedLocalVarsChecker
     }
   }
 };
+
+class UncountedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
+public:
+  UncountedLocalVarsChecker()
+      : RawPtrRefLocalVarsChecker("Uncounted raw pointer or reference not "
+                                  "provably backed by ref-counted variable") {}
+  std::optional<bool> isUnsafePtr(const QualType T) const final {
+    return isUncountedPtr(T);
+  }
+  const char *ptrKind() const final { return "uncounted"; }
+};
+
+class UncheckedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
+public:
+  UncheckedLocalVarsChecker()
+      : RawPtrRefLocalVarsChecker("Unchecked raw pointer or reference not "
+                                  "provably backed by checked variable") {}
+  std::optional<bool> isUnsafePtr(const QualType T) const final {
+    return isUncheckedPtr(T);
+  }
+  const char *ptrKind() const final { return "unchecked"; }
+};
+
 } // namespace
 
 void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
@@ -361,3 +387,11 @@ void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
 bool ento::shouldRegisterUncountedLocalVarsChecker(const CheckerManager &) {
   return true;
 }
+
+void ento::registerUncheckedLocalVarsChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<UncheckedLocalVarsChecker>();
+}
+
+bool ento::shouldRegisterUncheckedLocalVarsChecker(const CheckerManager &) {
+  return true;
+}

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

@@ -186,6 +186,8 @@ class CheckedObj {
 public:
   void incrementPtrCount();
   void decrementPtrCount();
+  void method();
+  int trivial() { return 123; }
 };
 
 class RefCountableAndCheckable {