[analyzer] Fix realloc related bug in the malloc checker.

When reallocation of a non-allocated (not owned) symbol fails do not
expect it to be freed.

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

Author: AnnaZaks

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -70,9 +70,16 @@ class RefState {
   }
 };
 
+/// \class ReallocPair
+/// \brief Stores information about the symbol being reallocated by a call to
+/// 'realloc' to allow modeling failed reallocation later in the path.
 struct ReallocPair {
+  // \brief The symbol which realloc reallocated.
   SymbolRef ReallocatedSym;
+  // \brief The flag is true if the symbol does not need to be freed after
+  // reallocation fails.
   bool IsFreeOnFailure;
+
   ReallocPair(SymbolRef S, bool F) : ReallocatedSym(S), IsFreeOnFailure(F) {}
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddInteger(IsFreeOnFailure);
@@ -177,11 +184,13 @@ class MallocChecker : public Checker<check::DeadSymbols,
                               const OwnershipAttr* Att) const;
   ProgramStateRef FreeMemAux(CheckerContext &C, const CallExpr *CE,
                              ProgramStateRef state, unsigned Num,
-                             bool Hold) const;
+                             bool Hold,
+                             bool &ReleasedAllocated) const;
   ProgramStateRef FreeMemAux(CheckerContext &C, const Expr *Arg,
                              const Expr *ParentExpr,
                              ProgramStateRef state,
-                             bool Hold) const;
+                             bool Hold,
+                             bool &ReleasedAllocated) const;
 
   ProgramStateRef ReallocMem(CheckerContext &C, const CallExpr *CE,
                              bool FreesMemOnFailure) const;
@@ -431,6 +440,7 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const {
     return;
 
   ProgramStateRef State = C.getState();
+  bool ReleasedAllocatedMemory = false;
 
   if (FD->getKind() == Decl::Function) {
     initIdentifierInfo(C.getASTContext());
@@ -447,7 +457,7 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const {
     } else if (FunI == II_calloc) {
       State = CallocMem(C, CE);
     } else if (FunI == II_free) {
-      State = FreeMemAux(C, CE, State, 0, false);
+      State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
     } else if (FunI == II_strdup) {
       State = MallocUpdateRefState(C, CE, State);
     } else if (FunI == II_strndup) {
@@ -494,14 +504,16 @@ void MallocChecker::checkPreObjCMessage(const ObjCMethodCall &Call,
   // Ex:  [NSData dataWithBytesNoCopy:bytes length:10];
   // Unless 'freeWhenDone' param set to 0.
   // TODO: Check that the memory was allocated with malloc.
+  bool ReleasedAllocatedMemory = false;
   Selector S = Call.getSelector();
   if ((S.getNameForSlot(0) == "dataWithBytesNoCopy" ||
        S.getNameForSlot(0) == "initWithBytesNoCopy" ||
        S.getNameForSlot(0) == "initWithCharactersNoCopy") &&
       !isFreeWhenDoneSetToZero(Call)){
     unsigned int argIdx  = 0;
     C.addTransition(FreeMemAux(C, Call.getArgExpr(argIdx),
-                    Call.getOriginExpr(), C.getState(), true));
+                    Call.getOriginExpr(), C.getState(), true,
+                    ReleasedAllocatedMemory));
   }
 }
 
@@ -584,11 +596,13 @@ ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C,
     return 0;
 
   ProgramStateRef State = C.getState();
+  bool ReleasedAllocated = false;
 
   for (OwnershipAttr::args_iterator I = Att->args_begin(), E = Att->args_end();
        I != E; ++I) {
     ProgramStateRef StateI = FreeMemAux(C, CE, State, *I,
-                               Att->getOwnKind() == OwnershipAttr::Holds);
+                               Att->getOwnKind() == OwnershipAttr::Holds,
+                               ReleasedAllocated);
     if (StateI)
       State = StateI;
   }
@@ -599,18 +613,20 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
                                           const CallExpr *CE,
                                           ProgramStateRef state,
                                           unsigned Num,
