[alpha.webkit.UnretainedCallArgsChecker] Add a checker for NS or CF type call arguments. (#130729)

This PR adds alpha.webkit.UnretainedCallArgsChecker by generalizing
RawPtrRefCallArgsChecker. It checks call arguments of NS or CF types are
backed by a RetainPtr or not. The new checker emits warnings for CF
types in Objective-C++ code regardless of ARC and it emits warning for
NS types when ARC is disabled.

Hard-code a few cases as safe: class and superclass properties on class
interfaces and constant dictionary or array literals as well as the
return value of NSClassFromString.

Finally, add the support for variadic function calls and selector
invocations.

Author: rniwa

clang/docs/analyzer/checkers.rst

@@ -3642,6 +3642,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.

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

@@ -1785,6 +1785,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>;

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"
@@ -28,6 +29,15 @@ bool tryToFindPtrOrigin(
     std::function<bool(const clang::QualType)> isSafePtrType,
     std::function<bool(const clang::Expr *, bool)> callback) {
   while (E) {
+    if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+      auto *ValDecl = DRE->getDecl();
+      auto QT = ValDecl->getType();
+      auto ValName = ValDecl->getName();
+      if (ValDecl && (ValName.starts_with('k') || ValName.starts_with("_k")) &&
+          QT.isConstQualified()) { // Treat constants such as kCF* as safe.
+        return callback(E, true);
+      }
+    }
     if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
       E = tempExpr->getSubExpr();
       continue;
@@ -57,6 +67,10 @@ 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,
                                 isSafePtr, isSafePtrType, callback) &&
@@ -129,14 +143,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();
@@ -156,6 +186,14 @@ bool isASafeCallArg(const Expr *E) {
     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()))

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

@@ -372,7 +372,8 @@ std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl *M) {
       if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M)) {
         auto QT = maybeRefToRawOperator->getConversionType();
         auto *T = QT.getTypePtrOrNull();
-        return T && (T->isPointerType() || T->isReferenceType());
+        return T && (T->isPointerType() || T->isReferenceType() ||
+                     T->isObjCObjectPointerType());
       }
     }
   }
@@ -415,7 +416,8 @@ bool isPtrConversion(const FunctionDecl *F) {
   if (FunctionName == "getPtr" || FunctionName == "WeakPtr" ||
       FunctionName == "dynamicDowncast" || FunctionName == "downcast" ||
       FunctionName == "checkedDowncast" ||
-      FunctionName == "uncheckedDowncast" || FunctionName == "bitwise_cast")
+      FunctionName == "uncheckedDowncast" || FunctionName == "bitwise_cast" ||
+      FunctionName == "bridge_cast")
     return true;
 
   return false;

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

@@ -13,6 +13,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DynamicRecursiveASTVisitor.h"
+#include "clang/Analysis/DomainSpecific/CocoaConventions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
@@ -35,6 +36,9 @@ class RawPtrRefCallArgsChecker
   TrivialFunctionAnalysis TFA;
   EnsureFunctionAnalysis EFA;
 
+protected:
+  mutable std::optional<RetainTypeChecker> RTC;
+
 public:
   RawPtrRefCallArgsChecker(const char *description)
       : Bug(this, description, "WebKit coding guidelines") {}
@@ -80,9 +84,22 @@ class RawPtrRefCallArgsChecker
         Checker->visitCallExpr(CE, DeclWithIssue);
         return true;
       }
+
+      bool VisitTypedefDecl(TypedefDecl *TD) override {
+        if (Checker->RTC)
+          Checker->RTC->visitTypedef(TD);
+        return true;
+      }
+
+      bool VisitObjCMessageExpr(ObjCMessageExpr *ObjCMsgExpr) override {
+        Checker->visitObjCMessageExpr(ObjCMsgExpr, DeclWithIssue);
+        return true;
+      }
     };
 
     LocalVisitor visitor(this);
+    if (RTC)
+      RTC->visitTranslationUnitDecl(TUD);
     visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
   }
 
@@ -122,7 +139,7 @@ class RawPtrRefCallArgsChecker
         // if ((*P)->hasAttr<SafeRefCntblRawPtrAttr>())
         //  continue;
 
