From 7677cf0aec72862d7ce8629ab1b059ead0757830 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Tue, 1 Apr 2014 03:40:38 +0000 Subject: [PATCH] [analyzer] Add double-unlock detection to PthreadLockChecker. We've decided to punt on supporting recursive locks for now; the common case is non-recursive. Patch by Daniel Fahlgren! git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@205274 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/PthreadLockChecker.cpp | 53 ++++++---- test/Analysis/pthreadlock.c | 96 +++++++++++++++++++ 2 files changed, 132 insertions(+), 17 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index 5afa2d1687..921184e757 100644 --- a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -26,6 +26,7 @@ using namespace ento; namespace { class PthreadLockChecker : public Checker< check::PostStmt > { mutable std::unique_ptr BT_doublelock; + mutable std::unique_ptr BT_doubleunlock; mutable std::unique_ptr BT_lor; enum LockingSemantics { NotApplicable = 0, @@ -45,6 +46,7 @@ public: // GDM Entry for tracking lock state. REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *) +REGISTER_SET_WITH_PROGRAMSTATE(UnlockSet, const MemRegion *) void PthreadLockChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { @@ -144,6 +146,7 @@ void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE, // Record that the lock was acquired. lockSucc = lockSucc->add(lockR); + lockSucc = lockSucc->remove(lockR); C.addTransition(lockSucc); } @@ -155,32 +158,48 @@ void PthreadLockChecker::ReleaseLock(CheckerContext &C, const CallExpr *CE, return; ProgramStateRef state = C.getState(); - LockSetTy LS = state->get(); - // 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")); + if (state->contains(lockR)) { + 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_lor, - "This was not the most " - "recently acquired lock. " - "Possible lock order " - "reversal", N); - report->addRange(CE->getArg(0)->getSourceRange()); - C.emitReport(report); + 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(); + + // FIXME: Better analysis requires IPA for wrappers. + + if (!LS.isEmpty()) { + 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) + 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(LS.getTail()); + state = state->add(lockR); C.addTransition(state); } diff --git a/test/Analysis/pthreadlock.c b/test/Analysis/pthreadlock.c index b904774245..bc7ef66353 100644 --- a/test/Analysis/pthreadlock.c +++ b/test/Analysis/pthreadlock.c @@ -68,6 +68,31 @@ ok7(void) lck_mtx_unlock(&lck1); // no-warning } +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) { @@ -135,3 +160,74 @@ bad8(void) 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}} +} -- 2.40.0