Hi Jordan,
This patch makes the pthread checker detect double unlocks. I have
copied the idea with a map from the stream checker, not sure if that is
the best approach?
Tracking the state will later make it possible to warn if you lock a
destroyed mutex, calls pthread_init twice etc. But I haven't had time
for that yet.
Best regards,
Daniel Fahlgren
Index: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp (revision 203839)
+++ lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp (working copy)
@@ -24,8 +24,29 @@
using namespace ento;
namespace {
+
+struct LockState {
+ enum Kind { Locked, Unlocked } K;
+
+ LockState(Kind K) : K(K) {}
+ static LockState getLocked(void) { return LockState(Locked); }
+ static LockState getUnlocked(void) { return LockState(Unlocked); }
+
+ bool operator==(const LockState &X) const {
+ return K == X.K;
+ }
+
+ bool isLocked() const { return K == Locked; }
+ bool isUnlocked() const { return K == Unlocked; }
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ ID.AddInteger(K);
+ }
+};
+
class PthreadLockChecker : public Checker< check::PostStmt<CallExpr> > {
mutable std::unique_ptr<BugType> BT_doublelock;
+ mutable std::unique_ptr<BugType> BT_doubleunlock;
mutable std::unique_ptr<BugType> BT_lor;
enum LockingSemantics {
NotApplicable = 0,
@@ -45,6 +66,7 @@
// GDM Entry for tracking lock state.
REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
+REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)
void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
CheckerContext &C) const {
@@ -144,6 +166,7 @@
// Record that the lock was acquired.
lockSucc = lockSucc->add<LockSet>(lockR);
+ lockSucc = lockSucc->set<LockMap>(lockR, LockState::getLocked());
C.addTransition(lockSucc);
}
@@ -155,32 +178,49 @@
return;
ProgramStateRef state = C.getState();
+
+ if (const struct LockState *LState = state->get<LockMap>(lockR)) {
+ if (LState->isUnlocked()) {
+ if (!BT_doubleunlock)
+ BT_doubleunlock.reset(new BugType(this, "Double unlocking",
+ "Lock checker"));
+ ExplodedNode *N = C.generateSink();
+ if (!N)
+ return;
+ BugReport *Report = new BugReport(*BT_doubleunlock,
+ "This lock has already "
+ "been unlocked", N);
+ Report->addRange(CE->getArg(0)->getSourceRange());
+ C.emitReport(Report);
+ return;
+ }
+ }
+
LockSetTy LS = state->get<LockSet>();
// FIXME: Better analysis requires IPA for wrappers.
- // FIXME: check for double unlocks
- if (LS.isEmpty())
- return;
-
- const MemRegion *firstLockR = LS.getHead();
- if (firstLockR != lockR) {
- if (!BT_lor)
- BT_lor.reset(new BugType(this, "Lock order reversal", "Lock checker"));
- ExplodedNode *N = C.generateSink();
- if (!N)
+
+ if (!LS.isEmpty()) {
+ if (LS.getHead() != lockR) {
+ if (!BT_lor)
+ BT_lor.reset(new BugType(this, "Lock order reversal", "Lock checker"));
+ ExplodedNode *N = C.generateSink();
+ if (!N)
+ return;
+ BugReport *Report = new BugReport(*BT_lor,
+ "This was not the most "
+ "recently acquired lock. "
+ "Possible lock order "
+ "reversal", N);
+ Report->addRange(CE->getArg(0)->getSourceRange());
+ C.emitReport(Report);
return;
- BugReport *report = new BugReport(*BT_lor,
- "This was not the most "
- "recently acquired lock. "
- "Possible lock order "
- "reversal", N);
- report->addRange(CE->getArg(0)->getSourceRange());
- C.emitReport(report);
- return;
+ }
+ // Record that the lock was released.
+ state = state->set<LockSet>(LS.getTail());
}
- // Record that the lock was released.
- state = state->set<LockSet>(LS.getTail());
+ state = state->set<LockMap>(lockR, LockState::getUnlocked());
C.addTransition(state);
}
Index: test/Analysis/pthreadlock.c
===================================================================
--- test/Analysis/pthreadlock.c (revision 203839)
+++ test/Analysis/pthreadlock.c (working copy)
@@ -69,6 +69,31 @@
}
void
+ok8(void)
+{
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_lock(&mtx2); // no-warning
+ pthread_mutex_unlock(&mtx2); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+}
+
+void
+ok9(void)
+{
+ pthread_mutex_unlock(&mtx1); // no-warning
+ if (pthread_mutex_trylock(&mtx1) == 0) // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+}
+
+void
+ok10(void)
+{
+ if (pthread_mutex_trylock(&mtx1) != 0) // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+}
+
+void
bad1(void)
{
pthread_mutex_lock(&mtx1); // no-warning
@@ -135,3 +160,74 @@
lck_mtx_lock(&lck2); // no-warning
lck_mtx_unlock(&lck1); // expected-warning{{This was not the most recently acquired lock}}
}
+
+void
+bad9(void)
+{
+ lck_mtx_unlock(&lck1); // no-warning
+ lck_mtx_unlock(&lck1); // expected-warning{{This lock has already been unlocked}}
+}
+
+void
+bad10(void)
+{
+ lck_mtx_lock(&lck1); // no-warning
+ lck_mtx_unlock(&lck1); // no-warning
+ lck_mtx_unlock(&lck1); // expected-warning{{This lock has already been unlocked}}
+}
+
+static void
+bad11_sub(pthread_mutex_t *lock)
+{
+ lck_mtx_unlock(lock); // expected-warning{{This lock has already been unlocked}}
+}
+
+void
+bad11(int i)
+{
+ lck_mtx_lock(&lck1); // no-warning
+ lck_mtx_unlock(&lck1); // no-warning
+ if (i < 5)
+ bad11_sub(&lck1);
+}
+
+void
+bad12(void)
+{
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // expected-warning{{This lock has already been unlocked}}
+}
+
+void
+bad13(void)
+{
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ pthread_mutex_lock(&mtx2); // no-warning
+ pthread_mutex_unlock(&mtx2); // no-warning
+ pthread_mutex_unlock(&mtx1); // expected-warning{{This lock has already been unlocked}}
+}
+
+void
+bad14(void)
+{
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_lock(&mtx2); // no-warning
+ pthread_mutex_unlock(&mtx2); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx2); // expected-warning{{This lock has already been unlocked}}
+}
+
+void
+bad15(void)
+{
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_lock(&mtx2); // no-warning
+ pthread_mutex_unlock(&mtx2); // no-warning
+ pthread_mutex_unlock(&mtx1); // no-warning
+ pthread_mutex_lock(&mtx1); // no-warning
+ pthread_mutex_unlock(&mtx2); // expected-warning{{This lock has already been unlocked}}
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
