From 703ffb11eff7bc6e8532bdbe54045e19a7732253 Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Fri, 12 Aug 2011 21:56:43 +0000 Subject: [PATCH] MacOSKeychainAPIChecker: Report errors earlier: on checkDeadSymbols() and clear the state after the symbol we are tracking goes out of scope. Also, perform lazy error checking. Instead of forcing the paths to be split depending one the return value of the allocator, make the return symbol depend on the allocated data symbol, which prolongs its life span to the time when the allocated data symbol becomes dead. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@137523 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/MacOSKeychainAPIChecker.cpp | 188 ++++++++++++++---- 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, check::PostStmt, - check::EndPath > { + check::EndPath, + check::DeadSymbols> { mutable llvm::OwningPtr 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(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(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(NoErrVal), - cast(State->getSVal(CE)), - CE->getCallReturnType()); - const GRState *Err = State->assume(cast(ReturnedError), true); - assert(Err); - C.addTransition(Err); + // Track the allocated value in the checker state. + State = State->set(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(); + if (ASet.isEmpty()) + return; + + bool Changed = false; + llvm::SmallVector Errors; + for (AllocatedSetTy::iterator I = ASet.begin(), E = ASet.end(); I != E; ++I) { + if (SR.isLive(I->first)) + continue; + + Changed = true; + State = State->remove(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::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(); - 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 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(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::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 -- 2.40.0