[cfe-commits] r137523 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp test/Analysis/keychainAPI.m
Anna Zaks
ganna at apple.com
Fri Aug 12 14:56:43 PDT 2011
Author: zaks
Date: Fri Aug 12 16:56:43 2011
New Revision: 137523
URL: http://llvm.org/viewvc/llvm-project?rev=137523&view=rev
Log:
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.
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
cfe/trunk/test/Analysis/keychainAPI.m
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp?rev=137523&r1=137522&r2=137523&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp Fri Aug 12 16:56:43 2011
@@ -27,7 +27,8 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
// - 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.
- SymbolRef RetStatusSymbol = State->getSVal(CE).getAsSymbol();
- NoErr = NoErr->set<AllocatedData>(V, AllocationState(ArgExpr, idx,
- RetStatusSymbol));
- assert(NoErr);
- C.addTransition(NoErr);
+ // 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();
+ C.getSymbolManager().addSymbolDependency(V, RetStatusSymbol);
- // 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 @@
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) {
Modified: cfe/trunk/test/Analysis/keychainAPI.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/keychainAPI.m?rev=137523&r1=137522&r2=137523&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/keychainAPI.m (original)
+++ cfe/trunk/test/Analysis/keychainAPI.m Fri Aug 12 16:56:43 2011
@@ -71,13 +71,13 @@
);
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 @@
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 @@
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
More information about the cfe-commits
mailing list