Nice! A few comments as usual... On Nov 12, 2012, at 19:18 , Anna Zaks <[email protected]> wrote:
> Author: zaks > Date: Mon Nov 12 21:18:01 2012 > New Revision: 167813 > > URL: http://llvm.org/viewvc/llvm-project?rev=167813&view=rev > Log: > 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. > > Modified: > cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > cfe/trunk/test/Analysis/malloc.mm > > Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=167813&r1=167812&r2=167813&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Mon Nov 12 > 21:18:01 2012 > @@ -107,7 +107,7 @@ > check::PreStmt<CallExpr>, > check::PostStmt<CallExpr>, > check::PostStmt<BlockExpr>, > - check::PreObjCMessage, > + check::PostObjCMessage, > check::Location, > check::Bind, > eval::Assume, > @@ -135,7 +135,7 @@ > > 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 @@ > 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 @@ > 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 @@ > 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 @@ > 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 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); > } This function signature is really ugly -- it returns two values as return values and one through an out parameter?? (And moreover it's failing to return through the out parameter because you forgot to make it a reference.) Please change. I would have the return value be the SymbolRef or NULL, and pass the state by reference, but if you have a better idea please use it. > 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 @@ > > // 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); Hm...we really need to make a cheaper isNull for SVals... (I know this is pre-existing code). > if (nullState && !notNullState) > return 0; > > @@ -683,10 +715,17 @@ > 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 @@ > "Attempt to free non-owned memory"), N); > R->addRange(ArgExpr->getSourceRange()); > R->markInteresting(Sym); > + if (RetStatusSymbolPtr) > + R->markInteresting(*RetStatusSymbolPtr); As mentioned above, this is never set.. > R->addVisitor(new MallocBugVisitor(Sym)); > C.emitReport(R); > } > @@ -704,10 +745,21 @@ > > 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 @@ > } > } > > + // 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()) { > > Modified: cfe/trunk/test/Analysis/malloc.mm > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.mm?rev=167813&r1=167812&r2=167813&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/malloc.mm (original) > +++ cfe/trunk/test/Analysis/malloc.mm Mon Nov 12 21:18:01 2012 > @@ -222,3 +222,50 @@ > 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 > +} The one test that's missing is a positive test: 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}} } // expected-warning{{leak}} > +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); > + } > + } > + } > +} > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
