[analyzer] For now model that OSDynamicCast never returns 0

[george.karpenkov]

Summary

Previously, OSDynamicCast was modeled as an identity.

This is not correct: the output of OSDynamicCast may be zero even if the input was not zero (if the class is not of desired type), and thus the modeling led to false positives.
This patch adds another overapproximation: assume that OSDynamicCast never returns zero.
What we probably really want is an implication x != 0 /\ a = OSDynamicCast(x, C) => a != 0,
but that is hard to assume with our interval solver (though could be done with a table lookup hack).
Modeling checking the class hierarchy is probably not practical.

This patch required a substantial refactoring of canEval infrastructure, as now it can return different function summaries, and not just true/false.

rdar://45497400

[NoQ]

Makes sense!

[george.karpenkov]

Or maybe it does not: we should still be able to catch cases where OSDynamicCast fails.

[NoQ]

On second thought, i actually think this should have been done as part of the dynamic type checker. This functionality doesn't have much to do with retain counts, but for more precise modeling some sort of reasoning about dynamic types behind pointers would be absolutely necessary.

I guess let's land this improvement, unless eager splits result in too many false alarms, and move to the right place later.

[NoQ]

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

813–814

If assume fails and yields a null state, you shouldn't transition into that state; it won't generate a sink but will instead transition into the original state (with the original program point but with checker tag).

[george.karpenkov]

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

813–814

That does not seem to be true: addTransition sets Tag to nullptr by default, and then addTransitionImpl has the following lines:

if (!State || (State == Pred->getState() && !Tag && !MarkAsSink))
      return Pred;

So if we create a transition to an empty state, that simply returns the original state, which caches out.

[NoQ]

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

813–814

Aha, aha, that's right, i was wrong and confused it with generateNonFatalErrorNode().

*runs away and blames API*

cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h

: Ctx(ctx),
      ObjCAllocRetE(usesARC ? RetEffect::MakeNotOwned(RetEffect::ObjC)
                                : RetEffect::MakeOwned(RetEffect::ObjC)),
      ObjCInitRetE(usesARC ? RetEffect::MakeNotOwned(RetEffect::ObjC)
                                : RetEffect::MakeOwnedWhenTrackedReceiver()) {
     InitializeClassMethodSummaries();
     InitializeMethodSummaries();
   }
 
