checkpoint

Author: jsinglet

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

@@ -20,27 +20,28 @@ import semmle.code.cpp.dataflow.TaintTracking
 
 /*
  * This query finds potential misuse of mutexes passed to threads by considering
- * cases where the underlying mutex is a stack variable; such a variable would
+ * cases where the underlying mutex is a local variable; such a variable would
  * go out of scope at the end of the calling function and thus would potentially
- * create issues for the thread depending on the mutex.
+ * create issues for the thread depending on the mutex. This query is primarily 
+ * targeted at C usages since in the case of CPP, many more cases can be covered
+ * via tracking of destructors. The main difference is that this query doesn't 
+ * expect an explicitly deleted call to be made. 
  *
- * It is of course possible to safely use such a pattern. A safe usage of this
- * pattern one would perform some sort of waiting or looping or control after
- * passing such a mutex in. For this reason we perform a broad analysis that
- * looks for waiting-like behavior following the call to `std::thread`. Since
- * there are a wide variety of correct behaviors that may be called we take the
- * very broad view that calling the `join` method subsequent to creating the
- * thread is enough to classify as "correct behavior."
+ * In order to safely destroy a dependent mutex, it is necessary both to not delete 
+ * it, but also if deletes do happen, one must wait for a thread to exit prior to 
+ * deleting it. We broadly model this by using standard language support for thread
+ * synchronization. 
  */
+from ThreadDependentMutex dm, LocalVariable lv 
+where 
+  not isExcluded(dm.asExpr(), ConcurrencyPackage::doNotDestroyAMutexWhileItIsLockedQuery()) and
+  not isExcluded(lv, ConcurrencyPackage::doNotDestroyAMutexWhileItIsLockedQuery()) and
+  not lv.isStatic() and 
+  TaintTracking::localTaint(dm.getAUsage(), DataFlow::exprNode(lv.getAnAssignedValue())) 
+  // ensure that each dependent thread is followed by some sort of joining
+  // behavior. 
+  and exists(DataFlow::Node n | n = dm.getADependentThreadCreationExpr()  | forall(ThreadWait tw |
+    not (tw = n.asExpr().getASuccessor*())
+  ))
 
-from MutexDependentThreadConstructor cc, StackVariable sv
-where
-  not isExcluded(cc, ConcurrencyPackage::doNotAllowAMutexToGoOutOfScopeWhileLockedQuery()) and
-  not isExcluded(sv, ConcurrencyPackage::doNotAllowAMutexToGoOutOfScopeWhileLockedQuery()) and
-  // find cases where stack local variable has flowed into a thread mutex argument
-  DataFlow::localFlow(DataFlow::exprNode(sv.getAnAccess()), DataFlow::exprNode(cc.dependentMutex())) and
-  // exempt cases where some "waiting like" behavior is detected
-  not exists(ThreadWait tw |
-    TaintTracking::localTaint(DataFlow::exprNode(cc), DataFlow::exprNode(tw.getQualifier()))
-  )
-select cc, "Mutex used by thread potentially destroyed while in use."
+  select dm, "Mutex used by thread potentially destroyed while in use."

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

@@ -19,28 +19,35 @@ 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 deleted explicitly. The scope of this
- * query is that it considers this behavior locally within the procedure. We do
- * this by looking for cases where the mutex is the target of a delete within
- * the same procedure following the creation of the thread.
+ * cases where the underlying mutex may be destroyed. The scope of this query is 
+ * that it performs this analysis both locally within the function but can also
+ * look through to the called thread to identify mutexes it may not own. 
+ * query is that it considers this behavior locally within the procedure. 
  *
- * It is of course possible to safely use such a pattern. A safe usage of this
- * pattern one would perform some sort of waiting or looping or control after
- * passing such a mutex in. For this reason we perform a broad analysis that
- * looks for waiting-like behavior following the call to `std::thread`. Since
- * there are a wide variety of correct behaviors that may be called we take the
- * very broad view that calling the `join` method subsequent to creating the
- * thread is enough to classify as "correct behavior."
+ * In order to safely destroy a dependent mutex, it is necessary both to not delete 
+ * it, but also if deletes do happen, one must wait for a thread to exit prior to 
+ * deleting it. We broadly model this by using standard language support for thread
+ * synchronization. 
  */
