[analyzer] [RetainCountChecker] Track input parameters to the top-level function

[george.karpenkov]

Summary

Track them for ISL/OS objects by default, and for NS/CF under a flag.

rdar://47536377

[NoQ]

I just fail to understand why nobody did that before. As far as i understand, that's a very large improvement.

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

713

This looks out-of-place here.

clang/test/Analysis/osobject-retain-release.cpp

63

// no-note?

[george.karpenkov]

I just fail to understand why nobody did that before. As far as i understand, that's a very large improvement.

I think because it required D57346 and all the AnyCall machinery.

[NoQ]

In D57356#1374662, @george.karpenkov wrote:

I just fail to understand why nobody did that before. As far as i understand, that's a very large improvement.

I think because it required D57346 and all the AnyCall machinery.

Heh. Indeed. Then it sounds like yet another great example of why cleaner code matters.

clang/include/clang/Analysis/RetainSummaryManager.h

public:
   /// getReceiverEffect - Returns the effect on the receiver of the call.
   ///  This is only meaningful if the summary applies to an ObjCMessageExpr*.
   ArgEffect getReceiverEffect() const { return Receiver; }
 
   /// \return the effect on the "this" receiver of the method call.
   /// This is only meaningful if the summary applies to CXXMethodDecl*.
   ArgEffect getThisEffect() const { return This; }
 
+  ArgEffect getDefaultEffect() const { return DefaultArgEffect; }
+
   /// Set the effect of the method on "this".
   void setThisEffect(ArgEffect e) { This = e; }
 
   bool isNoop() const {
     return Ret == RetEffect::MakeNoRet() && Receiver.getKind() == DoNothing
       && DefaultArgEffect.getKind() == MayEscape && This.getKind() == DoNothing
       && Args.isEmpty();
   }
private:
   /// of '[super init]' in cases when it is not consumed by another expression
   /// as if the call preserves the value of 'self'; essentially, assuming it can
   /// never fail and return 'nil'.
   /// Note, we don't want to just stop tracking the value since we want the
   /// RetainCount checker to report leaks and use-after-free if SelfInit checker
   /// is turned off.
   void updateSummaryForReceiverUnconsumedSelf(const RetainSummary *&S);
 
+  /// Set argument types for arguments which are not doing anything.
+  void updateSummaryForArgumentTypes(const AnyCall &C, const RetainSummary *&RS);
+
   /// Determine whether a declaration {@code D} of correspondent type (return
   /// type for functions/methods) {@code QT} has any of the given attributes,
   /// provided they pass necessary validation checks AND tracking the given
   /// attribute is enabled.
   /// Returns the object kind corresponding to the present attribute, or None,
   /// if none of the specified attributes are present.
   /// Crashes if passed an attribute which is not explicitly handled.
   template <class T>

clang/lib/Analysis/RetainSummaryManager.cpp

 static bool isSubclass(const Decl *D,
                        StringRef ClassName) {
   using namespace ast_matchers;
   DeclarationMatcher SubclassM = cxxRecordDecl(isSameOrDerivedFrom(ClassName));
   return !(match(SubclassM, *D, D->getASTContext()).empty());
 }
 
 static bool isOSObjectSubclass(const Decl *D) {
-  return isSubclass(D, "OSMetaClassBase");
+  return D && isSubclass(D, "OSMetaClassBase");
 }
 
 static bool isOSObjectDynamicCast(StringRef S) {
   return S == "safeMetaCast";
 }
 
 static bool isOSObjectThisCast(StringRef S) {
   return S == "metaCast";
 }
 
