[Analyzer][WebKit] UncountedLocalVarsChecker

Differential Review: https://reviews.llvm.org/D83259

clang/docs/analyzer/checkers.rst

@@ -2538,6 +2538,53 @@ We also define a set of safe transformations which if passed a safe value as an
 - casts
 - unary operators like ``&`` or ``*``
 
+alpha.webkit.UncountedLocalVarsChecker
+""""""""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure that any uncounted local variable is backed by a ref-counted object with lifetime that is strictly larger than the scope of the uncounted local variable. To be on the safe side we require the scope of an uncounted variable to be embedded in the scope of ref-counted object that backs it.
+
+These are examples of cases that we consider safe:
+
+  .. code-block:: cpp
+    void foo1() {
+      RefPtr<RefCountable> counted;
+      // The scope of uncounted is EMBEDDED in the scope of counted.
+      {
+        RefCountable* uncounted = counted.get(); // ok
+      }
+    }
+
+    void foo2(RefPtr<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 an argument is safe or it's considered if not a bug then bug-prone.
+
+  .. 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
+    }
+
+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
+
+For the time being we also don't warn about uninitialized uncounted local variables.
 
 Debug Checkers
 ---------------

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

@@ -1668,4 +1668,8 @@ def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
   HelpText<"Check uncounted call arguments.">,
   Documentation<HasDocumentation>;
 
+def UncountedLocalVarsChecker : Checker<"UncountedLocalVarsChecker">,
+  HelpText<"Check uncounted local variables.">,
+  Documentation<HasDocumentation>;
+
 } // end alpha.webkit

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

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

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

