zaks.anna updated this revision to Diff 83160.
zaks.anna added a comment.
Addressed all comments
https://reviews.llvm.org/D28330
Files:
lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
test/Analysis/keychainAPI.m
Index: test/Analysis/keychainAPI.m
===================================================================
--- test/Analysis/keychainAPI.m
+++ test/Analysis/keychainAPI.m
@@ -1,13 +1,13 @@
-// 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;
typedef unsigned int SecKeychainAttributeList;
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;
+}
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::PreStmt<CallExpr>,
check::PostStmt<CallExpr>,
- check::DeadSymbols> {
+ check::DeadSymbols,
+ eval::Assume> {
mutable std::unique_ptr<BugType> 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<SymbolRef, const AllocationState*> AllocationPair;
@@ -106,19 +111,6 @@
std::unique_ptr<BugReport> 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<AllocatedData>(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<AllocatedData>(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<BugReport>(*BT, os.str(), N);
- Report->addVisitor(llvm::make_unique<SecKeychainBugVisitor>(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<AllocatedData>(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<BugReport>(*BT, os.str(), N);
+ Report->addVisitor(llvm::make_unique<SecKeychainBugVisitor>(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<AllocatedData>(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<DefinedSVal>(), false) &&
- !definitelyDidnotReturnError(AS->Region, State, C.getSValBuilder())) {
- ExplodedNode *N = C.generateNonFatalErrorNode(State);
- if (!N)
- return;
- initBugType();
- auto Report = llvm::make_unique<BugReport>(
- *BT, "Only call free if a valid (non-NULL) buffer was returned.", N);
- Report->addVisitor(llvm::make_unique<SecKeychainBugVisitor>(ArgSM));
- Report->addRange(ArgExpr->getSourceRange());
- Report->markInteresting(AS->Region);
- C.emitReport(std::move(Report));
- return;
- }
-
C.addTransition(State);
}
@@ -540,27 +494,63 @@
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 AMap = State->get<AllocatedData>();
+ if (AMap.isEmpty())
+ return State;
+
+ auto *CondBSE = dyn_cast_or_null<BinarySymExpr>(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<SymIntExpr>(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 (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) {
+ if (ReturnSymbol == I->second.Region)
+ State = State->remove<AllocatedData>(I->first);
+ }
+
+ return State;
+}
+
void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
- AllocatedDataTy ASet = State->get<AllocatedData>();
- if (ASet.isEmpty())
+ AllocatedDataTy AMap = State->get<AllocatedData>();
+ if (AMap.isEmpty())
return;
bool Changed = false;
AllocationPairVec Errors;
- for (AllocatedDataTy::iterator I = ASet.begin(), E = ASet.end(); I != E; ++I) {
- if (SR.isLive(I->first))
+ for (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) {
+ if (!SR.isDead(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 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 +573,6 @@
C.addTransition(State, N);
}
-
PathDiagnosticPiece *MacOSKeychainAPIChecker::SecKeychainBugVisitor::VisitNode(
const ExplodedNode *N,
const ExplodedNode *PrevN,
@@ -613,6 +602,22 @@
return new PathDiagnosticEventPiece(Pos, "Data is allocated here.");
}
+void MacOSKeychainAPIChecker::printState(raw_ostream &Out,
+ ProgramStateRef State,
+ const char *NL,
+ const char *Sep) const {
+
+ AllocatedDataTy AMap = State->get<AllocatedData>();
+
+ if (!AMap.isEmpty()) {
+ Out << Sep << "KeychainAPIChecker :" << NL;
+ for (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) {
+ I.getKey()->dumpToStream(Out);
+ }
+ }
+}
+
+
void ento::registerMacOSKeychainAPIChecker(CheckerManager &mgr) {
mgr.registerChecker<MacOSKeychainAPIChecker>();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