+
+static bool isOSObjectPtr(QualType QT) {
+  return isOSObjectSubclass(QT->getPointeeCXXRecordDecl());
+}
+
+static bool isISLObjectRef(QualType Ty) {
+  return StringRef(Ty.getAsString()).startswith("isl_");
+}
+
 static bool isOSIteratorSubclass(const Decl *D) {
   return isSubclass(D, "OSIterator");
 }
 
 static bool hasRCAnnotation(const Decl *D, StringRef rcAnnotation) {
   for (const auto *Ann : D->specific_attrs<AnnotateAttr>()) {
     if (Ann->getAnnotation() == rcAnnotation)
       return true;
void RetainSummaryManager::updateSummaryForReceiverUnconsumedSelf(
     const RetainSummary *&S) {
 
   RetainSummaryTemplate Template(S, *this);
 
   Template->setReceiverEffect(ArgEffect(DoNothing));
   Template->setRetEffect(RetEffect::MakeNoRet());
 }
 
+
+void RetainSummaryManager::updateSummaryForArgumentTypes(
+  const AnyCall &C, const RetainSummary *&RS) {
+  RetainSummaryTemplate Template(RS, *this);
+
+  unsigned parm_idx = 0;
+  for (auto pi = C.param_begin(), pe = C.param_end(); pi != pe;
+       ++pi, ++parm_idx) {
+    QualType QT = (*pi)->getType();
+
+    // Skip already created values.
+    if (RS->getArgEffects().contains(parm_idx))
+      continue;
+
+    ObjKind K = ObjKind::AnyObj;
+
+    if (isISLObjectRef(QT)) {
+      K = ObjKind::Generalized;
+    } else if (isOSObjectPtr(QT)) {
+      K = ObjKind::OS;
+    } else if (cocoa::isCocoaObjectRef(QT)) {
+      K = ObjKind::ObjC;
+    } else if (coreFoundation::isCFObjectRef(QT)) {
+      K = ObjKind::CF;
+    }
+
+    if (K != ObjKind::AnyObj)
+      Template->addArg(AF, parm_idx,
+                       ArgEffect(RS->getDefaultArgEffect().getKind(), K));
+  }
+}
+
 const RetainSummary *
 RetainSummaryManager::getSummary(AnyCall C,
                                  bool HasNonZeroCallbackArg,
                                  bool IsReceiverUnconsumedSelf,
                                  QualType ReceiverType) {
   const RetainSummary *Summ;
   switch (C.getKind()) {
   case AnyCall::Function:
RetainSummaryManager::getSummary(AnyCall C,
   }
 
   if (HasNonZeroCallbackArg)
     Summ = updateSummaryForNonZeroCallbackArg(Summ, C);
 
   if (IsReceiverUnconsumedSelf)
     updateSummaryForReceiverUnconsumedSelf(Summ);
 
+  updateSummaryForArgumentTypes(C, Summ);
+
   assert(Summ && "Unknown call type?");
   return Summ;
 }
 
 
 const RetainSummary *
 RetainSummaryManager::getCFCreateGetRuleSummary(const FunctionDecl *FD) {
   if (coreFoundation::followsCreateRule(FD))

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h

 public:
 
   /// Track Objective-C and CoreFoundation objects.
   bool TrackObjCAndCFObjects = false;
 
   /// Track sublcasses of OSObject.
   bool TrackOSObjects = false;
 
+  /// Track initial parameters (for the entry point) for NS/CF objects.
+  bool TrackNSCFStartParam = false;
+
   RetainCountChecker() {};
 
   RetainSummaryManager &getSummaryManager(ASTContext &Ctx) const {
     if (!Summaries)
       Summaries.reset(
           new RetainSummaryManager(Ctx, TrackObjCAndCFObjects, TrackOSObjects));
     return *Summaries;
   }

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

       const RefCountBug &BT = Pred ? leakWithinFunction : leakAtReturn;
       Ctx.emitReport(llvm::make_unique<RefLeakReport>(BT, LOpts, N, L, Ctx));
     }
   }
 
   return N;
 }
 
-static bool isISLObjectRef(QualType Ty) {
-  return StringRef(Ty.getAsString()).startswith("isl_");
-}
-
 void RetainCountChecker::checkBeginFunction(CheckerContext &Ctx) const {
   if (!Ctx.inTopFrame())
     return;
 
   RetainSummaryManager &SmrMgr = getSummaryManager(Ctx);
   const LocationContext *LCtx = Ctx.getLocationContext();
   const FunctionDecl *FD = dyn_cast<FunctionDecl>(LCtx->getDecl());
 
   if (!FD || SmrMgr.isTrustedReferenceCountImplementation(FD))
     return;
 
   ProgramStateRef state = Ctx.getState();
   const RetainSummary *FunctionSummary = SmrMgr.getSummary(AnyCall(FD));
   ArgEffects CalleeSideArgEffects = FunctionSummary->getArgEffects();
 
   for (unsigned idx = 0, e = FD->getNumParams(); idx != e; ++idx) {
     const ParmVarDecl *Param = FD->getParamDecl(idx);
     SymbolRef Sym = state->getSVal(state->getRegion(Param, LCtx)).getAsSymbol();
 
     QualType Ty = Param->getType();
     const ArgEffect *AE = CalleeSideArgEffects.lookup(idx);
-    if (AE && AE->getKind() == DecRef && isISLObjectRef(Ty)) {
-      state = setRefBinding(
-          state, Sym, RefVal::makeOwned(ObjKind::Generalized, Ty));
-    } else if (isISLObjectRef(Ty)) {
-      state = setRefBinding(
-          state, Sym,
-          RefVal::makeNotOwned(ObjKind::Generalized, Ty));
+    if (AE) {
+      ObjKind K = AE->getObjKind();
+      if (K == ObjKind::Generalized || K == ObjKind::OS ||
+          (TrackNSCFStartParam && (K == ObjKind::ObjC || K == ObjKind::CF))) {
+        RefVal NewVal = AE->getKind() == DecRef ? RefVal::makeOwned(K, Ty)
+                                                : RefVal::makeNotOwned(K, Ty);
+        state = setRefBinding(state, Sym, NewVal);
+      }
     }
   }
 
   Ctx.addTransition(state);
 }
 
 void RetainCountChecker::checkEndFunction(const ReturnStmt *RS,
                                           CheckerContext &Ctx) const {

 
 //===----------------------------------------------------------------------===//
 // Checker registration.
 //===----------------------------------------------------------------------===//
 
 void ento::registerRetainCountChecker(CheckerManager &Mgr) {
   auto *Chk = Mgr.registerChecker<RetainCountChecker>();
   Chk->TrackObjCAndCFObjects = true;
+  Chk->TrackNSCFStartParam = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+      "TrackNSCFStartParam", false, Chk);
 }
 
 // FIXME: remove this, hack for backwards compatibility:
 // it should be possible to enable the NS/CF retain count checker as
 // osx.cocoa.RetainCount, and it should be possible to disable
 // osx.OSObjectRetainCount using osx.cocoa.RetainCount:CheckOSObject=false.
 static bool hasPrevCheckOSObjectOptionDisabled(AnalyzerOptions &Options) {
   auto I = Options.Config.find("osx.cocoa.RetainCount:CheckOSObject");

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

annotateConsumedSummaryMismatch(const ExplodedNode *N,
 
   if (os.str().empty())
     return nullptr;
 
   PathDiagnosticLocation L = PathDiagnosticLocation::create(CallExitLoc, SM);
   return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
 }
 
+/// Annotate the parameter at the analysis entry point.
+static std::shared_ptr<PathDiagnosticEventPiece>
+annotateStartParameter(const ExplodedNode *N, SymbolRef Sym,
+                       const SourceManager &SM) {
+  auto PP = N->getLocationAs<BlockEdge>();
+  if (!PP)
+    return nullptr;
+
+  const CFGBlock *Src = PP->getSrc();
+  const RefVal *CurrT = getRefBinding(N->getState(), Sym);
+
+  if (&Src->getParent()->getEntry() != Src || !CurrT ||
+      getRefBinding(N->getFirstPred()->getState(), Sym))
+    return nullptr;
+
+  const auto *VR = cast<VarRegion>(cast<SymbolRegionValue>(Sym)->getRegion());
+  const auto *PVD = cast<ParmVarDecl>(VR->getDecl());
+  PathDiagnosticLocation L = PathDiagnosticLocation(PVD, SM);
+
+  std::string s;
+  llvm::raw_string_ostream os(s);
+  os << "Parameter '" << PVD->getNameAsString()
+    << "' starts at +";
+  if (CurrT->getCount() == 1) {
+    os << "1, as it is marked as consuming";
+  } else {
+    assert(CurrT->getCount() == 0);
+    os << "0";
+  }
+  return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
+}
+
 std::shared_ptr<PathDiagnosticPiece>
 RefCountReportVisitor::VisitNode(const ExplodedNode *N,
                               BugReporterContext &BRC, BugReport &BR) {
 
   const auto &BT = static_cast<const RefCountBug&>(BR.getBugType());
   const auto *Checker =
       static_cast<const RetainCountChecker *>(BT.getChecker());
 
   bool IsFreeUnowned = BT.getBugType() == RefCountBug::FreeNotOwned ||
                        BT.getBugType() == RefCountBug::DeallocNotOwned;
 
   const SourceManager &SM = BRC.getSourceManager();
   CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager();
   if (auto CE = N->getLocationAs<CallExitBegin>())
     if (auto PD = annotateConsumedSummaryMismatch(N, *CE, SM, CEMgr))
       return PD;
 
+  if (auto PD = annotateStartParameter(N, Sym, SM))
+    return PD;
+
   // FIXME: We will eventually need to handle non-statement-based events
   // (__attribute__((cleanup))).
   if (!N->getLocation().getAs<StmtPoint>())
     return nullptr;
 
   // Check if the type state has changed.
   const ExplodedNode *PrevNode = N->getFirstPred();
   ProgramStateRef PrevSt = PrevNode->getState();
static AllocationInfo GetAllocationSite(ProgramStateManager &StateMgr,
   }
 
   // If allocation happened in a function different from the leak node context,
   // do not report the binding.
   assert(N && "Could not find allocation node");
 
   if (AllocationNodeInCurrentOrParentContext &&
       AllocationNodeInCurrentOrParentContext->getLocationContext() !=
-          LeakContext)
     FirstBinding = nullptr;
 
   return AllocationInfo(AllocationNodeInCurrentOrParentContext,
                         FirstBinding,
                         InterestingMethodContext);
 }
 
 std::shared_ptr<PathDiagnosticPiece>
 RefCountReportVisitor::getEndPath(BugReporterContext &BRC,
                                const ExplodedNode *EndN, BugReport &BR) {
if (D->hasAttr<CFReturnsNotRetainedAttr>()) {
           os << "whose name ('" << MD->getSelector().getAsString()
              << "') does not start with "
                 "'copy', 'mutableCopy', 'alloc' or 'new'."
                 "  This violates the naming convention rules"
                 " given in the Memory Management Guide for Cocoa";
         }
       } else {
         const FunctionDecl *FD = cast<FunctionDecl>(D);
+        ObjKind K = RV->getObjKind();
+        if (K == ObjKind::ObjC || K == ObjKind::CF) {
           os << "whose name ('" << *FD
-           << "') does not contain 'Copy' or 'Create'.  This violates the naming"
-              " convention rules given in the Memory Management Guide for Core"
+             << "') does not contain 'Copy' or 'Create'.  This violates the "
+                "naming"
+                " convention rules given in the Memory Management Guide for "
+                "Core"
                 " Foundation";
+        } else if (RV->getObjKind() == ObjKind::OS) {
+          std::string FuncName = FD->getNameAsString();
+          os << "whose name ('" << FuncName
+            << "') starts with '" << StringRef(FuncName).substr(0, 3) << "'";
+        }
       }
     }
   } else {
     os << " is not referenced later in this execution path and has a retain "
           "count of +" << RV->getCount();
   }
 
   return std::make_shared<PathDiagnosticEventPiece>(L, os.str());

clang/test/Analysis/osobject-retain-release.cpp

struct OSArray : public OSObject {
 
   static OSArray *generateArrayHasCode() {
     return new OSArray;
   }
 
   static OSArray *withCapacity(unsigned int capacity);
   static void consumeArray(OS_CONSUME OSArray * array);
 
-  static OSArray* consumeArrayHasCode(OS_CONSUME OSArray * array) {
-    return nullptr;
+  static OSArray* consumeArrayHasCode(OS_CONSUME OSArray * array) { // expected-note{{Parameter 'array' starts at +1, as it is marked as consuming}}
+    return nullptr; // expected-warning{{Potential leak of an object of type 'OSArray'}}
+// expected-note@-1{{Object leaked: allocated object of type 'OSArray' is not referenced later in this execution path and has a retain count of +1}}
   }
 
+
   static OS_RETURNS_NOT_RETAINED OSArray *MaskedGetter();
   static OS_RETURNS_RETAINED OSArray *getOoopsActuallyCreate();
 
   static const OSMetaClass * const metaClass;
 };
 
 struct MyArray : public OSArray {
   void consumeReference(OSArray *other) override;
 
   OSObject *identity() override;
 
   OSObject *generateObject(OSObject *input) override;
 };
 
 struct OtherStruct {
   static void doNothingToArray(OSArray *array);
   OtherStruct(OSArray *arr);
 };
 
 bool test_meta_cast_no_leak(OSMetaClassBase *arg) {
   return arg && arg->metaCast("blah") != nullptr;
 }
 
+static void consumedMismatch(OS_CONSUME OSObject *a,
+                             OSObject *b) { // expected-note{{Parameter 'b' starts at +0}}
+  a->release();
+  b->retain(); // expected-note{{Reference count incremented. The object now has a +1 retain count}}
+} // expected-warning{{Potential leak of an object of type 'OSObject'}}
+// expected-note@-1{{Object leaked: allocated object of type 'OSObject' is not referenced later in this execution path and has a retain count of +1}}
+
 void escape(void *);
 void escape_with_source(void *p) {}
 bool coin();
 
 typedef int kern_return_t;
 typedef kern_return_t IOReturn;
 typedef kern_return_t OSReturn;
 #define kOSReturnSuccess  0

 void test_smart_ptr_no_leak() {
   OSObject *obj = new OSObject;
   {
     OSObjectPtr p(obj);
   }
   obj->release();
 }
 
+OSObject *getRuleViolation() {
+  return new OSObject; // expected-warning{{Potential leak of an object of type 'OSObject'}}
+// expected-note@-1{{Operator 'new' returns an OSObject of type 'OSObject' with a +1 retain count}}
+// expected-note@-2{{Object leaked: allocated object of type 'OSObject' is returned from a function whose name ('getRuleViolation') starts with 'get'}}
+}
+
+OSObject *createRuleViolation(OSObject *param) { // expected-note{{Parameter 'param' starts at +0}}
+  return param; // expected-warning{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}}
+  // expected-note@-1{{Object with a +0 retain count returned to caller where a +1 (owning) retain count is expected}}
+}
+
 void test_ostypealloc_correct_diagnostic_name() {
   OSArray *arr = OSTypeAlloc(OSArray); // expected-note{{Call to method 'OSMetaClass::alloc' returns an OSObject of type 'OSArray' with a +1 retain count}}
   arr->retain(); // expected-note{{Reference count incremented. The object now has a +2 retain count}}
   arr->release(); // expected-note{{Reference count decremented. The object now has a +1 retain count}}
 } // expected-note{{Object leaked: object allocated and stored into 'arr' is not referenced later in this execution path and has a retain count of +1}}
   // expected-warning@-1{{Potential leak of an object stored into 'arr'}}
 
 void escape_elsewhere(OSObject *obj);
 
 void test_free_on_escaped_object_diagnostics() {
   OSObject *obj = new OSObject; // expected-note{{Operator 'new' returns an OSObject of type 'OSObject' with a +1 retain count}}
   escape_elsewhere(obj); // expected-note{{Object is now not exclusively owned}}
   obj->free(); // expected-note{{'free' called on an object that may be referenced elsewhere}}
   // expected-warning@-1{{'free' called on an object that may be referenced elsewhere}}
 }
-

clang/test/Analysis/retain-release.m

 // RUN: rm -f %t.objc.plist %t.objcpp.plist
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10\
 // RUN:     -analyzer-checker=core,osx.coreFoundation.CFRetainRelease\
 // RUN:     -analyzer-checker=osx.cocoa.ClassRelease,osx.cocoa.RetainCount\
 // RUN:     -analyzer-checker=debug.ExprInspection -fblocks -verify=expected,C %s\
 // RUN:     -Wno-objc-root-class -analyzer-output=plist -o %t.objc.plist
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10\
 // RUN:     -analyzer-checker=core,osx.coreFoundation.CFRetainRelease\
 // RUN:     -analyzer-checker=osx.cocoa.ClassRelease,osx.cocoa.RetainCount\
 // RUN:     -analyzer-checker=debug.ExprInspection -fblocks -verify %s\
 // RUN:     -Wno-objc-root-class -analyzer-output=plist -o %t.objcpp.plist\
 // RUN:     -x objective-c++ -std=gnu++98
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10\
+// RUN:     -analyzer-checker=core,osx.coreFoundation.CFRetainRelease\
+// RUN:     -analyzer-checker=osx.cocoa.ClassRelease,osx.cocoa.RetainCount\
+// RUN:     -analyzer-checker=debug.ExprInspection -fblocks -verify %s\
+// RUN:     -Wno-objc-root-class -x objective-c++ -std=gnu++98\
+// RUN:     -analyzer-config osx.cocoa.RetainCount:TrackNSCFStartParam=true\
+// RUN:     -DTRACK_START_PARAM
 // RUN: cat %t.objcpp.plist | %diff_plist %S/Inputs/expected-plists/retain-release.m.objcpp.plist -
 // RUN: cat %t.objc.plist | %diff_plist %S/Inputs/expected-plists/retain-release.m.objc.plist -
 
 void clang_analyzer_eval(int);
 
 #if __has_feature(attribute_ns_returns_retained)
 #define NS_RETURNS_RETAINED __attribute__((ns_returns_retained))
 #endif
case 3:
     break;
   default:
     break;
   }
 }
 
 // Test that an object is non-null after CFRetain/CFRelease/CFMakeCollectable/CFAutorelease.
 void f17(int x, CFTypeRef p) {
+#ifdef TRACK_START_PARAM
+  // expected-warning@-2{{Potential leak of an object of type 'CFTypeRef'}}
+#endif
   switch (x) {
   case 0:
     CFRelease(p);
+#ifdef TRACK_START_PARAM
+  // expected-warning@-2{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}
+#endif
     if (!p)
       CFRelease(0); // no-warning
     break;
   case 1:
     CFRetain(p);
     if (!p)
       CFRetain(0); // no-warning
     break;
   case 2:
     CFMakeCollectable(p);
     if (!p)
       CFMakeCollectable(0); // no-warning
     break;
   case 3:
     CFAutorelease(p);
     if (!p)
       CFAutorelease(0); // no-warning
     break;
   default:
     break;
   }
 }
+#ifdef TRACK_START_PARAM
+  // expected-warning@-2{{Object autoreleased too many times}}
+#endif
 
 __attribute__((annotate("rc_ownership_returns_retained"))) isl_basic_map *isl_basic_map_cow(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap);
 
 // Test custom diagnostics for generalized objects.
 void f18(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) {
   // After this call, 'bmap' has a +1 reference count.
   bmap = isl_basic_map_cow(bmap); // expected-warning {{Potential leak of an object}}
 }