Merge pull request #15 from jsinglet/jsinglet/concurrency-2-fork

Concurrency 2

Author: Nikita Kraiouchkine

c/cert/src/rules/CON35-C/DeadlockByLockingInPredefinedOrder.ql

@@ -0,0 +1,23 @@
+/**
+ * @id c/cert/deadlock-by-locking-in-predefined-order
+ * @name CON35-C: Avoid deadlock by locking in a predefined order
+ * @description Circular waits leading to thread deadlocks may be avoided by locking in a predefined
+ *              order.
+ * @kind problem
+ * @precision very-high
+ * @problem.severity error
+ * @tags external/cert/id/con35-c
+ *       correctness
+ *       concurrency
+ *       external/cert/obligation/rule
+ */
+
+import cpp
+import codingstandards.c.cert
+import codingstandards.cpp.rules.preventdeadlockbylockinginpredefinedorder.PreventDeadlockByLockingInPredefinedOrder
+
+class DeadlockByLockingInPredefinedOrderQuery extends PreventDeadlockByLockingInPredefinedOrderSharedQuery {
+  DeadlockByLockingInPredefinedOrderQuery() {
+    this = Concurrency2Package::deadlockByLockingInPredefinedOrderQuery()
+  }
+}

c/cert/src/rules/CON36-C/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.ql

@@ -0,0 +1,23 @@
+/**
+ * @id c/cert/wrap-functions-that-can-spuriously-wake-up-in-loop
+ * @name CON36-C: Wrap functions that can spuriously wake up in a loop
+ * @description Not wrapping functions that can wake up spuriously in a conditioned loop can result
+ *              race conditions.
+ * @kind problem
+ * @precision very-high
+ * @problem.severity error
+ * @tags external/cert/id/con36-c
+ *       correctness
+ *       concurrency
+ *       external/cert/obligation/rule
+ */
+
+import cpp
+import codingstandards.c.cert
+import codingstandards.cpp.rules.wrapspuriousfunctioninloop.WrapSpuriousFunctionInLoop
+
+class WrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery extends WrapSpuriousFunctionInLoopSharedQuery {
+  WrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery() {
+    this = Concurrency2Package::wrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery()
+  }
+}

c/cert/test/rules/CON35-C/DeadlockByLockingInPredefinedOrder.testref

@@ -0,0 +1 @@
+c/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql

c/cert/test/rules/CON36-C/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.testref

@@ -0,0 +1 @@
+c/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql

c/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.expected

@@ -0,0 +1,3 @@
+| test.c:15:5:15:6 | d1 | Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks. | test.c:22:3:22:10 | call to mtx_lock | lock 1 | test.c:23:3:23:10 | call to mtx_lock | lock 2 |
+| test.c:33:5:33:6 | d2 | Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks. | test.c:41:3:41:10 | call to mtx_lock | lock 1 | test.c:45:3:45:10 | call to mtx_lock | lock 2 |
+| test.c:88:5:88:6 | d4 | Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks. | test.c:111:3:111:10 | call to mtx_lock | lock 1 | test.c:112:3:112:10 | call to mtx_lock | lock 2 |

c/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql

@@ -0,0 +1,2 @@
+// GENERATED FILE - DO NOT MODIFY
+import codingstandards.cpp.rules.preventdeadlockbylockinginpredefinedorder.PreventDeadlockByLockingInPredefinedOrder

c/common/test/rules/preventdeadlockbylockinginpredefinedorder/test.c