-        QualType ArgType = (*P)->getType().getCanonicalType();
+        QualType ArgType = (*P)->getType();
         // FIXME: more complex types (arrays, references to raw pointers, etc)
         std::optional<bool> IsUncounted = isUnsafePtr(ArgType);
         if (!IsUncounted || !(*IsUncounted))
@@ -138,6 +155,58 @@ class RawPtrRefCallArgsChecker
 
         reportBug(Arg, *P, D);
       }
+      for (; ArgIdx < CE->getNumArgs(); ++ArgIdx) {
+        const auto *Arg = CE->getArg(ArgIdx);
+        auto ArgType = Arg->getType();
+        std::optional<bool> IsUncounted = isUnsafePtr(ArgType);
+        if (!IsUncounted || !(*IsUncounted))
+          continue;
+
+        if (auto *defaultArg = dyn_cast<CXXDefaultArgExpr>(Arg))
+          Arg = defaultArg->getExpr();
+
+        if (isPtrOriginSafe(Arg))
+          continue;
+
+        reportBug(Arg, nullptr, D);
+      }
+    }
+  }
+
+  void visitObjCMessageExpr(const ObjCMessageExpr *E, const Decl *D) const {
+    if (BR->getSourceManager().isInSystemHeader(E->getExprLoc()))
+      return;
+
+    auto Selector = E->getSelector();
+    if (auto *Receiver = E->getInstanceReceiver()->IgnoreParenCasts()) {
+      std::optional<bool> IsUnsafe = isUnsafePtr(E->getReceiverType());
+      if (IsUnsafe && *IsUnsafe && !isPtrOriginSafe(Receiver)) {
+        if (auto *InnerMsg = dyn_cast<ObjCMessageExpr>(Receiver)) {
+          auto InnerSelector = InnerMsg->getSelector();
+          if (InnerSelector.getNameForSlot(0) == "alloc" &&
+              Selector.getNameForSlot(0).starts_with("init"))
+            return;
+        }
+        reportBugOnReceiver(Receiver, D);
+      }
+    }
+
+    auto *MethodDecl = E->getMethodDecl();
+    if (!MethodDecl)
+      return;
+
+    auto ArgCount = E->getNumArgs();
+    for (unsigned i = 0; i < ArgCount; ++i) {
+      auto *Arg = E->getArg(i);
+      bool hasParam = i < MethodDecl->param_size();
+      auto *Param = hasParam ? MethodDecl->getParamDecl(i) : nullptr;
+      auto ArgType = Arg->getType();
+      std::optional<bool> IsUnsafe = isUnsafePtr(ArgType);
+      if (!IsUnsafe || !(*IsUnsafe))
+        continue;
+      if (isPtrOriginSafe(Arg))
+        continue;
+      reportBug(Arg, Param, D);
     }
   }
 
@@ -158,6 +227,8 @@ class RawPtrRefCallArgsChecker
             // foo(NULL)
             return true;
           }
+          if (isa<ObjCStringLiteral>(ArgOrigin))
+            return true;
           if (isASafeCallArg(ArgOrigin))
             return true;
           if (EFA.isACallToEnsureFn(ArgOrigin))
@@ -212,7 +283,7 @@ class RawPtrRefCallArgsChecker
         overloadedOperatorType == OO_PipePipe)
       return true;
 
-    if (isCtorOfRefCounted(Callee))
+    if (isCtorOfSafePtr(Callee))
       return true;
 
     auto name = safeGetName(Callee);
@@ -277,9 +348,10 @@ class RawPtrRefCallArgsChecker
     }
     Os << " is " << ptrKind() << " and unsafe.";
 
+    bool usesDefaultArgValue = isa<CXXDefaultArgExpr>(CallArg) && Param;
     const SourceLocation SrcLocToReport =
-        isa<CXXDefaultArgExpr>(CallArg) ? Param->getDefaultArg()->getExprLoc()
-                                        : CallArg->getSourceRange().getBegin();
+        usesDefaultArgValue ? Param->getDefaultArg()->getExprLoc()
+                            : CallArg->getSourceRange().getBegin();
 
     PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager());
     auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
@@ -304,6 +376,23 @@ class RawPtrRefCallArgsChecker
     Report->setDeclWithIssue(DeclWithIssue);
     BR->emitReport(std::move(Report));
   }
