diff options
| -rw-r--r-- | lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp | 188 | ||||
| -rw-r--r-- | test/Analysis/keychainAPI.m | 30 |
2 files changed, 167 insertions, 51 deletions
diff --git a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp index 4934cda1ae..d955f4bdb5 100644 --- a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -27,7 +27,8 @@ namespace { class MacOSKeychainAPIChecker : public Checker<check::PreStmt<CallExpr>, check::PreStmt<ReturnStmt>, check::PostStmt<CallExpr>, - check::EndPath > { + check::EndPath, + check::DeadSymbols> { mutable llvm::OwningPtr<BugType> BT; public: @@ -57,6 +58,7 @@ public: void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const; void checkPostStmt(const CallExpr *S, CheckerContext &C) const; + void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; void checkEndPath(EndOfFunctionNodeBuilder &B, ExprEngine &Eng) const; private: @@ -81,6 +83,24 @@ private: if (!BT) BT.reset(new BugType("Improper use of SecKeychain API", "Mac OS API")); } + + RangedBugReport *generateAllocatedDataNotReleasedReport( + const AllocationState &AS, + ExplodedNode *N) const; + + /// Check if RetSym evaluates to an error value in the current state. + bool definitelyReturnedError(SymbolRef RetSym, + const GRState *State, + SValBuilder &Builder, + bool noError = false) const; + + /// Check if RetSym evaluates to a NoErr value in the current state. + bool definitelyDidnotReturnError(SymbolRef RetSym, + const GRState *State, + SValBuilder &Builder) const { + return definitelyReturnedError(RetSym, State, Builder, true); + } + }; } @@ -167,6 +187,28 @@ static SymbolRef getAsPointeeSymbol(const Expr *Expr, return 0; } +// When checking for error code, we need to consider the following cases: +// 1) noErr / [0] +// 2) someErr / [1, inf] +// 3) unknown +// If noError, returns true iff (1). +// If !noError, returns true iff (2). +bool MacOSKeychainAPIChecker::definitelyReturnedError(SymbolRef RetSym, + const GRState *State, + SValBuilder &Builder, + bool noError) const { + DefinedOrUnknownSVal NoErrVal = Builder.makeIntVal(NoErr, + Builder.getSymbolManager().getType(RetSym)); + DefinedOrUnknownSVal NoErr = Builder.evalEQ(State, NoErrVal, + nonloc::SymbolVal(RetSym)); + const GRState *ErrState = State->assume(NoErr, noError); + if (ErrState == State) { + return true; + } + + return false; +} + void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE, CheckerContext &C) const { const GRState *State = C.getState(); @@ -270,6 +312,20 @@ void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE, // The call is deallocating a value we previously allocated, so remove it // from the next state. State = State->remove<AllocatedData>(ArgSM); + + // If the return status is undefined or is error, report a bad call to free. + if (!definitelyDidnotReturnError(AS->RetValue, State, C.getSValBuilder())) { + ExplodedNode *N = C.generateNode(State); + if (!N) + return; + initBugType(); + RangedBugReport *Report = new RangedBugReport(*BT, + "Call to free data when error was returned during allocation.", N); + Report->addRange(ArgExpr->getSourceRange()); + C.EmitReport(Report); + return; + } + C.addTransition(State); } @@ -301,31 +357,19 @@ void MacOSKeychainAPIChecker::checkPostStmt(const CallExpr *CE, // - constant (null - should not be tracked, // other constant will generate a compiler warning) // - goto (should be reported by other checker) - - // We only need to track the value if the function returned noErr(0), so - // bind the return value of the function to 0 and proceed from the no error - // state. - SValBuilder &Builder = C.getSValBuilder(); - SVal NoErrVal = Builder.makeIntVal(NoErr, CE->getCallReturnType()); - const GRState *NoErr = State->BindExpr(CE, NoErrVal); - // Add the symbolic value V, which represents the location of the allocated - // data, to the set. + + // The call return value symbol should stay alive for as long as the + // allocated value symbol, since our diagnostics depend on the value + // returned by the call. Ex: Data should only be freed if noErr was + // returned during allocation.) SymbolRef RetStatusSymbol = State->getSVal(CE).getAsSymbol(); - NoErr = NoErr->set<AllocatedData>(V, AllocationState(ArgExpr, idx, - RetStatusSymbol)); + C.getSymbolManager().addSymbolDependency(V, RetStatusSymbol); - assert(NoErr); - C.addTransition(NoErr); - - // Generate a transition to explore the state space when there is an error. - // In this case, we do not track the allocated data. - SVal ReturnedError = Builder.evalBinOpNN(State, BO_NE, - cast<NonLoc>(NoErrVal), - cast<NonLoc>(State->getSVal(CE)), - CE->getCallReturnType()); - const GRState *Err = State->assume(cast<NonLoc>(ReturnedError), true); - assert(Err); - C.addTransition(Err); + // Track the allocated value in the checker state. + State = State->set<AllocatedData>(V, AllocationState(ArgExpr, idx, + RetStatusSymbol)); + assert(State); + C.addTransition(State); } } @@ -346,31 +390,99 @@ void MacOSKeychainAPIChecker::checkPreStmt(const ReturnStmt *S, C.addTransition(state); } +// TODO: The report has to mention the expression which contains the +// allocated content as well as the point at which it has been allocated. +RangedBugReport *MacOSKeychainAPIChecker:: + generateAllocatedDataNotReleasedReport(const AllocationState &AS, + ExplodedNode *N) const { + const ADFunctionInfo &FI = FunctionsToTrack[AS.AllocatorIdx]; + initBugType(); + std::string sbuf; + llvm::raw_string_ostream os(sbuf); + os << "Allocated data is not released: missing a call to '" + << FunctionsToTrack[FI.DeallocatorIdx].Name << "'."; + RangedBugReport *Report = new RangedBugReport(*BT, os.str(), N); + Report->addRange(AS.Address->getSourceRange()); + return Report; +} + +void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR, + CheckerContext &C) const { + const GRState *State = C.getState(); + AllocatedSetTy ASet = State->get<AllocatedData>(); + if (ASet.isEmpty()) + return; + + bool Changed = false; + llvm::SmallVector<const AllocationState*, 1> Errors; + for (AllocatedSetTy::iterator I = ASet.begin(), E = ASet.end(); I != E; ++I) { + if (SR.isLive(I->first)) + continue; + + Changed = true; + State = State->remove<AllocatedData>(I->first); + // If the allocated symbol is null or if the allocation call might have + // returned an error, do not report. + if (State->getSymVal(I->first) || + definitelyReturnedError(I->second.RetValue, State, C.getSValBuilder())) + continue; + Errors.push_back(&I->second); + } + if (!Changed) + return; + + // Generate the new, cleaned up state. + ExplodedNode *N = C.generateNode(State); + if (!N) + return; + + // Generate the error reports. + for (llvm::SmallVector<const AllocationState*, 3>::iterator + I = Errors.begin(), E = Errors.end(); I != E; ++I) { + C.EmitReport(generateAllocatedDataNotReleasedReport(**I, N)); + } +} + +// TODO: Remove this after we ensure that checkDeadSymbols are always called. void MacOSKeychainAPIChecker::checkEndPath(EndOfFunctionNodeBuilder &B, ExprEngine &Eng) const { const GRState *state = B.getState(); AllocatedSetTy AS = state->get<AllocatedData>(); - ExplodedNode *N = B.generateNode(state); - if (!N) + if (AS.isEmpty()) return; - initBugType(); // Anything which has been allocated but not freed (nor escaped) will be // found here, so report it. + bool Changed = false; + llvm::SmallVector<const AllocationState*, 1> Errors; for (AllocatedSetTy::iterator I = AS.begin(), E = AS.end(); I != E; ++I ) { - const ADFunctionInfo &FI = FunctionsToTrack[I->second.AllocatorIdx]; + Changed = true; + state = state->remove<AllocatedData>(I->first); + // If the allocated symbol is null or if error code was returned at + // allocation, do not report. + if (state->getSymVal(I.getKey()) || + definitelyReturnedError(I->second.RetValue, state, + Eng.getSValBuilder())) { + continue; + } + Errors.push_back(&I->second); + } - std::string sbuf; - llvm::raw_string_ostream os(sbuf); - os << "Allocated data is not released: missing a call to '" - << FunctionsToTrack[FI.DeallocatorIdx].Name << "'."; - RangedBugReport *Report = new RangedBugReport(*BT, os.str(), N); - // TODO: The report has to mention the expression which contains the - // allocated content as well as the point at which it has been allocated. - // Currently, the next line is useless. - Report->addRange(I->second.Address->getSourceRange()); - Eng.getBugReporter().EmitReport(Report); + // If no change, do not generate a new state. + if (!Changed) + return; + + ExplodedNode *N = B.generateNode(state); + if (!N) + return; + + // Generate the error reports. + for (llvm::SmallVector<const AllocationState*, 3>::iterator + I = Errors.begin(), E = Errors.end(); I != E; ++I) { + Eng.getBugReporter().EmitReport( + generateAllocatedDataNotReleasedReport(**I, N)); } + } void ento::registerMacOSKeychainAPIChecker(CheckerManager &mgr) { diff --git a/test/Analysis/keychainAPI.m b/test/Analysis/keychainAPI.m index 6a9a75330d..be9d74c31e 100644 --- a/test/Analysis/keychainAPI.m +++ b/test/Analysis/keychainAPI.m @@ -71,13 +71,13 @@ OSStatus SecKeychainItemFreeAttributesAndData ( ); void errRetVal() { - unsigned int *ptr = 0; - OSStatus st = 0; - UInt32 length; - void *outData; - st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData); - if (st == GenericError) // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'.}} - SecKeychainItemFreeContent(ptr, outData); // expected-warning{{Trying to free data which has not been allocated.}} + unsigned int *ptr = 0; + OSStatus st = 0; + UInt32 length; + void *outData; + st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData); + if (st == GenericError) // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'.}} + SecKeychainItemFreeContent(ptr, outData); // expected-warning{{Call to free data when error was returned during allocation.}} } // If null is passed in, the data is not allocated, so no need for the matching free. @@ -123,7 +123,7 @@ void fooOnlyFreeParam(void *attrList, void* X) { SecKeychainItemFreeContent(attrList, X); }// no-warning -// If we are returning the value, no not report. +// If we are returning the value, do not report. void* returnContent() { unsigned int *ptr = 0; OSStatus st = 0; @@ -177,11 +177,15 @@ int foo() { OSStatus st = 0; UInt32 length; - void *outData; - - st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData); - if (st == noErr) - SecKeychainItemFreeContent(ptr, outData); + void *outData[5]; + st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &(outData[3])); + if (length == 5) { + if (st == noErr) + SecKeychainItemFreeContent(ptr, outData[3]); + } + if (length) { // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'.}} + length++; + } return 0; }// no-warning |