[Analyzer][WebKit] UncountedCallArgsChecker

[jkorous]

reflected feedback from reviews of other related checkers

[NoQ]

*requires a bit more time to digest this whole tryToFindPtrOrigin thing*
(thx for more comments!)

clang/docs/analyzer/checkers.rst

1434

I think it's worth it to add a few examples on which the checker *will* warn.

1464

You probably meant baz(this).

[jkorous]

clang/docs/analyzer/checkers.rst

1434

Actually, this is spot on! It feels like I am struggling to step back and not think about this from the implementation perspective...

[jkorous]

• rebased
• improved the docs

[NoQ]

Aha, ok, anyway, i think understand the rough idea. You're trying to make sure that the object is "pinned" onto a ref-counted smart pointer every time it's used. So you're banning use of raw pointers for any potentially prolonged duration of time.

Even if it's just within an expression, this way you make sure that it lasts for the entire duration of the full-expression. This makes your approach a bit stricter than ownership rules a-la Objective-C but i guess not intolerably so, so i don't mind.

I think this is a solid start. In most cases there shouldn't be a need to use a raw pointer, and it's also very easy to temporarily convert a raw pointer to a smart pointer in order to suppress the warning (yay C++ destructors) so it's likely that most of the warnings can be addressed or silenced with relative ease. The only reason not to have smart pointers is old APIs that you can't afford to break, but then NoUncountedMemberChecker makes sure that these raw pointers aren't stored anywhere for too long.

Please accept a bunch of minor nits.

clang/docs/analyzer/checkers.rst

1432

The first sentence seems to be lacking a predicate (probably better: "Here are some examples...").

1439

I definitely wouldn't mind banning direct delete invocations entirely. They're terrifying because they invalidate all references that were held by everyone else. This might as well be worth runtime protection, i.e. in the destructor of every reference-countable object there probably should be an assertion that there are no remaining references. I'd rather make sure that any normal code, in the worst case, calls deref() manually, but definitely not delete.

1505

Did you intend to capitalize those differently?

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

1627–1639

I'd like to remove the redundant WebKit prefix from these names as well. There's package name for this :)

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

31

I'm still worried about unwrapping implicit-lvalue-to-rvalue-casts. Do we have a test for it? Like, even if you're not sure about what's the right ownership model would be in this case, i'd rather add a test and have a quick look to make sure it isn't horribly wrong.

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

31

I still advocate for an even bolder syntax:

BugType Bug{this,
            "Uncounted call argument for a raw pointer/reference parameter",
            "WebKit coding guidelines"};

... and drop the constructor.

