[cfe-commits] r167813 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/malloc.mm
Jordan Rose
jordan_rose at apple.com
Tue Nov 13 09:15:26 PST 2012
Nice! A few comments as usual...
On Nov 12, 2012, at 19:18 , Anna Zaks <ganna at apple.com> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list