Index: lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -28,7 +28,8 @@ namespace { class MacOSKeychainAPIChecker : public Checker, check::PostStmt, - check::DeadSymbols> { + check::DeadSymbols, + eval::Assume> { mutable std::unique_ptr BT; public: @@ -57,6 +58,10 @@ void checkPreStmt(const CallExpr *S, CheckerContext &C) const; void checkPostStmt(const CallExpr *S, CheckerContext &C) const; void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; + ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond, + bool Assumption) const; + void printState(raw_ostream &Out, ProgramStateRef State, + const char *NL, const char *Sep) const; private: typedef std::pair AllocationPair; @@ -106,19 +111,6 @@ std::unique_ptr generateAllocatedDataNotReleasedReport( const AllocationPair &AP, ExplodedNode *N, CheckerContext &C) const; - /// Check if RetSym evaluates to an error value in the current state. - bool definitelyReturnedError(SymbolRef RetSym, - ProgramStateRef State, - SValBuilder &Builder, - bool noError = false) const; - - /// Check if RetSym evaluates to a NoErr value in the current state. - bool definitelyDidnotReturnError(SymbolRef RetSym, - ProgramStateRef State, - SValBuilder &Builder) const { - return definitelyReturnedError(RetSym, State, Builder, true); - } - /// Mark an AllocationPair interesting for diagnostic reporting. void markInteresting(BugReport *R, const AllocationPair &AP) const { R->markInteresting(AP.first); @@ -221,24 +213,6 @@ return nullptr; } -// 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, - ProgramStateRef State, - SValBuilder &Builder, - bool noError) const { - DefinedOrUnknownSVal NoErrVal = Builder.makeIntVal(NoErr, - Builder.getSymbolManager().getType(RetSym)); - DefinedOrUnknownSVal NoErr = Builder.evalEQ(State, NoErrVal, - nonloc::SymbolVal(RetSym)); - ProgramStateRef ErrState = State->assume(NoErr, noError); - return ErrState == State; -} - // Report deallocator mismatch. Remove the region from tracking - reporting a // missing free error after this one is redundant. void MacOSKeychainAPIChecker:: @@ -289,27 +263,25 @@ const Expr *ArgExpr = CE->getArg(paramIdx); if (SymbolRef V = getAsPointeeSymbol(ArgExpr, C)) if (const AllocationState *AS = State->get(V)) { - if (!definitelyReturnedError(AS->Region, State, C.getSValBuilder())) { - // Remove the value from the state. The new symbol will be added for - // tracking when the second allocator is processed in checkPostStmt(). - State = State->remove(V); - ExplodedNode *N = C.generateNonFatalErrorNode(State); - if (!N) - return; - initBugType(); - SmallString<128> sbuf; - llvm::raw_svector_ostream os(sbuf); - unsigned int DIdx = FunctionsToTrack[AS->AllocatorIdx].DeallocatorIdx; - os << "Allocated data should be released before another call to " - << "the allocator: missing a call to '" - << FunctionsToTrack[DIdx].Name - << "'."; - auto Report = llvm::make_unique(*BT, os.str(), N); - Report->addVisitor(llvm::make_unique(V)); - Report->addRange(ArgExpr->getSourceRange()); - Report->markInteresting(AS->Region); - C.emitReport(std::move(Report)); - } + // Remove the value from the state. The new symbol will be added for + // tracking when the second allocator is processed in checkPostStmt(). + State = State->remove(V); + ExplodedNode *N = C.generateNonFatalErrorNode(State); + if (!N) + return; + initBugType(); + SmallString<128> sbuf; + llvm::raw_svector_ostream os(sbuf); + unsigned int DIdx = FunctionsToTrack[AS->AllocatorIdx].DeallocatorIdx; + os << "Allocated data should be released before another call to " + << "the allocator: missing a call to '" + << FunctionsToTrack[DIdx].Name + << "'."; + auto Report = llvm::make_unique(*BT, os.str(), N); + Report->addVisitor(llvm::make_unique(V)); + Report->addRange(ArgExpr->getSourceRange()); + Report->markInteresting(AS->Region); + C.emitReport(std::move(Report)); } return; } @@ -344,13 +316,12 @@ // Is the argument to the call being tracked? const AllocationState *AS = State->get(ArgSM); - if (!AS && FunctionsToTrack[idx].Kind != ValidAPI) { + if (!AS) return; - } - // If trying to free data which has not been allocated yet, report as a bug. - // TODO: We might want a more precise diagnostic for double free + + // TODO: We might want to report double free here. // (that would involve tracking all the freed symbols in the checker state). - if (!AS || RegionArgIsBad) { + if (RegionArgIsBad) { // It is possible that this is a false positive - the argument might // have entered as an enclosing function parameter. if (isEnclosingFunctionParam(ArgExpr)) @@ -418,23 +389,6 @@ return; } - // If the buffer can be null and the return status can be an error, - // report a bad call to free. - if (State->assume(ArgSVal.castAs(), false) && - !definitelyDidnotReturnError(AS->Region, State, C.getSValBuilder())) { - ExplodedNode *N = C.generateNonFatalErrorNode(State); - if (!N) - return; - initBugType(); - auto Report = llvm::make_unique( - *BT, "Only call free if a valid (non-NULL) buffer was returned.", N); - Report->addVisitor(llvm::make_unique(ArgSM)); - Report->addRange(ArgExpr->getSourceRange()); - Report->markInteresting(AS->Region); - C.emitReport(std::move(Report)); - return; - } - C.addTransition(State); } @@ -540,6 +494,45 @@ return Report; } +/// If the return symbol is assumed to be error, remove the allocated info +/// from consideration. +ProgramStateRef MacOSKeychainAPIChecker::evalAssume(ProgramStateRef State, + SVal Cond, + bool Assumption) const { + AllocatedDataTy ASet = State->get(); + if (ASet.isEmpty()) + return State; + + auto *CondBSE = dyn_cast_or_null(Cond.getAsSymExpr()); + if (!CondBSE) + return State; + BinaryOperator::Opcode OpCode = CondBSE->getOpcode(); + if (OpCode != BO_EQ && OpCode != BO_NE) + return State; + + // Match for a restricted set of patterns for cmparison of error codes. + // Note, the comparisons of type '0 == st' are transformed into SymintExpr. + SymbolRef ReturnSymbol = nullptr; + if (auto *SIE = dyn_cast(CondBSE)) { + const llvm::APInt &RHS = SIE->getRHS(); + bool errorIsReturned = (OpCode == BO_EQ && RHS != NoErr) || + (OpCode == BO_NE && RHS == NoErr); + if (!Assumption) + errorIsReturned = !errorIsReturned; + if (errorIsReturned) + ReturnSymbol = SIE->getLHS(); + } + + if (ReturnSymbol) + for (AllocatedDataTy::iterator I = ASet.begin(), E = ASet.end(); + I != E; ++I) { + if (ReturnSymbol == I->second.Region) + State = State->remove(I->first); + } + + return State; +} + void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const { ProgramStateRef State = C.getState(); @@ -550,17 +543,15 @@ bool Changed = false; AllocationPairVec Errors; for (AllocatedDataTy::iterator I = ASet.begin(), E = ASet.end(); I != E; ++I) { - if (SR.isLive(I->first)) + if (!SR.isDead(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 the allocated symbol is null do not report. ConstraintManager &CMgr = State->getConstraintManager(); ConditionTruthVal AllocFailed = CMgr.isNull(State, I.getKey()); - if (AllocFailed.isConstrainedTrue() || - definitelyReturnedError(I->second.Region, State, C.getSValBuilder())) + if (AllocFailed.isConstrainedTrue()) continue; Errors.push_back(std::make_pair(I->first, &I->second)); } @@ -583,7 +574,6 @@ C.addTransition(State, N); } - PathDiagnosticPiece *MacOSKeychainAPIChecker::SecKeychainBugVisitor::VisitNode( const ExplodedNode *N, const ExplodedNode *PrevN, @@ -613,6 +603,23 @@ return new PathDiagnosticEventPiece(Pos, "Data is allocated here."); } +void MacOSKeychainAPIChecker::printState(raw_ostream &Out, + ProgramStateRef State, + const char *NL, + const char *Sep) const { + + AllocatedDataTy ASet = State->get(); + + if (!ASet.isEmpty()) { + Out << Sep << "KeychainAPIChecker :" << NL; + for (AllocatedDataTy::iterator I = ASet.begin(), E = ASet.end(); + I != E; ++I) { + I.getKey()->dumpToStream(Out); + } + } +} + + void ento::registerMacOSKeychainAPIChecker(CheckerManager &mgr) { mgr.registerChecker(); } Index: test/Analysis/keychainAPI.m =================================================================== --- test/Analysis/keychainAPI.m +++ test/Analysis/keychainAPI.m @@ -1,4 +1,6 @@ -// RUN: %clang_cc1 -analyze -analyzer-checker=osx.SecKeychainAPI %s -verify +// RUN: %clang_cc1 -analyze -analyzer-checker=osx.SecKeychainAPI -fblocks %s -verify + +#include "Inputs/system-header-simulator-objc.h" // Fake typedefs. typedef unsigned int OSStatus; @@ -6,8 +8,6 @@ typedef unsigned int SecKeychainItemRef; typedef unsigned int SecItemClass; typedef unsigned int UInt32; -typedef unsigned int CFTypeRef; -typedef unsigned int UInt16; typedef unsigned int SecProtocolType; typedef unsigned int SecAuthenticationType; typedef unsigned int SecKeychainAttributeInfo; @@ -77,7 +77,7 @@ void *outData; st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData); if (st == GenericError) - SecKeychainItemFreeContent(ptr, outData); // expected-warning{{Only call free if a valid (non-NULL) buffer was returned}} + SecKeychainItemFreeContent(ptr, outData); } // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}} // If null is passed in, the data is not allocated, so no need for the matching free. @@ -101,14 +101,6 @@ SecKeychainItemFreeContent(ptr, outData); } -void fooOnlyFree() { - unsigned int *ptr = 0; - OSStatus st = 0; - UInt32 length; - void *outData = &length; - SecKeychainItemFreeContent(ptr, outData);// expected-warning{{Trying to free data which has not been allocated}} -} - // Do not warn if undefined value is passed to a function. void fooOnlyFreeUndef() { unsigned int *ptr = 0; @@ -220,11 +212,27 @@ if (st == noErr) SecKeychainItemFreeContent(ptr, outData[3]); } - if (length) { // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}} + if (length) { // TODO: We do not report a warning here since the symbol is no longer live, but it's not marked as dead. length++; } return 0; -}// no-warning +} + +int testErrorCodeAsLHS(CFTypeRef keychainOrArray, SecProtocolType protocol, + SecAuthenticationType authenticationType, SecKeychainItemRef *itemRef) { + unsigned int *ptr = 0; + OSStatus st = 0; + UInt32 length; + void *outData; + st = SecKeychainFindInternetPassword(keychainOrArray, + 16, "server", 16, "domain", 16, "account", + 16, "path", 222, protocol, authenticationType, + &length, &outData, itemRef); + if (noErr == st) + SecKeychainItemFreeContent(ptr, outData); + + return 0; +} void free(void *ptr); void deallocateWithFree() { @@ -251,7 +259,6 @@ extern const CFAllocatorRef kCFAllocatorNull; extern const CFAllocatorRef kCFAllocatorUseContext; CFStringRef CFStringCreateWithBytesNoCopy(CFAllocatorRef alloc, const uint8_t *bytes, CFIndex numBytes, CFStringEncoding encoding, Boolean externalFormat, CFAllocatorRef contentsDeallocator); -extern void CFRelease(CFStringRef cf); void DellocWithCFStringCreate1(CFAllocatorRef alloc) { unsigned int *ptr = 0; @@ -333,11 +340,11 @@ SecKeychainItemFreeContent(0, pwdBytes); } -void radar10508828_2() { +void radar10508828_20092614() { UInt32 pwdLen = 0; void* pwdBytes = 0; OSStatus rc = SecKeychainFindGenericPassword(0, 3, "foo", 3, "bar", &pwdLen, &pwdBytes, 0); - SecKeychainItemFreeContent(0, pwdBytes); // expected-warning {{Only call free if a valid (non-NULL) buffer was returned}} + SecKeychainItemFreeContent(0, pwdBytes); } //Example from bug 10797. @@ -426,3 +433,24 @@ SecKeychainItemFreeContent(attrList, outData); } +typedef struct AuthorizationValue { + int length; + void *data; +} AuthorizationValue; +typedef struct AuthorizationCallback { + OSStatus (*SetContextVal)(AuthorizationValue *inValue); +} AuthorizationCallback; +static AuthorizationCallback cb; +int radar_19196494() { + @autoreleasepool { + AuthorizationValue login_password = {}; + UInt32 passwordLength; + void *passwordData = 0; + OSStatus err = SecKeychainFindGenericPassword(0, 0, "", 0, "", (UInt32 *)&login_password.length, (void**)&login_password.data, 0); + cb.SetContextVal(&login_password); + if (err == noErr) { + SecKeychainItemFreeContent(0, login_password.data); + } + } + return 0; +}