1 files changed, 150 insertions, 38 deletions

diff --git a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
index 4934cda1ae..d955f4bdb5 100644
--- a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
@@ -27,7 +27,8 @@ namespace {
 class MacOSKeychainAPIChecker : public Checker<check::PreStmt<CallExpr>,
                                                check::PreStmt<ReturnStmt>,
                                                check::PostStmt<CallExpr>,
-                                               check::EndPath > {
+                                               check::EndPath,
+                                               check::DeadSymbols> {
   mutable llvm::OwningPtr<BugType> BT;
 
 public:
@@ -57,6 +58,7 @@ public:
   void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
   void checkPostStmt(const CallExpr *S, CheckerContext &C) const;
 
+  void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
   void checkEndPath(EndOfFunctionNodeBuilder &B, ExprEngine &Eng) const;
 
 private:
@@ -81,6 +83,24 @@ private:
     if (!BT)
       BT.reset(new BugType("Improper use of SecKeychain API", "Mac OS API"));
   }
+
+  RangedBugReport *generateAllocatedDataNotReleasedReport(
+                                                      const AllocationState &AS,
+                                                      ExplodedNode *N) const;
+
+  /// Check if RetSym evaluates to an error value in the current state.
+  bool definitelyReturnedError(SymbolRef RetSym,
+                               const GRState *State,
+                               SValBuilder &Builder,
+                               bool noError = false) const;
+
+  /// Check if RetSym evaluates to a NoErr value in the current state.
+  bool definitelyDidnotReturnError(SymbolRef RetSym,
+                                   const GRState *State,
+                                   SValBuilder &Builder) const {
+    return definitelyReturnedError(RetSym, State, Builder, true);
+  }
+
 };
 }
 
@@ -167,6 +187,28 @@ static SymbolRef getAsPointeeSymbol(const Expr *Expr,
   return 0;
 }
 
+// When checking for error code, we need to consider the following cases:
+// 1) noErr / [0]
+// 2) someErr / [1, inf]
+// 3) unknown
+// If noError, returns true iff (1).
+// If !noError, returns true iff (2).
+bool MacOSKeychainAPIChecker::definitelyReturnedError(SymbolRef RetSym,
+                                                      const GRState *State,
+                                                      SValBuilder &Builder,
+                                                      bool noError) const {
+  DefinedOrUnknownSVal NoErrVal = Builder.makeIntVal(NoErr,
+    Builder.getSymbolManager().getType(RetSym));
+  DefinedOrUnknownSVal NoErr = Builder.evalEQ(State, NoErrVal,
+                                                     nonloc::SymbolVal(RetSym));
+  const GRState *ErrState = State->assume(NoErr, noError);
+  if (ErrState == State) {
+    return true;
+  }
+
+  return false;
+}
+
 void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE,
                                            CheckerContext &C) const {
   const GRState *State = C.getState();
@@ -270,6 +312,20 @@ void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE,
   // The call is deallocating a value we previously allocated, so remove it
   // from the next state.
   State = State->remove<AllocatedData>(ArgSM);
+
+  // If the return status is undefined or is error, report a bad call to free.
+  if (!definitelyDidnotReturnError(AS->RetValue, State, C.getSValBuilder())) {
+    ExplodedNode *N = C.generateNode(State);
+    if (!N)
+      return;
+    initBugType();
+    RangedBugReport *Report = new RangedBugReport(*BT,
+        "Call to free data when error was returned during allocation.", N);
+    Report->addRange(ArgExpr->getSourceRange());
+    C.EmitReport(Report);
+    return;
+  }
+
   C.addTransition(State);
 }
 
@@ -301,31 +357,19 @@ void MacOSKeychainAPIChecker::checkPostStmt(const CallExpr *CE,
     //  - constant (null - should not be tracked,
     //              other constant will generate a compiler warning)
     //  - goto (should be reported by other checker)
-    
-    // We only need to track the value if the function returned noErr(0), so
-    // bind the return value of the function to 0 and proceed from the no error
-    // state.
-    SValBuilder &Builder = C.getSValBuilder();
-    SVal NoErrVal = Builder.makeIntVal(NoErr, CE->getCallReturnType());
-    const GRState *NoErr = State->BindExpr(CE, NoErrVal);
-    // Add the symbolic value V, which represents the location of the allocated
-    // data, to the set.
+
+    // The call return value symbol should stay alive for as long as the
+    // allocated value symbol, since our diagnostics depend on the value
+    // returned by the call. Ex: Data should only be freed if noErr was
+    // returned during allocation.)
     SymbolRef RetStatusSymbol = State->getSVal(CE).getAsSymbol();
-    NoErr = NoErr->set<AllocatedData>(V, AllocationState(ArgExpr, idx, 
-                                                         RetStatusSymbol));
+    C.getSymbolManager().addSymbolDependency(V, RetStatusSymbol);
 
-    assert(NoErr);
-    C.addTransition(NoErr);
-
-    // Generate a transition to explore the state space when there is an error.
-    // In this case, we do not track the allocated data.
-    SVal ReturnedError = Builder.evalBinOpNN(State, BO_NE,
-                                             cast<NonLoc>(NoErrVal),
-                                             cast<NonLoc>(State->getSVal(CE)),
-                                             CE->getCallReturnType());
-    const GRState *Err = State->assume(cast<NonLoc>(ReturnedError), true);
-    assert(Err);
-    C.addTransition(Err);
+    // Track the allocated value in the checker state.
+    State = State->set<AllocatedData>(V, AllocationState(ArgExpr, idx,
+                                                         RetStatusSymbol));
+    assert(State);
+    C.addTransition(State);
   }
 }
 