-
-from MutexDependentThreadConstructor cc, DeleteExpr deleteExpr, VariableAccess va
-where
-  not isExcluded(cc, ConcurrencyPackage::doNotDestroyAMutexWhileItIsLockedQuery()) and
-  deleteExpr = cc.getASuccessor*() and
-  DataFlow::localFlow(DataFlow::exprNode(va), DataFlow::exprNode(cc.dependentMutex())) and
-  deleteExpr.getExpr() = va.getTarget().getAnAccess() and
-  // exempt cases where some "waiting like" behavior is detected
-  not exists(ThreadWait tw |
-    TaintTracking::localTaint(DataFlow::exprNode(cc), DataFlow::exprNode(tw.getQualifier()))
+from ThreadDependentMutex dm, MutexDestroyer md
+where 
+  not isExcluded(dm.asExpr(), ConcurrencyPackage::doNotDestroyAMutexWhileItIsLockedQuery()) and
+  not isExcluded(md, ConcurrencyPackage::doNotDestroyAMutexWhileItIsLockedQuery()) and
+  // find all instances where a usage of a dependent mutex flows into a
+  // expression that will destroy it. 
+  TaintTracking::localTaint(dm.getAUsage(), DataFlow::exprNode(md.getMutexExpr()))
+  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.
+    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
+    // was passed into a function. 
+    not md.getMutexExpr().getEnclosingFunction() = dm.asExpr().getEnclosingFunction() or
+    // this leaves only destructions of mutexes locally near the thread that may
+    // consume them. We allow this only if there has been some effort to
+    // synchronize the threads prior to destroying the mutex.
+    not exists(ThreadWait tw | tw = md.getAPredecessor*())
   )
-select cc, "Mutex used by thread potentially deleted while in use."
+select dm, "Mutex used by thread potentially $@ while in use.", md, "deleted"

cpp/cert/src/rules/CON50-CPP/MutexDependentThreadConstructor.qll

@@ -0,0 +1,41 @@
+#include <stdatomic.h>
+#include <stddef.h>
+#include <threads.h>
+  
+mtx_t mx;
+ 
+int t1(void *dummy) {
+  mtx_lock(&mx);
+  mtx_unlock(&mx);
+  return 0;
+}
+ 
+int f1() {
+  thrd_t threads[5];
+   
+  mtx_init(&mx, mtx_plain);
+  
+  for (size_t i = 0; i < 5; i++) {
+    thrd_create(&threads[i], t1, NULL);
+    
+  }
+  for (size_t i = 0; i < 5; i++) {
+    thrd_join(threads[i], 0);
+  }
+ 
+  mtx_destroy(&mx);
+  return 0;
+}
+
+int f2() {
+  thrd_t threads[5];
+   
+  mtx_init(&mx, mtx_plain);
+  
+  for (size_t i = 0; i < 5; i++) {
+    thrd_create(&threads[i], t1, NULL);   
+  }
+
+  mtx_destroy(&mx);
+  return 0;
+}

cpp/cert/src/rules/CON50-CPP/ThreadWait.qll

@@ -3,6 +3,8 @@
 
 void t1(int i, std::mutex *pm) {}
 void t2(int i, std::mutex **pm) {}
+void t3(int i, std::mutex *pm) { delete pm; }
+
 void f1() {
   std::thread threads[5];
   std::mutex m1;
@@ -44,7 +46,13 @@ void f4() {
   std::thread threads[5];
 
   for (int i = 0; i < 5; ++i) {
-    threads[i] = std::thread(t1, i, m3); // NON_COMPLIANT
+    threads[i] = std::thread(t1, i, m3); // COMPLIANT
+  }
+
+  // since we wait here, and the local function created the
+  // mutex, the following delete is allowed.
+  for (int i = 0; i < 5; ++i) {
+    threads[i].join();
   }
 
   delete m3;
@@ -74,4 +82,30 @@ void f6() {
   }
 
   delete m3;
-}
+}
+
+void f7() {
+  m3 = new std::mutex();
+
+  std::thread threads[5];
+
+  for (int i = 0; i < 5; ++i) {
+    threads[i] = std::thread(
+        t3, i, m3); // NON_COMPLIANT - t3 will attempt to delete the mutex.
+  }
+
+  for (int i = 0; i < 5; ++i) {
+    threads[i].join();
+  }
+}
+
+void f8() {
+  std::mutex *m = new std::mutex();
+  delete m; // COMPLIANT
+}
+
+void f9() {
+  std::mutex m; // COMPLIANT
+}
+
+std::mutex *m4 = new std::mutex();

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