@@ -0,0 +1,250 @@
+//=======- UncountedLocalVarsChecker.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.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ASTUtils.h"
+#include "DiagOutputUtils.h"
+#include "PtrTypesSemantics.h"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/ParentMapContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "llvm/ADT/DenseSet.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+// for ( int a = ...) ... true
+// for ( int a : ...) ... true
+// if ( int* a = ) ... true
+// anything else ... false
+bool isDeclaredInForOrIf(const VarDecl *Var) {
+  assert(Var);
+  auto &ASTCtx = Var->getASTContext();
+  auto parent = ASTCtx.getParents(*Var);
+
+  if (parent.size() == 1) {
+    if (auto *DS = parent.begin()->get<DeclStmt>()) {
+      DynTypedNodeList grandParent = ASTCtx.getParents(*DS);
+      if (grandParent.size() == 1) {
+        return grandParent.begin()->get<ForStmt>() ||
+               grandParent.begin()->get<IfStmt>() ||
+               grandParent.begin()->get<CXXForRangeStmt>();
+      }
+    }
+  }
+  return false;
+}
+
+// FIXME: should be defined by anotations in the future
+bool isRefcountedStringsHack(const VarDecl *V) {
+  assert(V);
+  auto safeClass = [](const std::string &className) {
+    return className == "String" || className == "AtomString" ||
+           className == "UniquedString" || className == "Identifier";
+  };
+  QualType QT = V->getType();
+  auto *T = QT.getTypePtr();
+  if (auto *CXXRD = T->getAsCXXRecordDecl()) {
+    if (safeClass(safeGetName(CXXRD)))
+      return true;
+  }
+  if (T->isPointerType() || T->isReferenceType()) {
+    if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
+      if (safeClass(safeGetName(CXXRD)))
+        return true;
+    }
+  }
+  return false;
+}
+
+bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
+                                           const VarDecl *MaybeGuardian) {
+  assert(Guarded);
+  assert(MaybeGuardian);
+
+  if (!MaybeGuardian->isLocalVarDecl())
+    return false;
+
+  const CompoundStmt *guardiansClosestCompStmtAncestor = nullptr;
+
+  ASTContext &ctx = MaybeGuardian->getASTContext();
+
+  for (DynTypedNodeList guardianAncestors = ctx.getParents(*MaybeGuardian);
+       !guardianAncestors.empty();
+       guardianAncestors = ctx.getParents(
+           *guardianAncestors
+                .begin()) // FIXME - should we handle all of the parents?
+  ) {
+    for (auto &guardianAncestor : guardianAncestors) {
+      if (auto *CStmtParentAncestor = guardianAncestor.get<CompoundStmt>()) {
+        guardiansClosestCompStmtAncestor = CStmtParentAncestor;
+        break;
+      }
+    }
+    if (guardiansClosestCompStmtAncestor)
+      break;
+  }
+
+  if (!guardiansClosestCompStmtAncestor)
+    return false;
+
+  // We need to skip the first CompoundStmt to avoid situation when guardian is
+  // defined in the same scope as guarded variable.
+  bool HaveSkippedFirstCompoundStmt = false;
+  for (DynTypedNodeList guardedVarAncestors = ctx.getParents(*Guarded);
+       !guardedVarAncestors.empty();
+       guardedVarAncestors = ctx.getParents(
+           *guardedVarAncestors
+                .begin()) // FIXME - should we handle all of the parents?
+  ) {
+    for (auto &guardedVarAncestor : guardedVarAncestors) {
+      if (auto *CStmtAncestor = guardedVarAncestor.get<CompoundStmt>()) {
+        if (!HaveSkippedFirstCompoundStmt) {
+          HaveSkippedFirstCompoundStmt = true;
+          continue;
+        }
+        if (CStmtAncestor == guardiansClosestCompStmtAncestor)
+          return true;
+      }
+    }
+  }
+
+  return false;
+}
+
+class UncountedLocalVarsChecker
+    : public Checker<check::ASTDecl<TranslationUnitDecl>> {
+  BugType Bug{this,
+              "Uncounted raw pointer or reference not provably backed by "
+              "ref-counted variable",
+              "WebKit coding guidelines"};
+  mutable BugReporter *BR;
+
+public:
+  void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
+                    BugReporter &BRArg) const {
+    BR = &BRArg;
+
+    // The calls to checkAST* from AnalysisConsumer don't
+    // 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;
+      explicit LocalVisitor(const UncountedLocalVarsChecker *Checker)
+          : Checker(Checker) {
+        assert(Checker);
+      }
+
+      bool shouldVisitTemplateInstantiations() const { return true; }
+      bool shouldVisitImplicitCode() const { return false; }
+
+      bool VisitVarDecl(VarDecl *V) {
+        Checker->visitVarDecl(V);
+        return true;
+      }
+    };
+
+    LocalVisitor visitor(this);
+    visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
+  }
+
+  void visitVarDecl(const VarDecl *V) const {
+    if (shouldSkipVarDecl(V))
+      return;
+
+    const auto *ArgType = V->getType().getTypePtr();
+    if (!ArgType)
+      return;
+
+    if (isUncountedPtr(ArgType)) {
+      const Expr *const InitExpr = V->getInit();
+      if (!InitExpr)
+        return; // FIXME: later on we might warn on uninitialized vars too
+
+      const clang::Expr *const InitArgOrigin =
+          tryToFindPtrOrigin(InitExpr, /*StopAtFirstRefCountedObj=*/false)
+              .first;
+      if (!InitArgOrigin)
+        return;
+
+      if (isa<CXXThisExpr>(InitArgOrigin))
+        return;
+
+      if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
+        if (auto *MaybeGuardian =
+                dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
+          const auto *MaybeGuardianArgType =
+              MaybeGuardian->getType().getTypePtr();
+          if (!MaybeGuardianArgType)
+            return;
+          const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
+              MaybeGuardianArgType->getAsCXXRecordDecl();
+          if (!MaybeGuardianArgCXXRecord)
+            return;
+
+          if (MaybeGuardian->isLocalVarDecl() &&
+              (isRefCounted(MaybeGuardianArgCXXRecord) ||
+               isRefcountedStringsHack(MaybeGuardian)) &&
+              isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) {
+            return;
+          }
+
+          // Parameters are guaranteed to be safe for the duration of the call
+          // by another checker.
+          if (isa<ParmVarDecl>(MaybeGuardian))
+            return;
+        }
+      }
+
+      reportBug(V);
+    }
+  }
+
+  bool shouldSkipVarDecl(const VarDecl *V) const {
+    assert(V);
+    if (!V->isLocalVarDecl())
+      return true;
+
+    if (isDeclaredInForOrIf(V))
+      return true;
+
+    return false;
+  }
+
+  void reportBug(const VarDecl *V) const {
+    assert(V);
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+
+    Os << "Local variable ";
+    printQuotedQualifiedName(Os, V);
+    Os << " is uncounted and unsafe.";
+
+    PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    Report->addRange(V->getSourceRange());
+    BR->emitReport(std::move(Report));
+  }
+};
+} // namespace
+
+void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<UncountedLocalVarsChecker>();
+}
+
+bool ento::shouldRegisterUncountedLocalVarsChecker(const CheckerManager &) {
+  return true;
+}

clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp

@@ -0,0 +1,99 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedLocalVarsChecker -verify %s
+
+#include "mock-types.h"
+
+namespace raw_ptr {
+void foo() {
+  RefCountable *bar;
+  // FIXME: later on we might warn on uninitialized vars too
+}
+
+void bar(RefCountable *) {}
+} // namespace raw_ptr
+
+namespace reference {
+void foo_ref() {
+  RefCountable automatic;
+  RefCountable &bar = automatic;
+  // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+}
+
+void bar_ref(RefCountable &) {}
+} // namespace reference
+
+namespace guardian_scopes {
+void foo1() {
+  RefPtr<RefCountable> foo;
+  { RefCountable *bar = foo.get(); }
+}
+
+void foo2() {
+  RefPtr<RefCountable> foo;
+  // missing embedded scope here
+  RefCountable *bar = foo.get();
+  // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+}
+
+void foo3() {
+  RefPtr<RefCountable> foo;
+  {
+    { RefCountable *bar = foo.get(); }
+  }
+}
+
+void foo4() {
+  {
+    RefPtr<RefCountable> foo;
+    { RefCountable *bar = foo.get(); }
+  }
+}
+} // namespace guardian_scopes
+
+namespace auto_keyword {
+class Foo {
+  RefCountable *provide_ref_ctnbl() { return nullptr; }
+
+  void evil_func() {
+    RefCountable *bar = provide_ref_ctnbl();
+    // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+    auto *baz = provide_ref_ctnbl();
+    // expected-warning@-1{{Local variable 'baz' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+    auto *baz2 = this->provide_ref_ctnbl();
+    // expected-warning@-1{{Local variable 'baz2' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+  }
+};
+} // namespace auto_keyword
+
+namespace guardian_casts {
+void foo1() {
+  RefPtr<RefCountable> foo;
+  { RefCountable *bar = downcast<RefCountable>(foo.get()); }
+}
+
+void foo2() {
+  RefPtr<RefCountable> foo;
+  {
+    RefCountable *bar =
+        static_cast<RefCountable *>(downcast<RefCountable>(foo.get()));
+  }
+}
+} // namespace guardian_casts
+
+namespace guardian_ref_conversion_operator {
+void foo() {
+  Ref<RefCountable> rc;
+  { RefCountable &rr = rc; }
+}
+} // namespace guardian_ref_conversion_operator
+
+namespace ignore_for_if {
+RefCountable *provide_ref_ctnbl() { return nullptr; }
+
+void foo() {
+  // no warnings
+  if (RefCountable *a = provide_ref_ctnbl()) { }
+  for (RefCountable *a = provide_ref_ctnbl(); a != nullptr;) { }
+  RefCountable *array[1];
+  for (RefCountable *a : array) { }
+}
+} // namespace ignore_for_if