From: Ted Kremenek <kremenek@apple.com>
Date: Thu, 10 Apr 2008 23:44:06 +0000 (+0000)
Subject: Fixed some logic errors in the CF ref count checker; we now can detect simple
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=940b1d85d3f786c90e845429a60ffd0be736984e;p=clang

Fixed some logic errors in the CF ref count checker; we now can detect simple
use-after-release errors.  Added test case.


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@49509 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/Analysis/CFRefCount.cpp b/lib/Analysis/CFRefCount.cpp
index d9de2cf2b5..575eb4c291 100644
--- a/lib/Analysis/CFRefCount.cpp
+++ b/lib/Analysis/CFRefCount.cpp
@@ -48,7 +48,8 @@ namespace {
   
 class RetEffect {
 public:
-  enum Kind { Alias = 0x0, OwnedSymbol = 0x1, NotOwnedSymbol = 0x2 };
+  enum Kind { NoRet = 0x0, Alias = 0x1, OwnedSymbol = 0x2,
+              NotOwnedSymbol = 0x3 };
 
 private:
   unsigned Data;
@@ -69,6 +70,8 @@ public:
   
   static RetEffect MakeNotOwned() { return RetEffect(NotOwnedSymbol, 0); }
   
+  static RetEffect MakeNoRet() { return RetEffect(NoRet, 0); }
+  
   operator Kind() const { return getKind(); }
   
   void Profile(llvm::FoldingSetNodeID& ID) const { ID.AddInteger(Data); }
@@ -133,6 +136,10 @@ class CFRefSummaryManager {
   
   CFRefSummary* getPersistentSummary(ArgEffects* AE, RetEffect RE);
   
+  CFRefSummary* getDoNothingSummary(unsigned Args);
+  void FillDoNothing(unsigned Args);
+
+  
 public:
   CFRefSummaryManager(ASTContext& ctx) : Ctx(ctx) {}
   ~CFRefSummaryManager();
@@ -158,8 +165,6 @@ CFRefSummaryManager::~CFRefSummaryManager() {
 
 ArgEffects* CFRefSummaryManager::getArgEffects() {
 
-  assert (!ScratchArgs.empty());
-  
   llvm::FoldingSetNodeID profile;
   profile.Add(ScratchArgs);
   void* InsertPos;
@@ -314,20 +319,22 @@ CFRefSummary* CFRefSummaryManager::getCannedCFSummary(FunctionTypeProto* FT,
     // CFRetain: the return type should also be "CFTypeRef".
     if (RetTy.getTypePtr() != ArgT)
       return NULL;
+    
+    // The function's interface checks out.  Generate a canned summary.    
+    assert (ScratchArgs.empty());
+    ScratchArgs.push_back(IncRef);
+    return getPersistentSummary(getArgEffects(), RetEffect::MakeAlias(0));
   }
   else {
     // CFRelease: the return type should be void.
     
     if (RetTy != Ctx.VoidTy)
       return NULL;
-  }
     
-  // The function's interface checks out.  Generate a canned summary.
-  
-  assert (ScratchArgs.empty());
-  ScratchArgs.push_back(isRetain ? IncRef : DecRef);
-  
-  return getPersistentSummary(getArgEffects(), RetEffect::MakeAlias(0));
+    assert (ScratchArgs.empty());
+    ScratchArgs.push_back(DecRef);
+    return getPersistentSummary(getArgEffects(), RetEffect::MakeNoRet());
+  }
 }
 
 static bool isCFRefType(QualType T) {
@@ -354,21 +361,28 @@ static bool isCFRefType(QualType T) {
   return true;
 }
   
+void CFRefSummaryManager::FillDoNothing(unsigned Args) {
+  for (unsigned i = 0; i != Args; ++i)
+    ScratchArgs.push_back(DoNothing);
+}
+
+CFRefSummary* CFRefSummaryManager::getDoNothingSummary(unsigned Args) {
+  FillDoNothing(Args);
+  return getPersistentSummary(getArgEffects(), RetEffect::MakeNoRet());  
+}
 
 CFRefSummary*
 CFRefSummaryManager::getCFSummaryCreateRule(FunctionTypeProto* FT) {
  
   if (!isCFRefType(FT->getResultType()))
-    return NULL;
+    return getDoNothingSummary(FT->getNumArgs());
 
   assert (ScratchArgs.empty());
   
   // FIXME: Add special-cases for functions that retain/release.  For now
   //  just handle the default case.
   
-  for (unsigned i = 0, n = FT->getNumArgs(); i != n; ++i)
-    ScratchArgs.push_back(DoNothing);
-  
+  FillDoNothing(FT->getNumArgs());  
   return getPersistentSummary(getArgEffects(), RetEffect::MakeOwned());
 }
 
@@ -376,16 +390,14 @@ CFRefSummary*
 CFRefSummaryManager::getCFSummaryGetRule(FunctionTypeProto* FT) {
   
   if (!isCFRefType(FT->getResultType()))
-    return NULL;
+    return getDoNothingSummary(FT->getNumArgs());
   
   assert (ScratchArgs.empty());
   
   // FIXME: Add special-cases for functions that retain/release.  For now
   //  just handle the default case.
   
-  for (unsigned i = 0, n = FT->getNumArgs(); i != n; ++i)
-    ScratchArgs.push_back(DoNothing);
-  
+  FillDoNothing(FT->getNumArgs());  
   return getPersistentSummary(getArgEffects(), RetEffect::MakeNotOwned());
 }
 
@@ -711,11 +723,11 @@ void CFRefCount::EvalCall(ExplodedNodeSet<ValueState>& Dst,
     
   if (hasError) {
     St = StateMgr.getPersistentState(StVals);
-    GRExprEngine::NodeTy* N = Builder.generateNode(CE, St, Pred);
+    
+    Builder.BuildSinks = true;
+    GRExprEngine::NodeTy* N  = Builder.MakeNode(Dst, CE, Pred, St);
 
     if (N) {
-      N->markAsSink();
-      
       switch (hasError) {
         default: assert(false);
         case RefVal::ErrorUseAfterRelease:
@@ -741,6 +753,9 @@ void CFRefCount::EvalCall(ExplodedNodeSet<ValueState>& Dst,
     default:
       assert (false && "Unhandled RetEffect."); break;
     
+    case RetEffect::NoRet:
+      break;
+      
     case RetEffect::Alias: {
       unsigned idx = RE.getValue();
       assert (idx < CE->getNumArgs());
@@ -810,7 +825,8 @@ CFRefCount::RefBindings CFRefCount::Update(RefBindings B, SymbolID sym,
           assert(false);
 
         case RefVal::Owned:
-          V = RefVal::makeOwned(V.getCount()+1); break;
+          V = RefVal::makeOwned(V.getCount()+1);
+          break;
                     
         case RefVal::NotOwned:
           V = RefVal::makeNotOwned(V.getCount()+1);
@@ -822,6 +838,8 @@ CFRefCount::RefBindings CFRefCount::Update(RefBindings B, SymbolID sym,
           break;
       }
       
+      break;
+      
     case DecRef:
       switch (V.getKind()) {
         default:
@@ -851,6 +869,8 @@ CFRefCount::RefBindings CFRefCount::Update(RefBindings B, SymbolID sym,
           hasError = V.getKind();
           break;          
       }
+      
+      break;
   }
 
   return RefBFactory.Add(B, sym, V);
diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp
index 6d4217774d..3211da2918 100644
--- a/lib/Analysis/GRExprEngine.cpp
+++ b/lib/Analysis/GRExprEngine.cpp
@@ -678,6 +678,8 @@ void GRExprEngine::VisitCall(CallExpr* CE, NodeTy* Pred,
       
       unsigned size = Dst.size();
       
+      SaveAndRestore<bool> OldSink(Builder->BuildSinks);
+      
       EvalCall(Dst, CE, cast<LVal>(L), *DI);
       
       if (!Builder->BuildSinks && Dst.size() == size)
diff --git a/test/Analysis-Apple/CFDate.c b/test/Analysis-Apple/CFDate.c
new file mode 100644
index 0000000000..ea15994e4d
--- /dev/null
+++ b/test/Analysis-Apple/CFDate.c
@@ -0,0 +1,15 @@
+// RUN: clang -checker-cfref -verify %s
+
+#include <CoreFoundation/CFDate.h>
+
+CFAbsoluteTime f1() {
+  CFAbsoluteTime t = CFAbsoluteTimeGetCurrent();
+  CFDateRef date = CFDateCreate(NULL, t);
+  CFRetain(date);
+  CFRelease(date);
+  t = CFDateGetAbsoluteTime(date);
+  CFRelease(date);
+  t = CFDateGetAbsoluteTime(date);   // expected-warning{{Reference-counted object is used after it is released.}}
+  return t;
+}
+