@@ -48,7 +48,7 @@ class ThreadConstructorCall extends ConstructorCall, ThreadCreationFunction {
 }
 
 /**
- * Models a call to a thread constructor via `std::thread`.
+ * Models a call to a thread constructor via `thrd_create`.
  */
 class C11ThreadCreateCall extends ThreadCreationFunction {
   Function f;
@@ -458,13 +458,139 @@ class MutexDependentThreadConstructor extends ThreadConstructorCall {
 }
 
 /**
- * Models a call to a a `std::thread` join.
+ * Models thread waiting functions.
  */
-class ThreadWait extends FunctionCall {
+abstract class ThreadWait extends FunctionCall { }
+
+/**
+ * Models a call to a `std::thread` join.
+ */
+class CPPThreadWait extends ThreadWait {
   VariableAccess var;
 
-  ThreadWait() {
+  CPPThreadWait() {
     getTarget().(MemberFunction).getDeclaringType().hasQualifiedName("std", "thread") and
     getTarget().getName() = "join"
   }
 }
+
+/**
+ * Models a call to `thread_join` in C11.
+ */
+class C11ThreadWait extends FunctionCall {
+  VariableAccess var;
+
+  C11ThreadWait() { getTarget().getName() = "thread_join" }
+}
+
+abstract class MutexSource extends FunctionCall { }
+
+/**
+ * Models a C++ style mutex.
+ */
+class CPPMutexSource extends MutexSource, ConstructorCall {
+  CPPMutexSource() { getTarget().getDeclaringType().hasQualifiedName("std", "mutex") }
+}
+
+/**
+ * Models a C11 style mutex.
+ */
+class C11MutexSource extends MutexSource, FunctionCall {
+  C11MutexSource() { getTarget().hasName("mtx_init") }
+}
+
+/**
+ * Models a thread dependent mutex. A thread dependent mutex is a mutex
+ * that is used by a thread.
+ */
+class ThreadDependentMutex extends DataFlow::Node {
+  DataFlow::Node sink;
+
+  ThreadDependentMutex() {
+    exists(ThreadDependentMutexTaintTrackingConfiguration config | config.hasFlow(this, sink))
+  }
+
+  DataFlow::Node getASource() {
+    // the source is either the thing that declared
+    // the mutex
+    result = this
+    or
+    // or the thread we are using it in
+    result = getAThreadSource()
+  }
+
+  /**
+   * Gets the dataflow nodes corresponding to thread local usages of the
+   * dependent mutex.
+   */
+  DataFlow::Node getAThreadSource() {
+    exists(FunctionCall fc, Function f, int n |
+      fc.getArgument(n) = sink.asExpr() and
+      f = fc.getArgument(0).(FunctionAccess).getTarget() and
+      result = DataFlow::exprNode(f.getParameter(n - 1).getAnAccess())
+    )
+  }
+
+  /**
+   * Produces the set of dataflow nodes to thread creation for threads
+   * that are dependent on this mutex.
+   */
+  DataFlow::Node getADependentThreadCreationExpr() {
+    exists(FunctionCall fc |
+      fc.getAnArgument() = sink.asExpr() and
+      result = DataFlow::exprNode(fc)
+    )
+  }
+
+  /**
+   * Gets a set of usages of this mutex in both the local and thread scope.
+   */
+  DataFlow::Node getAUsage() { TaintTracking::localTaint(getASource(), result) }
+}
+
+class ThreadDependentMutexTaintTrackingConfiguration extends TaintTracking::Configuration {
+  ThreadDependentMutexTaintTrackingConfiguration() {
+    this = "ThreadDependentMutexTaintTrackingConfiguration"
+  }
+
+  override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof MutexSource }
+
+  override predicate isSink(DataFlow::Node node) {
+    exists(ThreadCreationFunction f | f.getAnArgument() = node.asExpr())
+  }
+}
+
+/**
+ * Models expressions that destroy mutexes.
+ */
+abstract class MutexDestroyer extends Expr {
+  /**
+   * Gets the expression that references the mutex being destroyed.
+   */
+  abstract Expr getMutexExpr();
+}
+
+/**
+ * Models C style mutex destruction via `mtx_destroy`.
+ */
+class C11MutexDestroyer extends MutexDestroyer, FunctionCall {
+  C11MutexDestroyer() { getTarget().getName() = "mtx_destroy" }
+
+  /**
+   * Returns the `Expr` being destroyed.
+   */
+  override Expr getMutexExpr() { result = getArgument(0) }
+}
+
+/**
+ * Models implicit or explicit calls to the destructor of a mutex, either via
+ * a `delete` statement or a variable going out of scope.
+ */
+class DestructorMutexDestroyer extends MutexDestroyer, DestructorCall {
+  DestructorMutexDestroyer() { getTarget().getDeclaringType().hasQualifiedName("std", "mutex") }
+
+  /**
+   * Returns the `Expr` being deleted.
+   */
+  override Expr getMutexExpr() { this.getQualifier() = result }
+}