work

Author: jsinglet

cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql

@@ -17,6 +17,7 @@ import codingstandards.cpp.Concurrency
 import semmle.code.cpp.dataflow.DataFlow
 import semmle.code.cpp.dataflow.TaintTracking
 
+
 /*
  * This query finds potential misuse of mutexes passed to threads by considering
  * cases where the underlying mutex may be destroyed. The scope of this query is 
@@ -39,7 +40,12 @@ where
   and 
   (
     // firstly, we assume it is never safe to destroy a global mutex, but it is
-    // difficult to make assumptions about the intended control flow.
+    // difficult to make assumptions about the intended control flow. Note that
+    // this means the point at where the mutex is defined -- not where the variable
+    // that contains it is scoped -- a `ThreadDependentMutex` is bound to the
+    // function that creates an initialized mutex. For example, in `C`
+    // `mtx_init` is called to initialize the mutex and in C++, the constructor 
+    // of std::mutex is called.     
     not exists(dm.asExpr().getEnclosingFunction()) or 
     // secondly, we assume it is never safe to destroy a mutex created by
     // another function scope -- which includes trying to destroy a mutex that
@@ -50,4 +56,4 @@ where
     // synchronize the threads prior to destroying the mutex.
     not exists(ThreadWait tw | tw = md.getAPredecessor*())
   )
-select dm, "Mutex used by thread potentially $@ while in use.", md, "deleted"
+select dm, "Mutex used by thread potentially $@ while in use.", md, "destroyed"

cpp/cert/test/rules/CON50-CPP/test.cpp

@@ -1,9 +1,15 @@
 #include <mutex>
 #include <thread>
 
+std::mutex *m4 = new std::mutex(); // NON_COMPLIANT
+std::mutex m5;                     // COMPLIANT
+std::mutex *m6 = new std::mutex(); // COMPLIANT
+
 void t1(int i, std::mutex *pm) {}
 void t2(int i, std::mutex **pm) {}
 void t3(int i, std::mutex *pm) { delete pm; }
+void t4(int i) { delete m4; }
+void t5(int i) { std::lock_guard<std::mutex> lk(m5); }
 
 void f1() {
   std::thread threads[5];
@@ -16,7 +22,7 @@ void f1() {
 
 void f2() {
   std::thread threads[5];
-  std::mutex m1; // COMPLIANT - due to check below
+  std::mutex m1; // COMPLIANT
 
   for (int i = 0; i < 5; ++i) {
     threads[i] = std::thread(t1, i, &m1);
@@ -27,7 +33,8 @@ void f2() {
   }
 }
 
-std::mutex m2; // COMPLIANT - since m2 will not go out of scope.
+std::mutex m2; // COMPLIANT - m2 is not deleted and never goes out of scope.
+               // There is no delete
 
 void f3() {
   std::thread threads[5];
@@ -106,4 +113,34 @@ void f9() {
   std::mutex m; // COMPLIANT
 }
 
-std::mutex *m4 = new std::mutex(); // COMPLIANT
+// f10 does not wait but it is OK since m5 is global and doesn't go out of
+// scope -- the destructor isn't called until the program exists.
+void f10() {
+  std::thread threads[5];
+
+  for (int i = 0; i < 5; ++i) {
+    threads[i] = std::thread(t5, i);
+  }
+}
+
+// f11 represents an invalid usage of the global mutex `m4` since it attempts to
+// delete it from within the thread.
+void f11() {
+  std::thread threads[5];
+
+  for (int i = 0; i < 5; ++i) {
+    threads[i] = std::thread(t4, i);
+  }
+}
+
+// f12 represents a valid but tricky usage of the global mutex `m6` since it is
+// not deleted/accessed from the thread
+void f12() {
+  std::thread threads[5];
+
+  for (int i = 0; i < 5; ++i) {
+    threads[i] = std::thread(t4, i);
+  }
+
+  delete m6;
+}