+
+  void reportBugOnReceiver(const Expr *CallArg,
+                           const Decl *DeclWithIssue) const {
+    assert(CallArg);
+
+    const SourceLocation SrcLocToReport = CallArg->getSourceRange().getBegin();
+
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+    Os << "Reciever is " << ptrKind() << " and unsafe.";
+
+    PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    Report->addRange(CallArg->getSourceRange());
+    Report->setDeclWithIssue(DeclWithIssue);
+    BR->emitReport(std::move(Report));
+  }
 };
 
 class UncountedCallArgsChecker final : public RawPtrRefCallArgsChecker {
@@ -317,7 +406,7 @@ class UncountedCallArgsChecker final : public RawPtrRefCallArgsChecker {
   }
 
   std::optional<bool> isUnsafePtr(QualType QT) const final {
-    return isUncountedPtr(QT);
+    return isUncountedPtr(QT.getCanonicalType());
   }
 
   bool isSafePtr(const CXXRecordDecl *Record) const final {
@@ -342,7 +431,7 @@ class UncheckedCallArgsChecker final : public RawPtrRefCallArgsChecker {
   }
 
   std::optional<bool> isUnsafePtr(QualType QT) const final {
-    return isUncheckedPtr(QT);
+    return isUncheckedPtr(QT.getCanonicalType());
   }
 
   bool isSafePtr(const CXXRecordDecl *Record) const final {
@@ -356,6 +445,33 @@ class UncheckedCallArgsChecker final : public RawPtrRefCallArgsChecker {
   const char *ptrKind() const final { return "unchecked"; }
 };
 
+class UnretainedCallArgsChecker final : public RawPtrRefCallArgsChecker {
+public:
+  UnretainedCallArgsChecker()
+      : RawPtrRefCallArgsChecker("Unretained call argument for a raw "
+                                 "pointer/reference parameter") {
+    RTC = RetainTypeChecker();
+  }
+
+  std::optional<bool> isUnsafeType(QualType QT) const final {
+    return RTC->isUnretained(QT);
+  }
+
+  std::optional<bool> isUnsafePtr(QualType QT) const final {
+    return RTC->isUnretained(QT);
+  }
+
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRetainPtr(Record);
+  }
+
+  bool isSafePtrType(const QualType type) const final {
+    return isRetainPtrType(type);
+  }
+
+  const char *ptrKind() const final { return "unretained"; }
+};
+
 } // namespace
 
 void ento::registerUncountedCallArgsChecker(CheckerManager &Mgr) {
@@ -373,3 +489,11 @@ void ento::registerUncheckedCallArgsChecker(CheckerManager &Mgr) {
 bool ento::shouldRegisterUncheckedCallArgsChecker(const CheckerManager &) {
   return true;
 }
+
+void ento::registerUnretainedCallArgsChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<UnretainedCallArgsChecker>();
+}
+
+bool ento::shouldRegisterUnretainedCallArgsChecker(const CheckerManager &) {
+  return true;
+}

clang/test/Analysis/Checkers/WebKit/unretained-call-args-arc.mm

@@ -0,0 +1,30 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UnretainedCallArgsChecker -fobjc-arc -verify %s
+
+#import "objc-mock-types.h"
+
+SomeObj *provide();
+CFMutableArrayRef provide_cf();
+void someFunction();
+
+namespace raw_ptr {
+
+void foo() {
+  [provide() doWork];
+  CFArrayAppendValue(provide_cf(), nullptr);
+  // expected-warning@-1{{Call argument for parameter 'theArray' is unretained and unsafe [alpha.webkit.UnretainedCallArgsChecker]}}
+}
+
+} // namespace raw_ptr
+
+@interface AnotherObj : NSObject
+- (void)foo:(SomeObj *)obj;
+@end
+
+@implementation AnotherObj
+- (void)foo:(SomeObj*)obj {
+  [obj doWork];
+  [provide() doWork];
+  CFArrayAppendValue(provide_cf(), nullptr);
+  // expected-warning@-1{{Call argument for parameter 'theArray' is unretained and unsafe [alpha.webkit.UnretainedCallArgsChecker]}}
+}
+@end