[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 ↗

(On Diff #171223)

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 ↗

(On Diff #171223)

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 ↗

(On Diff #171223)

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

*runs away and blames API*

clang/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,
+  class BehaviorSummary {
+    bool IsIdentity;
+    bool IsNoop;
+    bool IsNonZero;
+
+  public:
+    BehaviorSummary(bool IsIdentity,
+                    bool IsNoop,
+                    bool IsNonZero)
+        : IsIdentity(IsIdentity),
+          IsNoop(IsNoop),
+          IsNonZero(IsNonZero) {}
+
+    bool isIdentity() {
+      return IsIdentity;
+    }
+
+    bool isNonZero() {
+      return IsNonZero;
+    }
+
+    bool isNoop() {
+      return IsNoop;
+    }
+
+    // Function does not do anything.
+    static const BehaviorSummary NoOp;
+
+    // Function returns the first argument.
+    static const BehaviorSummary Identity;
+
+    // Function is an identity, and both input and output are
+    // guaranteed to be non zero.
+    static const BehaviorSummary IdentityNonZero;
+  };
+
+  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);
 

clang/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();
 
+  Optional<RetainSummaryManager::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->isIdentity()) {
     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);
+
+    if (BSmr->isNonZero()) {
+      if (auto L = RetVal.getAs<DefinedOrUnknownSVal>())
+        // Assume that both input and output has to be non-zero.
+        state = state->assume(*L, /*Assumption=*/true);
+    }
   }
 
   C.addTransition(state);
   return true;
 }
 
 ExplodedNode * RetainCountChecker::processReturn(const ReturnStmt *S,
                                                  CheckerContext &C) const {

clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

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);
+      if (isRetain(FD, FName) || isAutorelease(FD, FName) ||
+          isMakeCollectable(FName))
+        return BehaviorSummary::Identity;
 
     // 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;
+      return BehaviorSummary::IdentityNonZero;
     }
 
     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.
CallEffects CallEffects::getEffect(const FunctionDecl *FD) {
   const RetainSummary *S = M.getFunctionSummary(FD);
   CallEffects CE(S->getRetEffect());
   unsigned N = FD->param_size();
   for (unsigned i = 0; i < N; ++i) {
     CE.Args.push_back(S->getArg(i));
   }
   return CE;
 }
+
+const RetainSummaryManager::BehaviorSummary
+    RetainSummaryManager::BehaviorSummary::NoOp = {/*Identity=*/false,
+                                                   /*IsNoop=*/true,
+                                                   /*IsNonZero=*/false};
+
+const RetainSummaryManager::BehaviorSummary
+    RetainSummaryManager::BehaviorSummary::Identity = {/*Identity=*/true,
+                                                       /*IsNoop=*/false,
+                                                       /*IsNonZero=*/false};
+
+const RetainSummaryManager::BehaviorSummary
+    RetainSummaryManager::BehaviorSummary::IdentityNonZero = {
+        /*Identity=*/true,
+        /*IsNoop=*/false,
+        /*IsNonZero=*/true};
+

clang/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();
 
struct OSArray : public OSObject {
   static const OSMetaClass * const metaClass;
 };
 
 struct OtherStruct {
   static void doNothingToArray(OSArray *array);
 };
 
 struct OSMetaClassBase {
-  static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
+  static OS_RETURNS_NOT_RETAINED OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
 void check_no_invalidation() {
   OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to function 'withCapacity' returns an OSObject of type struct OSArray * with a +1 retain count}}
   OtherStruct::doNothingToArray(arr);
 } // expected-warning{{Potential leak of an object stored into 'arr'}}
   // expected-note@-1{{Object leaked}}
 

 }
 
 
 void check_dynamic_cast() {
   OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1));
   arr->release();
 }
 
+unsigned int check_dynamic_cast_no_null(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_check() {
   OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1));
   if (!arr)
     return;
   arr->release();
 }
 
 void use_after_release() {