Merge Webkit checker fixes

clang/docs/analyzer/checkers.rst

@@ -3433,6 +3433,24 @@ Check for non-determinism caused by sorting of pointers.
 alpha.WebKit
 ^^^^^^^^^^^^
 
+alpha.webkit.ForwardDeclChecker
+"""""""""""""""""""""""""""""""
+Check for local variables, member variables, and function arguments that are forward declared.
+
+.. code-block:: cpp
+
+ struct Obj;
+ Obj* provide();
+
+ struct Foo {
+   Obj* ptr; // warn
+ };
+
+  void foo() {
+    Obj* obj = provide(); // warn
+    consume(obj); // warn
+  }
+
 .. _alpha-webkit-NoUncheckedPtrMemberChecker:
 
 alpha.webkit.MemoryUnsafeCastChecker
@@ -3479,6 +3497,31 @@ Raw pointers and references to an object which supports CheckedPtr or CheckedRef
 
 See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
 
+alpha.webkit.NoUnretainedMemberChecker
+""""""""""""""""""""""""""""""""""""""""
+Raw pointers and references to a NS or CF object can't be used as class members or ivars. Only RetainPtr is allowed for CF types regardless of whether ARC is enabled or disabled. Only RetainPtr is allowed for NS types when ARC is disabled.
+
+.. code-block:: cpp
+
+ struct Foo {
+   NSObject *ptr; // warn
+   // ...
+ };
+
+See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
+
+alpha.webkit.UnretainedLambdaCapturesChecker
+""""""""""""""""""""""""""""""""""""""""""""
+Raw pointers and references to NS or CF types can't be captured in lambdas. Only RetainPtr is allowed for CF types regardless of whether ARC is enabled or disabled, and only RetainPtr is allowed for NS types when ARC is disabled.
+
+.. code-block:: cpp
+
+ void foo(NSObject *a, NSObject *b) {
+   [&, a](){ // warn about 'a'
+     do_something(b); // warn about 'b'
+   };
+ };
+
 .. _alpha-webkit-UncountedCallArgsChecker:
 
 alpha.webkit.UncountedCallArgsChecker
@@ -3574,6 +3617,12 @@ The goal of this rule is to make sure that lifetime of any dynamically allocated
 
 The rules of when to use and not to use CheckedPtr / CheckedRef are same as alpha.webkit.UncountedCallArgsChecker for ref-counted objects.
 
+alpha.webkit.UnretainedCallArgsChecker
+""""""""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure that lifetime of any dynamically allocated NS or CF objects passed as a call argument keeps its memory region past the end of the call. This applies to call to any function, method, lambda, function pointer or functor. NS or CF objects aren't supposed to be allocated on stack so we check arguments for parameters of raw pointers and references to unretained types.
+
+The rules of when to use and not to use RetainPtr are same as alpha.webkit.UncountedCallArgsChecker for ref-counted objects.
+
 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.
@@ -3660,6 +3709,71 @@ Here are some examples of situations that we warn about as they *might* be poten
       RefCountable* uncounted = counted.get(); // warn
     }
 
+alpha.webkit.UnretainedLocalVarsChecker
+"""""""""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure that any NS or CF local variable is backed by a RetainPtr with lifetime that is strictly larger than the scope of the unretained local variable. To be on the safe side we require the scope of an unretained variable to be embedded in the scope of Retainptr object that backs it.
+
+The rules of when to use and not to use RetainPtr are same as alpha.webkit.UncountedCallArgsChecker for ref-counted objects.
+
+These are examples of cases that we consider safe:
+
+  .. code-block:: cpp
+
+    void foo1() {
+      RetainPtr<NSObject> retained;
+      // The scope of unretained is EMBEDDED in the scope of retained.
+      {
+        NSObject* unretained = retained.get(); // ok
+      }
+    }
+
+    void foo2(RetainPtr<NSObject> retained_param) {
+      NSObject* unretained = retained_param.get(); // ok
+    }
+
+    void FooClass::foo_method() {
+      NSObject* unretained = 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() {
+      NSObject* unretained = [[NSObject alloc] init]; // warn
+    }
+
+    NSObject* global_unretained;
+    void foo2() {
+      NSObject* unretained = global_unretained; // warn
+    }
+
+    void foo3() {
+      RetainPtr<NSObject> retained;
+      // The scope of unretained is not EMBEDDED in the scope of retained.
+      NSObject* unretained = retained.get(); // warn
+    }
+
+webkit.RetainPtrCtorAdoptChecker
+""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure the constructor of RetainPtr as well as adoptNS and adoptCF are used correctly.
+When creating a RetainPtr with +1 semantics, adoptNS or adoptCF should be used, and in +0 semantics, RetainPtr constructor should be used.
+Warn otherwise.
+
+These are examples of cases that we consider correct:
+
+  .. code-block:: cpp
+
+    RetainPtr ptr = adoptNS([[NSObject alloc] init]); // ok
+    RetainPtr ptr = CGImageGetColorSpace(image); // ok
+
+Here are some examples of cases that we consider incorrect use of RetainPtr constructor and adoptCF
+
+  .. code-block:: cpp
+
+    RetainPtr ptr = [[NSObject alloc] init]; // warn
+    auto ptr = adoptCF(CGImageGetColorSpace(image)); // warn
+
 Debug Checkers
 ---------------
 

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

@@ -1787,6 +1787,10 @@ def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">,
 
 let ParentPackage = WebKitAlpha in {
 
+def ForwardDeclChecker : Checker<"ForwardDeclChecker">,
+  HelpText<"Check for forward declared local or member variables and function arguments">,
+  Documentation<HasDocumentation>;
+
 def MemoryUnsafeCastChecker : Checker<"MemoryUnsafeCastChecker">,
   HelpText<"Check for memory unsafe casts from base type to derived type.">,
   Documentation<HasDocumentation>;
@@ -1795,6 +1799,14 @@ def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">,
   HelpText<"Check for no unchecked member variables.">,
   Documentation<HasDocumentation>;
 
+def NoUnretainedMemberChecker : Checker<"NoUnretainedMemberChecker">,
+  HelpText<"Check for no unretained member variables.">,
+  Documentation<HasDocumentation>;
+
+def UnretainedLambdaCapturesChecker : Checker<"UnretainedLambdaCapturesChecker">,
+  HelpText<"Check unretained lambda captures.">,
+  Documentation<HasDocumentation>;
+
 def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
   HelpText<"Check uncounted call arguments.">,
   Documentation<HasDocumentation>;
@@ -1803,6 +1815,10 @@ def UncheckedCallArgsChecker : Checker<"UncheckedCallArgsChecker">,
   HelpText<"Check unchecked call arguments.">,
   Documentation<HasDocumentation>;
 
+def UnretainedCallArgsChecker : Checker<"UnretainedCallArgsChecker">,
+  HelpText<"Check unretained call arguments.">,
+  Documentation<HasDocumentation>;
+
 def UncountedLocalVarsChecker : Checker<"UncountedLocalVarsChecker">,
   HelpText<"Check uncounted local variables.">,
   Documentation<HasDocumentation>;
@@ -1811,4 +1827,12 @@ def UncheckedLocalVarsChecker : Checker<"UncheckedLocalVarsChecker">,
   HelpText<"Check unchecked local variables.">,
   Documentation<HasDocumentation>;
 
+def UnretainedLocalVarsChecker : Checker<"UnretainedLocalVarsChecker">,
+  HelpText<"Check unretained local variables.">,
+  Documentation<HasDocumentation>;
+
+def RetainPtrCtorAdoptChecker : Checker<"RetainPtrCtorAdoptChecker">,
+  HelpText<"Check for correct use of RetainPtr constructor, adoptNS, and adoptCF">,
+  Documentation<HasDocumentation>;
+
 } // end alpha.webkit

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

@@ -133,14 +133,16 @@ add_clang_library(clangStaticAnalyzerCheckers
   VLASizeChecker.cpp
   ValistChecker.cpp
   VirtualCallChecker.cpp
-  WebKit/RawPtrRefMemberChecker.cpp
   WebKit/ASTUtils.cpp
+  WebKit/ForwardDeclChecker.cpp
   WebKit/MemoryUnsafeCastChecker.cpp
   WebKit/PtrTypesSemantics.cpp
   WebKit/RefCntblBaseVirtualDtorChecker.cpp
+  WebKit/RetainPtrCtorAdoptChecker.cpp
   WebKit/RawPtrRefCallArgsChecker.cpp
-  WebKit/UncountedLambdaCapturesChecker.cpp
+  WebKit/RawPtrRefLambdaCapturesChecker.cpp
   WebKit/RawPtrRefLocalVarsChecker.cpp
+  WebKit/RawPtrRefMemberChecker.cpp
 
   LINK_LIBS
   clangAST

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

@@ -8,6 +8,7 @@
 
 #include "ASTUtils.h"
 #include "PtrTypesSemantics.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
@@ -24,8 +25,18 @@ bool isSafePtr(clang::CXXRecordDecl *Decl) {
 
 bool tryToFindPtrOrigin(
     const Expr *E, bool StopAtFirstRefCountedObj,
+    std::function<bool(const clang::CXXRecordDecl *)> isSafePtr,
+    std::function<bool(const clang::QualType)> isSafePtrType,
     std::function<bool(const clang::Expr *, bool)> callback) {
   while (E) {
+    if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+      if (auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) {
+        auto QT = VD->getType();
+        if (VD->hasGlobalStorage() && QT.isConstQualified()) {
+          return callback(E, true);
+        }
+      }
+    }
     if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
       E = tempExpr->getSubExpr();
       continue;
@@ -41,6 +52,10 @@ bool tryToFindPtrOrigin(
         break;
       }
     }
+    if (auto *TempExpr = dyn_cast<CXXUnresolvedConstructExpr>(E)) {
+      if (isSafePtrType(TempExpr->getTypeAsWritten()))
+        return callback(TempExpr, true);
+    }
     if (auto *POE = dyn_cast<PseudoObjectExpr>(E)) {
       if (auto *RF = POE->getResultExpr()) {
         E = RF;
@@ -51,11 +66,15 @@ bool tryToFindPtrOrigin(
       E = tempExpr->getSubExpr();
       continue;
     }
+    if (auto *OpaqueValue = dyn_cast<OpaqueValueExpr>(E)) {
+      E = OpaqueValue->getSourceExpr();
+      continue;
+    }
     if (auto *Expr = dyn_cast<ConditionalOperator>(E)) {
       return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj,
-                                callback) &&
+                                isSafePtr, isSafePtrType, callback) &&
              tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj,
-                                callback);
+                                isSafePtr, isSafePtrType, callback);
     }
     if (auto *cast = dyn_cast<CastExpr>(E)) {
       if (StopAtFirstRefCountedObj) {
@@ -64,6 +83,8 @@ bool tryToFindPtrOrigin(
           if (isCtorOfSafePtr(ConversionFunc))
             return callback(E, true);
         }
+        if (isa<CXXFunctionalCastExpr>(E) && isSafePtrType(cast->getType()))
+          return callback(E, true);
       }
       // FIXME: This can give false "origin" that would lead to false negatives
       // in checkers. See https://reviews.llvm.org/D37023 for reference.
@@ -85,14 +106,22 @@ bool tryToFindPtrOrigin(
       }
 
       if (auto *operatorCall = dyn_cast<CXXOperatorCallExpr>(E)) {
-        if (operatorCall->getNumArgs() == 1) {
-          E = operatorCall->getArg(0);
-          continue;
+        if (auto *Callee = operatorCall->getDirectCallee()) {
+          auto ClsName = safeGetName(Callee->getParent());
+          if (isRefType(ClsName) || isCheckedPtr(ClsName) ||
+              isRetainPtr(ClsName) || ClsName == "unique_ptr" ||
+              ClsName == "UniqueRef" || ClsName == "WeakPtr" ||
+              ClsName == "WeakRef") {
+            if (operatorCall->getNumArgs() == 1) {
+              E = operatorCall->getArg(0);
+              continue;
+            }
+          }
         }
       }
 
       if (auto *callee = call->getDirectCallee()) {
-        if (isCtorOfRefCounted(callee) || isCtorOfCheckedPtr(callee)) {
+        if (isCtorOfSafePtr(callee)) {
           if (StopAtFirstRefCountedObj)
             return callback(E, true);
 
@@ -115,14 +144,30 @@ bool tryToFindPtrOrigin(
           E = call->getArg(0);
           continue;
         }
+
+        auto Name = safeGetName(callee);
+        if (Name == "__builtin___CFStringMakeConstantString" ||
+            Name == "NSClassFromString")
+          return callback(E, true);
       }
     }
     if (auto *ObjCMsgExpr = dyn_cast<ObjCMessageExpr>(E)) {
       if (auto *Method = ObjCMsgExpr->getMethodDecl()) {
         if (isSafePtrType(Method->getReturnType()))
           return callback(E, true);
       }
+      auto Selector = ObjCMsgExpr->getSelector();
+      auto NameForFirstSlot = Selector.getNameForSlot(0);
+      if ((NameForFirstSlot == "class" || NameForFirstSlot == "superclass") &&
+          !Selector.getNumArgs())
+        return callback(E, true);
     }
+    if (auto *ObjCDict = dyn_cast<ObjCDictionaryLiteral>(E))
+      return callback(ObjCDict, true);
+    if (auto *ObjCArray = dyn_cast<ObjCArrayLiteral>(E))
+      return callback(ObjCArray, true);
+    if (auto *ObjCStr = dyn_cast<ObjCStringLiteral>(E))
+      return callback(ObjCStr, true);
     if (auto *unaryOp = dyn_cast<UnaryOperator>(E)) {
       // FIXME: Currently accepts ANY unary operator. Is it OK?
       E = unaryOp->getSubExpr();
@@ -138,9 +183,22 @@ bool tryToFindPtrOrigin(
 bool isASafeCallArg(const Expr *E) {
   assert(E);
   if (auto *Ref = dyn_cast<DeclRefExpr>(E)) {
-    if (auto *D = dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
+    auto *FoundDecl = Ref->getFoundDecl();
+    if (auto *D = dyn_cast_or_null<VarDecl>(FoundDecl)) {
       if (isa<ParmVarDecl>(D) || D->isLocalVarDecl())
         return true;
+      if (auto *ImplicitP = dyn_cast<ImplicitParamDecl>(D)) {
+        auto Kind = ImplicitP->getParameterKind();
+        if (Kind == ImplicitParamKind::ObjCSelf ||
+            Kind == ImplicitParamKind::ObjCCmd ||
+            Kind == ImplicitParamKind::CXXThis ||
+            Kind == ImplicitParamKind::CXXVTT)
+          return true;
+      }
+    } else if (auto *BD = dyn_cast_or_null<BindingDecl>(FoundDecl)) {
+      VarDecl *VD = BD->getHoldingVar();
+      if (VD && (isa<ParmVarDecl>(VD) || VD->isLocalVarDecl()))
+        return true;
     }
   }
   if (isConstOwnerPtrMemberExpr(E))
@@ -204,7 +262,7 @@ bool EnsureFunctionAnalysis::isACallToEnsureFn(const clang::Expr *E) const {
   if (!Callee)
     return false;
   auto *Body = Callee->getBody();
-  if (!Body)
+  if (!Body || Callee->isVirtualAsWritten())
     return false;
   auto [CacheIt, IsNew] = Cache.insert(std::make_pair(Callee, false));
   if (IsNew)

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

@@ -54,6 +54,8 @@ class Expr;
 /// Returns false if any of calls to callbacks returned false. Otherwise true.
 bool tryToFindPtrOrigin(
     const clang::Expr *E, bool StopAtFirstRefCountedObj,
+    std::function<bool(const clang::CXXRecordDecl *)> isSafePtr,
+    std::function<bool(const clang::QualType)> isSafePtrType,
     std::function<bool(const clang::Expr *, bool)> callback);
 
 /// For \p E referring to a ref-countable/-counted pointer/reference we return