Fix a Malloc Checker FP by tracking return values from initWithCharacter

and other functions.

When these functions return null, the pointer is not freed by
them/ownership is not transfered. So we should allow the user to free
the pointer by calling another function when the return value is NULL.

llvm-svn: 167813

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index b1cce85..887f387 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -107,7 +107,7 @@ class MallocChecker : public Checker<check::DeadSymbols,
                                      check::PreStmt<CallExpr>,
                                      check::PostStmt<CallExpr>,
                                      check::PostStmt<BlockExpr>,
-                                     check::PreObjCMessage,
+                                     check::PostObjCMessage,
                                      check::Location,
                                      check::Bind,
                                      eval::Assume,
@@ -135,7 +135,7 @@ public:
 
   void checkPreStmt(const CallExpr *S, CheckerContext &C) const;
   void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
-  void checkPreObjCMessage(const ObjCMethodCall &Call, CheckerContext &C) const;
+  void checkPostObjCMessage(const ObjCMethodCall &Call, CheckerContext &C) const;
   void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   void checkEndPath(CheckerContext &C) const;
@@ -193,12 +193,14 @@ private:
   ProgramStateRef FreeMemAux(CheckerContext &C, const CallExpr *CE,
                              ProgramStateRef state, unsigned Num,
                              bool Hold,
-                             bool &ReleasedAllocated) const;
+                             bool &ReleasedAllocated,
+                             bool ReturnsNullOnFailure = false) const;
   ProgramStateRef FreeMemAux(CheckerContext &C, const Expr *Arg,
                              const Expr *ParentExpr,
-                             ProgramStateRef state,
+                             ProgramStateRef State,
                              bool Hold,
-                             bool &ReleasedAllocated) const;
+                             bool &ReleasedAllocated,
+                             bool ReturnsNullOnFailure = false) const;
 
   ProgramStateRef ReallocMem(CheckerContext &C, const CallExpr *CE,
                              bool FreesMemOnFailure) const;
@@ -341,6 +343,10 @@ private:
 REGISTER_MAP_WITH_PROGRAMSTATE(RegionState, SymbolRef, RefState)
 REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair)
 
+// A map from the freed symbol to the symbol representing the return value of 
+// the free function.
+REGISTER_MAP_WITH_PROGRAMSTATE(FreeReturnValue, SymbolRef, SymbolRef)
+
 namespace {
 class StopTrackingCallback : public SymbolVisitor {
   ProgramStateRef state;
@@ -492,8 +498,8 @@ static bool isFreeWhenDoneSetToZero(const ObjCMethodCall &Call) {
   return false;
 }
 
-void MallocChecker::checkPreObjCMessage(const ObjCMethodCall &Call,
-                                        CheckerContext &C) const {
+void MallocChecker::checkPostObjCMessage(const ObjCMethodCall &Call,
+                                         CheckerContext &C) const {
   // If the first selector is dataWithBytesNoCopy, assume that the memory will
   // be released with 'free' by the new object.
   // Ex:  [NSData dataWithBytesNoCopy:bytes length:10];
@@ -506,9 +512,12 @@ void MallocChecker::checkPreObjCMessage(const ObjCMethodCall &Call,
        S.getNameForSlot(0) == "initWithCharactersNoCopy") &&
       !isFreeWhenDoneSetToZero(Call)){
     unsigned int argIdx  = 0;
-    C.addTransition(FreeMemAux(C, Call.getArgExpr(argIdx),
-                    Call.getOriginExpr(), C.getState(), true,
-                    ReleasedAllocatedMemory));
+    ProgramStateRef State = FreeMemAux(C, Call.getArgExpr(argIdx),
+                                       Call.getOriginExpr(), C.getState(), true,
+                                       ReleasedAllocatedMemory,
+                                       /* RetNullOnFailure*/ true);
+
+    C.addTransition(State);
   }
 }
 
@@ -609,21 +618,44 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
                                           ProgramStateRef state,
                                           unsigned Num,
                                           bool Hold,
-                                          bool &ReleasedAllocated) const {
+                                          bool &ReleasedAllocated,
+                                          bool ReturnsNullOnFailure) const {
   if (CE->getNumArgs() < (Num + 1))
     return 0;
 
-  return FreeMemAux(C, CE->getArg(Num), CE, state, Hold, ReleasedAllocated);
+  return FreeMemAux(C, CE->getArg(Num), CE, state, Hold,
+                    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);
+  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);
+  }
+  return std::pair<bool, ProgramStateRef>(false, State);
 }
 
 ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
                                           const Expr *ArgExpr,
                                           const Expr *ParentExpr,
-                                          ProgramStateRef state,
+                                          ProgramStateRef State,
                                           bool Hold,
