changes for making query more robust

Author: jsinglet

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

@@ -15,6 +15,7 @@
 import cpp
 import codingstandards.cpp.cert
 import codingstandards.cpp.Concurrency
+import semmle.code.cpp.controlflow.Dominance
 
 /**
  * Gets a pair of locks guarding a `LockProtectedControlFlowNode` in an order
@@ -27,7 +28,8 @@ predicate getAnOrderedLockPair(
   lock1 = node.coveredByLock() and
   lock2 = node.coveredByLock() and
   not lock1 = lock2 and
-  lock1.getFile() = lock2.getFile() and
+  lock1.getEnclosingFunction() = lock2.getEnclosingFunction() and
+  node.(Expr).getEnclosingFunction() = lock1.getEnclosingFunction() and
   exists(Location l1Loc, Location l2Loc |
     l1Loc = lock1.getLocation() and
     l2Loc = lock2.getLocation()
@@ -53,15 +55,30 @@ predicate getAnOrderedLockPair(
  * same time that are invoked from a thread.
  */
 
+predicate isUnSerializedLock(LockingOperation lock) {
+  exists(VariableAccess va |
+    va = lock.getArgument(0).getAChild().(VariableAccess) and
+    not exists(Assignment assn |
+      assn = va.getTarget().getAnAssignment() and
+      not bbDominates(assn.getBasicBlock(), lock.getBasicBlock())
+    )
+  )
+}
+
+predicate isSerializedLock(LockingOperation lock) { not isUnSerializedLock(lock) }
+
 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
+  // simple ordering is applied here for presentation purposes.
   getAnOrderedLockPair(lock1, lock2, node) and
+  // it is difficult to determine if the ordering applied to the locks is "safe"
+  // so here we simply look to see that there exists at least one other program
+  // path that would yield different argument values to the lock functions
+  // perhaps arising from some logic that applies an ordering to the locking.
+  not (isSerializedLock(lock1) and isSerializedLock(lock2)) 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.

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

@@ -1,4 +1,5 @@
 #include <mutex>
+#include <stdlib.h>
 #include <thread>
 
 class B {
@@ -54,9 +55,64 @@ int d3(B *from, B *to, int amount) { // NON_COMPLIANT
   return 0;
 }
 
+int getA() { return 0; }
+int getARand() { return rand(); }
+
+int d4(B *from, B *to, int amount) { // COMPLIANT
+  std::mutex *one;
+  std::mutex *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;
+  }
+
+  std::lock_guard<std::mutex> from_lock(*one);
+  std::lock_guard<std::mutex> to_lock(*two);
+
+  from->set(from->get() - amount);
+  to->set(to->get() + amount);
+
+  return 0;
+}
+
+int d5(B *from, B *to, int amount) { // NON_COMPLIANT
+  std::mutex *one;
+  std::mutex *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;
+  }
+
+  std::lock_guard<std::mutex> from_lock(from->mu);
+  std::lock_guard<std::mutex> to_lock(to->mu);
+
+  from->set(from->get() - amount);
+  to->set(to->get() + amount);
+
+  return 0;
+}
+
 void f(B *ba1, B *ba2) {
   std::thread thr1(d1, ba1, ba2, 100);
-  std::thread thr2(d2, ba2, ba1, 100);
+  std::thread thr2(d1, ba2, ba1, 100);
   thr1.join();
   thr2.join();
 }
@@ -73,4 +129,18 @@ void f3(B *ba1, B *ba2) {
   std::thread thr2(d3, ba1, ba2, 100);
   thr1.join();
   thr2.join();
-}
+}
+
+void f4(B *ba1, B *ba2) {
+  std::thread thr1(d4, ba1, ba2, 100);
+  std::thread thr2(d4, ba1, ba2, 100);
+  thr1.join();
+  thr2.join();
+}
+
+void f5(B *ba1, B *ba2) {
+  std::thread thr1(d5, ba1, ba2, 100);
+  std::thread thr2(d5, ba1, ba2, 100);
+  thr1.join();
+  thr2.join();
+}

cpp/common/src/codingstandards/cpp/Concurrency.qll

@@ -318,7 +318,7 @@ class RAIIStyleLock extends LockingOperation {
    */
   override predicate isLock() {
     this instanceof ConstructorCall and
-    lock = getArgument(0) and
+    lock = getArgument(0).getAChild() and
     // defer_locks don't cause a lock
     not exists(Expr exp |
       exp = getArgument(1) and