@@ -346,31 +390,99 @@ void MacOSKeychainAPIChecker::checkPreStmt(const ReturnStmt *S,
   C.addTransition(state);
 }
 
+// TODO: The report has to mention the expression which contains the
+// allocated content as well as the point at which it has been allocated.
+RangedBugReport *MacOSKeychainAPIChecker::
+  generateAllocatedDataNotReleasedReport(const AllocationState &AS,
+                                         ExplodedNode *N) const {
+  const ADFunctionInfo &FI = FunctionsToTrack[AS.AllocatorIdx];
+  initBugType();
+  std::string sbuf;
+  llvm::raw_string_ostream os(sbuf);
+  os << "Allocated data is not released: missing a call to '"
+      << FunctionsToTrack[FI.DeallocatorIdx].Name << "'.";
+  RangedBugReport *Report = new RangedBugReport(*BT, os.str(), N);
+  Report->addRange(AS.Address->getSourceRange());
+  return Report;
+}
+
+void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR,
+                                               CheckerContext &C) const {
+  const GRState *State = C.getState();
+  AllocatedSetTy ASet = State->get<AllocatedData>();
+  if (ASet.isEmpty())
+    return;
+
+  bool Changed = false;
+  llvm::SmallVector<const AllocationState*, 1> Errors;
+  for (AllocatedSetTy::iterator I = ASet.begin(), E = ASet.end(); I != E; ++I) {
+    if (SR.isLive(I->first))
+      continue;
+
+    Changed = true;
+    State = State->remove<AllocatedData>(I->first);
+    // If the allocated symbol is null or if the allocation call might have
+    // returned an error, do not report.
+    if (State->getSymVal(I->first) ||
+        definitelyReturnedError(I->second.RetValue, State, C.getSValBuilder()))
+      continue;
+    Errors.push_back(&I->second);
+  }
+  if (!Changed)
+    return;
+
+  // Generate the new, cleaned up state.
+  ExplodedNode *N = C.generateNode(State);
+  if (!N)
+    return;
+
+  // Generate the error reports.
+  for (llvm::SmallVector<const AllocationState*, 3>::iterator
+      I = Errors.begin(), E = Errors.end(); I != E; ++I) {
+    C.EmitReport(generateAllocatedDataNotReleasedReport(**I, N));
+  }
+}
+
+// TODO: Remove this after we ensure that checkDeadSymbols are always called.
 void MacOSKeychainAPIChecker::checkEndPath(EndOfFunctionNodeBuilder &B,
                                            ExprEngine &Eng) const {
   const GRState *state = B.getState();
   AllocatedSetTy AS = state->get<AllocatedData>();
-  ExplodedNode *N = B.generateNode(state);
-  if (!N)
+  if (AS.isEmpty())
     return;
-  initBugType();
 
   // Anything which has been allocated but not freed (nor escaped) will be
   // found here, so report it.
+  bool Changed = false;
+  llvm::SmallVector<const AllocationState*, 1> Errors;
   for (AllocatedSetTy::iterator I = AS.begin(), E = AS.end(); I != E; ++I ) {
-    const ADFunctionInfo &FI = FunctionsToTrack[I->second.AllocatorIdx];
+    Changed = true;
+    state = state->remove<AllocatedData>(I->first);
+    // If the allocated symbol is null or if error code was returned at
+    // allocation, do not report.
+    if (state->getSymVal(I.getKey()) ||
+        definitelyReturnedError(I->second.RetValue, state,
+                                Eng.getSValBuilder())) {
+      continue;
+    }
+    Errors.push_back(&I->second);
+  }
 
-    std::string sbuf;
-    llvm::raw_string_ostream os(sbuf);
-    os << "Allocated data is not released: missing a call to '"
-       << FunctionsToTrack[FI.DeallocatorIdx].Name << "'.";
-    RangedBugReport *Report = new RangedBugReport(*BT, os.str(), N);
-    // TODO: The report has to mention the expression which contains the
-    // allocated content as well as the point at which it has been allocated.
-    // Currently, the next line is useless.
-    Report->addRange(I->second.Address->getSourceRange());
-    Eng.getBugReporter().EmitReport(Report);
+  // If no change, do not generate a new state.
+  if (!Changed)
+    return;
+
+  ExplodedNode *N = B.generateNode(state);
+  if (!N)
+    return;
+
+  // Generate the error reports.
+  for (llvm::SmallVector<const AllocationState*, 3>::iterator
+      I = Errors.begin(), E = Errors.end(); I != E; ++I) {
+    Eng.getBugReporter().EmitReport(
+      generateAllocatedDataNotReleasedReport(**I, N));
   }
+
 }
 
 void ento::registerMacOSKeychainAPIChecker(CheckerManager &mgr) {