-                                          bool &ReleasedAllocated) const {
+                                          bool &ReleasedAllocated,
+                                          bool ReturnsNullOnFailure) const {
 
-  SVal ArgVal = state->getSVal(ArgExpr, C.getLocationContext());
+  SVal ArgVal = State->getSVal(ArgExpr, C.getLocationContext());
   if (!isa<DefinedOrUnknownSVal>(ArgVal))
     return 0;
   DefinedOrUnknownSVal location = cast<DefinedOrUnknownSVal>(ArgVal);
@@ -634,7 +666,7 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
 
   // The explicit NULL case, no operation is performed.
   ProgramStateRef notNullState, nullState;
-  llvm::tie(notNullState, nullState) = state->assume(location);
+  llvm::tie(notNullState, nullState) = State->assume(location);
   if (nullState && !notNullState)
     return 0;
 
@@ -683,10 +715,17 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
     return 0;
 
   SymbolRef Sym = SR->getSymbol();
-  const RefState *RS = state->get<RegionState>(Sym);
+  const RefState *RS = State->get<RegionState>(Sym);
+  bool FailedToFree = false;
+  const SymbolRef *RetStatusSymbolPtr = 0;
+  llvm::tie(FailedToFree, State) =
+      checkAndCleanFreeFailedInfo(State, Sym, RetStatusSymbolPtr);
 
   // Check double free.
-  if (RS && (RS->isReleased() || RS->isRelinquished())) {
+  if (RS &&
+      (RS->isReleased() || RS->isRelinquished()) &&
+      !FailedToFree) {
+
     if (ExplodedNode *N = C.generateSink()) {
       if (!BT_DoubleFree)
         BT_DoubleFree.reset(
@@ -696,6 +735,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);
       R->addVisitor(new MallocBugVisitor(Sym));
       C.emitReport(R);
     }
@@ -704,10 +745,21 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
 
   ReleasedAllocated = (RS != 0);
 
+  // Keep track of the return value. If it is NULL, we will know that free 
+  // failed.
+  if (ReturnsNullOnFailure) {
+    SVal RetVal = C.getSVal(ParentExpr);
+    SymbolRef RetStatusSymbol = RetVal.getAsSymbol();
+    if (RetStatusSymbol) {
+      C.getSymbolManager().addSymbolDependency(Sym, RetStatusSymbol);
+      State = State->set<FreeReturnValue>(Sym, RetStatusSymbol);
+    }
+  }
+
   // Normal free.
   if (Hold)
-    return state->set<RegionState>(Sym, RefState::getRelinquished(ParentExpr));
-  return state->set<RegionState>(Sym, RefState::getReleased(ParentExpr));
+    return State->set<RegionState>(Sym, RefState::getRelinquished(ParentExpr));
+  return State->set<RegionState>(Sym, RefState::getReleased(ParentExpr));
 }
 
 bool MallocChecker::SummarizeValue(raw_ostream &os, SVal V) {
@@ -1064,6 +1116,15 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
     }
   }
 
+  // Cleanup the FreeReturnValue Map.
+  FreeReturnValueTy FR = state->get<FreeReturnValue>();
+  for (FreeReturnValueTy::iterator I = FR.begin(), E = FR.end(); I != E; ++I) {
+    if (SymReaper.isDead(I->first) ||
+        SymReaper.isDead(I->second)) {
+      state = state->remove<FreeReturnValue>(I->first);
+    }
+  }
+
   // Generate leak node.
   ExplodedNode *N = C.getPredecessor();
   if (!Errors.empty()) {

diff --git a/clang/test/Analysis/malloc.mm b/clang/test/Analysis/malloc.mm
index b5a1aeb..34b3c33 100644 (file)
--- a/clang/test/Analysis/malloc.mm
+++ b/clang/test/Analysis/malloc.mm
@@ -222,3 +222,50 @@ void noCrashOnVariableArgumentSelector() {
   NSMutableString *myString = [NSMutableString stringWithString:@"some text"];
   [myString appendFormat:@"some text = %d", 3];
 }
+
+void test12365078_check() {
+  unichar *characters = (unichar*)malloc(12);
+  NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+  if (!string) free(characters); // no-warning
+}
+
+void test12365078_nocheck() {
+  unichar *characters = (unichar*)malloc(12);
+  NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+}
+
+void test12365078_false_negative() {
+  unichar *characters = (unichar*)malloc(12);
+  NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+  if (!string) {;}
+}
+
+void test12365078_no_malloc(unichar *characters) {
+  NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+  if (!string) {free(characters);}
+}
+
+void test12365078_false_negative_no_malloc(unichar *characters) {
+  NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+  if (!string) {;}
+}
+
+void test12365078_nocheck_nomalloc(unichar *characters) {
+  NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+  free(characters); // expected-warning {{Attempt to free non-owned memory}}
+}
+
+void test12365078_nested(unichar *characters) {
+  NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+  if (!string) {    
+    NSString *string2 = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+    if (!string2) {    
+      NSString *string3 = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+      if (!string3) {    
+        NSString *string4 = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
+        if (!string4)
+          free(characters);
+      }
+    }
+  }
+}