Also we usually tend to leave out private: when it's at the beginning of the class (where it's assumed), not sure why (so i don't really care).

82–84

I don't think this is actually the case. You'll get some CXXDefaultArgExpr as the argument but it'll still be something.

90

Not sure if i already asked - why not let isUncountedPtr() accept a QualType and drop the getTypePtrOrNull()? Like, QualType::operator->() still works so you don't need to change implementation, it's just less code this way.

[aaron.ballman]

clang/docs/analyzer/checkers.rst

1466

Can you pick a different term that "whitelist" to use here and elsewhere? How about "Allowed Arguments", "Allowed Values", or something along those lines?

[jkorous]

I'll add a bit more context still. The WebKit folks are ultimately aiming for increased memory safety while (and this is just as important) doing it in as non-intrusive way as possible and with regards to practical considerations - bandwidth available for eventual code changes mostly. We were brainstorming ideas and iterating prototypes against the WebKit codebase.
What's important is that we aren't aiming for "perfect" (or 100% correct) but "improvement" (in a statistical sense) here. I'd say we've gone a long way since the start but I won't mind making this checker alpha for now.

In D77179#2080379, @NoQ wrote:

Aha, ok, anyway, i think understand the rough idea. You're trying to make sure that the object is "pinned" onto a ref-counted smart pointer every time it's used. So you're banning use of raw pointers for any potentially prolonged duration of time.

Yes, that's the underlying idea. For both conceptual and implementation simplicity we decided to formulate it as multiple simpler rules/checkers that refer to only small isolated part of code - we hope that by applying a set of "local" rules (think a single function scope or single class scope) we'd reduce propensity to memory related bugs (even those whose cause can't be pinpointed to a single location but orchestrated actions of different parts of the program).

The idea behind this checker is that if we're able to make sure an argument is safe at a call-site then no matter what happens in callee's code (short of delete-ing it which we'll address in another checker) it'll remain safe for the duration of the call. Doing that we're able to prevent bugs caused by say another function called from callee that would delete a parameter:

void maybe_delete(Foo*);
void callee(Foo* f) {
  assert(f);
  maybe_delete(f);
  if (f) {
    do_more_stuff(f); // potentially dangerous
  }
}

Given the complexity of and nature of web-browser engine's work things and potential security consequences it's apparently worth to avoid situations that might be fine in other codebases.

Of course, given we're dealing with C++ if someone goes out of their way they can still shoot themselves in the foot. The goal here is to reduce propensity to honest mistakes. Preventing people determined to do scary things is both a lost battle and too restrictive for the kind of project WebKit is. There are probably better means to reach such goals anyway (code review, sanitizers, etc).

Even if it's just within an expression, this way you make sure that it lasts for the entire duration of the full-expression. This makes your approach a bit stricter than ownership rules a-la Objective-C but i guess not intolerably so, so i don't mind.

The exact measure of intolerability is actually still yet to be assessed. I've done some measurements on WebKit codebase and we discussed the results with WebKit folks - it's possible we'd have to tweak the checker before we're happy to deploy it.

I think this is a solid start. In most cases there shouldn't be a need to use a raw pointer, and it's also very easy to temporarily convert a raw pointer to a smart pointer in order to suppress the warning (yay C++ destructors) so it's likely that most of the warnings can be addressed or silenced with relative ease. The only reason not to have smart pointers is old APIs that you can't afford to break, but then NoUncountedMemberChecker makes sure that these raw pointers aren't stored anywhere for too long.

That and apparently also churn caused by reference counting in some cases.

Please accept a bunch of minor nits.

Going through them now!

[jkorous]

clang/docs/analyzer/checkers.rst

1439

Ah! This is actually wrong!
Delete is forbidden, I should've said "calling deref()" on the raw pointer (intrusive ref-counting).

1466

Of course!

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

31

My thinking was:

• By being overly permissive the worst that can happen is a false negative.
• The prototype of this checker was finding more true positives (as defined by the rule we are considering) than we could handle anyway.
• We would initially be fine doing some very bad things from the correctness perspective as long as they don't add any kind of positives and silence significant number of false negatives (in absolute numbers).
• This checker would be iterated on.

Would you be fine with me just talking my way out of writing proper tests now (to save the effort in case we change the checker later) if we put this checker to alpha?

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

82–84

Oh, ok. I'll look into it. All I remember is that args <-> params correspondence was more complicated than I expected.

90

My concern is that since isUncountedPtr is a generic predicate used in different contexts we'd need to introduce a way how to return "unknown". We couldn't just return false as in some contexts that wouldn't work. Having to deal with Optional<bool> values (or similar) is IMO a bigger hassle than this.

[jkorous]

addressed some of the comments

[NoQ]

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

31

Ok but let's at least put a FiXME there so that we didn't forget to eventually ask ourselves this question. Because it's a completely counterintuitive part of this code's behavior that is also very hard to notice.

Note: I'm mostly fighting for this out of my PTSD from multi-week investigation of D37023. This was... hard to forget.

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

82–84

I also vaguely remember a hazard on member operators in which parameters in decls and arguments in exprs are indexed differently (offset by one).

90

*visible confusion*
I mean, i was questioning the parameter of isUncountedPtr(), not its return value(?)

[jkorous]

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

31

Oh! Good to know about this!

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

82–84

I see. I've ran into member call operators but didn't realize it's a general thing. Are you saying I should simplify ParamIdx initialization on L70 to this?

unsigned ParamIdx = dyn_cast<CXXOperatorCallExpr>(CE);

82–84

You were right about default args. For some reason they don't have source location so I guess finding the relevant parameter, getDefaultArg() and using its source location is what I should do.
https://clang.llvm.org/doxygen/classclang_1_1CXXDefaultArgExpr.html#a1539fe8f7e94440aed15edab794f0ab2

90

Sorry - I wasn't clear. What I am saying is that IIUC we need to use getTypePtrOrNull at some point anyway since:

https://clang.llvm.org/doxygen/Type_8h_source.html#l00712

const Type *operator->() const {
  return getTypePtr();
}

and from the doc comments it seems we can't just use this:

/// Retrieves a pointer to the underlying (unqualified) type.
///
/// This function requires that the type not be NULL. If the type might be
/// NULL, use the (slightly less efficient) \c getTypePtrOrNull().
const Type *getTypePtr() const;
 
const Type *getTypePtrOrNull() const;

... I am making an assumption that "type not be NULL" won't always be true because templates etc. Please tell me if that's wrong!

So, assuming we need to handle the possibility of type == NULL then if we move this into code isUncountedPtr() implementation I think we'd have to change the return value to a tri-state type (or similar alternative) which I consider a PITA to handle at all the call-sites.

[jkorous]

addressed the remaining comments except the C++ member call operators ParamIdx discussion

[NoQ]

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

82–84

unsigned ParamIdx = dyn_cast<CXXOperatorCallExpr>(CE);

I think it's only broken for member operators that are also non-static. CXXOperatorCallExpr is produced for all calls with operator syntax which includes both global overloads and methods, so in order to treat them uniformly it has to treat this in methods as a separate argument rather than something special.

(side note: can we call this variable ArgIdx instead of ParamIdx in order to minimise confusion in this very messy situation?)

90

There's also QualType.isNull() for that:

QualType ArgType = (*P)->getType();
if (ArgType.isNull())
  continue; // FIXME? Should we bail?

isUncountedPtr(ArgType);

I think it's almost never beneficial to call getTypePtr()/getTypePtrOrNull() in terms of conciseness and readability; it's mostly for low-level manipulations like hashing :/ Even for the purposes of dyn_casting the pointer it's easier to simply call QualType->getAs<>().

clang/test/Analysis/Checkers/WebKit/call-args.cpp

345

Phabricator doesn't seem to like this :(

[jkorous]

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

82–84

Ahh, right. I oversimplified it. Anyway, let me just add a bunch of test cases and see how it behaves. And thanks for the ArgIdx!

90

I see what you mean.

Actually my preferred interface would be isUncountedPtr(const Type&). That way the interface clearly states what are the valid inputs and type system takes care of enforcing it. Not sure why I chose Type* T and assert(T) - might be because "that's what we do in clang most of the time"?

Would this be ok with you?

QualType ArgType = (*P)->getType();
if (ArgType.isNull())
  continue; // FIXME? Should we bail?

isUncountedPtr(ArgType->getAs<Type>());

[jkorous]

• addressed most of the feedback
• added couple tests

[NoQ]

Great, thank you! I think this is good to land and i'm looking forward for more updates.

clang/docs/analyzer/checkers.rst

2407

I guess let's fix this checker name as well eventually?

[jkorous]

clang/docs/analyzer/checkers.rst

2407

Definitely, I will just will land it as a separate patch

clang/docs/analyzer/checkers.rst

struct RefCntblBase {
    void deref() {}
  };
 
  struct Derived : RefCntblBase { }; // warn
 
 .. _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
 
  struct RefCntbl {
    void ref() {}
    void deref() {}
  };
 
  struct Foo {
    RefCntbl * ptr; // warn
    RefCntbl & ptr; // warn
    // ...
  };
 
 .. _alpha-checkers:
 
 Experimental Checkers
 ---------------------
 
 *These are checkers with known issues or limitations that keep them from being on by default. They are likely to have false positives. Bug reports and especially patches are welcome.*
 
 alpha.clone
 ^^^^^^^^^^^
 
 .. _alpha-clone-CloneChecker:
 
 alpha.clone.CloneChecker (C, C++, ObjC)
 """""""""""""""""""""""""""""""""""""""
 Reports similar pieces of code.
 
 .. code-block:: c
 
  void log();
 
  int max(int a, int b) { // warn
    log();
    if (a > b)
      return a;
    return b;
  }
 
  int maxClone(int x, int y) { // similar code here
    log();
    if (x > y)
      return x;
    return y;
  }
 
 .. _alpha-core-BoolAssignment:
 
 alpha.core.BoolAssignment (ObjC)
 """"""""""""""""""""""""""""""""
 Warn about assigning non-{0,1} values to boolean variables.
 
 .. code-block:: objc
 
  void test() {
    BOOL b = -1; // warn
  }
 
 alpha.core
 ^^^^^^^^^^
 

 .. _alpha-core-CastSize:
 
 alpha.core.CastSize (C)
 """""""""""""""""""""""
 Check when casting a malloc'ed type ``T``, whether the size is a multiple of the size of ``T``.
 
 .. code-block:: c
 
  void test() {
    int *x = (int *) malloc(11); // warn
  }
 
 .. _alpha-core-CastToStruct:
 
 alpha.core.CastToStruct (C, C++)
 """"""""""""""""""""""""""""""""
 Check for cast from non-struct pointer to struct pointer.

 
  void test() {
   int a = 1, b = 2;
   std::vector<int *> V = {&a, &b};
   std::sort(V.begin(), V.end()); // warn
  }
 
 
+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
 ---------------
 
 .. _debug-checkers:
 
 
 debug
 ^^^^^

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

 //===----------------------------------------------------------------------===//
 
 let ParentPackage = WebKit in {
 
 def RefCntblBaseVirtualDtorChecker : Checker<"RefCntblBaseVirtualDtor">,
   HelpText<"Check for any ref-countable base class having virtual destructor.">,
   Documentation<HasDocumentation>;
 
 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

add_clang_library(clangStaticAnalyzerCheckers
   UninitializedObject/UninitializedPointee.cpp
   UnixAPIChecker.cpp
   UnreachableCodeChecker.cpp
   VforkChecker.cpp
   VLASizeChecker.cpp
   ValistChecker.cpp
   VirtualCallChecker.cpp
   WebKit/NoUncountedMembersChecker.cpp
+  WebKit/ASTUtils.cpp
   WebKit/PtrTypesSemantics.cpp
   WebKit/RefCntblBaseVirtualDtorChecker.cpp
+  WebKit/UncountedCallArgsChecker.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangAnalysis
   clangBasic
   clangLex
   clangStaticAnalyzerCore
   )

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

 
 namespace clang {
 class CXXRecordDecl;
 class CXXBaseSpecifier;
 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
 /// subexpressions to get to the relevant child nodes. Whenever we encounter a
 /// subexpression that either can't be ignored, we don't model its semantics or
 /// that has multiple children we stop.
 ///
 /// \p E is an expression of uncounted pointer/reference type.

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

+//=======- 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/UncountedCallArgsChecker.cpp

+//=======- UncountedCallArgsChecker.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/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 {
+
+class UncountedCallArgsChecker
+    : public Checker<check::ASTDecl<TranslationUnitDecl>> {
+  BugType Bug{this,
+            "Uncounted call argument for a raw pointer/reference parameter",
+            "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 UncountedCallArgsChecker *Checker;
+      explicit LocalVisitor(const UncountedCallArgsChecker *Checker)
+          : Checker(Checker) {
+        assert(Checker);
+      }
+
+      bool shouldVisitTemplateInstantiations() const { return true; }
+      bool shouldVisitImplicitCode() const { return false; }
+
+      bool VisitCallExpr(const CallExpr *CE) {
+        Checker->visitCallExpr(CE);
+        return true;
+      }
+    };
+
+    LocalVisitor visitor(this);
+    visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
+  }
+
+  void visitCallExpr(const CallExpr *CE) const {
+    if (shouldSkipCall(CE))
+      return;
+
+    if (auto *F = CE->getDirectCallee()) {
+      // Skip the first argument for overloaded member operators (e. g. lambda
+      // or std::function call operator).
+      unsigned ArgIdx =
+          isa<CXXOperatorCallExpr>(CE) && dyn_cast_or_null<CXXMethodDecl>(F);
+
+      for (auto P = F->param_begin();
+           // FIXME: Also check variadic function parameters.
+           // FIXME: Also check default function arguments. Probably a different
+           // checker. In case there are default arguments the call can have
+           // fewer arguments than the callee has parameters.
+           P < F->param_end() && ArgIdx < CE->getNumArgs(); ++P, ++ArgIdx) {
+        // TODO: attributes.
+        // if ((*P)->hasAttr<SafeRefCntblRawPtrAttr>())
+        //  continue;
+
+        const auto *ArgType = (*P)->getType().getTypePtrOrNull();
+        if (!ArgType)
+          continue; // FIXME? Should we bail?
+
+        // FIXME: more complex types (arrays, references to raw pointers, etc)
+        if (!isUncountedPtr(ArgType))
+          continue;
+
+        const auto *Arg = CE->getArg(ArgIdx);
+
+        std::pair<const clang::Expr *, bool> ArgOrigin =
+            tryToFindPtrOrigin(Arg, true);
+
+        // Temporary ref-counted object created as part of the call argument
+        // would outlive the call.
+        if (ArgOrigin.second)
+          continue;
+
+        if (isa<CXXNullPtrLiteralExpr>(ArgOrigin.first)) {
+          // foo(nullptr)
+          continue;
+        }
+        if (isa<IntegerLiteral>(ArgOrigin.first)) {
+          // FIXME: Check the value.
+          // foo(NULL)
+          continue;
+        }
+
+        if (isASafeCallArg(ArgOrigin.first))
+          continue;
+
+        reportBug(Arg, *P);
+      }
+    }
+  }
+
+  bool shouldSkipCall(const CallExpr *CE) const {
+    if (CE->getNumArgs() == 0)
+      return false;
+
+    // If an assignment is problematic we should warn about the sole existence
+    // of object on LHS.
+    if (auto *MemberOp = dyn_cast<CXXOperatorCallExpr>(CE)) {
+      // Note: assignemnt to built-in type isn't derived from CallExpr.
+      if (MemberOp->isAssignmentOp())
+        return false;
+    }
+
+    const auto *Callee = CE->getDirectCallee();
+    if (!Callee)
+      return false;
+
+    auto overloadedOperatorType = Callee->getOverloadedOperator();
+    if (overloadedOperatorType == OO_EqualEqual ||
+        overloadedOperatorType == OO_ExclaimEqual ||
+        overloadedOperatorType == OO_LessEqual ||
+        overloadedOperatorType == OO_GreaterEqual ||
+        overloadedOperatorType == OO_Spaceship ||
+        overloadedOperatorType == OO_AmpAmp ||
+        overloadedOperatorType == OO_PipePipe)
+      return true;
+
+    if (isCtorOfRefCounted(Callee))
+      return true;
+
+    auto name = safeGetName(Callee);
+    if (name == "adoptRef" || name == "getPtr" || name == "WeakPtr" ||
+        name == "makeWeakPtr" || name == "downcast" || name == "bitwise_cast" ||
+        name == "is" || name == "equal" || name == "hash" ||
+        name == "isType"
+        // FIXME: Most/all of these should be implemented via attributes.
+        || name == "equalIgnoringASCIICase" ||
+        name == "equalIgnoringASCIICaseCommon" ||
+        name == "equalIgnoringNullity")
+      return true;
+
+    return false;
+  }
+
+  void reportBug(const Expr *CallArg, const ParmVarDecl *Param) const {
+    assert(CallArg);
+
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+
+    const std::string paramName = safeGetName(Param);
+    Os << "Call argument";
+    if (!paramName.empty()) {
+      Os << " for parameter ";
+      printQuotedQualifiedName(Os, Param);
+    }
+    Os << " is uncounted and unsafe.";
+
+    const SourceLocation SrcLocToReport =
+        isa<CXXDefaultArgExpr>(CallArg) ? Param->getDefaultArg()->getExprLoc()
+                                        : CallArg->getSourceRange().getBegin();
+
+    PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    Report->addRange(CallArg->getSourceRange());
+    BR->emitReport(std::move(Report));
+  }
+};
+} // namespace
+
+void ento::registerUncountedCallArgsChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<UncountedCallArgsChecker>();
+}
+
+bool ento::shouldRegisterUncountedCallArgsChecker(const CheckerManager &) {
+  return true;
+}

clang/test/Analysis/Checkers/WebKit/call-args.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
+
+#include "mock-types.h"
+
+RefCountable* provide() { return nullptr; }
+void consume_refcntbl(RefCountable*) {}
+
+namespace simple {
+  void foo() {
+    consume_refcntbl(provide());
+    // expected-warning@-1{{Call argument is uncounted and unsafe}}
+  }
+}
+
+namespace multi_arg {
+  void consume_refcntbl(int, RefCountable* foo, bool) {}
+  void foo() {
+    consume_refcntbl(42, provide(), true);
+    // expected-warning@-1{{Call argument for parameter 'foo' is uncounted and unsafe}}
+  }
+}
+
+namespace ref_counted {
+  Ref<RefCountable> provide_ref_counted() { return Ref<RefCountable>{}; }
+  void consume_ref_counted(Ref<RefCountable>) {}
+
+  void foo() {
+    consume_refcntbl(provide_ref_counted().get());
+    // no warning
+  }
+}
+
+namespace methods {
+  struct Consumer {
+    void consume_ptr(RefCountable* ptr) {}
+    void consume_ref(const RefCountable& ref) {}
+  };
+
+  void foo() {
+    Consumer c;
+
+    c.consume_ptr(provide());
+    // expected-warning@-1{{Call argument for parameter 'ptr' is uncounted and unsafe}}
+    c.consume_ref(*provide());
+    // expected-warning@-1{{Call argument for parameter 'ref' is uncounted and unsafe}}
+  }
+
+  void foo2() {
+    struct Consumer {
+      void consume(RefCountable*) { }
+      void whatever() {
+        consume(provide());
+        // expected-warning@-1{{Call argument is uncounted and unsafe}}
+      }
+    };
+  }
+
+  void foo3() {
+    struct Consumer {
+      void consume(RefCountable*) { }
+      void whatever() {
+        this->consume(provide());
+        // expected-warning@-1{{Call argument is uncounted and unsafe}}
+      }
+    };
+  }
+}
+
+namespace casts {
+  RefCountable* downcast(RefCountable*) { return nullptr; }
+
+  void foo() {
+    consume_refcntbl(provide());
+    // expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+    consume_refcntbl(static_cast<RefCountable*>(provide()));
+    // expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+    consume_refcntbl(dynamic_cast<RefCountable*>(provide()));
+    // expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+    consume_refcntbl(const_cast<RefCountable*>(provide()));
+    // expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+    consume_refcntbl(reinterpret_cast<RefCountable*>(provide()));
+    // expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+    consume_refcntbl(downcast(provide()));
+    // expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+    consume_refcntbl(
+      static_cast<RefCountable*>(
+        downcast(
+          static_cast<RefCountable*>(
+            provide()
+          )
+        )
+      )
+    );
+    // expected-warning@-8{{Call argument is uncounted and unsafe}}
+  }
+}
+
+namespace null_ptr {
+  void foo_ref() {
+    consume_refcntbl(nullptr);
+    consume_refcntbl(0);
+  }
+}
+
+namespace ref_counted_lookalike {
+  struct Decoy {
+    RefCountable* get() { return nullptr; }
+  };
+
+  void foo() {
+    Decoy D;
+
+    consume_refcntbl(D.get());
+    // expected-warning@-1{{Call argument is uncounted and unsafe}}
+  }
+}
+
+namespace Ref_to_reference_conversion_operator {
+  template<typename T> struct Ref {
+    Ref() = default;
+    Ref(T*) { }
+    T* get() { return nullptr; }
+    operator T& () { return t; }
+    T t;
+  };
+
+  void consume_ref(RefCountable&) {}
+
+  void foo() {
+    Ref<RefCountable> bar;
+    consume_ref(bar);
+  }
+}
+
+namespace param_formarding_function {
+  void consume_ref_countable_ref(RefCountable&) {}
+  void consume_ref_countable_ptr(RefCountable*) {}
+
+  namespace ptr {
+    void foo(RefCountable* param) {
+      consume_ref_countable_ptr(param);
+    }
+  }
+
+  namespace ref {
+    void foo(RefCountable& param) {
+      consume_ref_countable_ref(param);
+    }
+  }
+
+  namespace ref_deref_operators {
+    void foo_ref(RefCountable& param) {
+      consume_ref_countable_ptr(&param);
+    }
+
+    void foo_ptr(RefCountable* param) {
+      consume_ref_countable_ref(*param);
+    }
+  }
+
+  namespace casts {
+
+  RefCountable* downcast(RefCountable*) { return nullptr; }
+
+  template<class T>
+  T* bitwise_cast(T*) { return nullptr; }
+
+    void foo(RefCountable* param) {
+      consume_ref_countable_ptr(downcast(param));
+      consume_ref_countable_ptr(bitwise_cast(param));
+     }
+  }
+}
+
+namespace param_formarding_lambda {
+  auto consume_ref_countable_ref = [](RefCountable&) {};
+  auto consume_ref_countable_ptr = [](RefCountable*) {};
+
+  namespace ptr {
+    void foo(RefCountable* param) {
+      consume_ref_countable_ptr(param);
+    }
+  }
+
+  namespace ref {
+    void foo(RefCountable& param) {
+      consume_ref_countable_ref(param);
+    }
+  }
+
+  namespace ref_deref_operators {
+    void foo_ref(RefCountable& param) {
+      consume_ref_countable_ptr(&param);
+    }
+
+    void foo_ptr(RefCountable* param) {
+      consume_ref_countable_ref(*param);
+    }
+  }
+
+  namespace casts {
+
+  RefCountable* downcast(RefCountable*) { return nullptr; }
+
+  template<class T>
+  T* bitwise_cast(T*) { return nullptr; }
+
+    void foo(RefCountable* param) {
+      consume_ref_countable_ptr(downcast(param));
+      consume_ref_countable_ptr(bitwise_cast(param));
+    }
+  }
+}
+
+namespace param_forwarding_method {
+  struct methodclass {
+    void consume_ref_countable_ref(RefCountable&) {};
+    static void consume_ref_countable_ptr(RefCountable*) {};
+  };
+
+  namespace ptr {
+    void foo(RefCountable* param) {
+      methodclass::consume_ref_countable_ptr(param);
+     }
+  }
+
+  namespace ref {
+    void foo(RefCountable& param) {
+      methodclass mc;
+      mc.consume_ref_countable_ref(param);
+     }
+  }
+
+  namespace ref_deref_operators {
+    void foo_ref(RefCountable& param) {
+      methodclass::consume_ref_countable_ptr(&param);
+     }
+
+    void foo_ptr(RefCountable* param) {
+      methodclass mc;
+      mc.consume_ref_countable_ref(*param);
+     }
+  }
+
+  namespace casts {
+
+  RefCountable* downcast(RefCountable*) { return nullptr; }
+
+  template<class T>
+  T* bitwise_cast(T*) { return nullptr; }
+
+    void foo(RefCountable* param) {
+      methodclass::consume_ref_countable_ptr(downcast(param));
+       methodclass::consume_ref_countable_ptr(bitwise_cast(param));
+     }
+  }
+}
+
+namespace make_ref {
+  void makeRef(RefCountable*) {}
+  void makeRefPtr(RefCountable*) {}
+  void makeWeakPtr(RefCountable*) {}
+  void makeWeakPtr(RefCountable&) {}
+
+  void foo() {
+    makeRef(provide());
+    makeRefPtr(provide());
+    RefPtr<RefCountable> a(provide());
+    Ref<RefCountable> b(provide());
+    makeWeakPtr(provide());
+    makeWeakPtr(*provide());
+  }
+}
+
+namespace downcast {
+  void consume_ref_countable(RefCountable*) {}
+  RefCountable* downcast(RefCountable*) { return nullptr; }
+
+  void foo() {
+    RefPtr<RefCountable> bar;
+    consume_ref_countable( downcast(bar.get()) );
+  }
+}
+
+namespace string_impl {
+  struct String {
+    RefCountable* impl() { return nullptr; }
+  };
+
+  struct AtomString {
+    RefCountable rc;
+    RefCountable& impl() { return rc; }
+  };
+
+  void consume_ptr(RefCountable*) {}
+  void consume_ref(RefCountable&) {}
+
+  namespace simple {
+    void foo() {
+      String s;
+      AtomString as;
+      consume_ptr(s.impl());
+      consume_ref(as.impl());
+    }
+  }
+}
+
+namespace default_arg {
+  RefCountable* global;
+
+  void function_with_default_arg(RefCountable* param = global) {}
+  // expected-warning@-1{{Call argument for parameter 'param' is uncounted and unsafe}}
+
+  void foo() {
+    function_with_default_arg();
+  }
+}
+
+namespace cxx_member_operator_call {
+  // The hidden this-pointer argument without a corresponding parameter caused couple bugs in parameter <-> argument attribution.
+  struct Foo {
+    Foo& operator+(RefCountable* bad) { return *this; }
+    friend Foo& operator-(Foo& lhs, RefCountable* bad) { return lhs; }
+    void operator()(RefCountable* bad) { }
+  };
+
+  RefCountable* global;
+
+  void foo() {
+    Foo f;
+    f + global;
+    // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
+    f - global;
+    // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
+    f(global);
+    // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
+  }
+}