[Analyzer][WebKit] UncountedCallArgsChecker

Differential Revision: https://reviews.llvm.org/D77179

Author: jkorous-apple

clang/docs/analyzer/checkers.rst

@@ -1407,7 +1407,7 @@ Ref-counted types hold their ref-countable data by a raw pointer and allow impli
 .. _webkit-WebKitNoUncountedMemberChecker:
 
 webkit.WebKitNoUncountedMemberChecker
-""""""""""""""""""""""""""""""""""""
+"""""""""""""""""""""""""""""""""""""
 Raw pointers and references to uncounted types can't be used as class members. Only ref-counted types are allowed.
 
 .. code-block:: cpp
@@ -2377,6 +2377,99 @@ Check for non-determinism caused by sorting of pointers.
  }
 
 
+alpha.WebKit
+^^^^^^^^^^^^
+
+.. _alpha-webkit-UncountedCallArgsChecker:
+
+alpha.webkit.UncountedCallArgsChecker
+"""""""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure that lifetime of any dynamically allocated ref-countable object passed as a call argument spans past the end of the call. This applies to call to any function, method, lambda, function pointer or functor. Ref-countable types aren't supposed to be allocated on stack so we check arguments for parameters of raw pointers and references to uncounted types.
+
+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
+
+    RefCountable* provide_uncounted();
+    void consume(RefCountable*);
+
+    // In these cases we can't make sure callee won't directly or indirectly call `deref()` on the argument which could make it unsafe from such point until the end of the call.
+
+    void foo1() {
+      consume(provide_uncounted()); // warn
+    }
+
+    void foo2() {
+      RefCountable* uncounted = provide_uncounted();
+      consume(uncounted); // warn
+    }
+
+Although we are enforcing member variables to be ref-counted by `webkit.WebKitNoUncountedMemberChecker` any method of the same class still has unrestricted access to these. Since from a caller's perspective we can't guarantee a particular member won't get modified by callee (directly or indirectly) we don't consider values obtained from members safe.
+
+Note: It's likely this heuristic could be made more precise with fewer false positives - for example calls to free functions that don't have any parameter other than the pointer should be safe as the callee won't be able to tamper with the member unless it's a global variable.
+
+  .. code-block:: cpp
+
+    struct Foo {
+      RefPtr<RefCountable> member;
+      void consume(RefCountable*) { /* ... */ }
+      void bugprone() {
+        consume(member.get()); // warn
+      }
+    };
+
+The implementation of this rule is a heuristic - we define a whitelist of kinds of values that are considered safe to be passed as arguments. If we can't prove an argument is safe it's considered an error.
+
+Allowed kinds of arguments:
+
+- values obtained from ref-counted objects (including temporaries as those survive the call too)
+
+  .. code-block:: cpp
+
+    RefCountable* provide_uncounted();
+    void consume(RefCountable*);
+
+    void foo() {
+      RefPtr<RefCountable> rc = makeRef(provide_uncounted());
+      consume(rc.get()); // ok
+      consume(makeRef(provide_uncounted()).get()); // ok
+    }
+
+- forwarding uncounted arguments from caller to callee
+
+  .. code-block:: cpp
+
+    void foo(RefCountable& a) {
+      bar(a); // ok
+    }
+
+  Caller of ``foo()`` is responsible for  ``a``'s lifetime.
+
+- ``this`` pointer
+
+  .. code-block:: cpp
+
+    void Foo::foo() {
+      baz(this);  // ok
+    }
+
+  Caller of ``foo()`` is responsible for keeping the memory pointed to by ``this`` pointer safe.
+
+- constants
+
+  .. code-block:: cpp
+
+    foo(nullptr, NULL, 0); // ok
+
+We also define a set of safe transformations which if passed a safe value as an input provide (usually it's the return value) a safe value (or an object that provides safe values). This is also a heuristic.
+
+- constructors of ref-counted types (including factory methods)
+- getters of ref-counted types
+- member overloaded operators
+- casts
+- unary operators like ``&`` or ``*``
+
+
 Debug Checkers
 ---------------
 

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

@@ -1628,4 +1628,13 @@ def RefCntblBaseVirtualDtorChecker : Checker<"RefCntblBaseVirtualDtor">,
 def WebKitNoUncountedMemberChecker : Checker<"WebKitNoUncountedMemberChecker">,
   HelpText<"Check for no uncounted member variables.">,
   Documentation<HasDocumentation>;
+
 } // end webkit