@@ -0,0 +1,166 @@
+#include <stdlib.h>
+#include <threads.h>
+
+typedef struct {
+  int sum;
+  mtx_t mu;
+} B;
+
+typedef struct {
+  B *from;
+  B *to;
+  int amount;
+} thread_arg;
+
+int d1(void *arg) { // NON_COMPLIANT
+  thread_arg *targ = (thread_arg *)arg;
+
+  B *from = targ->from;
+  B *to = targ->to;
+  int amount = targ->amount;
+
+  mtx_lock(&from->mu);
+  mtx_lock(&to->mu);
+
+  if (from->sum >= amount) {
+    from->sum = from->sum - amount;
+    to->sum = to->sum + amount;
+    return 0;
+  }
+  return -1;
+}
+
+int d2(void *arg) { // NON_COMPLIANT
+
+  thread_arg *targ = (thread_arg *)arg;
+
+  B *from = targ->from;
+  B *to = targ->to;
+  int amount = targ->amount;
+
+  mtx_lock(&from->mu);
+  if (from->sum < amount) {
+    return -1;
+  }
+  mtx_lock(&to->mu);
+  from->sum = (from->sum - amount);
+  to->sum = (to->sum + amount);
+
+  return 0;
+}
+
+int getA() { return 0; }
+int getARand() { return rand(); }
+
+int d3(void *arg) { // COMPLIANT
+
+  thread_arg *targ = (thread_arg *)arg;
+
+  B *from = targ->from;
+  B *to = targ->to;
+  int amount = targ->amount;
+
+  mtx_t *one;
+  mtx_t *two;
+
+  int a = getARand();
+
+  // here a may take on multiple different
+  // values and thus different values may flow
+  // into the locks
+  if (a == 9) {
+    one = &from->mu;
+    two = &to->mu;
+  } else {
+    one = &to->mu;
+    two = &from->mu;
+  }
+
+  mtx_lock(one);
+  mtx_lock(two);
+
+  from->sum = (from->sum - amount);
+  to->sum = (to->sum + amount);
+
+  return 0;
+}
+
+int d4(void *arg) { // NON_COMPLIANT
+
+  thread_arg *targ = (thread_arg *)arg;
+
+  B *from = targ->from;
+  B *to = targ->to;
+  int amount = targ->amount;
+
+  mtx_t *one;
+  mtx_t *two;
+  int a = getARand();
+
+  // here a may take on multiple different
+  // values and thus different values may flow
+  // into the locks
+  if (a == 9) {
+    one = &from->mu;
+    two = &to->mu;
+  } else {
+    one = &to->mu;
+    two = &from->mu;
+  }
+
+  mtx_lock(&from->mu);
+  mtx_lock(&to->mu);
+
+  from->sum = (from->sum - amount);
+  to->sum = (to->sum + amount);
+
+  return 0;
+}
+
+void f(B *ba1, B *ba2) {
+  thrd_t t1, t2;
+
+  // unsafe - but used for testing
+  thread_arg a1 = {.from = ba1, .to = ba2, .amount = 100};
+
+  thread_arg a2 = {.from = ba2, .to = ba1, .amount = 100};
+
+  thrd_create(&t1, d1, &a1);
+  thrd_create(&t2, d1, &a2);
+}
+
+void f2(B *ba1, B *ba2) {
+  thrd_t t1, t2;
+
+  // unsafe - but used for testing
+  thread_arg a1 = {.from = ba1, .to = ba2, .amount = 100};
+
+  thread_arg a2 = {.from = ba2, .to = ba1, .amount = 100};
+
+  thrd_create(&t1, d2, &a1);
+  thrd_create(&t2, d2, &a2);
+}
+
+void f3(B *ba1, B *ba2) {
+  thrd_t t1, t2;
+
+  // unsafe - but used for testing
+  thread_arg a1 = {.from = ba1, .to = ba2, .amount = 100};
+
+  thread_arg a2 = {.from = ba2, .to = ba1, .amount = 100};
+
+  thrd_create(&t1, d3, &a1);
+  thrd_create(&t2, d3, &a2);
+}
+
+void f4(B *ba1, B *ba2) {
+  thrd_t t1, t2;
+
+  // unsafe - but used for testing
+  thread_arg a1 = {.from = ba1, .to = ba2, .amount = 100};
+
+  thread_arg a2 = {.from = ba2, .to = ba1, .amount = 100};
+
+  thrd_create(&t1, d4, &a1);
+  thrd_create(&t2, d4, &a2);
+}

c/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.expected

@@ -0,0 +1,3 @@
+| test.c:11:5:11:12 | call to cnd_wait | Use of a function that may wake up spuriously without a controlling loop. |
+| test.c:49:3:49:10 | call to cnd_wait | Use of a function that may wake up spuriously without a controlling loop. |
+| test.c:59:5:59:12 | call to cnd_wait | Use of a function that may wake up spuriously without a controlling loop. |

c/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql

@@ -0,0 +1,2 @@
+// GENERATED FILE - DO NOT MODIFY
+import codingstandards.cpp.rules.wrapspuriousfunctioninloop.WrapSpuriousFunctionInLoop

c/common/test/rules/wrapspuriousfunctioninloop/test.c

@@ -0,0 +1,62 @@
+#include <stddef.h>
+#include <threads.h>
+
+static mtx_t lk;
+static cnd_t cnd;
+
+void f1() {
+  mtx_lock(&lk);
+
+  if (1) {
+    cnd_wait(&cnd, &lk); // NON_COMPLIANT
+  }
+}
+
+void f2() {
+  mtx_lock(&lk);
+  int i = 2;
+  while (i > 0) {
+    cnd_wait(&cnd, &lk); // COMPLIANT
+    i--;
+  }
+}
+
+void f3() {
+  mtx_lock(&lk);
+  int i = 2;
+  do {
+    cnd_wait(&cnd, &lk); // COMPLIANT
+    i--;
+  } while (i > 0);
+}
+
+void f4() {
+  mtx_lock(&lk);
+
+  for (;;) {
+    cnd_wait(&cnd, &lk); // COMPLIANT
+  }
+}
+
+void f5() {
+  mtx_lock(&lk);
+
+  int i = 2;
+  while (i > 0) {
+    i--;
+  }
+
+  cnd_wait(&cnd, &lk); // NON_COMPLIANT
+}
+
+void f6() {
+  mtx_lock(&lk);
+
+  for (int i = 0; i < 10; i++) {
+  }
+  int i = 0;
+  if (i > 0) {
+    cnd_wait(&cnd, &lk); // NON_COMPLIANT
+    i--;
+  }
+}

