[analyzer] Address Jordan's code review for r167813.

This simplifies logic, fixes a bug, and adds a test case.
Thanks Jordan!

llvm-svn: 167868

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 887f387..caf70ca 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -627,24 +627,19 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
                     ReleasedAllocated, ReturnsNullOnFailure);
 }
 
-/// Check if the previous call to free on the given symbol failed.
-///
-/// For example, if free failed, returns true. In addition, cleans out the 
-/// state from the corresponding failure info. Retuns the cleaned out state 
-/// and the corresponding return value symbol.
-static std::pair<bool, ProgramStateRef>
-checkAndCleanFreeFailedInfo(ProgramStateRef State,
-                            SymbolRef Sym, const SymbolRef *Ret) {
-  Ret = State->get<FreeReturnValue>(Sym);
+/// Checks if the previous call to free on the given symbol failed - if free
+/// failed, returns true. Also, returns the corresponding return value symbol.
+bool didPreviousFreeFail(ProgramStateRef State,
+                         SymbolRef Sym, SymbolRef &RetStatusSymbol) {
+  const SymbolRef *Ret = State->get<FreeReturnValue>(Sym);
   if (Ret) {
     assert(*Ret && "We should not store the null return symbol");
     ConstraintManager &CMgr = State->getConstraintManager();
     ConditionTruthVal FreeFailed = CMgr.isNull(State, *Ret);
-    State = State->remove<FreeReturnValue>(Sym);
-    return std::pair<bool, ProgramStateRef>(FreeFailed.isConstrainedTrue(),
-                                            State);
+    RetStatusSymbol = *Ret;
+    return FreeFailed.isConstrainedTrue();
   }
-  return std::pair<bool, ProgramStateRef>(false, State);
+  return false;
 }
 
 ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
@@ -716,15 +711,12 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
 
   SymbolRef Sym = SR->getSymbol();
   const RefState *RS = State->get<RegionState>(Sym);
-  bool FailedToFree = false;
-  const SymbolRef *RetStatusSymbolPtr = 0;
-  llvm::tie(FailedToFree, State) =
-      checkAndCleanFreeFailedInfo(State, Sym, RetStatusSymbolPtr);
+  SymbolRef PreviousRetStatusSymbol = 0;
 
   // Check double free.
   if (RS &&
       (RS->isReleased() || RS->isRelinquished()) &&
-      !FailedToFree) {
+      !didPreviousFreeFail(State, Sym, PreviousRetStatusSymbol)) {
 
     if (ExplodedNode *N = C.generateSink()) {
       if (!BT_DoubleFree)
@@ -735,8 +727,8 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
                             "Attempt to free non-owned memory"), N);
       R->addRange(ArgExpr->getSourceRange());
       R->markInteresting(Sym);
-      if (RetStatusSymbolPtr)
-        R->markInteresting(*RetStatusSymbolPtr);
+      if (PreviousRetStatusSymbol)
+        R->markInteresting(PreviousRetStatusSymbol);
       R->addVisitor(new MallocBugVisitor(Sym));
       C.emitReport(R);
     }
@@ -745,6 +737,9 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
 
   ReleasedAllocated = (RS != 0);
 
+  // Clean out the info on previous call to free return info.
+  State = State->remove<FreeReturnValue>(Sym);
+
   // Keep track of the return value. If it is NULL, we will know that free 
   // failed.
   if (ReturnsNullOnFailure) {

diff --git a/clang/test/Analysis/malloc.mm b/clang/test/Analysis/malloc.mm
index 99d1de8..c92c966 100644 (file)
--- a/clang/test/Analysis/malloc.mm
+++ b/clang/test/Analysis/malloc.mm
@@ -272,3 +272,9 @@ void test12365078_nested(unichar *characters) {
     }
   }
 }
+
+void test12365078_check_positive() {
+  unichar *characters = (unichar*)malloc(12);
+  NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+  if (string) free(characters); // expected-warning{{Attempt to free non-owned memory}}
+}