r304159 - [analyzer] PthreadLockChecker: model failed pthread_mutex_destroy() calls.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Mon May 29 07:51:39 PDT 2017
Author: dergachev
Date: Mon May 29 09:51:39 2017
New Revision: 304159
URL: http://llvm.org/viewvc/llvm-project?rev=304159&view=rev
Log:
[analyzer] PthreadLockChecker: model failed pthread_mutex_destroy() calls.
pthread_mutex_destroy() may fail, returning a non-zero error number, and
keeping the mutex untouched. The mutex can be used on the execution branch
that follows such failure, so the analyzer shouldn't warn on using
a mutex that was previously destroyed, when in fact the destroy call has failed.
Patch by Malhar Thakkar!
Differential revision: https://reviews.llvm.org/D32449
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
cfe/trunk/test/Analysis/pthreadlock.c
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp?rev=304159&r1=304158&r2=304159&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp Mon May 29 09:51:39 2017
@@ -25,7 +25,13 @@ using namespace ento;
namespace {
struct LockState {
- enum Kind { Destroyed, Locked, Unlocked } K;
+ enum Kind {
+ Destroyed,
+ Locked,
+ Unlocked,
+ UntouchedAndPossiblyDestroyed,
+ UnlockedAndPossiblyDestroyed
+ } K;
private:
LockState(Kind K) : K(K) {}
@@ -34,6 +40,12 @@ public:
static LockState getLocked() { return LockState(Locked); }
static LockState getUnlocked() { return LockState(Unlocked); }
static LockState getDestroyed() { return LockState(Destroyed); }
+ static LockState getUntouchedAndPossiblyDestroyed() {
+ return LockState(UntouchedAndPossiblyDestroyed);
+ }
+ static LockState getUnlockedAndPossiblyDestroyed() {
+ return LockState(UnlockedAndPossiblyDestroyed);
+ }
bool operator==(const LockState &X) const {
return K == X.K;
@@ -42,13 +54,20 @@ public:
bool isLocked() const { return K == Locked; }
bool isUnlocked() const { return K == Unlocked; }
bool isDestroyed() const { return K == Destroyed; }
+ bool isUntouchedAndPossiblyDestroyed() const {
+ return K == UntouchedAndPossiblyDestroyed;
+ }
+ bool isUnlockedAndPossiblyDestroyed() const {
+ return K == UnlockedAndPossiblyDestroyed;
+ }
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddInteger(K);
}
};
-class PthreadLockChecker : public Checker< check::PostStmt<CallExpr> > {
+class PthreadLockChecker
+ : public Checker<check::PostStmt<CallExpr>, check::DeadSymbols> {
mutable std::unique_ptr<BugType> BT_doublelock;
mutable std::unique_ptr<BugType> BT_doubleunlock;
mutable std::unique_ptr<BugType> BT_destroylock;
@@ -61,22 +80,31 @@ class PthreadLockChecker : public Checke
};
public:
void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
void AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock,
bool isTryLock, enum LockingSemantics semantics) const;
void ReleaseLock(CheckerContext &C, const CallExpr *CE, SVal lock) const;
- void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
+ void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock,
+ enum LockingSemantics semantics) const;
void InitLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
void reportUseDestroyedBug(CheckerContext &C, const CallExpr *CE) const;
+ ProgramStateRef resolvePossiblyDestroyedMutex(ProgramStateRef state,
+ const MemRegion *lockR,
+ const SymbolRef *sym) const;
};
} // end anonymous namespace
-// GDM Entry for tracking lock state.
+// A stack of locks for tracking lock-unlock order.
REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
+// An entry for tracking lock states.
REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)
+// Return values for unresolved calls to pthread_mutex_destroy().
+REGISTER_MAP_WITH_PROGRAMSTATE(DestroyRetVal, const MemRegion *, SymbolRef)
+
void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
@@ -113,13 +141,49 @@ void PthreadLockChecker::checkPostStmt(c
FName == "lck_mtx_unlock" ||
FName == "lck_rw_done")
ReleaseLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
- else if (FName == "pthread_mutex_destroy" ||
- FName == "lck_mtx_destroy")
- DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
+ else if (FName == "pthread_mutex_destroy")
+ DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), PthreadSemantics);
+ else if (FName == "lck_mtx_destroy")
+ DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), XNUSemantics);
else if (FName == "pthread_mutex_init")
InitLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
}
+// When a lock is destroyed, in some semantics(like PthreadSemantics) we are not
+// sure if the destroy call has succeeded or failed, and the lock enters one of
+// the 'possibly destroyed' state. There is a short time frame for the
+// programmer to check the return value to see if the lock was successfully
+// destroyed. Before we model the next operation over that lock, we call this
+// function to see if the return value was checked by now and set the lock state
+// - either to destroyed state or back to its previous state.
+
+// In PthreadSemantics, pthread_mutex_destroy() returns zero if the lock is
+// successfully destroyed and it returns a non-zero value otherwise.
+ProgramStateRef PthreadLockChecker::resolvePossiblyDestroyedMutex(
+ ProgramStateRef state, const MemRegion *lockR, const SymbolRef *sym) const {
+ const LockState *lstate = state->get<LockMap>(lockR);
+ // Existence in DestroyRetVal ensures existence in LockMap.
+ // Existence in Destroyed also ensures that the lock state for lockR is either
+ // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
+ assert(lstate->isUntouchedAndPossiblyDestroyed() ||
+ lstate->isUnlockedAndPossiblyDestroyed());
+
+ ConstraintManager &CMgr = state->getConstraintManager();
+ ConditionTruthVal retZero = CMgr.isNull(state, *sym);
+ if (retZero.isConstrainedFalse()) {
+ if (lstate->isUntouchedAndPossiblyDestroyed())
+ state = state->remove<LockMap>(lockR);
+ else if (lstate->isUnlockedAndPossiblyDestroyed())
+ state = state->set<LockMap>(lockR, LockState::getUnlocked());
+ } else
+ state = state->set<LockMap>(lockR, LockState::getDestroyed());
+
+ // Removing the map entry (lockR, sym) from DestroyRetVal as the lock state is
+ // now resolved.
+ state = state->remove<DestroyRetVal>(lockR);
+ return state;
+}
+
void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE,
SVal lock, bool isTryLock,
enum LockingSemantics semantics) const {
@@ -129,6 +193,9 @@ void PthreadLockChecker::AcquireLock(Che
return;
ProgramStateRef state = C.getState();
+ const SymbolRef *sym = state->get<DestroyRetVal>(lockR);
+ if (sym)
+ state = resolvePossiblyDestroyedMutex(state, lockR, sym);
SVal X = state->getSVal(CE, C.getLocationContext());
if (X.isUnknownOrUndef())
@@ -197,6 +264,9 @@ void PthreadLockChecker::ReleaseLock(Che
return;
ProgramStateRef state = C.getState();
+ const SymbolRef *sym = state->get<DestroyRetVal>(lockR);
+ if (sym)
+ state = resolvePossiblyDestroyedMutex(state, lockR, sym);
if (const LockState *LState = state->get<LockMap>(lockR)) {
if (LState->isUnlocked()) {
@@ -245,7 +315,8 @@ void PthreadLockChecker::ReleaseLock(Che
}
void PthreadLockChecker::DestroyLock(CheckerContext &C, const CallExpr *CE,
- SVal Lock) const {
+ SVal Lock,
+ enum LockingSemantics semantics) const {
const MemRegion *LockR = Lock.getAsRegion();
if (!LockR)
@@ -253,13 +324,38 @@ void PthreadLockChecker::DestroyLock(Che
ProgramStateRef State = C.getState();
+ const SymbolRef *sym = State->get<DestroyRetVal>(LockR);
+ if (sym)
+ State = resolvePossiblyDestroyedMutex(State, LockR, sym);
+
const LockState *LState = State->get<LockMap>(LockR);
- if (!LState || LState->isUnlocked()) {
- State = State->set<LockMap>(LockR, LockState::getDestroyed());
- C.addTransition(State);
- return;
+ // Checking the return value of the destroy method only in the case of
+ // PthreadSemantics
+ if (semantics == PthreadSemantics) {
+ if (!LState || LState->isUnlocked()) {
+ SymbolRef sym = C.getSVal(CE).getAsSymbol();
+ if (!sym) {
+ State = State->remove<LockMap>(LockR);
+ C.addTransition(State);
+ return;
+ }
+ State = State->set<DestroyRetVal>(LockR, sym);
+ if (LState && LState->isUnlocked())
+ State = State->set<LockMap>(
+ LockR, LockState::getUnlockedAndPossiblyDestroyed());
+ else
+ State = State->set<LockMap>(
+ LockR, LockState::getUntouchedAndPossiblyDestroyed());
+ C.addTransition(State);
+ return;
+ }
+ } else {
+ if (!LState || LState->isUnlocked()) {
+ State = State->set<LockMap>(LockR, LockState::getDestroyed());
+ C.addTransition(State);
+ return;
+ }
}
-
StringRef Message;
if (LState->isLocked()) {
@@ -288,6 +384,10 @@ void PthreadLockChecker::InitLock(Checke
ProgramStateRef State = C.getState();
+ const SymbolRef *sym = State->get<DestroyRetVal>(LockR);
+ if (sym)
+ State = resolvePossiblyDestroyedMutex(State, LockR, sym);
+
const struct LockState *LState = State->get<LockMap>(LockR);
if (!LState || LState->isDestroyed()) {
State = State->set<LockMap>(LockR, LockState::getUnlocked());
@@ -328,6 +428,26 @@ void PthreadLockChecker::reportUseDestro
C.emitReport(std::move(Report));
}
+void PthreadLockChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+
+ // TODO: Clean LockMap when a mutex region dies.
+
+ DestroyRetValTy TrackedSymbols = State->get<DestroyRetVal>();
+ for (DestroyRetValTy::iterator I = TrackedSymbols.begin(),
+ E = TrackedSymbols.end();
+ I != E; ++I) {
+ const SymbolRef Sym = I->second;
+ const MemRegion *lockR = I->first;
+ bool IsSymDead = SymReaper.isDead(Sym);
+ // Remove the dead symbol from the return value symbols map.
+ if (IsSymDead)
+ State = resolvePossiblyDestroyedMutex(State, lockR, &Sym);
+ }
+ C.addTransition(State);
+}
+
void ento::registerPthreadLockChecker(CheckerManager &mgr) {
mgr.registerChecker<PthreadLockChecker>();
}
Modified: cfe/trunk/test/Analysis/pthreadlock.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/pthreadlock.c?rev=304159&r1=304158&r2=304159&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/pthreadlock.c (original)
+++ cfe/trunk/test/Analysis/pthreadlock.c Mon May 29 09:51:39 2017
@@ -176,6 +176,42 @@ ok22(void) {
pthread_mutex_unlock(pmtx); // no-warning
}
+void ok23(void) {
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_destroy(&mtx1); // no-warning
+}
+
+void ok24(void) {
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+}
+
+void ok25(void) {
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+}
+
+void ok26(void) {
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+}
+
+void ok27(void) {
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ else
+ pthread_mutex_init(&mtx1, NULL); // no-warning
+}
+
+void ok28(void) {
+ if (pthread_mutex_destroy(&mtx1) != 0) { // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ pthread_mutex_destroy(&mtx1); // no-warning
+ }
+}
void
bad1(void)
@@ -392,3 +428,46 @@ bad26(void)
pthread_mutex_unlock(&mtx1); // no-warning
pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
}
+
+void bad27(void) {
+ pthread_mutex_unlock(&mtx1); // no-warning
+ int ret = pthread_mutex_destroy(&mtx1); // no-warning
+ if (ret != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ else
+ pthread_mutex_unlock(&mtx1); // expected-warning{{This lock has already been destroyed}}
+}
+
+void bad28(void) {
+ pthread_mutex_unlock(&mtx1); // no-warning
+ int ret = pthread_mutex_destroy(&mtx1); // no-warning
+ if (ret != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ else
+ pthread_mutex_lock(&mtx1); // expected-warning{{This lock has already been destroyed}}
+}
+
+void bad29(void) {
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
+ else
+ pthread_mutex_init(&mtx1, NULL); // no-warning
+}
+
+void bad30(void) {
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
+ pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
+ else
+ pthread_mutex_destroy(&mtx1); // expected-warning{{This lock has already been destroyed}}
+}
+
+void bad31(void) {
+ int ret = pthread_mutex_destroy(&mtx1); // no-warning
+ pthread_mutex_lock(&mtx1); // expected-warning{{This lock has already been destroyed}}
+ if (ret != 0)
+ pthread_mutex_lock(&mtx1);
+}
More information about the cfe-commits
mailing list