change_notes/2022-07-13-improvements-to-lock-detection.md

@@ -0,0 +1,6 @@
+- `CON53-CPP` - `DeadlockByLockingInPredefinedOrder.ql`
+  - Optimized performance and expanded coverage to include cases where locking
+    order is not serialized 
+- `CON52-CPP` - `PreventBitFieldAccessFromMultipleThreads.ql`
+  - Fixed an issue with RAII-style locks and scope causing locks to not be
+    correctly identified. 

cpp/cert/src/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.ql

@@ -14,67 +14,10 @@
 
 import cpp
 import codingstandards.cpp.cert
-import codingstandards.cpp.Concurrency
+import codingstandards.cpp.rules.preventdeadlockbylockinginpredefinedorder.PreventDeadlockByLockingInPredefinedOrder
 
-/**
- * Gets a pair of locks guarding a `LockProtectedControlFlowNode` in an order
- * specified by the locking function's call site.
- */
-pragma[inline]
-predicate getAnOrderedLockPair(
-  FunctionCall lock1, FunctionCall lock2, LockProtectedControlFlowNode node
-) {
-  lock1 = node.coveredByLock() and
-  lock2 = node.coveredByLock() and
-  not lock1 = lock2 and
-  lock1.getFile() = lock2.getFile() and
-  exists(Location l1Loc, Location l2Loc |
-    l1Loc = lock1.getLocation() and
-    l2Loc = lock2.getLocation()
-  |
-    l1Loc.getEndLine() < l2Loc.getStartLine()
-    or
-    l1Loc.getStartLine() = l2Loc.getEndLine() and
-    l1Loc.getEndColumn() < l2Loc.getStartColumn()
-  )
+class DeadlockByLockingInPredefinedOrderQuery extends PreventDeadlockByLockingInPredefinedOrderSharedQuery {
+  DeadlockByLockingInPredefinedOrderQuery() {
+    this = ConcurrencyPackage::deadlockByLockingInPredefinedOrderQuery()
+  }
 }
-
-/*
- * There are two ways to safely avoid deadlock. One involves doing the locking
- * in a specific order that is guaranteed to be the same across all thread
- * invocations. This is especially hard to check and thus we adopt an
- * alternative viewpoint wherein we view a "safe" usage of multiple locks to be
- * one that uses the built in `std::lock` functionality which avoids this
- * problem.
- *
- * To properly deadlock, a thread must have at least two different locks (i.e.,
- * mutual exclusion) which are used in an order that causes a problem.  Thus we
- * look for functions with CFNs wherein there may be two locks active at the
- * same time that are invoked from a thread.
- */
-
-from LockProtectedControlFlowNode node, Function f, FunctionCall lock1, FunctionCall lock2
-where
-  not isExcluded(node, ConcurrencyPackage::deadlockByLockingInPredefinedOrderQuery()) and
-  // we can get into trouble when we get into a situation where there may be two
-  // locks in the same threaded function active at the same time.
-  // simple ordering
-  // lock1 = node.coveredByLock() and
-  // lock2 = node.coveredByLock() and
-  getAnOrderedLockPair(lock1, lock2, node) and
-  // To reduce the noise (and increase usefulness) we alert the user at the
-  // level of the function, which is the unit the synchronization should be
-  // performed.
-  f = node.getEnclosingStmt().getEnclosingFunction() and
-  // Because `std::lock` isn't included in our definition of a 'lock'
-  // it is not necessary to check to see if it is in fact what is protecting
-  // these CNFs.
-  // However, to reduce noise, we shall require that the function we are
-  // reporting makes some sort of locking call since this is likely where the
-  // user intends to perform the locking operations. Our implementation will
-  // currently look into all of these nodes which is less helpful for the user
-  // but useful for our analysis.
-  any(LockingOperation l).getEnclosingFunction() = f
-select f,
-  "Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks.",
-  lock1, "lock 1", lock2, "lock 2"

cpp/cert/src/rules/CON54-CPP/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.ql

@@ -14,10 +14,10 @@
 
 import cpp
 import codingstandards.cpp.cert
-import codingstandards.cpp.Concurrency
+import codingstandards.cpp.rules.wrapspuriousfunctioninloop.WrapSpuriousFunctionInLoop
 
-from ConditionalWait cw
-where
-  not isExcluded(cw, ConcurrencyPackage::wrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery()) and
-  not cw.getEnclosingStmt().getParentStmt*() instanceof Loop
-select cw, "Use of a function that may wake up spuriously without a controlling loop."
+class WrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery extends WrapSpuriousFunctionInLoopSharedQuery {
+  WrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery() {
+    this = ConcurrencyPackage::wrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery()
+  }
+}