Commit 4ee1a188 authored by Anna Zaks's avatar Anna Zaks
Browse files

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.

Commits: 167813, 167814, 167868
llvm-svn: 167870
parent 6a22c4c8

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

+76 −20
Original line number Diff line number Diff line
@@ -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,7 +498,7 @@ static bool isFreeWhenDoneSetToZero(const ObjCMethodCall &Call) { 
  return false;

}



void MallocChecker::checkPreObjCMessage(const ObjCMethodCall &Call,

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.
@@ -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),

    ProgramStateRef State = FreeMemAux(C, Call.getArgExpr(argIdx),

                                       Call.getOriginExpr(), C.getState(), true,

                    ReleasedAllocatedMemory));

                                       ReleasedAllocatedMemory,

                                       /* RetNullOnFailure*/ true);



    C.addTransition(State);

  }

}
@@ -609,21 +618,39 @@ 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);

}



/// 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);

    RetStatusSymbol = *Ret;

    return FreeFailed.isConstrainedTrue();

  }

  return false;

}



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 +661,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 +710,14 @@ 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);

  SymbolRef PreviousRetStatusSymbol = 0;



  // Check double free.

  if (RS && (RS->isReleased() || RS->isRelinquished())) {

  if (RS &&

      (RS->isReleased() || RS->isRelinquished()) &&

      !didPreviousFreeFail(State, Sym, PreviousRetStatusSymbol)) {



    if (ExplodedNode *N = C.generateSink()) {

      if (!BT_DoubleFree)

        BT_DoubleFree.reset(
@@ -696,6 +727,8 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C, 
                            "Attempt to free non-owned memory"), N);

      R->addRange(ArgExpr->getSourceRange());

      R->markInteresting(Sym);

      if (PreviousRetStatusSymbol)

        R->markInteresting(PreviousRetStatusSymbol);

      R->addVisitor(new MallocBugVisitor(Sym));

      C.emitReport(R);

    }
@@ -704,10 +737,24 @@ 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) {

    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 +1111,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()) {

clang/test/Analysis/malloc.mm

+56 −0
Original line number Diff line number Diff line
@@ -222,3 +222,59 @@ 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);}

}



NSString *test12365078_no_malloc_returnValue(unichar *characters) {

  NSString *string = [[NSString alloc] initWithCharactersNoCopy:characters length:12 freeWhenDone:1];

  if (!string) {

    return 0; // no-warning

  }

  return 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);

      }

    }

  }

}



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}}

}