-                                          bool Hold) const {
+                                          bool Hold,
+                                          bool &ReleasedAllocated) const {
   if (CE->getNumArgs() < (Num + 1))
     return 0;
 
-  return FreeMemAux(C, CE->getArg(Num), CE, state, Hold);
+  return FreeMemAux(C, CE->getArg(Num), CE, state, Hold, ReleasedAllocated);
 }
 
 ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
                                           const Expr *ArgExpr,
                                           const Expr *ParentExpr,
                                           ProgramStateRef state,
-                                          bool Hold) const {
+                                          bool Hold,
+                                          bool &ReleasedAllocated) const {
 
   SVal ArgVal = state->getSVal(ArgExpr, C.getLocationContext());
   if (!isa<DefinedOrUnknownSVal>(ArgVal))
@@ -691,6 +707,8 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
     return 0;
   }
 
+  ReleasedAllocated = (RS != 0);
+
   // Normal free.
   if (Hold)
     return state->set<RegionState>(Sym, RefState::getRelinquished(ParentExpr));
@@ -886,9 +904,12 @@ ProgramStateRef MallocChecker::ReallocMem(CheckerContext &C,
   if (!FromPtr || !ToPtr)
     return 0;
 
+  bool ReleasedAllocated = false;
+
   // If the size is 0, free the memory.
   if (SizeIsZero)
-    if (ProgramStateRef stateFree = FreeMemAux(C, CE, StateSizeIsZero,0,false)){
+    if (ProgramStateRef stateFree = FreeMemAux(C, CE, StateSizeIsZero, 0,
+                                               false, ReleasedAllocated)){
       // The semantics of the return value are:
       // If size was equal to 0, either NULL or a pointer suitable to be passed
       // to free() is returned. We just free the input pointer and do not add
@@ -897,14 +918,19 @@ ProgramStateRef MallocChecker::ReallocMem(CheckerContext &C,
     }
 
   // Default behavior.
-  if (ProgramStateRef stateFree = FreeMemAux(C, CE, state, 0, false)) {
-    // FIXME: We should copy the content of the original buffer.
+  if (ProgramStateRef stateFree =
+        FreeMemAux(C, CE, state, 0, false, ReleasedAllocated)) {
+
     ProgramStateRef stateRealloc = MallocMemAux(C, CE, CE->getArg(1),
                                                 UnknownVal(), stateFree);
     if (!stateRealloc)
       return 0;
+
+    // Record the info about the reallocated symbol so that we could properly
+    // process failed reallocation.
     stateRealloc = stateRealloc->set<ReallocPairs>(ToPtr,
-                                            ReallocPair(FromPtr, FreesOnFail));
+                     ReallocPair(FromPtr, FreesOnFail || !ReleasedAllocated));
+    // The reallocated symbol should stay alive for as long as the new symbol.
     C.getSymbolManager().addSymbolDependency(ToPtr, FromPtr);
     return stateRealloc;
   }

test/Analysis/malloc.c

@@ -1000,17 +1000,26 @@ void freeButNoMalloc(int *p, int x){
 }
 
 struct HasPtr {
-  int *p;
+  char *p;
 };
 
-int* reallocButNoMalloc(struct HasPtr *a, int c, int size) {
+char* reallocButNoMalloc(struct HasPtr *a, int c, int size) {
   int *s;
-  a->p = (int *)realloc(a->p, size);
-  if (a->p == 0)
-    return 0; // expected-warning{{Memory is never released; potential leak}}
+  char *b = realloc(a->p, size);
+  char *m = realloc(a->p, size); // expected-warning {{Attempt to free released memory}}
   return a->p;
 }
 
+// We should not warn in this case since the caller will presumably free a->p in all cases.
+int reallocButNoMallocPR13674(struct HasPtr *a, int c, int size) {
+  int *s;
+  char *b = realloc(a->p, size);
+  if (b == 0)
+    return -1;
+  a->p = b;
+  return 0;
+}
+
 // ----------------------------------------------------------------------------
 // False negatives.