zaks.anna created this revision.
zaks.anna added a reviewer: dcoughlin.
zaks.anna added subscribers: cfe-commits, dergachev.a.
The checker has several false positives that this patch addresses:
1. Do not check if the return status has been compared to error (or no error)
at the time when leaks are reported since the status symbol might no longer be
alive. Instead, pattern match on the assume and stop tracking allocated symbols
on error paths.
2. The checker used to report error when an unknown symbol was freed. This
could lead to false positives, let's not repot those. This leads to loss of
coverage in double frees.
3. Do not enforce that we should only call free if we are sure that error was
not returned and the pointer is not null. That warning is too noisy and we
received several false positive reports about it. (I removed: "Only call free
if a valid (non-NULL) buffer was returned")
1. Use !isDead instead of isLive in leak reporting. Otherwise, we report leaks
for objects we loose track of. This change triggered change #1.
This also adds checker specific dump to the state.
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,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<AllocatedData>();
+ if (ASet.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 (AllocatedDataTy::iterator I = ASet.begin(), E = ASet.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();
@@ -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<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 +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<AllocatedData>();
+
+ 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<MacOSKeychainAPIChecker>();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