+
+let ParentPackage = WebKitAlpha in {
+
+def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
+  HelpText<"Check uncounted call arguments.">,
+  Documentation<HasDocumentation>;
+
+} // end alpha.webkit

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

@@ -122,8 +122,10 @@ add_clang_library(clangStaticAnalyzerCheckers
   ValistChecker.cpp
   VirtualCallChecker.cpp
   WebKit/NoUncountedMembersChecker.cpp
+  WebKit/ASTUtils.cpp
   WebKit/PtrTypesSemantics.cpp
   WebKit/RefCntblBaseVirtualDtorChecker.cpp
+  WebKit/UncountedCallArgsChecker.cpp
 
   LINK_LIBS
   clangAST

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

@@ -0,0 +1,93 @@
+//=======- ASTUtils.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 "PtrTypesSemantics.h"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/ExprCXX.h"
+
+using llvm::Optional;
+namespace clang {
+
+std::pair<const Expr *, bool>
+tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
+  while (E) {
+    if (auto *cast = dyn_cast<CastExpr>(E)) {
+      if (StopAtFirstRefCountedObj) {
+        if (auto *ConversionFunc =
+                dyn_cast_or_null<FunctionDecl>(cast->getConversionFunction())) {
+          if (isCtorOfRefCounted(ConversionFunc))
+            return {E, true};
+        }
+      }
+      // FIXME: This can give false "origin" that would lead to false negatives
+      // in checkers. See https://reviews.llvm.org/D37023 for reference.
+      E = cast->getSubExpr();
+      continue;
+    }
+    if (auto *call = dyn_cast<CallExpr>(E)) {
+      if (auto *memberCall = dyn_cast<CXXMemberCallExpr>(call)) {
+        if (isGetterOfRefCounted(memberCall->getMethodDecl())) {
+          E = memberCall->getImplicitObjectArgument();
+          if (StopAtFirstRefCountedObj) {
+            return {E, true};
+          }
+          continue;
+        }
+      }
+
+      if (auto *operatorCall = dyn_cast<CXXOperatorCallExpr>(E)) {
+        if (operatorCall->getNumArgs() == 1) {
+          E = operatorCall->getArg(0);
+          continue;
+        }
+      }
+
+      if (auto *callee = call->getDirectCallee()) {
+        if (isCtorOfRefCounted(callee)) {
+          if (StopAtFirstRefCountedObj)
+            return {E, true};
+
+          E = call->getArg(0);
+          continue;
+        }
+
+        if (isPtrConversion(callee)) {
+          E = call->getArg(0);
+          continue;
+        }
+      }
+    }
+    if (auto *unaryOp = dyn_cast<UnaryOperator>(E)) {
+      // FIXME: Currently accepts ANY unary operator. Is it OK?
+      E = unaryOp->getSubExpr();
+      continue;
+    }
+
+    break;
+  }
+  // Some other expression.
+  return {E, false};
+}
+
+bool isASafeCallArg(const Expr *E) {
+  assert(E);
+  if (auto *Ref = dyn_cast<DeclRefExpr>(E)) {
+    if (auto *D = dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
+      if (isa<ParmVarDecl>(D) || D->isLocalVarDecl())
+        return true;
+    }
+  }
+
+  // TODO: checker for method calls on non-refcounted objects
+  return isa<CXXThisExpr>(E);
+}
+
+} // namespace clang

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

@@ -23,9 +23,23 @@ class FunctionDecl;
 class CXXMethodDecl;
 class Expr;
 
+/// This function de-facto defines a set of transformations that we consider
+/// safe (in heuristical sense). These transformation if passed a safe value as
+/// an input should provide a safe value (or an object that provides safe
+/// values).
+///
+/// For more context see Static Analyzer checkers documentation - specifically
+/// webkit.UncountedCallArgsChecker checker. Whitelist of transformations:
+/// - constructors of ref-counted types (including factory methods)
+/// - getters of ref-counted types
+/// - member overloaded operators
+/// - casts
+/// - unary operators like ``&`` or ``*``
+///
 /// If passed expression is of type uncounted pointer/reference we try to find
-/// the origin of this pointer. Example: Origin can be a local variable, nullptr
-/// constant or this-pointer.
+/// the "origin" of the pointer value.
+/// Origin can be for example a local variable, nullptr, constant or
+/// this-pointer.
 ///
 /// Certain subexpression nodes represent transformations that don't affect
 /// where the memory address originates from. We try to traverse such