diff --git a/c/cert/src/rules/CON35-C/DeadlockByLockingInPredefinedOrder.ql b/c/cert/src/rules/CON35-C/DeadlockByLockingInPredefinedOrder.ql new file mode 100644 index 0000000000..00fa021878 --- /dev/null +++ b/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() + } +} diff --git a/c/cert/src/rules/CON36-C/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.ql b/c/cert/src/rules/CON36-C/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.ql new file mode 100644 index 0000000000..430a0e7c19 --- /dev/null +++ b/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() + } +} diff --git a/c/cert/test/rules/CON35-C/DeadlockByLockingInPredefinedOrder.testref b/c/cert/test/rules/CON35-C/DeadlockByLockingInPredefinedOrder.testref new file mode 100644 index 0000000000..4625d1a24d --- /dev/null +++ b/c/cert/test/rules/CON35-C/DeadlockByLockingInPredefinedOrder.testref @@ -0,0 +1 @@ +c/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql \ No newline at end of file diff --git a/c/cert/test/rules/CON36-C/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.testref b/c/cert/test/rules/CON36-C/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.testref new file mode 100644 index 0000000000..562c8a0f1b --- /dev/null +++ b/c/cert/test/rules/CON36-C/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.testref @@ -0,0 +1 @@ +c/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql \ No newline at end of file diff --git a/c/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.expected b/c/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.expected new file mode 100644 index 0000000000..2d0b60ab9b --- /dev/null +++ b/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 | diff --git a/c/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql b/c/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql new file mode 100644 index 0000000000..6412db389a --- /dev/null +++ b/c/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.preventdeadlockbylockinginpredefinedorder.PreventDeadlockByLockingInPredefinedOrder diff --git a/c/common/test/rules/preventdeadlockbylockinginpredefinedorder/test.c b/c/common/test/rules/preventdeadlockbylockinginpredefinedorder/test.c new file mode 100644 index 0000000000..6d8d25755b --- /dev/null +++ b/c/common/test/rules/preventdeadlockbylockinginpredefinedorder/test.c @@ -0,0 +1,166 @@ +#include +#include + +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); +} diff --git a/c/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.expected b/c/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.expected new file mode 100644 index 0000000000..c38811788f --- /dev/null +++ b/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. | diff --git a/c/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql b/c/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql new file mode 100644 index 0000000000..6f4ad4c40e --- /dev/null +++ b/c/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.wrapspuriousfunctioninloop.WrapSpuriousFunctionInLoop diff --git a/c/common/test/rules/wrapspuriousfunctioninloop/test.c b/c/common/test/rules/wrapspuriousfunctioninloop/test.c new file mode 100644 index 0000000000..31299be82e --- /dev/null +++ b/c/common/test/rules/wrapspuriousfunctioninloop/test.c @@ -0,0 +1,62 @@ +#include +#include + +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--; + } +} diff --git a/change_notes/2022-07-13-improvements-to-lock-detection.md b/change_notes/2022-07-13-improvements-to-lock-detection.md new file mode 100644 index 0000000000..bf07c25778 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/cpp/cert/src/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.ql b/cpp/cert/src/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.ql index e8eb79351b..3989464f70 100644 --- a/cpp/cert/src/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.ql +++ b/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" diff --git a/cpp/cert/src/rules/CON54-CPP/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.ql b/cpp/cert/src/rules/CON54-CPP/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.ql index 25d5123815..5584b7bec2 100644 --- a/cpp/cert/src/rules/CON54-CPP/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.ql +++ b/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() + } +} diff --git a/cpp/cert/test/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.expected b/cpp/cert/test/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.expected deleted file mode 100644 index f0bf4fcc85..0000000000 --- a/cpp/cert/test/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.expected +++ /dev/null @@ -1,2 +0,0 @@ -| test.cpp:29:5:29:6 | d2 | Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks. | test.cpp:30:41:30:49 | call to lock_guard | lock 1 | test.cpp:35:39:35:45 | call to lock_guard | lock 2 | -| test.cpp:43:5:43:6 | d3 | Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks. | test.cpp:44:41:44:49 | call to lock_guard | lock 1 | test.cpp:49:39:49:45 | call to lock_guard | lock 2 | diff --git a/cpp/cert/test/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.qlref b/cpp/cert/test/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.qlref deleted file mode 100644 index 1263f38964..0000000000 --- a/cpp/cert/test/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.qlref +++ /dev/null @@ -1 +0,0 @@ -rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.ql \ No newline at end of file diff --git a/cpp/cert/test/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.testref b/cpp/cert/test/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.testref new file mode 100644 index 0000000000..7992d86205 --- /dev/null +++ b/cpp/cert/test/rules/CON53-CPP/DeadlockByLockingInPredefinedOrder.testref @@ -0,0 +1 @@ +cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql \ No newline at end of file diff --git a/cpp/cert/test/rules/CON54-CPP/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.testref b/cpp/cert/test/rules/CON54-CPP/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.testref new file mode 100644 index 0000000000..dff5c1b232 --- /dev/null +++ b/cpp/cert/test/rules/CON54-CPP/WrapFunctionsThatCanSpuriouslyWakeUpInLoop.testref @@ -0,0 +1 @@ +cpp/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index a7234d601e..badd7d8fea 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/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 @@ -421,8 +421,13 @@ class LockProtectedControlFlowNode extends ThreadedCFN { /** * Models a function that conditionally waits. */ -class ConditionalWait extends FunctionCall { - ConditionalWait() { +abstract class ConditionalWait extends FunctionCall { } + +/** + * Models a function in CPP that will conditionally wait. + */ +class CPPConditionalWait extends ConditionalWait { + CPPConditionalWait() { exists(MemberFunction mf | mf = getTarget() and mf.getDeclaringType().hasQualifiedName("std", "condition_variable") and @@ -431,6 +436,13 @@ class ConditionalWait extends FunctionCall { } } +/** + * Models a function in C that will conditionally wait. + */ +class CConditionalWait extends ConditionalWait { + CConditionalWait() { getTarget().getName() in ["cnd_wait"] } +} + /** * Models a call to a `std::thread` constructor that depends on a mutex. */ diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency2.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency2.qll new file mode 100644 index 0000000000..7e3bbe10a7 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency2.qll @@ -0,0 +1,42 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype Concurrency2Query = + TDeadlockByLockingInPredefinedOrderQuery() or + TWrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery() + +predicate isConcurrency2QueryMetadata(Query query, string queryId, string ruleId) { + query = + // `Query` instance for the `deadlockByLockingInPredefinedOrder` query + Concurrency2Package::deadlockByLockingInPredefinedOrderQuery() and + queryId = + // `@id` for the `deadlockByLockingInPredefinedOrder` query + "c/cert/deadlock-by-locking-in-predefined-order" and + ruleId = "CON35-C" + or + query = + // `Query` instance for the `wrapFunctionsThatCanSpuriouslyWakeUpInLoop` query + Concurrency2Package::wrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery() and + queryId = + // `@id` for the `wrapFunctionsThatCanSpuriouslyWakeUpInLoop` query + "c/cert/wrap-functions-that-can-spuriously-wake-up-in-loop" and + ruleId = "CON36-C" +} + +module Concurrency2Package { + Query deadlockByLockingInPredefinedOrderQuery() { + //autogenerate `Query` type + result = + // `Query` type for `deadlockByLockingInPredefinedOrder` query + TQueryC(TConcurrency2PackageQuery(TDeadlockByLockingInPredefinedOrderQuery())) + } + + Query wrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery() { + //autogenerate `Query` type + result = + // `Query` type for `wrapFunctionsThatCanSpuriouslyWakeUpInLoop` query + TQueryC(TConcurrency2PackageQuery(TWrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index 24c09a514d..43a33af2e7 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -4,6 +4,7 @@ import codingstandards.cpp.exclusions.RuleMetadata //** Import packages for this language **/ import Banned import Concurrency1 +import Concurrency2 import IO1 import IO2 import IO3 @@ -24,6 +25,7 @@ import Syntax newtype TCQuery = TBannedPackageQuery(BannedQuery q) or TConcurrency1PackageQuery(Concurrency1Query q) or + TConcurrency2PackageQuery(Concurrency2Query q) or TIO1PackageQuery(IO1Query q) or TIO2PackageQuery(IO2Query q) or TIO3PackageQuery(IO3Query q) or @@ -44,6 +46,7 @@ newtype TCQuery = predicate isQueryMetadata(Query query, string queryId, string ruleId) { isBannedQueryMetadata(query, queryId, ruleId) or isConcurrency1QueryMetadata(query, queryId, ruleId) or + isConcurrency2QueryMetadata(query, queryId, ruleId) or isIO1QueryMetadata(query, queryId, ruleId) or isIO2QueryMetadata(query, queryId, ruleId) or isIO3QueryMetadata(query, queryId, ruleId) or diff --git a/cpp/common/src/codingstandards/cpp/rules/guardaccesstobitfields/GuardAccessToBitFields.qll b/cpp/common/src/codingstandards/cpp/rules/guardaccesstobitfields/GuardAccessToBitFields.qll index f07d9d94bd..5b03a4f8bd 100644 --- a/cpp/common/src/codingstandards/cpp/rules/guardaccesstobitfields/GuardAccessToBitFields.qll +++ b/cpp/common/src/codingstandards/cpp/rules/guardaccesstobitfields/GuardAccessToBitFields.qll @@ -42,6 +42,24 @@ ControlFlowNode getAReachableLockCFN(MutexFunctionCall mfc) { query predicate problems(BitFieldAccess ba, string message) { not isExcluded(ba, getQuery()) and ba instanceof ThreadedCFN and - not ba instanceof LockProtectedControlFlowNode and + // to be a valid bit field access there must be + // a RAII-style lock before this access + not exists(RAIIStyleLock lock | + // A lock came before this node + lock = ba.getAPredecessor*() and + lock.isLock() and + // But wasn't followed by an unlock + not exists(RAIIStyleLock unlock | + // That worked on the same underlying lock variable + unlock.isUnlock() and + unlock.getLock() = lock.getLock() and + // such that the unlock came after the lock + unlock.getAPredecessor*() = lock and + // and after before the access + ba.getAPredecessor*() = unlock + ) + ) and + // or the bit field access must be protected by a lock region + not exists(MutexFunctionCall mfc | ba = getAReachableLockCFN(mfc)) and message = "Access to a bit-field without a concurrency guard." } diff --git a/cpp/common/src/codingstandards/cpp/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.qll b/cpp/common/src/codingstandards/cpp/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.qll new file mode 100644 index 0000000000..ca54e3c2d8 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.qll @@ -0,0 +1,99 @@ +/** + * Provides a library which includes a `problems` predicate for locks that do not + * lock in a predefined order. + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import codingstandards.cpp.Concurrency +import semmle.code.cpp.controlflow.Dominance + +abstract class PreventDeadlockByLockingInPredefinedOrderSharedQuery extends Query { } + +Query getQuery() { result instanceof PreventDeadlockByLockingInPredefinedOrderSharedQuery } + +/** + * 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.getEnclosingFunction() = lock2.getEnclosingFunction() and + node.(Expr).getEnclosingFunction() = lock1.getEnclosingFunction() 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() + ) +} + +/* + * 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. + */ + +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) } + +query predicate problems( + Function f, string message, FunctionCall lock1, string lock1String, FunctionCall lock2, + string lock2String +) { + exists(LockProtectedControlFlowNode node | + not isExcluded(node, getQuery()) 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 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. + 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 and + lock1String = "lock 1" and + lock2String = "lock 2" + ) and + message = + "Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks." +} diff --git a/cpp/common/src/codingstandards/cpp/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.qll b/cpp/common/src/codingstandards/cpp/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.qll new file mode 100644 index 0000000000..99bdbeee5d --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.qll @@ -0,0 +1,19 @@ +/** + * Provides a library which includes a `problems` predicate for reporting + * functions that should be wrapped in a loop because they may wake up spuriously. + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import codingstandards.cpp.Concurrency + +abstract class WrapSpuriousFunctionInLoopSharedQuery extends Query { } + +Query getQuery() { result instanceof WrapSpuriousFunctionInLoopSharedQuery } + +query predicate problems(ConditionalWait cw, string message) { + not isExcluded(cw, getQuery()) and + not cw.getEnclosingStmt().getParentStmt*() instanceof Loop and + message = "Use of a function that may wake up spuriously without a controlling loop." +} diff --git a/cpp/common/test/rules/guardaccesstobitfields/GuardAccessToBitFields.expected b/cpp/common/test/rules/guardaccesstobitfields/GuardAccessToBitFields.expected index 3f45e93d06..dd317d0778 100644 --- a/cpp/common/test/rules/guardaccesstobitfields/GuardAccessToBitFields.expected +++ b/cpp/common/test/rules/guardaccesstobitfields/GuardAccessToBitFields.expected @@ -1,3 +1,3 @@ | test.cpp:67:7:67:8 | f2 | Access to a bit-field without a concurrency guard. | | test.cpp:91:7:91:8 | f2 | Access to a bit-field without a concurrency guard. | -| test.cpp:97:7:97:8 | f2 | Access to a bit-field without a concurrency guard. | +| test.cpp:102:7:102:8 | f2 | Access to a bit-field without a concurrency guard. | diff --git a/cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.expected b/cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.expected new file mode 100644 index 0000000000..3fc6c0b277 --- /dev/null +++ b/cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.expected @@ -0,0 +1,3 @@ +| test.cpp:30:5:30:6 | d2 | Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks. | test.cpp:31:41:31:49 | call to lock_guard | lock 1 | test.cpp:36:39:36:45 | call to lock_guard | lock 2 | +| test.cpp:44:5:44:6 | d3 | Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks. | test.cpp:45:41:45:49 | call to lock_guard | lock 1 | test.cpp:50:39:50:45 | call to lock_guard | lock 2 | +| test.cpp:86:5:86:6 | d5 | Threaded function may be called from a context that uses $@ and $@ which may lead to deadlocks. | test.cpp:103:41:103:49 | call to lock_guard | lock 1 | test.cpp:104:39:104:45 | call to lock_guard | lock 2 | diff --git a/cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql b/cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql new file mode 100644 index 0000000000..6412db389a --- /dev/null +++ b/cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.preventdeadlockbylockinginpredefinedorder.PreventDeadlockByLockingInPredefinedOrder diff --git a/cpp/cert/test/rules/CON53-CPP/test.cpp b/cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/test.cpp similarity index 52% rename from cpp/cert/test/rules/CON53-CPP/test.cpp rename to cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/test.cpp index af1e6dfdc7..52aaf912b7 100644 --- a/cpp/cert/test/rules/CON53-CPP/test.cpp +++ b/cpp/common/test/rules/preventdeadlockbylockinginpredefinedorder/test.cpp @@ -1,4 +1,5 @@ #include +#include #include class B { @@ -54,9 +55,63 @@ int d3(B *from, B *to, int amount) { // NON_COMPLIANT return 0; } +int getA() { return 0; } + +int d4(B *from, B *to, int amount) { // COMPLIANT + std::mutex *one; + std::mutex *two; + + int a = getA(); + + // 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 from_lock(*one); + std::lock_guard 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 = getA(); + + // 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 from_lock(from->mu); + std::lock_guard 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 +128,18 @@ void f3(B *ba1, B *ba2) { std::thread thr2(d3, ba1, ba2, 100); thr1.join(); thr2.join(); -} \ No newline at end of file +} + +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(); +} diff --git a/cpp/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.expected b/cpp/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.expected new file mode 100644 index 0000000000..218612ef21 --- /dev/null +++ b/cpp/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.expected @@ -0,0 +1,3 @@ +| test.cpp:11:8:11:11 | call to wait | Use of a function that may wake up spuriously without a controlling loop. | +| test.cpp:49:6:49:9 | call to wait | Use of a function that may wake up spuriously without a controlling loop. | +| test.cpp:59:8:59:11 | call to wait | Use of a function that may wake up spuriously without a controlling loop. | diff --git a/cpp/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql b/cpp/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql new file mode 100644 index 0000000000..6f4ad4c40e --- /dev/null +++ b/cpp/common/test/rules/wrapspuriousfunctioninloop/WrapSpuriousFunctionInLoop.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.wrapspuriousfunctioninloop.WrapSpuriousFunctionInLoop diff --git a/cpp/common/test/rules/wrapspuriousfunctioninloop/test.cpp b/cpp/common/test/rules/wrapspuriousfunctioninloop/test.cpp new file mode 100644 index 0000000000..29c5a217d0 --- /dev/null +++ b/cpp/common/test/rules/wrapspuriousfunctioninloop/test.cpp @@ -0,0 +1,62 @@ +#include +#include + +static std::mutex mu; +static std::condition_variable cv; + +void f1() { + std::unique_lock lk(mu); + + if (1) { + cv.wait(lk); // NON_COMPLIANT + } +} + +void f2() { + std::unique_lock lk(mu); + int i = 2; + while (i > 0) { + cv.wait(lk); // COMPLIANT + i--; + } +} + +void f3() { + std::unique_lock lk(mu); + int i = 2; + do { + cv.wait(lk); // COMPLIANT + i--; + } while (i > 0); +} + +void f4() { + std::unique_lock lk(mu); + + for (;;) { + cv.wait(lk); // COMPLIANT + } +} + +void f5() { + std::unique_lock lk(mu); + + int i = 2; + while (i > 0) { + i--; + } + + cv.wait(lk); // NON_COMPLIANT +} + +void f6() { + std::unique_lock lk(mu); + + for (int i = 0; i < 10; i++) { + } + int i = 0; + if (i > 0) { + cv.wait(lk); // NON_COMPLIANT + i--; + } +} diff --git a/rule_packages/c/Concurrency2.json b/rule_packages/c/Concurrency2.json new file mode 100644 index 0000000000..d9102a07df --- /dev/null +++ b/rule_packages/c/Concurrency2.json @@ -0,0 +1,46 @@ +{ + "CERT-C": { + "CON35-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Circular waits leading to thread deadlocks may be avoided by locking in a predefined order.", + "kind": "problem", + "name": "Avoid deadlock by locking in a predefined order", + "precision": "very-high", + "severity": "error", + "short_name": "DeadlockByLockingInPredefinedOrder", + "shared_implementation_short_name": "PreventDeadlockByLockingInPredefinedOrder", + "tags": [ + "correctness", + "concurrency" + ] + } + ], + "title": "Avoid deadlock by locking in a predefined order" + }, + "CON36-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Not wrapping functions that can wake up spuriously in a conditioned loop can result race conditions.", + "kind": "problem", + "name": "Wrap functions that can spuriously wake up in a loop", + "precision": "very-high", + "severity": "error", + "short_name": "WrapFunctionsThatCanSpuriouslyWakeUpInLoop", + "shared_implementation_short_name": "WrapSpuriousFunctionInLoop", + "tags": [ + "correctness", + "concurrency" + ] + } + ], + "title": "Wrap functions that can spuriously wake up in a loop" + } + } +} \ No newline at end of file diff --git a/rule_packages/cpp/Concurrency.json b/rule_packages/cpp/Concurrency.json index c79c7dc777..80bd09dfd8 100644 --- a/rule_packages/cpp/Concurrency.json +++ b/rule_packages/cpp/Concurrency.json @@ -85,6 +85,7 @@ "precision": "medium", "severity": "error", "short_name": "DeadlockByLockingInPredefinedOrder", + "shared_implementation_short_name": "PreventDeadlockByLockingInPredefinedOrder", "tags": [ "correctness", "concurrency" @@ -105,6 +106,7 @@ "precision": "very-high", "severity": "error", "short_name": "WrapFunctionsThatCanSpuriouslyWakeUpInLoop", + "shared_implementation_short_name": "WrapSpuriousFunctionInLoop", "tags": [ "correctness", "concurrency" diff --git a/rules.csv b/rules.csv index 03c7f9c8cd..60b582015c 100755 --- a/rules.csv +++ b/rules.csv @@ -485,18 +485,18 @@ c,CERT-C,ARR36-C,Yes,Rule,,,Do not subtract or compare two pointers that do not c,CERT-C,ARR37-C,Yes,Rule,,,Do not add or subtract an integer to a pointer to a non-array object,,InvalidMemory,Medium, c,CERT-C,ARR38-C,Yes,Rule,,,Guarantee that library functions do not form invalid pointers,,Pointers2,Very Hard, c,CERT-C,ARR39-C,Yes,Rule,,,Do not add or subtract a scaled integer to a pointer,,Pointers2,Medium, -c,CERT-C,CON30-C,Yes,Rule,,,Clean up thread-specific storage,,Concurrency2,Very Hard, -c,CERT-C,CON31-C,Yes,Rule,,,Do not destroy a mutex while it is locked,CON50-CPP,Concurrency2,Very Hard, +c,CERT-C,CON30-C,Yes,Rule,,,Clean up thread-specific storage,,Concurrency3,Very Hard, +c,CERT-C,CON31-C,Yes,Rule,,,Do not destroy a mutex while it is locked,CON50-CPP,Concurrency3,Very Hard, c,CERT-C,CON32-C,Yes,Rule,,,Prevent data races when accessing bit-fields from multiple threads,,Concurrency1,Easy, c,CERT-C,CON33-C,Yes,Rule,,,Avoid race conditions when using library functions,,Concurrency1,Easy, -c,CERT-C,CON34-C,Yes,Rule,,,Declare objects shared between threads with appropriate storage durations,,Concurrency2,Hard, +c,CERT-C,CON34-C,Yes,Rule,,,Declare objects shared between threads with appropriate storage durations,,Concurrency3,Hard, c,CERT-C,CON35-C,Yes,Rule,,,Avoid deadlock by locking in a predefined order,CON53-CPP,Concurrency2,Medium, c,CERT-C,CON36-C,Yes,Rule,,,Wrap functions that can spuriously wake up in a loop,CON54-CPP,Concurrency2,Medium, c,CERT-C,CON37-C,Yes,Rule,,,Do not call signal() in a multithreaded program,,Concurrency1,Easy, -c,CERT-C,CON38-C,Yes,Rule,,,Preserve thread safety and liveness when using condition variables,CON55-CPP,Concurrency2,Medium, -c,CERT-C,CON39-C,Yes,Rule,,,Do not join or detach a thread that was previously joined or detached,,Concurrency2,Hard, -c,CERT-C,CON40-C,Yes,Rule,,,Do not refer to an atomic variable twice in an expression,,Concurrency2,Medium, -c,CERT-C,CON41-C,Yes,Rule,,,Wrap functions that can fail spuriously in a loop,CON53-CPP,Concurrency2,Medium, +c,CERT-C,CON38-C,Yes,Rule,,,Preserve thread safety and liveness when using condition variables,CON55-CPP,Concurrency3,Medium, +c,CERT-C,CON39-C,Yes,Rule,,,Do not join or detach a thread that was previously joined or detached,,Concurrency3,Hard, +c,CERT-C,CON40-C,Yes,Rule,,,Do not refer to an atomic variable twice in an expression,,Concurrency3,Medium, +c,CERT-C,CON41-C,Yes,Rule,,,Wrap functions that can fail spuriously in a loop,CON53-CPP,Concurrency3,Medium, c,CERT-C,CON43-C,OutOfScope,Rule,,,Do not allow data races in multithreaded code,,,, c,CERT-C,DCL30-C,Yes,Rule,,,Declare objects with appropriate storage durations,,Declarations,Hard, c,CERT-C,DCL31-C,Yes,Rule,,,Declare identifiers before using them,,Declarations,Medium,