[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

diff --git a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
index 5afa2d16877b7862460262d9b10c9649e50f18b8..921184e757750326a52fa6143d9ef9b701a720b8 100644 (file)
--- 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<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 +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<LockSet>(lockR);
+  lockSucc = lockSucc->remove<UnlockSet>(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<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"));
+  if (state->contains<UnlockSet>(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<LockSet>();
+
+  // 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<LockSet>(LS.getTail());
+  state = state->add<UnlockSet>(lockR);
   C.addTransition(state);
 }
 

diff --git a/test/Analysis/pthreadlock.c b/test/Analysis/pthreadlock.c
index b90477424511453a6005376fa6a4706a919b0933..bc7ef66353f7fdff2f713bc3f69fed9714eb2281 100644 (file)
--- 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}}
+}