-  bool canEval(const CallExpr *CE,
-               const FunctionDecl *FD,
+  enum class BehaviorSummary {
+    // Function does not return.
+    NoOp,
+
+    // Function returns the first argument.
+    Identity,
+
+    // Function either returns zero, or the input parameter.
+    IdentityOrZero
+  };
+
+  Optional<BehaviorSummary> canEval(const CallExpr *CE, const FunctionDecl *FD,
                                     bool &hasTrustedImplementationAnnotation);
 
   bool isTrustedReferenceCountImplementation(const FunctionDecl *FD);
 
   const RetainSummary *getSummary(const CallEvent &Call,
                                   QualType ReceiverType=QualType());
 
   const RetainSummary *getFunctionSummary(const FunctionDecl *FD);
 

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

bool RetainCountChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
   QualType ResultTy = CE->getCallReturnType(C.getASTContext());
 
   // See if the function has 'rc_ownership_trusted_implementation'
   // annotate attribute. If it does, we will not inline it.
   bool hasTrustedImplementationAnnotation = false;
 
   const LocationContext *LCtx = C.getLocationContext();
 
+  using BehaviorSummary = RetainSummaryManager::BehaviorSummary;
+  Optional<BehaviorSummary> BSmr =
+      SmrMgr.canEval(CE, FD, hasTrustedImplementationAnnotation);
+
   // See if it's one of the specific functions we know how to eval.
-  if (!SmrMgr.canEval(CE, FD, hasTrustedImplementationAnnotation))
+  if (!BSmr)
     return false;
 
   // Bind the return value.
-  // For now, all the functions which we can evaluate and which take
-  // at least one argument are identities.
-  if (CE->getNumArgs() >= 1) {
+  if (BSmr == BehaviorSummary::Identity ||
+      BSmr == BehaviorSummary::IdentityOrZero) {
     SVal RetVal = state->getSVal(CE->getArg(0), LCtx);
 
     // If the receiver is unknown or the function has
     // 'rc_ownership_trusted_implementation' annotate attribute, conjure a
     // return value.
     if (RetVal.isUnknown() ||
         (hasTrustedImplementationAnnotation && !ResultTy.isNull())) {
       SValBuilder &SVB = C.getSValBuilder();
       RetVal =
           SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy, C.blockCount());
     }
-    state = state->BindExpr(CE, LCtx, RetVal, false);
+    state = state->BindExpr(CE, LCtx, RetVal, /*Invalidate=*/false);
+
+    if (BSmr == BehaviorSummary::IdentityOrZero) {
+      // Add a branch where the output is zero.
+      ProgramStateRef NullOutputState = C.getState();
+
+      // Assume that output is zero on the other branch.
+      NullOutputState = NullOutputState->BindExpr(
+          CE, LCtx, C.getSValBuilder().makeNull(), /*Invalidate=*/false);
+
+      C.addTransition(NullOutputState);
+
+      // And on the original branch assume that both input and
+      // output are non-zero.
+      if (auto L = RetVal.getAs<DefinedOrUnknownSVal>())
+        state = state->assume(*L, /*Assumption=*/true);
+
+    }
   }
 
   C.addTransition(state);
   return true;
 }
 
 ExplodedNode * RetainCountChecker::processReturn(const ReturnStmt *S,
                                                  CheckerContext &C) const {

cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

static bool isSubclass(const Decl *D,
   DeclarationMatcher SubclassM = cxxRecordDecl(isSameOrDerivedFrom(ClassName));
   return !(match(SubclassM, *D, D->getASTContext()).empty());
 }
 
 static bool isOSObjectSubclass(const Decl *D) {
   return isSubclass(D, "OSObject");
 }
 
+static bool isOSObjectDynamicCast(StringRef S) {
+  return S == "safeMetaCast";
+}
+
 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;
RetainSummaryManager::generateSummary(const FunctionDecl *FD,
   }
 
   if (RetTy->isPointerType()) {
 
     const CXXRecordDecl *PD = RetTy->getPointeeType()->getAsCXXRecordDecl();
     if (TrackOSObjects && PD && isOSObjectSubclass(PD)) {
       if (const IdentifierInfo *II = FD->getIdentifier()) {
 
+        if (isOSObjectDynamicCast(II->getName()))
+          return getDefaultSummary();
+
         // All objects returned with functions starting with "get" are getters.
         if (II->getName().startswith("get")) {
 
           // ...except for iterators.
           if (isOSIteratorSubclass(PD))
             return getOSSummaryCreateRule(FD);
           return getOSSummaryGetRule(FD);
         } else {
RetainSummaryManager::getCFCreateGetRuleSummary(const FunctionDecl *FD) {
   return getCFSummaryGetRule(FD);
 }
 
 bool RetainSummaryManager::isTrustedReferenceCountImplementation(
     const FunctionDecl *FD) {
   return hasRCAnnotation(FD, "rc_ownership_trusted_implementation");
 }
 
-bool RetainSummaryManager::canEval(const CallExpr *CE,
-                                   const FunctionDecl *FD,
-                                   bool &hasTrustedImplementationAnnotation) {
+Optional<RetainSummaryManager::BehaviorSummary>
+RetainSummaryManager::canEval(const CallExpr *CE, const FunctionDecl *FD,
 
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
-    return false;
+    return None;
 
   StringRef FName = II->getName();
   FName = FName.substr(FName.find_first_not_of('_'));
 
   QualType ResultTy = CE->getCallReturnType(Ctx);
   if (ResultTy->isObjCIdType()) {
-    return II->isStr("NSMakeCollectable");
+    if (II->isStr("NSMakeCollectable"))
+      return BehaviorSummary::Identity;
   } else if (ResultTy->isPointerType()) {
     // Handle: (CF|CG|CV)Retain
     //         CFAutorelease
     // It's okay to be a little sloppy here.
     if (cocoa::isRefType(ResultTy, "CF", FName) ||
         cocoa::isRefType(ResultTy, "CG", FName) ||
         cocoa::isRefType(ResultTy, "CV", FName))
-      return isRetain(FD, FName) || isAutorelease(FD, FName) ||
-             isMakeCollectable(FName);
-
-    // Process OSDynamicCast: should just return the first argument.
-    // For now, treating the cast as a no-op, and disregarding the case where
-    // the output becomes null due to the type mismatch.
-    if (TrackOSObjects && FName == "safeMetaCast") {
-      return true;
+      if (isRetain(FD, FName) || isAutorelease(FD, FName) ||
+          isMakeCollectable(FName))
+        return BehaviorSummary::Identity;
+
+    // safeMetaCast is called by OSDynamicCast.
+    // We assume that OSDynamicCast is either an identity (cast is OK,
+    // the input was non-zero),
+    // or that it returns zero (when the cast failed, or the input
+    // was zero).
+    if (TrackOSObjects && isOSObjectDynamicCast(FName)) {
+      return BehaviorSummary::IdentityOrZero;
     }
 
     const FunctionDecl* FDD = FD->getDefinition();
     if (FDD && isTrustedReferenceCountImplementation(FDD)) {
       hasTrustedImplementationAnnotation = true;
-      return true;
+      return BehaviorSummary::Identity;
     }
   }
 
   if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
     const CXXRecordDecl *Parent = MD->getParent();
     if (TrackOSObjects && Parent && isOSObjectSubclass(Parent))
-      return FName == "release" || FName == "retain";
+      if (FName == "release" || FName == "retain")
+        return BehaviorSummary::NoOp;
   }
 
-  return false;
-
+  return None;
 }
 
 const RetainSummary *
 RetainSummaryManager::getUnarySummary(const FunctionType* FT,
                                       UnaryFuncKind func) {
 
   // Sanity check that this is *really* a unary function.  This can
   // happen if people do weird things.

cfe/trunk/test/Analysis/osobject-retain-release.cpp

 #define OSDynamicCast(type, inst)   \
     ((type *) OSMetaClassBase::safeMetaCast((inst), OSTypeID(type)))
 
 struct OSObject {
   virtual void retain();
   virtual void release() {};
   virtual ~OSObject(){}
 
+  unsigned int foo() { return 42; }
+
   static OSObject *generateObject(int);
 
   static const OSMetaClass * const metaClass;
 };
 
 struct OSArray : public OSObject {
   unsigned int getCount();
 

 }
 
 
 void check_dynamic_cast() {
   OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1));
   arr->release();
 }
 
+unsigned int check_dynamic_cast_no_null_on_orig(OSObject *obj) {
+  OSArray *arr = OSDynamicCast(OSArray, obj);
+  if (arr) {
+    return arr->getCount();
+  } else {
+
+    // The fact that dynamic cast has failed should not imply that
+    // the input object was null.
+    return obj->foo(); // no-warning
+  }
+}
+
+void check_dynamic_cast_null_branch(OSObject *obj) {
+  OSArray *arr1 = OSArray::withCapacity(10); // expected-note{{Call to function 'withCapacity' returns an OSObject}}
+  OSArray *arr = OSDynamicCast(OSArray, obj);
+  if (!arr) // expected-note{{Taking true branch}}
+    return; // expected-warning{{Potential leak}}
+            // expected-note@-1{{Object leaked}}
+  arr1->release();
+}
+
 void check_dynamic_cast_null_check() {
-  OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1));
+  OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1)); // expected-note{{Call to function 'generateObject' returns an OSObject}}
+    // expected-warning@-1{{Potential leak of an object}}
+    // expected-note@-2{{Object leaked}}
   if (!arr)
     return;
   arr->release();
 }
 
 void use_after_release() {
   OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to function 'withCapacity' returns an OSObject of type struct OSArray * with a +1 retain count}}
   arr->release(); // expected-note{{Object released}}