From 3646d594371ec2a07d019b616029640363409e3f Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 3 Aug 2022 14:02:28 -0400 Subject: [PATCH 01/42] checkpoint --- ...NotAllowAMutexToGoOutOfScopeWhileLocked.ql | 41 +++--- .../DoNotDestroyAMutexWhileItIsLocked.ql | 51 ++++--- .../MutexDependentThreadConstructor.qll | 16 --- cpp/cert/src/rules/CON50-CPP/ThreadWait.qll | 13 -- cpp/cert/test/rules/CON50-CPP/test.c | 41 ++++++ cpp/cert/test/rules/CON50-CPP/test.cpp | 38 ++++- .../src/codingstandards/cpp/Concurrency.qll | 134 +++++++++++++++++- 7 files changed, 257 insertions(+), 77 deletions(-) delete mode 100644 cpp/cert/src/rules/CON50-CPP/MutexDependentThreadConstructor.qll delete mode 100644 cpp/cert/src/rules/CON50-CPP/ThreadWait.qll create mode 100644 cpp/cert/test/rules/CON50-CPP/test.c diff --git a/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql b/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql index 716c02a06d..f8a399d8c5 100644 --- a/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql +++ b/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." diff --git a/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql b/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql index e1cfc98397..f60b3a5921 100644 --- a/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql +++ b/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" diff --git a/cpp/cert/src/rules/CON50-CPP/MutexDependentThreadConstructor.qll b/cpp/cert/src/rules/CON50-CPP/MutexDependentThreadConstructor.qll deleted file mode 100644 index 6c88775647..0000000000 --- a/cpp/cert/src/rules/CON50-CPP/MutexDependentThreadConstructor.qll +++ /dev/null @@ -1,16 +0,0 @@ -import cpp -import codingstandards.cpp.Concurrency - -/** - * Models a call to a `std::thread` constructor that depends on a mutex. - */ -class MutexDependentThreadConstructor extends ThreadConstructorCall { - Expr mutexExpr; - - MutexDependentThreadConstructor() { - mutexExpr = getAnArgument() and - mutexExpr.getUnderlyingType().stripType() instanceof MutexType - } - - Expr dependentMutex() { result = mutexExpr } -} diff --git a/cpp/cert/src/rules/CON50-CPP/ThreadWait.qll b/cpp/cert/src/rules/CON50-CPP/ThreadWait.qll deleted file mode 100644 index 7a18842acc..0000000000 --- a/cpp/cert/src/rules/CON50-CPP/ThreadWait.qll +++ /dev/null @@ -1,13 +0,0 @@ -import cpp - -/** - * Models a call to a a `std::thread` join. - */ -class ThreadWait extends FunctionCall { - VariableAccess var; - - ThreadWait() { - getTarget().(MemberFunction).getDeclaringType().hasQualifiedName("std", "thread") and - getTarget().getName() = "join" - } -} diff --git a/cpp/cert/test/rules/CON50-CPP/test.c b/cpp/cert/test/rules/CON50-CPP/test.c new file mode 100644 index 0000000000..7770c8b2c7 --- /dev/null +++ b/cpp/cert/test/rules/CON50-CPP/test.c @@ -0,0 +1,41 @@ +#include +#include +#include + +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; +} \ No newline at end of file diff --git a/cpp/cert/test/rules/CON50-CPP/test.cpp b/cpp/cert/test/rules/CON50-CPP/test.cpp index 241d0b08c2..fd737c76f9 100644 --- a/cpp/cert/test/rules/CON50-CPP/test.cpp +++ b/cpp/cert/test/rules/CON50-CPP/test.cpp @@ -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; -} \ No newline at end of file +} + +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(); diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index badd7d8fea..d524c3e714 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -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 } +} From f73ee762f7b469cdf7f12253df94fe6056eb5b0d Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Mon, 8 Aug 2022 14:11:59 -0400 Subject: [PATCH 02/42] fix up test case --- cpp/cert/test/rules/CON50-CPP/test.cpp | 36 ++++++++++++-------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/cpp/cert/test/rules/CON50-CPP/test.cpp b/cpp/cert/test/rules/CON50-CPP/test.cpp index fd737c76f9..3c2f4bf6ad 100644 --- a/cpp/cert/test/rules/CON50-CPP/test.cpp +++ b/cpp/cert/test/rules/CON50-CPP/test.cpp @@ -7,19 +7,19 @@ void t3(int i, std::mutex *pm) { delete pm; } void f1() { std::thread threads[5]; - std::mutex m1; + std::mutex m1; // NON_COMPLIANT for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t1, i, &m1); // NON_COMPLIANT + threads[i] = std::thread(t1, i, &m1); } } void f2() { std::thread threads[5]; - std::mutex m1; + std::mutex m1; // COMPLIANT - due to check below for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t1, i, &m1); // COMPLIANT - due to check below + threads[i] = std::thread(t1, i, &m1); } for (int i = 0; i < 5; ++i) { @@ -27,26 +27,25 @@ void f2() { } } -std::mutex m2; +std::mutex m2; // COMPLIANT - since m2 will not go out of scope. void f3() { std::thread threads[5]; for (int i = 0; i < 5; ++i) { - threads[i] = std::thread( - t1, i, &m2); // COMPLIANT - since m2 will not go out of scope. + threads[i] = std::thread(t1, i, &m2); } } std::mutex *m3; void f4() { - m3 = new std::mutex(); + m3 = new std::mutex(); // COMPLIANT std::thread threads[5]; for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t1, i, m3); // COMPLIANT + threads[i] = std::thread(t1, i, m3); } // since we wait here, and the local function created the @@ -59,22 +58,22 @@ void f4() { } void f5() { - m3 = new std::mutex(); + m3 = new std::mutex(); // COMPLIANT std::thread threads[5]; for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t2, i, &m3); // COMPLIANT + threads[i] = std::thread(t2, i, &m3); } } void f6() { - m3 = new std::mutex(); + m3 = new std::mutex(); // COMPLIANT std::thread threads[5]; for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t1, i, m3); // COMPLIANT + threads[i] = std::thread(t1, i, m3); } for (int i = 0; i < 5; ++i) { @@ -85,13 +84,12 @@ void f6() { } void f7() { - m3 = new std::mutex(); + m3 = new std::mutex(); // NON_COMPLIANT - t3 will attempt to delete the 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. + threads[i] = std::thread(t3, i, m3); } for (int i = 0; i < 5; ++i) { @@ -100,12 +98,12 @@ void f7() { } void f8() { - std::mutex *m = new std::mutex(); - delete m; // COMPLIANT + std::mutex *m = new std::mutex(); // COMPLIANT + delete m; } void f9() { std::mutex m; // COMPLIANT } -std::mutex *m4 = new std::mutex(); +std::mutex *m4 = new std::mutex(); // COMPLIANT From eca76091fd29b361853dc754de0be5608fbb76ee Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Mon, 8 Aug 2022 16:37:02 -0400 Subject: [PATCH 03/42] checkpoint --- .../src/codingstandards/cpp/Concurrency.qll | 58 +++++++++++++++++-- 1 file changed, 53 insertions(+), 5 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index d524c3e714..6f9b338f50 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -501,13 +501,42 @@ class C11MutexSource extends MutexSource, FunctionCall { /** * Models a thread dependent mutex. A thread dependent mutex is a mutex - * that is used by a thread. + * that is used by a thread. This dependency is established either by directly + * passing in a mutex or by referencing a mutex that is in the local scope. The utility + * of this class is it captures the `DataFlow::Node` source at which the mutex + * came from. For example, if it is passed in from a local function to a thread. + * This functionality is critical, since it allows one to inspect how the thread + * behaves with respect to the owner of a resource. */ -class ThreadDependentMutex extends DataFlow::Node { +abstract class ThreadDependentMutex extends DataFlow::Node { + DataFlow::Node sink; + +} +class FlowBasedThreadDependentMutex extends ThreadDependentMutex { DataFlow::Node sink; ThreadDependentMutex() { - exists(ThreadDependentMutexTaintTrackingConfiguration config | config.hasFlow(this, sink)) + + // this predicate captures two cases. The first being some sort of dataflow, + // likely through parameter passing. + exists(ThreadDependentMutexTaintTrackingConfiguration config | config.hasFlow(this, sink)) or + + // the second encapsulates usages from outside scopes not directly expressed + // in dataflow. + exists(MutexSource mutexSrc, ThreadedFunction f, Variable variableSource | + DataFlow::exprNode(mutexSrc) = this and + + // find a variable that was assigned the mutex + TaintTracking::localTaint(DataFlow::exprNode(mutexSrc), DataFlow::exprNode(variableSource.getAnAssignedValue())) and + + // find all subsequent accesses of that variable that are within a + // function and set those to the sink + exists(VariableAccess va | + va = variableSource.getAnAccess() and + va.getEnclosingFunction() = f + and sink = DataFlow::exprNode(va) + ) + ) } DataFlow::Node getASource() { @@ -524,10 +553,23 @@ class ThreadDependentMutex extends DataFlow::Node { * dependent mutex. */ DataFlow::Node getAThreadSource() { + // here we line up the actual parameter at the thread creation + // site with the formal parameter in the target thread. + // Note that there are differences between the C and C++ versions + // of the argument ordering in the thread creation function. However, + // since the C version only takes one parameter (as opposed to multiple) + // we can simplify this search by considering only the first argument. exists(FunctionCall fc, Function f, int n | + // Get the argument to which the mutex flowed. fc.getArgument(n) = sink.asExpr() and + // Get the thread function we are calling. f = fc.getArgument(0).(FunctionAccess).getTarget() and - result = DataFlow::exprNode(f.getParameter(n - 1).getAnAccess()) + // in C++, there is an extra argument to the `std::thread` call + // so we must subtract 1 since this is not passed to the thread. + (result = DataFlow::exprNode(f.getParameter(n - 1).getAnAccess()) or + // In C, only one argument is allowed. Thus IF the flow predicate holds, + // it will be to the first argument + result = DataFlow::exprNode(f.getParameter(0).getAnAccess())) ) } @@ -543,11 +585,17 @@ class ThreadDependentMutex extends DataFlow::Node { } /** - * Gets a set of usages of this mutex in both the local and thread scope. + * Gets a set of usages of this mutex in both the local and thread scope. + * In the case of scoped usage, this also captures typical accesses of variables. */ DataFlow::Node getAUsage() { TaintTracking::localTaint(getASource(), result) } } +class AccessBasedThreadDependentMutex extends ThreadDependentMutex { + +} + + class ThreadDependentMutexTaintTrackingConfiguration extends TaintTracking::Configuration { ThreadDependentMutexTaintTrackingConfiguration() { this = "ThreadDependentMutexTaintTrackingConfiguration" From 16f46f2f512e319505d64ebec2ecd72c97a975e1 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Tue, 9 Aug 2022 09:33:18 -0400 Subject: [PATCH 04/42] adding usage patterns for C++/C --- .../src/codingstandards/cpp/Concurrency.qll | 188 +++++++++++++++--- 1 file changed, 156 insertions(+), 32 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index 6f9b338f50..b711673d7b 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -475,12 +475,12 @@ class CPPThreadWait extends ThreadWait { } /** - * Models a call to `thread_join` in C11. + * Models a call to `thrd_join` in C11. */ -class C11ThreadWait extends FunctionCall { +class C11ThreadWait extends ThreadWait { VariableAccess var; - C11ThreadWait() { getTarget().getName() = "thread_join" } + C11ThreadWait() { getTarget().getName() = "thrd_join" } } abstract class MutexSource extends FunctionCall { } @@ -507,38 +507,14 @@ class C11MutexSource extends MutexSource, FunctionCall { * came from. For example, if it is passed in from a local function to a thread. * This functionality is critical, since it allows one to inspect how the thread * behaves with respect to the owner of a resource. + * + * To model the myriad ways this can happen, the subclasses of this class are + * responsible for implementing the various usage patterns. */ abstract class ThreadDependentMutex extends DataFlow::Node { + DataFlow::Node sink; -} -class FlowBasedThreadDependentMutex extends ThreadDependentMutex { - DataFlow::Node sink; - - ThreadDependentMutex() { - - // this predicate captures two cases. The first being some sort of dataflow, - // likely through parameter passing. - exists(ThreadDependentMutexTaintTrackingConfiguration config | config.hasFlow(this, sink)) or - - // the second encapsulates usages from outside scopes not directly expressed - // in dataflow. - exists(MutexSource mutexSrc, ThreadedFunction f, Variable variableSource | - DataFlow::exprNode(mutexSrc) = this and - - // find a variable that was assigned the mutex - TaintTracking::localTaint(DataFlow::exprNode(mutexSrc), DataFlow::exprNode(variableSource.getAnAssignedValue())) and - - // find all subsequent accesses of that variable that are within a - // function and set those to the sink - exists(VariableAccess va | - va = variableSource.getAnAccess() and - va.getEnclosingFunction() = f - and sink = DataFlow::exprNode(va) - ) - ) - } - DataFlow::Node getASource() { // the source is either the thing that declared // the mutex @@ -584,24 +560,172 @@ class FlowBasedThreadDependentMutex extends ThreadDependentMutex { ) } + /** * Gets a set of usages of this mutex in both the local and thread scope. * In the case of scoped usage, this also captures typical accesses of variables. */ DataFlow::Node getAUsage() { TaintTracking::localTaint(getASource(), result) } + } +/** + * This class models the type of thread/mutex dependency that is established + * through the typical parameter passing mechanisms found in C++. + */ +class FlowBasedThreadDependentMutex extends ThreadDependentMutex { + FlowBasedThreadDependentMutex() { + // some sort of dataflow, likely through parameter passing. + exists(ThreadDependentMutexTaintTrackingConfiguration config | config.hasFlow(this, sink)) + } +} + + +/** + * This class models the type of thread/mutex dependency that is established by + * either scope based accesses (e.g., global variables) or block scope differences. + */ class AccessBasedThreadDependentMutex extends ThreadDependentMutex { + Variable variableSource; + + AccessBasedThreadDependentMutex(){ + // encapsulates usages from outside scopes not directly expressed + // in dataflow. + exists(MutexSource mutexSrc, ThreadedFunction f | + DataFlow::exprNode(mutexSrc) = this and + + // find a variable that was assigned the mutex + TaintTracking::localTaint(DataFlow::exprNode(mutexSrc), DataFlow::exprNode(variableSource.getAnAssignedValue())) and + + // find all subsequent accesses of that variable that are within a + // function and set those to the sink + exists(VariableAccess va | + va = variableSource.getAnAccess() and + va.getEnclosingFunction() = f + and sink = DataFlow::exprNode(va) + ) + ) + } + + override DataFlow::Node getAUsage() { DataFlow::exprNode(variableSource.getAnAccess()) = result } +} + + + +/** + * In the typical C thread model, a mutex is a created by a function that is not responsible + * for creating the variable. Thus this class encodes a slightly different semantics + * wherein the usage pattern is that of variables that have been both initialized + * and then subsequently passed into a thread directly. + */ +class DeclarationInitBasedThreadDependentMutex extends ThreadDependentMutex { + + Variable variableSource; + + DeclarationInitBasedThreadDependentMutex(){ + + exists(MutexSource ms, ThreadCreationFunction tcf | + this = DataFlow::exprNode(ms) and + // accessed as a mutex source + TaintTracking::localTaint(DataFlow::exprNode(variableSource.getAnAccess()), DataFlow::exprNode(ms.getAnArgument())) and + // subsequently passed to a thread creation function (order not strictly + // enforced for performance reasons) + sink = DataFlow::exprNode(tcf.getAnArgument()) and + TaintTracking::localTaint(DataFlow::exprNode(variableSource.getAnAccess()), sink) + ) + } + + override DataFlow::Node getAUsage() { TaintTracking::localTaint(getASource(), result) or + DataFlow::exprNode(variableSource.getAnAccess()) = result + } + + override 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() + } + + DataFlow::Node getSink(){ + result = sink + } + + /** + * Gets the dataflow nodes corresponding to thread local usages of the + * dependent mutex. + */ + override DataFlow::Node getAThreadSource() { + // here we line up the actual parameter at the thread creation + // site with the formal parameter in the target thread. + // Note that there are differences between the C and C++ versions + // of the argument ordering in the thread creation function. However, + // since the C version only takes one parameter (as opposed to multiple) + // we can simplify this search by considering only the first argument. + + + exists(FunctionCall fc, Function f, int n | // CPP Version + fc.getArgument(n) = sink.asExpr() and + f = fc.getArgument(0).(FunctionAccess).getTarget() and + // in C++, there is an extra argument to the `std::thread` call + // so we must subtract 1 since this is not passed to the thread. + result = DataFlow::exprNode(f.getParameter(n - 1).getAnAccess()) + ) or + exists(FunctionCall fc, Function f | // C Version + fc.getAnArgument() = sink.asExpr() and + // in C, the second argument is the function + f = fc.getArgument(1).(FunctionAccess).getTarget() and + // in C, the passed argument is always the zeroth argument + result = DataFlow::exprNode(f.getParameter(0).getAnAccess()) + ) + + } + +} + + +/** + * In the typical C model, another way to use mutexes is to work with global variables + * that can be initialized at various points -- one of which must be inside a thread. + * This class encapsulates this pattern. + */ +class DeclarationInitAccessBasedThreadDependentMutex extends ThreadDependentMutex { + + Variable variableSource; + + DeclarationInitAccessBasedThreadDependentMutex(){ + + exists(MutexSource ms, ThreadedFunction tf, VariableAccess va | + this = DataFlow::exprNode(ms) and + // accessed as a mutex source + TaintTracking::localTaint(DataFlow::exprNode(variableSource.getAnAccess()), DataFlow::exprNode(ms.getAnArgument())) and + // is accessed somewhere else + va = variableSource.getAnAccess() and + sink = DataFlow::exprNode(va) and + // one of which must be a thread + va.getEnclosingFunction() = tf + ) + } + + override DataFlow::Node getAUsage() { + result = DataFlow::exprNode(variableSource.getAnAccess()) + } } + + + class ThreadDependentMutexTaintTrackingConfiguration extends TaintTracking::Configuration { ThreadDependentMutexTaintTrackingConfiguration() { this = "ThreadDependentMutexTaintTrackingConfiguration" } - override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof MutexSource } + override predicate isSource(DataFlow::Node node) { + node.asExpr() instanceof MutexSource + } override predicate isSink(DataFlow::Node node) { exists(ThreadCreationFunction f | f.getAnArgument() = node.asExpr()) From e82f522b4d66f305084a237430199423dfe3be95 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Tue, 9 Aug 2022 19:18:00 -0400 Subject: [PATCH 05/42] work --- .../DoNotDestroyAMutexWhileItIsLocked.ql | 10 +- cpp/cert/test/rules/CON50-CPP/test.cpp | 43 +++- .../src/codingstandards/cpp/Concurrency.qll | 206 ++++++++++-------- 3 files changed, 160 insertions(+), 99 deletions(-) diff --git a/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql b/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql index f60b3a5921..65983395ad 100644 --- a/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql +++ b/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" diff --git a/cpp/cert/test/rules/CON50-CPP/test.cpp b/cpp/cert/test/rules/CON50-CPP/test.cpp index 3c2f4bf6ad..f069315ebe 100644 --- a/cpp/cert/test/rules/CON50-CPP/test.cpp +++ b/cpp/cert/test/rules/CON50-CPP/test.cpp @@ -1,9 +1,15 @@ #include #include +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 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; +} diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index b711673d7b..fe5833f7c7 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -501,18 +501,17 @@ class C11MutexSource extends MutexSource, FunctionCall { /** * Models a thread dependent mutex. A thread dependent mutex is a mutex - * that is used by a thread. This dependency is established either by directly - * passing in a mutex or by referencing a mutex that is in the local scope. The utility + * that is used by a thread. This dependency is established either by directly + * passing in a mutex or by referencing a mutex that is in the local scope. The utility * of this class is it captures the `DataFlow::Node` source at which the mutex - * came from. For example, if it is passed in from a local function to a thread. - * This functionality is critical, since it allows one to inspect how the thread - * behaves with respect to the owner of a resource. - * - * To model the myriad ways this can happen, the subclasses of this class are - * responsible for implementing the various usage patterns. + * came from. For example, if it is passed in from a local function to a thread. + * This functionality is critical, since it allows one to inspect how the thread + * behaves with respect to the owner of a resource. + * + * To model the myriad ways this can happen, the subclasses of this class are + * responsible for implementing the various usage patterns. */ abstract class ThreadDependentMutex extends DataFlow::Node { - DataFlow::Node sink; DataFlow::Node getASource() { @@ -529,23 +528,26 @@ abstract class ThreadDependentMutex extends DataFlow::Node { * dependent mutex. */ DataFlow::Node getAThreadSource() { - // here we line up the actual parameter at the thread creation - // site with the formal parameter in the target thread. + // here we line up the actual parameter at the thread creation + // site with the formal parameter in the target thread. // Note that there are differences between the C and C++ versions - // of the argument ordering in the thread creation function. However, + // of the argument ordering in the thread creation function. However, // since the C version only takes one parameter (as opposed to multiple) // we can simplify this search by considering only the first argument. exists(FunctionCall fc, Function f, int n | - // Get the argument to which the mutex flowed. + // Get the argument to which the mutex flowed. fc.getArgument(n) = sink.asExpr() and // Get the thread function we are calling. f = fc.getArgument(0).(FunctionAccess).getTarget() and - // in C++, there is an extra argument to the `std::thread` call + // in C++, there is an extra argument to the `std::thread` call // so we must subtract 1 since this is not passed to the thread. - (result = DataFlow::exprNode(f.getParameter(n - 1).getAnAccess()) or - // In C, only one argument is allowed. Thus IF the flow predicate holds, - // it will be to the first argument - result = DataFlow::exprNode(f.getParameter(0).getAnAccess())) + ( + result = DataFlow::exprNode(f.getParameter(n - 1).getAnAccess()) + or + // In C, only one argument is allowed. Thus IF the flow predicate holds, + // it will be to the first argument + result = DataFlow::exprNode(f.getParameter(0).getAnAccess()) + ) ) } @@ -560,13 +562,11 @@ abstract class ThreadDependentMutex extends DataFlow::Node { ) } - /** - * Gets a set of usages of this mutex in both the local and thread scope. - * In the case of scoped usage, this also captures typical accesses of variables. + * Gets a set of usages of this mutex in both the local and thread scope. + * In the case of scoped usage, this also captures typical accesses of variables. */ DataFlow::Node getAUsage() { TaintTracking::localTaint(getASource(), result) } - } /** @@ -576,67 +576,62 @@ abstract class ThreadDependentMutex extends DataFlow::Node { class FlowBasedThreadDependentMutex extends ThreadDependentMutex { FlowBasedThreadDependentMutex() { // some sort of dataflow, likely through parameter passing. - exists(ThreadDependentMutexTaintTrackingConfiguration config | config.hasFlow(this, sink)) + exists(ThreadDependentMutexTaintTrackingConfiguration config | config.hasFlow(this, sink)) } } - /** - * This class models the type of thread/mutex dependency that is established by + * This class models the type of thread/mutex dependency that is established by * either scope based accesses (e.g., global variables) or block scope differences. */ class AccessBasedThreadDependentMutex extends ThreadDependentMutex { Variable variableSource; - AccessBasedThreadDependentMutex(){ + AccessBasedThreadDependentMutex() { // encapsulates usages from outside scopes not directly expressed // in dataflow. - exists(MutexSource mutexSrc, ThreadedFunction f | - DataFlow::exprNode(mutexSrc) = this and - + exists(MutexSource mutexSrc, ThreadedFunction f | + DataFlow::exprNode(mutexSrc) = this and // find a variable that was assigned the mutex - TaintTracking::localTaint(DataFlow::exprNode(mutexSrc), DataFlow::exprNode(variableSource.getAnAssignedValue())) and - + TaintTracking::localTaint(DataFlow::exprNode(mutexSrc), + DataFlow::exprNode(variableSource.getAnAssignedValue())) and // find all subsequent accesses of that variable that are within a - // function and set those to the sink - exists(VariableAccess va | - va = variableSource.getAnAccess() and - va.getEnclosingFunction() = f - and sink = DataFlow::exprNode(va) - ) + // function and set those to the sink + exists(VariableAccess va | + va = variableSource.getAnAccess() and + va.getEnclosingFunction() = f and + sink = DataFlow::exprNode(va) ) + ) } override DataFlow::Node getAUsage() { DataFlow::exprNode(variableSource.getAnAccess()) = result } } - - - /** * In the typical C thread model, a mutex is a created by a function that is not responsible - * for creating the variable. Thus this class encodes a slightly different semantics - * wherein the usage pattern is that of variables that have been both initialized - * and then subsequently passed into a thread directly. + * for creating the variable. Thus this class encodes a slightly different semantics + * wherein the usage pattern is that of variables that have been both initialized + * and then subsequently passed into a thread directly. */ class DeclarationInitBasedThreadDependentMutex extends ThreadDependentMutex { - Variable variableSource; - DeclarationInitBasedThreadDependentMutex(){ - - exists(MutexSource ms, ThreadCreationFunction tcf | - this = DataFlow::exprNode(ms) and + DeclarationInitBasedThreadDependentMutex() { + exists(MutexSource ms, ThreadCreationFunction tcf | + this = DataFlow::exprNode(ms) and // accessed as a mutex source - TaintTracking::localTaint(DataFlow::exprNode(variableSource.getAnAccess()), DataFlow::exprNode(ms.getAnArgument())) and + TaintTracking::localTaint(DataFlow::exprNode(variableSource.getAnAccess()), + DataFlow::exprNode(ms.getAnArgument())) and // subsequently passed to a thread creation function (order not strictly // enforced for performance reasons) sink = DataFlow::exprNode(tcf.getAnArgument()) and - TaintTracking::localTaint(DataFlow::exprNode(variableSource.getAnAccess()), sink) - ) + TaintTracking::localTaint(DataFlow::exprNode(variableSource.getAnAccess()), sink) + ) } - override DataFlow::Node getAUsage() { TaintTracking::localTaint(getASource(), result) or + override DataFlow::Node getAUsage() { + TaintTracking::localTaint(getASource(), result) or DataFlow::exprNode(variableSource.getAnAccess()) = result } @@ -649,83 +644,72 @@ class DeclarationInitBasedThreadDependentMutex extends ThreadDependentMutex { result = getAThreadSource() } - DataFlow::Node getSink(){ - result = sink - } + DataFlow::Node getSink() { result = sink } /** * Gets the dataflow nodes corresponding to thread local usages of the * dependent mutex. */ override DataFlow::Node getAThreadSource() { - // here we line up the actual parameter at the thread creation - // site with the formal parameter in the target thread. + // here we line up the actual parameter at the thread creation + // site with the formal parameter in the target thread. // Note that there are differences between the C and C++ versions - // of the argument ordering in the thread creation function. However, + // of the argument ordering in the thread creation function. However, // since the C version only takes one parameter (as opposed to multiple) // we can simplify this search by considering only the first argument. - - - exists(FunctionCall fc, Function f, int n | // CPP Version - fc.getArgument(n) = sink.asExpr() and + exists( + FunctionCall fc, Function f, int n // CPP Version + | + fc.getArgument(n) = sink.asExpr() and f = fc.getArgument(0).(FunctionAccess).getTarget() and - // in C++, there is an extra argument to the `std::thread` call + // in C++, there is an extra argument to the `std::thread` call // so we must subtract 1 since this is not passed to the thread. result = DataFlow::exprNode(f.getParameter(n - 1).getAnAccess()) - ) or - exists(FunctionCall fc, Function f | // C Version + ) + or + exists( + FunctionCall fc, Function f // C Version + | fc.getAnArgument() = sink.asExpr() and - // in C, the second argument is the function + // in C, the second argument is the function f = fc.getArgument(1).(FunctionAccess).getTarget() and - // in C, the passed argument is always the zeroth argument + // in C, the passed argument is always the zeroth argument result = DataFlow::exprNode(f.getParameter(0).getAnAccess()) - ) - + ) } - } - /** - * In the typical C model, another way to use mutexes is to work with global variables + * In the typical C model, another way to use mutexes is to work with global variables * that can be initialized at various points -- one of which must be inside a thread. - * This class encapsulates this pattern. + * This class encapsulates this pattern. */ class DeclarationInitAccessBasedThreadDependentMutex extends ThreadDependentMutex { - Variable variableSource; - DeclarationInitAccessBasedThreadDependentMutex(){ - - exists(MutexSource ms, ThreadedFunction tf, VariableAccess va | - this = DataFlow::exprNode(ms) and + DeclarationInitAccessBasedThreadDependentMutex() { + exists(MutexSource ms, ThreadedFunction tf, VariableAccess va | + this = DataFlow::exprNode(ms) and // accessed as a mutex source - TaintTracking::localTaint(DataFlow::exprNode(variableSource.getAnAccess()), DataFlow::exprNode(ms.getAnArgument())) and + TaintTracking::localTaint(DataFlow::exprNode(variableSource.getAnAccess()), + DataFlow::exprNode(ms.getAnArgument())) and // is accessed somewhere else - va = variableSource.getAnAccess() and + va = variableSource.getAnAccess() and sink = DataFlow::exprNode(va) and // one of which must be a thread - va.getEnclosingFunction() = tf - ) + va.getEnclosingFunction() = tf + ) } - override DataFlow::Node getAUsage() { - result = DataFlow::exprNode(variableSource.getAnAccess()) - } + override DataFlow::Node getAUsage() { result = DataFlow::exprNode(variableSource.getAnAccess()) } } - - - - class ThreadDependentMutexTaintTrackingConfiguration extends TaintTracking::Configuration { ThreadDependentMutexTaintTrackingConfiguration() { this = "ThreadDependentMutexTaintTrackingConfiguration" } - override predicate isSource(DataFlow::Node node) { - node.asExpr() instanceof MutexSource - } + override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof MutexSource } override predicate isSink(DataFlow::Node node) { exists(ThreadCreationFunction f | f.getAnArgument() = node.asExpr()) @@ -735,7 +719,7 @@ class ThreadDependentMutexTaintTrackingConfiguration extends TaintTracking::Conf /** * Models expressions that destroy mutexes. */ -abstract class MutexDestroyer extends Expr { +abstract class MutexDestroyer extends StmtParent { /** * Gets the expression that references the mutex being destroyed. */ @@ -754,6 +738,40 @@ class C11MutexDestroyer extends MutexDestroyer, FunctionCall { override Expr getMutexExpr() { result = getArgument(0) } } +/** + * Models a delete expression -- note it is necessary to add this in + * addition to destructors to handle certain implementations of the + * standard library which obscure the destructors of mutexes. + */ +class DeleteMutexDestroyer extends MutexDestroyer { + DeleteMutexDestroyer() { this instanceof DeleteExpr } + + override Expr getMutexExpr() { this.(DeleteExpr).getExpr() = result } +} + +/** + * Models a possible mutex variable that if it goes + * out of scope would destroy an underlying mutex. + */ +class LocalMutexDestroyer extends MutexDestroyer { + Expr assignedValue; + + LocalMutexDestroyer() { + exists(LocalVariable lv | + not lv.isStatic() and + lv.getAnAssignedValue() = assignedValue and + // map the location to the return statements of the + // enclosing function + exists(ReturnStmt rs | + rs.getEnclosingFunction() = assignedValue.getEnclosingFunction() and + rs = this + ) + ) + } + + override Expr getMutexExpr() { result = assignedValue } +} + /** * Models implicit or explicit calls to the destructor of a mutex, either via * a `delete` statement or a variable going out of scope. @@ -764,5 +782,5 @@ class DestructorMutexDestroyer extends MutexDestroyer, DestructorCall { /** * Returns the `Expr` being deleted. */ - override Expr getMutexExpr() { this.getQualifier() = result } + override Expr getMutexExpr() { getQualifier() = result } } From 73499268743b0d7dead0d941421745663cfee9be Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 10:20:40 -0400 Subject: [PATCH 06/42] work --- .../DoNotDestroyAMutexWhileItIsLocked.ql | 3 - cpp/cert/test/rules/CON50-CPP/test.c | 135 +++++++++++++++--- .../src/codingstandards/cpp/Concurrency.qll | 3 + 3 files changed, 116 insertions(+), 25 deletions(-) diff --git a/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql b/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql index 65983395ad..1c1f292a03 100644 --- a/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql +++ b/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql @@ -10,14 +10,11 @@ * concurrency * external/cert/obligation/rule */ - import cpp import codingstandards.cpp.cert 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 diff --git a/cpp/cert/test/rules/CON50-CPP/test.c b/cpp/cert/test/rules/CON50-CPP/test.c index 7770c8b2c7..d6caa21c13 100644 --- a/cpp/cert/test/rules/CON50-CPP/test.c +++ b/cpp/cert/test/rules/CON50-CPP/test.c @@ -1,41 +1,132 @@ #include #include #include - -mtx_t mx; - -int t1(void *dummy) { - mtx_lock(&mx); - mtx_unlock(&mx); + +/// Note that since this is a C test case, mutexes +/// are not created/valid until `mtx_init` is called. +mtx_t mx1; +mtx_t mx2; + +int t1(void *data) { + mtx_lock(&mx1); + mtx_unlock(&mx1); + return 0; +} + +int t2(void *data) { + mtx_lock(&mx2); + mtx_unlock(&mx2); + return 0; +} + +int t3(void *data) { + mtx_t *mxl = (mtx_t *)data; + mtx_lock(mxl); + mtx_unlock(mxl); return 0; } - + +int t4(void *data) { + mtx_t *mxl = (mtx_t *)data; + mtx_lock(mxl); + mtx_unlock(mxl); + return 0; +} + +// case 1 - correctly waiting for a well-behaved thread int f1() { thrd_t threads[5]; - - mtx_init(&mx, mtx_plain); - - for (size_t i = 0; i < 5; i++) { + + mtx_init(&mx1, mtx_plain); // COMPLIANT + + for (size_t i = 0; i < 1; i++) { thrd_create(&threads[i], t1, NULL); - } - for (size_t i = 0; i < 5; i++) { + for (size_t i = 0; i < 1; i++) { thrd_join(threads[i], 0); } - - mtx_destroy(&mx); + + mtx_destroy(&mx1); return 0; } +// case 2 - incorrectly waiting for a well behaved thread. 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_init(&mx2, mtx_plain); // NON_COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t2, NULL); + } + + mtx_destroy(&mx2); + return 0; +} + +// case 3 - correctly waiting for a well-behaved thread, with a local mutex. +int f3() { + thrd_t threads[5]; + + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t3, &mxl); + } + + for (size_t i = 0; i < 1; i++) { + thrd_join(threads[i], 0); } - mtx_destroy(&mx); + mtx_destroy(&mxl); return 0; -} \ No newline at end of file +} + +// case 4 - incorrectly waiting for a well behaved thread, with a local mutex. +int f4() { + thrd_t threads[5]; + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // NON_COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t3, &mxl); + } + + mtx_destroy(&mxl); + return 0; +} + +// case 5 - incorrectly waiting with a stack local variable. +int f5() { + thrd_t threads[5]; + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // NON_COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t4, &mxl); + } + + return 0; +} + +// case 6 - correctly waiting with a stack local variable. +int f6() { + thrd_t threads[5]; + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t4, &mxl); + } + + for (size_t i = 0; i < 1; i++) { + thrd_join(threads[i], 0); + } + + return 0; +} diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index fe5833f7c7..9caf085325 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -758,7 +758,10 @@ class LocalMutexDestroyer extends MutexDestroyer { LocalMutexDestroyer() { exists(LocalVariable lv | + // static types aren't destroyers not lv.isStatic() and + // neither are pointers + not lv.getType() instanceof PointerType and lv.getAnAssignedValue() = assignedValue and // map the location to the return statements of the // enclosing function From b4dd1a2c59aa0d67fbcd8c66ddee184243a88ae1 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:24:37 -0400 Subject: [PATCH 07/42] work --- rule_packages/c/Concurrency3.json | 140 +++++++++++++++++++++++++++++ rule_packages/cpp/Concurrency.json | 2 + 2 files changed, 142 insertions(+) create mode 100644 rule_packages/c/Concurrency3.json diff --git a/rule_packages/c/Concurrency3.json b/rule_packages/c/Concurrency3.json new file mode 100644 index 0000000000..66394323b3 --- /dev/null +++ b/rule_packages/c/Concurrency3.json @@ -0,0 +1,140 @@ +{ + "CERT-C": { + "CON30-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "", + "kind": "problem", + "name": "Clean up thread-specific storage", + "precision": "medium", + "severity": "error", + "short_name": "CleanUpThreadSpecificStorage", + "tags": [] + } + ], + "title": "Clean up thread-specific storage" + }, + "CON31-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Allowing a mutex to go out of scope while it is locked removes protections around shared resources.", + "kind": "problem", + "name": "Do not destroy a mutex while it is locked", + "precision": "high", + "severity": "error", + "short_name": "DoNotAllowAMutexToGoOutOfScopeWhileLocked", + "shared_implementation_short_name": "DoNotAllowAMutexToGoOutOfScopeWhileLocked", + "tags": [ + "correctness", + "concurrency" + ] + }, + { + "description": "Calling delete on a locked mutex removes protections around shared resources.", + "kind": "problem", + "name": "Do not destroy a mutex while it is locked", + "precision": "high", + "severity": "error", + "short_name": "DoNotDestroyAMutexWhileItIsLocked", + "shared_implementation_short_name": "DoNotDestroyAMutexWhileItIsLocked", + "tags": [ + "correctness", + "concurrency" + ] + } + ], + "title": "Do not destroy a mutex while it is locked" + }, + "CON34-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "", + "kind": "problem", + "name": "Declare objects shared between threads with appropriate storage durations", + "precision": "high", + "severity": "error", + "short_name": "DeclareThreadsWithAppropriateStorageDurations", + "tags": [] + } + ], + "title": "Declare objects shared between threads with appropriate storage durations" + }, + "CON38-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "", + "kind": "problem", + "name": "Preserve thread safety and liveness when using condition variables", + "precision": "very-high", + "severity": "error", + "short_name": "PreserveSafetyWhenUsingConditionVariables", + "tags": [] + } + ], + "title": "Preserve thread safety and liveness when using condition variables" + }, + "CON39-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "", + "kind": "problem", + "name": "Do not join or detach a thread that was previously joined or detached", + "precision": "high", + "severity": "error", + "short_name": "ThreadPreviouslyJoinedOrDetached", + "tags": [] + } + ], + "title": "Do not join or detach a thread that was previously joined or detached" + }, + "CON40-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "", + "kind": "problem", + "name": "Do not refer to an atomic variable twice in an expression", + "precision": "very-high", + "severity": "error", + "short_name": "DoNotReferToAnAtomicVariableTwiceInExpression", + "tags": [] + } + ], + "title": "Do not refer to an atomic variable twice in an expression" + }, + "CON41-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "", + "kind": "problem", + "name": "Wrap functions that can fail spuriously in a loop", + "precision": "very-high", + "severity": "error", + "short_name": "WrapFunctionsThatCanFailSpuriouslyInLoop", + "tags": [] + } + ], + "title": "Wrap functions that can fail spuriously in a loop" + } + } +} \ No newline at end of file diff --git a/rule_packages/cpp/Concurrency.json b/rule_packages/cpp/Concurrency.json index 80bd09dfd8..e47f1004ed 100644 --- a/rule_packages/cpp/Concurrency.json +++ b/rule_packages/cpp/Concurrency.json @@ -12,6 +12,7 @@ "precision": "high", "severity": "error", "short_name": "DoNotAllowAMutexToGoOutOfScopeWhileLocked", + "shared_implementation_short_name": "DoNotAllowAMutexToGoOutOfScopeWhileLocked", "tags": [ "correctness", "concurrency" @@ -24,6 +25,7 @@ "precision": "high", "severity": "error", "short_name": "DoNotDestroyAMutexWhileItIsLocked", + "shared_implementation_short_name": "DoNotDestroyAMutexWhileItIsLocked", "tags": [ "correctness", "concurrency" From 7f3b4929ef0480e526e17138b13baec68d61b21b Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:24:54 -0400 Subject: [PATCH 08/42] migrate --- .../CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected | 1 - .../CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.qlref | 1 - .../CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref | 1 + .../rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.expected | 1 - .../test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.qlref | 1 - .../rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.testref | 1 + 6 files changed, 2 insertions(+), 4 deletions(-) delete mode 100644 cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected delete mode 100644 cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.qlref create mode 100644 cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref delete mode 100644 cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.expected delete mode 100644 cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.qlref create mode 100644 cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.testref diff --git a/cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected b/cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected deleted file mode 100644 index e50955351e..0000000000 --- a/cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected +++ /dev/null @@ -1 +0,0 @@ -| test.cpp:11:18:11:40 | call to thread | Mutex used by thread potentially destroyed while in use. | diff --git a/cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.qlref b/cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.qlref deleted file mode 100644 index 58f6de376e..0000000000 --- a/cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.qlref +++ /dev/null @@ -1 +0,0 @@ -rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql \ No newline at end of file diff --git a/cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref b/cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref new file mode 100644 index 0000000000..bc7da53823 --- /dev/null +++ b/cpp/cert/test/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref @@ -0,0 +1 @@ +cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql \ No newline at end of file diff --git a/cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.expected b/cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.expected deleted file mode 100644 index 30c13408b0..0000000000 --- a/cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.expected +++ /dev/null @@ -1 +0,0 @@ -| test.cpp:47:18:47:39 | call to thread | Mutex used by thread potentially deleted while in use. | diff --git a/cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.qlref b/cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.qlref deleted file mode 100644 index 61f2ee69b6..0000000000 --- a/cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.qlref +++ /dev/null @@ -1 +0,0 @@ -rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql \ No newline at end of file diff --git a/cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.testref b/cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.testref new file mode 100644 index 0000000000..961e0f941b --- /dev/null +++ b/cpp/cert/test/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.testref @@ -0,0 +1 @@ +cpp/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql \ No newline at end of file From c69756fc422529269a4de203135feec81fb13e07 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:25:52 -0400 Subject: [PATCH 09/42] import --- .../cpp/exclusions/c/Concurrency3.qll | 138 ++++++++++++++++++ .../cpp/exclusions/c/RuleMetadata.qll | 3 + 2 files changed, 141 insertions(+) create mode 100644 cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency3.qll diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency3.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency3.qll new file mode 100644 index 0000000000..13ca308d1f --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency3.qll @@ -0,0 +1,138 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype Concurrency3Query = + TCleanUpThreadSpecificStorageQuery() or + TDoNotAllowAMutexToGoOutOfScopeWhileLockedQuery() or + TDoNotDestroyAMutexWhileItIsLockedQuery() or + TDeclareThreadsWithAppropriateStorageDurationsQuery() or + TPreserveSafetyWhenUsingConditionVariablesQuery() or + TThreadPreviouslyJoinedOrDetachedQuery() or + TDoNotReferToAnAtomicVariableTwiceInExpressionQuery() or + TWrapFunctionsThatCanFailSpuriouslyInLoopQuery() + +predicate isConcurrency3QueryMetadata(Query query, string queryId, string ruleId) { + query = + // `Query` instance for the `cleanUpThreadSpecificStorage` query + Concurrency3Package::cleanUpThreadSpecificStorageQuery() and + queryId = + // `@id` for the `cleanUpThreadSpecificStorage` query + "c/cert/clean-up-thread-specific-storage" and + ruleId = "CON30-C" + or + query = + // `Query` instance for the `doNotAllowAMutexToGoOutOfScopeWhileLocked` query + Concurrency3Package::doNotAllowAMutexToGoOutOfScopeWhileLockedQuery() and + queryId = + // `@id` for the `doNotAllowAMutexToGoOutOfScopeWhileLocked` query + "c/cert/do-not-allow-a-mutex-to-go-out-of-scope-while-locked" and + ruleId = "CON31-C" + or + query = + // `Query` instance for the `doNotDestroyAMutexWhileItIsLocked` query + Concurrency3Package::doNotDestroyAMutexWhileItIsLockedQuery() and + queryId = + // `@id` for the `doNotDestroyAMutexWhileItIsLocked` query + "c/cert/do-not-destroy-a-mutex-while-it-is-locked" and + ruleId = "CON31-C" + or + query = + // `Query` instance for the `declareThreadsWithAppropriateStorageDurations` query + Concurrency3Package::declareThreadsWithAppropriateStorageDurationsQuery() and + queryId = + // `@id` for the `declareThreadsWithAppropriateStorageDurations` query + "c/cert/declare-threads-with-appropriate-storage-durations" and + ruleId = "CON34-C" + or + query = + // `Query` instance for the `preserveSafetyWhenUsingConditionVariables` query + Concurrency3Package::preserveSafetyWhenUsingConditionVariablesQuery() and + queryId = + // `@id` for the `preserveSafetyWhenUsingConditionVariables` query + "c/cert/preserve-safety-when-using-condition-variables" and + ruleId = "CON38-C" + or + query = + // `Query` instance for the `threadPreviouslyJoinedOrDetached` query + Concurrency3Package::threadPreviouslyJoinedOrDetachedQuery() and + queryId = + // `@id` for the `threadPreviouslyJoinedOrDetached` query + "c/cert/thread-previously-joined-or-detached" and + ruleId = "CON39-C" + or + query = + // `Query` instance for the `doNotReferToAnAtomicVariableTwiceInExpression` query + Concurrency3Package::doNotReferToAnAtomicVariableTwiceInExpressionQuery() and + queryId = + // `@id` for the `doNotReferToAnAtomicVariableTwiceInExpression` query + "c/cert/do-not-refer-to-an-atomic-variable-twice-in-expression" and + ruleId = "CON40-C" + or + query = + // `Query` instance for the `wrapFunctionsThatCanFailSpuriouslyInLoop` query + Concurrency3Package::wrapFunctionsThatCanFailSpuriouslyInLoopQuery() and + queryId = + // `@id` for the `wrapFunctionsThatCanFailSpuriouslyInLoop` query + "c/cert/wrap-functions-that-can-fail-spuriously-in-loop" and + ruleId = "CON41-C" +} + +module Concurrency3Package { + Query cleanUpThreadSpecificStorageQuery() { + //autogenerate `Query` type + result = + // `Query` type for `cleanUpThreadSpecificStorage` query + TQueryC(TConcurrency3PackageQuery(TCleanUpThreadSpecificStorageQuery())) + } + + Query doNotAllowAMutexToGoOutOfScopeWhileLockedQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotAllowAMutexToGoOutOfScopeWhileLocked` query + TQueryC(TConcurrency3PackageQuery(TDoNotAllowAMutexToGoOutOfScopeWhileLockedQuery())) + } + + Query doNotDestroyAMutexWhileItIsLockedQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotDestroyAMutexWhileItIsLocked` query + TQueryC(TConcurrency3PackageQuery(TDoNotDestroyAMutexWhileItIsLockedQuery())) + } + + Query declareThreadsWithAppropriateStorageDurationsQuery() { + //autogenerate `Query` type + result = + // `Query` type for `declareThreadsWithAppropriateStorageDurations` query + TQueryC(TConcurrency3PackageQuery(TDeclareThreadsWithAppropriateStorageDurationsQuery())) + } + + Query preserveSafetyWhenUsingConditionVariablesQuery() { + //autogenerate `Query` type + result = + // `Query` type for `preserveSafetyWhenUsingConditionVariables` query + TQueryC(TConcurrency3PackageQuery(TPreserveSafetyWhenUsingConditionVariablesQuery())) + } + + Query threadPreviouslyJoinedOrDetachedQuery() { + //autogenerate `Query` type + result = + // `Query` type for `threadPreviouslyJoinedOrDetached` query + TQueryC(TConcurrency3PackageQuery(TThreadPreviouslyJoinedOrDetachedQuery())) + } + + Query doNotReferToAnAtomicVariableTwiceInExpressionQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotReferToAnAtomicVariableTwiceInExpression` query + TQueryC(TConcurrency3PackageQuery(TDoNotReferToAnAtomicVariableTwiceInExpressionQuery())) + } + + Query wrapFunctionsThatCanFailSpuriouslyInLoopQuery() { + //autogenerate `Query` type + result = + // `Query` type for `wrapFunctionsThatCanFailSpuriouslyInLoop` query + TQueryC(TConcurrency3PackageQuery(TWrapFunctionsThatCanFailSpuriouslyInLoopQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index d4813a7e58..a3f6b5b958 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -5,6 +5,7 @@ import codingstandards.cpp.exclusions.RuleMetadata import Banned import Concurrency1 import Concurrency2 +import Concurrency3 import IO1 import IO2 import IO3 @@ -27,6 +28,7 @@ newtype TCQuery = TBannedPackageQuery(BannedQuery q) or TConcurrency1PackageQuery(Concurrency1Query q) or TConcurrency2PackageQuery(Concurrency2Query q) or + TConcurrency3PackageQuery(Concurrency3Query q) or TIO1PackageQuery(IO1Query q) or TIO2PackageQuery(IO2Query q) or TIO3PackageQuery(IO3Query q) or @@ -49,6 +51,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId) { isBannedQueryMetadata(query, queryId, ruleId) or isConcurrency1QueryMetadata(query, queryId, ruleId) or isConcurrency2QueryMetadata(query, queryId, ruleId) or + isConcurrency3QueryMetadata(query, queryId, ruleId) or isIO1QueryMetadata(query, queryId, ruleId) or isIO2QueryMetadata(query, queryId, ruleId) or isIO3QueryMetadata(query, queryId, ruleId) or From 5decd4cedf38dbbef8ba863b17b77961587c9534 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:26:03 -0400 Subject: [PATCH 10/42] new queries --- ...otAllowAMutexToGoOutOfScopeWhileLocked.qll | 39 ++++++++++++++ .../DoNotDestroyAMutexWhileItIsLocked.qll | 54 +++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 cpp/common/src/codingstandards/cpp/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.qll create mode 100644 cpp/common/src/codingstandards/cpp/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.qll diff --git a/cpp/common/src/codingstandards/cpp/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.qll b/cpp/common/src/codingstandards/cpp/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.qll new file mode 100644 index 0000000000..759d235eb4 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.qll @@ -0,0 +1,39 @@ +/* + * This query finds potential misuse of mutexes passed to threads by considering + * 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. 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. + * + * 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. + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import codingstandards.cpp.Concurrency +import semmle.code.cpp.dataflow.TaintTracking + +abstract class DoNotAllowAMutexToGoOutOfScopeWhileLockedSharedQuery extends Query { } + +Query getQuery() { result instanceof DoNotAllowAMutexToGoOutOfScopeWhileLockedSharedQuery } + +query predicate problems(ThreadDependentMutex dm, string message) { + not isExcluded(dm.asExpr(), getQuery()) and + exists(LocalVariable lv | + not isExcluded(lv, getQuery()) and + not lv.isStatic() and + TaintTracking::localTaint(dm.getAUsage(), DataFlow::exprNode(lv.getAnAssignedValue())) and + // ensure that each dependent thread is followed by some sort of joining + // behavior. + exists(DataFlow::Node n | n = dm.getADependentThreadCreationExpr() | + forall(ThreadWait tw | not tw = n.asExpr().getASuccessor*()) + ) and + message = "Mutex used by thread potentially destroyed while in use." + ) +} diff --git a/cpp/common/src/codingstandards/cpp/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.qll b/cpp/common/src/codingstandards/cpp/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.qll new file mode 100644 index 0000000000..d77ae8cf39 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.qll @@ -0,0 +1,54 @@ +/* + * 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 + * 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. + * + * 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. + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import codingstandards.cpp.Concurrency +import semmle.code.cpp.dataflow.TaintTracking + +abstract class DoNotDestroyAMutexWhileItIsLockedSharedQuery extends Query { } + +Query getQuery() { result instanceof DoNotDestroyAMutexWhileItIsLockedSharedQuery } + +query predicate problems( + ThreadDependentMutex dm, string message, MutexDestroyer md, string mdMessage +) { + not isExcluded(dm.asExpr(), getQuery()) and + not isExcluded(md, getQuery()) 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. 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 + // 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*()) + ) and + message = "Mutex used by thread potentially $@ while in use." and + mdMessage = "destroyed" +} From 8cfed0623eb74b4b32170492db9669ee7cac3138 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:26:16 -0400 Subject: [PATCH 11/42] new tests --- ...owAMutexToGoOutOfScopeWhileLocked.expected | 1 + ...NotAllowAMutexToGoOutOfScopeWhileLocked.ql | 2 + .../test.cpp | 124 ++++++++++++++++++ ...DoNotDestroyAMutexWhileItIsLocked.expected | 6 + .../DoNotDestroyAMutexWhileItIsLocked.ql | 2 + .../test.cpp | 0 6 files changed, 135 insertions(+) create mode 100644 cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected create mode 100644 cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql create mode 100644 cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp create mode 100644 cpp/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.expected create mode 100644 cpp/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql rename cpp/{cert/test/rules/CON50-CPP => common/test/rules/donotdestroyamutexwhileitislocked}/test.cpp (100%) diff --git a/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected new file mode 100644 index 0000000000..4bc23b0367 --- /dev/null +++ b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected @@ -0,0 +1 @@ +| test.cpp:16:14:16:15 | call to mutex | Mutex used by thread potentially destroyed while in use. | diff --git a/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql new file mode 100644 index 0000000000..43bc4286e6 --- /dev/null +++ b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.donotallowamutextogooutofscopewhilelocked.DoNotAllowAMutexToGoOutOfScopeWhileLocked diff --git a/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp new file mode 100644 index 0000000000..624aeab272 --- /dev/null +++ b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp @@ -0,0 +1,124 @@ +#include +#include + +std::mutex *m4 = new std::mutex(); // 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 lk(m5); } + +void f1() { + std::thread threads[5]; + std::mutex m1; // NON_COMPLIANT + + for (int i = 0; i < 5; ++i) { + threads[i] = std::thread(t1, i, &m1); + } +} + +void f2() { + std::thread threads[5]; + std::mutex m1; // COMPLIANT + + for (int i = 0; i < 5; ++i) { + threads[i] = std::thread(t1, i, &m1); + } + + for (int i = 0; i < 5; ++i) { + threads[i].join(); + } +} + +std::mutex m2; // COMPLIANT - m2 is not deleted and never goes out of scope. + // There is no delete + +void f3() { + std::thread threads[5]; + + for (int i = 0; i < 5; ++i) { + threads[i] = std::thread(t1, i, &m2); + } +} + +std::mutex *m3; + +void f4() { + m3 = new std::mutex(); // COMPLIANT + + std::thread threads[5]; + + for (int i = 0; i < 5; ++i) { + threads[i] = std::thread(t1, i, m3); + } + + // 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; +} + +void f5() { + m3 = new std::mutex(); // COMPLIANT + + std::thread threads[5]; + + for (int i = 0; i < 5; ++i) { + threads[i] = std::thread(t2, i, &m3); + } +} + +void f6() { + m3 = new std::mutex(); // COMPLIANT + + std::thread threads[5]; + + for (int i = 0; i < 5; ++i) { + threads[i] = std::thread(t1, i, m3); + } + + for (int i = 0; i < 5; ++i) { + threads[i].join(); + } + + delete m3; +} + +void f7() { + m3 = new std::mutex(); // COMPLIANT - Not related to scope + + std::thread threads[5]; + + for (int i = 0; i < 5; ++i) { + threads[i] = std::thread(t3, i, m3); + } + + for (int i = 0; i < 5; ++i) { + threads[i].join(); + } +} + +void f8() { + std::mutex *m = new std::mutex(); // COMPLIANT + delete m; +} + +void f9() { + std::mutex m; // 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); + } +} diff --git a/cpp/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.expected b/cpp/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.expected new file mode 100644 index 0000000000..0ae4aafa66 --- /dev/null +++ b/cpp/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.expected @@ -0,0 +1,6 @@ +| test.cpp:4:18:4:33 | call to mutex | Mutex used by thread potentially $@ while in use. | test.cpp:11:18:11:26 | call to ~mutex | destroyed | +| test.cpp:4:18:4:33 | call to mutex | Mutex used by thread potentially $@ while in use. | test.cpp:11:18:11:26 | delete | destroyed | +| test.cpp:16:14:16:15 | call to mutex | Mutex used by thread potentially $@ while in use. | test.cpp:21:1:21:1 | call to ~mutex | destroyed | +| test.cpp:16:14:16:15 | call to mutex | Mutex used by thread potentially $@ while in use. | test.cpp:21:1:21:1 | return ... | destroyed | +| test.cpp:94:8:94:23 | call to mutex | Mutex used by thread potentially $@ while in use. | test.cpp:10:34:10:42 | call to ~mutex | destroyed | +| test.cpp:94:8:94:23 | call to mutex | Mutex used by thread potentially $@ while in use. | test.cpp:10:34:10:42 | delete | destroyed | diff --git a/cpp/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql b/cpp/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql new file mode 100644 index 0000000000..f3ba1667a8 --- /dev/null +++ b/cpp/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.donotdestroyamutexwhileitislocked.DoNotDestroyAMutexWhileItIsLocked diff --git a/cpp/cert/test/rules/CON50-CPP/test.cpp b/cpp/common/test/rules/donotdestroyamutexwhileitislocked/test.cpp similarity index 100% rename from cpp/cert/test/rules/CON50-CPP/test.cpp rename to cpp/common/test/rules/donotdestroyamutexwhileitislocked/test.cpp From fec7e5df874ae3016e23c01634259ec88b9cd348 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:28:37 -0400 Subject: [PATCH 12/42] work --- ...NotAllowAMutexToGoOutOfScopeWhileLocked.ql | 36 +++---------- .../DoNotDestroyAMutexWhileItIsLocked.ql | 50 +++---------------- 2 files changed, 14 insertions(+), 72 deletions(-) diff --git a/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql b/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql index f8a399d8c5..f14dad7091 100644 --- a/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql +++ b/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql @@ -14,34 +14,10 @@ import cpp import codingstandards.cpp.cert -import codingstandards.cpp.Concurrency -import semmle.code.cpp.dataflow.DataFlow -import semmle.code.cpp.dataflow.TaintTracking +import codingstandards.cpp.rules.donotallowamutextogooutofscopewhilelocked.DoNotAllowAMutexToGoOutOfScopeWhileLocked -/* - * This query finds potential misuse of mutexes passed to threads by considering - * 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. 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. - * - * 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*()) - )) - - select dm, "Mutex used by thread potentially destroyed while in use." +class DoNotAllowAMutexToGoOutOfScopeWhileLockedQuery extends DoNotAllowAMutexToGoOutOfScopeWhileLockedSharedQuery { + DoNotAllowAMutexToGoOutOfScopeWhileLockedQuery() { + this = ConcurrencyPackage::doNotAllowAMutexToGoOutOfScopeWhileLockedQuery() + } +} diff --git a/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql b/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql index 1c1f292a03..2f2f5a6cdb 100644 --- a/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql +++ b/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql @@ -10,47 +10,13 @@ * concurrency * external/cert/obligation/rule */ + import cpp import codingstandards.cpp.cert -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 - * 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. - * - * 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, 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. 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 - // 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 dm, "Mutex used by thread potentially $@ while in use.", md, "destroyed" +import codingstandards.cpp.rules.donotdestroyamutexwhileitislocked.DoNotDestroyAMutexWhileItIsLocked + +class DoNotDestroyAMutexWhileItIsLockedQuery extends DoNotDestroyAMutexWhileItIsLockedSharedQuery { + DoNotDestroyAMutexWhileItIsLockedQuery() { + this = ConcurrencyPackage::doNotDestroyAMutexWhileItIsLockedQuery() + } +} From 64c1c9a27f6c0f15eaab109be69f40ce24a08413 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:29:27 -0400 Subject: [PATCH 13/42] tests --- ...owAMutexToGoOutOfScopeWhileLocked.expected | 2 + ...NotAllowAMutexToGoOutOfScopeWhileLocked.ql | 2 + .../test.c | 83 +++++++++++++++++++ ...DoNotDestroyAMutexWhileItIsLocked.expected | 2 + .../DoNotDestroyAMutexWhileItIsLocked.ql | 2 + .../donotdestroyamutexwhileitislocked}/test.c | 40 --------- 6 files changed, 91 insertions(+), 40 deletions(-) create mode 100644 c/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected create mode 100644 c/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql create mode 100644 c/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.c create mode 100644 c/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.expected create mode 100644 c/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql rename {cpp/cert/test/rules/CON50-CPP => c/common/test/rules/donotdestroyamutexwhileitislocked}/test.c (70%) diff --git a/c/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected b/c/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected new file mode 100644 index 0000000000..6cd128ea0f --- /dev/null +++ b/c/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected @@ -0,0 +1,2 @@ +| test.c:43:3:43:10 | call to mtx_init | Mutex used by thread potentially destroyed while in use. | +| test.c:58:3:58:10 | call to mtx_init | Mutex used by thread potentially destroyed while in use. | diff --git a/c/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql b/c/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql new file mode 100644 index 0000000000..43bc4286e6 --- /dev/null +++ b/c/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.donotallowamutextogooutofscopewhilelocked.DoNotAllowAMutexToGoOutOfScopeWhileLocked diff --git a/c/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.c b/c/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.c new file mode 100644 index 0000000000..23cd8d1593 --- /dev/null +++ b/c/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.c @@ -0,0 +1,83 @@ +#include +#include + +int t3(void *data) { + mtx_t *mxl = (mtx_t *)data; + mtx_lock(mxl); + mtx_unlock(mxl); + return 0; +} + +int t4(void *data) { + mtx_t *mxl = (mtx_t *)data; + mtx_lock(mxl); + mtx_unlock(mxl); + return 0; +} + +// case 3 - correctly waiting for a well-behaved thread, with a local mutex. +int f3() { + thrd_t threads[5]; + + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t3, &mxl); + } + + for (size_t i = 0; i < 1; i++) { + thrd_join(threads[i], 0); + } + + mtx_destroy(&mxl); + return 0; +} + +// case 4 - incorrectly waiting for a well behaved thread, with a local mutex. +int f4() { + thrd_t threads[5]; + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // NON_COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t3, &mxl); + } + + mtx_destroy(&mxl); + return 0; +} + +// case 5 - incorrectly waiting with a stack local variable. +int f5() { + thrd_t threads[5]; + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // NON_COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t4, &mxl); + } + + return 0; +} + +// case 6 - correctly waiting with a stack local variable. +int f6() { + thrd_t threads[5]; + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t4, &mxl); + } + + for (size_t i = 0; i < 1; i++) { + thrd_join(threads[i], 0); + } + + return 0; +} diff --git a/c/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.expected b/c/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.expected new file mode 100644 index 0000000000..549d641ae6 --- /dev/null +++ b/c/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.expected @@ -0,0 +1,2 @@ +| test.c:49:3:49:10 | call to mtx_init | Mutex used by thread potentially $@ while in use. | test.c:55:3:55:13 | call to mtx_destroy | destroyed | +| test.c:84:3:84:10 | call to mtx_init | Mutex used by thread potentially $@ while in use. | test.c:90:3:90:13 | call to mtx_destroy | destroyed | diff --git a/c/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql b/c/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql new file mode 100644 index 0000000000..f3ba1667a8 --- /dev/null +++ b/c/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.donotdestroyamutexwhileitislocked.DoNotDestroyAMutexWhileItIsLocked diff --git a/cpp/cert/test/rules/CON50-CPP/test.c b/c/common/test/rules/donotdestroyamutexwhileitislocked/test.c similarity index 70% rename from cpp/cert/test/rules/CON50-CPP/test.c rename to c/common/test/rules/donotdestroyamutexwhileitislocked/test.c index d6caa21c13..0d9173c5a2 100644 --- a/cpp/cert/test/rules/CON50-CPP/test.c +++ b/c/common/test/rules/donotdestroyamutexwhileitislocked/test.c @@ -1,4 +1,3 @@ -#include #include #include @@ -26,13 +25,6 @@ int t3(void *data) { return 0; } -int t4(void *data) { - mtx_t *mxl = (mtx_t *)data; - mtx_lock(mxl); - mtx_unlock(mxl); - return 0; -} - // case 1 - correctly waiting for a well-behaved thread int f1() { thrd_t threads[5]; @@ -98,35 +90,3 @@ int f4() { mtx_destroy(&mxl); return 0; } - -// case 5 - incorrectly waiting with a stack local variable. -int f5() { - thrd_t threads[5]; - mtx_t mxl; - - mtx_init(&mxl, mtx_plain); // NON_COMPLIANT - - for (size_t i = 0; i < 1; i++) { - thrd_create(&threads[i], t4, &mxl); - } - - return 0; -} - -// case 6 - correctly waiting with a stack local variable. -int f6() { - thrd_t threads[5]; - mtx_t mxl; - - mtx_init(&mxl, mtx_plain); // COMPLIANT - - for (size_t i = 0; i < 1; i++) { - thrd_create(&threads[i], t4, &mxl); - } - - for (size_t i = 0; i < 1; i++) { - thrd_join(threads[i], 0); - } - - return 0; -} From aa02fd296cee12182c013995cd0e2db197dbee3a Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:29:39 -0400 Subject: [PATCH 14/42] test --- .../CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref | 1 + .../test/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.testref | 1 + 2 files changed, 2 insertions(+) create mode 100644 c/cert/test/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref create mode 100644 c/cert/test/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.testref diff --git a/c/cert/test/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref b/c/cert/test/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref new file mode 100644 index 0000000000..531a88ca22 --- /dev/null +++ b/c/cert/test/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.testref @@ -0,0 +1 @@ +c/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql \ No newline at end of file diff --git a/c/cert/test/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.testref b/c/cert/test/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.testref new file mode 100644 index 0000000000..54d3a7499b --- /dev/null +++ b/c/cert/test/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.testref @@ -0,0 +1 @@ +c/common/test/rules/donotdestroyamutexwhileitislocked/DoNotDestroyAMutexWhileItIsLocked.ql \ No newline at end of file From a6783791d3d1f042edf33223bb05bbb4d7a0764b Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:29:50 -0400 Subject: [PATCH 15/42] quewry --- ...NotAllowAMutexToGoOutOfScopeWhileLocked.md | 18 +++++++++++++++ ...NotAllowAMutexToGoOutOfScopeWhileLocked.ql | 23 +++++++++++++++++++ .../DoNotDestroyAMutexWhileItIsLocked.md | 18 +++++++++++++++ .../DoNotDestroyAMutexWhileItIsLocked.ql | 22 ++++++++++++++++++ 4 files changed, 81 insertions(+) create mode 100644 c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md create mode 100644 c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql create mode 100644 c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md create mode 100644 c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.ql diff --git a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md new file mode 100644 index 0000000000..6e56d3cbbb --- /dev/null +++ b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md @@ -0,0 +1,18 @@ +# CON31-C: Do not destroy a mutex while it is locked + +This query implements the CERT-C rule CON31-C: + +> Do not destroy a mutex while it is locked + + +## CERT + +** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## Implementation notes + +None + +## References + +* CERT-C: [CON31-C: Do not destroy a mutex while it is locked](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql new file mode 100644 index 0000000000..4e32b6eb15 --- /dev/null +++ b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql @@ -0,0 +1,23 @@ +/** + * @id c/cert/do-not-allow-a-mutex-to-go-out-of-scope-while-locked + * @name CON31-C: Do not destroy a mutex while it is locked + * @description Allowing a mutex to go out of scope while it is locked removes protections around + * shared resources. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/con31-c + * correctness + * concurrency + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.rules.donotallowamutextogooutofscopewhilelocked.DoNotAllowAMutexToGoOutOfScopeWhileLocked + +class DoNotAllowAMutexToGoOutOfScopeWhileLockedQuery extends DoNotAllowAMutexToGoOutOfScopeWhileLockedSharedQuery { + DoNotAllowAMutexToGoOutOfScopeWhileLockedQuery() { + this = Concurrency3Package::doNotAllowAMutexToGoOutOfScopeWhileLockedQuery() + } +} diff --git a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md new file mode 100644 index 0000000000..6e56d3cbbb --- /dev/null +++ b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md @@ -0,0 +1,18 @@ +# CON31-C: Do not destroy a mutex while it is locked + +This query implements the CERT-C rule CON31-C: + +> Do not destroy a mutex while it is locked + + +## CERT + +** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## Implementation notes + +None + +## References + +* CERT-C: [CON31-C: Do not destroy a mutex while it is locked](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.ql b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.ql new file mode 100644 index 0000000000..b37dccab3a --- /dev/null +++ b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.ql @@ -0,0 +1,22 @@ +/** + * @id c/cert/do-not-destroy-a-mutex-while-it-is-locked + * @name CON31-C: Do not destroy a mutex while it is locked + * @description Calling delete on a locked mutex removes protections around shared resources. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/con31-c + * correctness + * concurrency + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.rules.donotdestroyamutexwhileitislocked.DoNotDestroyAMutexWhileItIsLocked + +class DoNotDestroyAMutexWhileItIsLockedQuery extends DoNotDestroyAMutexWhileItIsLockedSharedQuery { + DoNotDestroyAMutexWhileItIsLockedQuery() { + this = Concurrency3Package::doNotDestroyAMutexWhileItIsLockedQuery() + } +} From dbd81f293a60c32c47d544f1883559391ba548e4 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:35:34 -0400 Subject: [PATCH 16/42] import help --- ...NotAllowAMutexToGoOutOfScopeWhileLocked.md | 159 +++++++++++++++++- .../DoNotDestroyAMutexWhileItIsLocked.md | 159 +++++++++++++++++- 2 files changed, 314 insertions(+), 4 deletions(-) diff --git a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md index 6e56d3cbbb..aaf1a5b480 100644 --- a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md +++ b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md @@ -5,9 +5,164 @@ This query implements the CERT-C rule CON31-C: > Do not destroy a mutex while it is locked -## CERT -** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** +## Description + +Mutexes are used to protect shared data structures being concurrently accessed. If a mutex is destroyed while a thread is blocked waiting for that mutex, [critical sections](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-criticalsections) and shared data are no longer protected. + +The C Standard, 7.26.4.1, paragraph 2 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states + +> The `mtx_destroy` function releases any resources used by the mutex pointed to by `mtx`. No threads can be blocked waiting for the mutex pointed to by `mtx`. + + +This statement implies that destroying a mutex while a thread is waiting on it is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). + +## Noncompliant Code Example + +This noncompliant code example creates several threads that each invoke the `do_work()` function, passing a unique number as an ID. The `do_work()` function initializes the `lock` mutex if the argument is 0 and destroys the mutex if the argument is `max_threads - 1`. In all other cases, the `do_work()` function provides normal processing. Each thread, except the final cleanup thread, increments the atomic `completed` variable when it is finished. + +Unfortunately, this code contains several race conditions, allowing the mutex to be destroyed before it is unlocked. Additionally, there is no guarantee that `lock` will be initialized before it is passed to `mtx_lock()`. Each of these behaviors is [undefined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). + +```cpp +#include +#include +#include + +mtx_t lock; +/* Atomic so multiple threads can modify safely */ +atomic_int completed = ATOMIC_VAR_INIT(0); +enum { max_threads = 5 }; + +int do_work(void *arg) { + int *i = (int *)arg; + + if (*i == 0) { /* Creation thread */ + if (thrd_success != mtx_init(&lock, mtx_plain)) { + /* Handle error */ + } + atomic_store(&completed, 1); + } else if (*i < max_threads - 1) { /* Worker thread */ + if (thrd_success != mtx_lock(&lock)) { + /* Handle error */ + } + /* Access data protected by the lock */ + atomic_fetch_add(&completed, 1); + if (thrd_success != mtx_unlock(&lock)) { + /* Handle error */ + } + } else { /* Destruction thread */ + mtx_destroy(&lock); + } + return 0; +} + +int main(void) { + thrd_t threads[max_threads]; + + for (size_t i = 0; i < max_threads; i++) { + if (thrd_success != thrd_create(&threads[i], do_work, &i)) { + /* Handle error */ + } + } + for (size_t i = 0; i < max_threads; i++) { + if (thrd_success != thrd_join(threads[i], 0)) { + /* Handle error */ + } + } + return 0; +} + +``` + +## Compliant Solution + +This compliant solution eliminates the race conditions by initializing the mutex in `main()` before creating the threads and by destroying the mutex in `main()` after joining the threads: + +```cpp +#include +#include +#include + +mtx_t lock; +/* Atomic so multiple threads can increment safely */ +atomic_int completed = ATOMIC_VAR_INIT(0); +enum { max_threads = 5 }; + +int do_work(void *dummy) { + if (thrd_success != mtx_lock(&lock)) { + /* Handle error */ + } + /* Access data protected by the lock */ + atomic_fetch_add(&completed, 1); + if (thrd_success != mtx_unlock(&lock)) { + /* Handle error */ + } + + return 0; +} + +int main(void) { + thrd_t threads[max_threads]; + + if (thrd_success != mtx_init(&lock, mtx_plain)) { + /* Handle error */ + } + for (size_t i = 0; i < max_threads; i++) { + if (thrd_success != thrd_create(&threads[i], do_work, NULL)) { + /* Handle error */ + } + } + for (size_t i = 0; i < max_threads; i++) { + if (thrd_success != thrd_join(threads[i], 0)) { + /* Handle error */ + } + } + + mtx_destroy(&lock); + return 0; +} + +``` + +## Risk Assessment + +Destroying a mutex while it is locked may result in invalid control flow and data corruption. + +
Rule Severity Likelihood Remediation Cost Priority Level
CON31-C Medium Probable High P4 L3
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 Supported, but no explicit checker
CodeSonar 7.0p0 CONCURRENCY.LOCALARG Local Variable Passed to Thread
Helix QAC 2022.2 C4961, C4962
PRQA QA-C 9.7 4961, 4962
Parasoft C/C++test 2022.1 CERT_C-CON31-a CERT_C-CON31-b CERT_C-CON31-c Do not destroy another thread's mutex Do not use resources that have been freed Do not free resources using invalid pointers
Polyspace Bug Finder R2022a CERT C: Rule CON31-C Checks for destruction of locked mutex (rule fully covered)
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+CON31-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CWE 2.11 CWE-667 , Improper Locking 2017-07-10: CERT: Rule subset of CWE
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-667 and CON31-C/POS48-C** + +Intersection( CON31-C, POS48-C) = Ø + +CWE-667 = Union, CON31-C, POS48-C, list) where list = + +* Locking & Unlocking issues besides unlocking another thread’s C mutex or pthread mutex. + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] 7.26.4.1, "The mtx_destroy Function"
+ ## Implementation notes diff --git a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md index 6e56d3cbbb..aaf1a5b480 100644 --- a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md +++ b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md @@ -5,9 +5,164 @@ This query implements the CERT-C rule CON31-C: > Do not destroy a mutex while it is locked -## CERT -** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** +## Description + +Mutexes are used to protect shared data structures being concurrently accessed. If a mutex is destroyed while a thread is blocked waiting for that mutex, [critical sections](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-criticalsections) and shared data are no longer protected. + +The C Standard, 7.26.4.1, paragraph 2 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states + +> The `mtx_destroy` function releases any resources used by the mutex pointed to by `mtx`. No threads can be blocked waiting for the mutex pointed to by `mtx`. + + +This statement implies that destroying a mutex while a thread is waiting on it is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). + +## Noncompliant Code Example + +This noncompliant code example creates several threads that each invoke the `do_work()` function, passing a unique number as an ID. The `do_work()` function initializes the `lock` mutex if the argument is 0 and destroys the mutex if the argument is `max_threads - 1`. In all other cases, the `do_work()` function provides normal processing. Each thread, except the final cleanup thread, increments the atomic `completed` variable when it is finished. + +Unfortunately, this code contains several race conditions, allowing the mutex to be destroyed before it is unlocked. Additionally, there is no guarantee that `lock` will be initialized before it is passed to `mtx_lock()`. Each of these behaviors is [undefined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). + +```cpp +#include +#include +#include + +mtx_t lock; +/* Atomic so multiple threads can modify safely */ +atomic_int completed = ATOMIC_VAR_INIT(0); +enum { max_threads = 5 }; + +int do_work(void *arg) { + int *i = (int *)arg; + + if (*i == 0) { /* Creation thread */ + if (thrd_success != mtx_init(&lock, mtx_plain)) { + /* Handle error */ + } + atomic_store(&completed, 1); + } else if (*i < max_threads - 1) { /* Worker thread */ + if (thrd_success != mtx_lock(&lock)) { + /* Handle error */ + } + /* Access data protected by the lock */ + atomic_fetch_add(&completed, 1); + if (thrd_success != mtx_unlock(&lock)) { + /* Handle error */ + } + } else { /* Destruction thread */ + mtx_destroy(&lock); + } + return 0; +} + +int main(void) { + thrd_t threads[max_threads]; + + for (size_t i = 0; i < max_threads; i++) { + if (thrd_success != thrd_create(&threads[i], do_work, &i)) { + /* Handle error */ + } + } + for (size_t i = 0; i < max_threads; i++) { + if (thrd_success != thrd_join(threads[i], 0)) { + /* Handle error */ + } + } + return 0; +} + +``` + +## Compliant Solution + +This compliant solution eliminates the race conditions by initializing the mutex in `main()` before creating the threads and by destroying the mutex in `main()` after joining the threads: + +```cpp +#include +#include +#include + +mtx_t lock; +/* Atomic so multiple threads can increment safely */ +atomic_int completed = ATOMIC_VAR_INIT(0); +enum { max_threads = 5 }; + +int do_work(void *dummy) { + if (thrd_success != mtx_lock(&lock)) { + /* Handle error */ + } + /* Access data protected by the lock */ + atomic_fetch_add(&completed, 1); + if (thrd_success != mtx_unlock(&lock)) { + /* Handle error */ + } + + return 0; +} + +int main(void) { + thrd_t threads[max_threads]; + + if (thrd_success != mtx_init(&lock, mtx_plain)) { + /* Handle error */ + } + for (size_t i = 0; i < max_threads; i++) { + if (thrd_success != thrd_create(&threads[i], do_work, NULL)) { + /* Handle error */ + } + } + for (size_t i = 0; i < max_threads; i++) { + if (thrd_success != thrd_join(threads[i], 0)) { + /* Handle error */ + } + } + + mtx_destroy(&lock); + return 0; +} + +``` + +## Risk Assessment + +Destroying a mutex while it is locked may result in invalid control flow and data corruption. + +
Rule Severity Likelihood Remediation Cost Priority Level
CON31-C Medium Probable High P4 L3
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 Supported, but no explicit checker
CodeSonar 7.0p0 CONCURRENCY.LOCALARG Local Variable Passed to Thread
Helix QAC 2022.2 C4961, C4962
PRQA QA-C 9.7 4961, 4962
Parasoft C/C++test 2022.1 CERT_C-CON31-a CERT_C-CON31-b CERT_C-CON31-c Do not destroy another thread's mutex Do not use resources that have been freed Do not free resources using invalid pointers
Polyspace Bug Finder R2022a CERT C: Rule CON31-C Checks for destruction of locked mutex (rule fully covered)
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+CON31-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CWE 2.11 CWE-667 , Improper Locking 2017-07-10: CERT: Rule subset of CWE
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-667 and CON31-C/POS48-C** + +Intersection( CON31-C, POS48-C) = Ø + +CWE-667 = Union, CON31-C, POS48-C, list) where list = + +* Locking & Unlocking issues besides unlocking another thread’s C mutex or pthread mutex. + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] 7.26.4.1, "The mtx_destroy Function"
+ ## Implementation notes From c1db06cafbb21bd1a4411cd5d49a076d0bd92b8c Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 13:39:18 -0400 Subject: [PATCH 17/42] change note --- change_notes/2022-08-10-improvements-to-thread-edge-cases.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 change_notes/2022-08-10-improvements-to-thread-edge-cases.md diff --git a/change_notes/2022-08-10-improvements-to-thread-edge-cases.md b/change_notes/2022-08-10-improvements-to-thread-edge-cases.md new file mode 100644 index 0000000000..4f83f8e279 --- /dev/null +++ b/change_notes/2022-08-10-improvements-to-thread-edge-cases.md @@ -0,0 +1,4 @@ +- `CON50-CPP` - `DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql` + - Improvements to detection of mutexes shared across threads and expanded test coverage. +- `CON50-CPP` - `DoNotDestroyAMutexWhileItIsLocked.ql` + - Improvements to detection of mutexes shared across threads and expanded test coverage. From fc9c20626fa834c39362e6b902477016c597e992 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 16:45:34 -0400 Subject: [PATCH 18/42] new ruels --- rule_packages/c/Concurrency3.json | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rule_packages/c/Concurrency3.json b/rule_packages/c/Concurrency3.json index 66394323b3..609674a5dd 100644 --- a/rule_packages/c/Concurrency3.json +++ b/rule_packages/c/Concurrency3.json @@ -125,13 +125,16 @@ }, "queries": [ { - "description": "", + "description": "Failing to wrap a function that may fail spuriously may result in unreliable program behavior.", "kind": "problem", "name": "Wrap functions that can fail spuriously in a loop", "precision": "very-high", "severity": "error", "short_name": "WrapFunctionsThatCanFailSpuriouslyInLoop", - "tags": [] + "tags": [ + "correctness", + "concurrency" + ] } ], "title": "Wrap functions that can fail spuriously in a loop" From 83844f49130612824124f6f74e8847b06bb0393d Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 16:45:46 -0400 Subject: [PATCH 19/42] adding header --- c/common/test/includes/standard-library/stdatomic.h | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 c/common/test/includes/standard-library/stdatomic.h diff --git a/c/common/test/includes/standard-library/stdatomic.h b/c/common/test/includes/standard-library/stdatomic.h new file mode 100644 index 0000000000..89f78d9025 --- /dev/null +++ b/c/common/test/includes/standard-library/stdatomic.h @@ -0,0 +1,2 @@ +#define atomic_compare_exchange_weak(a, b, c) 0 +#define atomic_compare_exchange_weak_explicit(a, b, c, d, e) 0 From 2a42deb41a500608922bc06043122ead18976cbd Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 16:46:00 -0400 Subject: [PATCH 20/42] test case --- ...ctionsThatCanFailSpuriouslyInLoop.expected | 4 ++ ...FunctionsThatCanFailSpuriouslyInLoop.qlref | 1 + c/cert/test/rules/CON41-C/test.c | 49 +++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.expected create mode 100644 c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.qlref create mode 100644 c/cert/test/rules/CON41-C/test.c diff --git a/c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.expected b/c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.expected new file mode 100644 index 0000000000..e376acecbf --- /dev/null +++ b/c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.expected @@ -0,0 +1,4 @@ +| test.c:5:8:5:46 | atomic_compare_exchange_weak(a,b,c) | Function that can spuriously fail not wrapped in a loop. | +| test.c:9:3:9:41 | atomic_compare_exchange_weak(a,b,c) | Function that can spuriously fail not wrapped in a loop. | +| test.c:11:8:12:47 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | Function that can spuriously fail not wrapped in a loop. | +| test.c:16:3:16:56 | atomic_compare_exchange_weak_explicit(a,b,c,d,e) | Function that can spuriously fail not wrapped in a loop. | diff --git a/c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.qlref b/c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.qlref new file mode 100644 index 0000000000..1a03961c0d --- /dev/null +++ b/c/cert/test/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.qlref @@ -0,0 +1 @@ +rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.ql \ No newline at end of file diff --git a/c/cert/test/rules/CON41-C/test.c b/c/cert/test/rules/CON41-C/test.c new file mode 100644 index 0000000000..58cb4d0c18 --- /dev/null +++ b/c/cert/test/rules/CON41-C/test.c @@ -0,0 +1,49 @@ +#include "stdatomic.h" + +void f1() { + int a, b, c; + if (!atomic_compare_exchange_weak(&a, &b, c)) { // NON_COMPLIANT + (void)0; /* no-op */ + } + + atomic_compare_exchange_weak(&a, &b, c); // NON_COMPLIANT + + if (!atomic_compare_exchange_weak_explicit(&a, &b, c, 0, + 0)) { // NON_COMPLIANT + (void)0; /* no-op */ + } + + atomic_compare_exchange_weak_explicit(&a, &b, c, 0, 0); // NON_COMPLIANT +} + +void f2() { + int a, b, c; + while (1 == 1) { + if (!atomic_compare_exchange_weak(&a, &b, c)) { // COMPLIANT + (void)0; /* no-op */ + } + } + + while (!atomic_compare_exchange_weak(&a, &b, c)) { // COMPLIANT + (void)0; /* no-op */ + } + + do { + (void)0; /* no-op */ + } while (!atomic_compare_exchange_weak(&a, &b, c)); // COMPLIANT + + while (1 == 1) { + if (!atomic_compare_exchange_weak_explicit(&a, &b, c, 0, 0)) { // COMPLIANT + (void)0; /* no-op */ + } + } + + while (!atomic_compare_exchange_weak_explicit(&a, &b, c, 0, 0)) { // COMPLIANT + (void)0; /* no-op */ + } + + do { + (void)0; /* no-op */ + } while ( + !atomic_compare_exchange_weak_explicit(&a, &b, c, 0, 0)); // COMPLIANT +} \ No newline at end of file From c58e19040d03fa3529e21e8fe1edfd23f5954cc2 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 10 Aug 2022 16:46:20 -0400 Subject: [PATCH 21/42] docs --- ...rapFunctionsThatCanFailSpuriouslyInLoop.md | 157 ++++++++++++++++++ ...rapFunctionsThatCanFailSpuriouslyInLoop.ql | 39 +++++ 2 files changed, 196 insertions(+) create mode 100644 c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md create mode 100644 c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.ql diff --git a/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md new file mode 100644 index 0000000000..419783f68d --- /dev/null +++ b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md @@ -0,0 +1,157 @@ +# CON41-C: Wrap functions that can fail spuriously in a loop + +This query implements the CERT-C rule CON41-C: + +> Wrap functions that can fail spuriously in a loop + + + +## Description + +Functions that can fail spuriously should be wrapped in a loop. The `atomic_compare_exchange_weak()` and `atomic_compare_exchange_weak_explicit()` functions both attempt to set an atomic variable to a new value but only if it currently possesses a known old value. Unlike the related functions `atomic_compare_exchange_strong()` and `atomic_compare_exchange_strong_explicit()`, these functions are permitted to *fail spuriously*. This makes these functions faster on some platforms—for example, on architectures that implement compare-and-exchange using load-linked/store-conditional instructions, such as Alpha, ARM, MIPS, and PowerPC. The C Standard, 7.17.7.4, paragraph 4 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO%2FIEC9899-2011)\], describes this behavior: + +> A weak compare-and-exchange operation may fail spuriously. That is, even when the contents of memory referred to by `expected` and `object` are equal, it may return zero and store back to `expected` the same memory contents that were originally there. + + +## Noncompliant Code Example + +In this noncompliant code example, `reorganize_data_structure()` is to be used as an argument to `thrd_create()`. After reorganizing, the function attempts to replace the head pointer so that it points to the new version. If no other thread has changed the head pointer since it was originally loaded, `reorganize_data_structure()` is intended to exit the thread with a result of `true`, indicating success. Otherwise, the new reorganization attempt is discarded and the thread is exited with a result of `false`. However, `atomic_compare_exchange_weak()` may fail even when the head pointer has not changed. Therefore, `reorganize_data_structure()` may perform the work and then discard it unnecessarily. + +```cpp +#include +#include + +struct data { + struct data *next; + /* ... */ +}; + +extern void cleanup_data_structure(struct data *head); + +int reorganize_data_structure(void *thread_arg) { + struct data *_Atomic *ptr_to_head = thread_arg; + struct data *old_head = atomic_load(ptr_to_head); + struct data *new_head; + bool success; + + /* ... Reorganize the data structure ... */ + + success = atomic_compare_exchange_weak(ptr_to_head, + &old_head, new_head); + if (!success) { + cleanup_data_structure(new_head); + } + return success; /* Exit the thread */ +} + +``` + +## Compliant Solution (atomic_compare_exchange_weak()) + +To recover from spurious failures, a loop must be used. However, `atomic_compare_exchange_weak()` might fail because the head pointer changed, or the failure may be spurious. In either case, the thread must perform the work repeatedly until the compare-and-exchange succeeds, as shown in this compliant solution: + +```cpp +#include +#include +#include + +struct data { + struct data *next; + /* ... */ +}; + +extern void cleanup_data_structure(struct data *head); + +int reorganize_data_structure(void *thread_arg) { + struct data *_Atomic *ptr_to_head = thread_arg; + struct data *old_head = atomic_load(ptr_to_head); + struct data *new_head = NULL; + struct data *saved_old_head; + bool success; + + do { + if (new_head != NULL) { + cleanup_data_structure(new_head); + } + saved_old_head = old_head; + + /* ... Reorganize the data structure ... */ + + } while (!(success = atomic_compare_exchange_weak( + ptr_to_head, &old_head, new_head + )) && old_head == saved_old_head); + return success; /* Exit the thread */ +} + +``` +This loop could also be part of a larger control flow; for example, the thread from the noncompliant code example could be retried if it returns `false`. + +## Compliant Solution (atomic_compare_exchange_strong()) + +When a weak compare-and-exchange would require a loop and a strong one would not, the strong one is preferable, as in this compliant solution: + +```cpp +#include +#include + +struct data { + struct data *next; + /* ... */ +}; + +extern void cleanup_data_structure(struct data *head); + +int reorganize_data_structure(void *thread_arg) { + struct data *_Atomic *ptr_to_head = thread_arg; + struct data *old_head = atomic_load(ptr_to_head); + struct data *new_head; + bool success; + + /* ... Reorganize the data structure ... */ + + success = atomic_compare_exchange_strong( + ptr_to_head, &old_head, new_head + ); + if (!success) { + cleanup_data_structure(new_head); + } + return success; /* Exit the thread */ +} + +``` + +## Risk Assessment + +Failing to wrap the `atomic_compare_exchange_weak()` and `atomic_compare_exchange_weak_explicit()` functions in a loop can result in incorrect values and control flow. + +
Rule Severity Likelihood Remediation Cost Priority Level
CON41-C Low Unlikely Medium P2 L3
+ + +## Automated Detection + +
Tool Version Checker Description
CodeSonar 7.0p0 LANG.STRUCT.ICOL Inappropriate Call Outside Loop
Coverity 2017.07 BAD_CHECK_OF_WAIT_COND Implemented
Helix QAC 2022.2 C2026 C++5023
Klocwork 2022.2 CERT.CONC.ATOMIC_COMP_FAIL_IN_LOOP
Parasoft C/C++test 2022.1 CERT_C-CON41-a Wrap functions that can fail spuriously in a loop
Polyspace Bug Finder R2022a CERT C: Rule CON41-C Checks for situations where functions that can spuriously fail are not wrapped in loop (rule fully covered)
PRQA QA-C 9.7 2026
PRQA QA-C++ 4.4 5023
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://www.securecoding.cert.org/confluence/display/seccode/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+CON41-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CERT Oracle Secure Coding Standard for Java THI03-J. Always invoke wait() and await() methods inside a loop Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] 7.17.7.4, "The atomic_compare_exchange Generic Functions"
\[ Lea 2000 \] 1.3.2, "Liveness" 3.2.2, "Monitor Mechanics"
+ + +## Implementation notes + +None + +## References + +* CERT-C: [CON41-C: Wrap functions that can fail spuriously in a loop](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.ql b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.ql new file mode 100644 index 0000000000..62c8ec5dc4 --- /dev/null +++ b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.ql @@ -0,0 +1,39 @@ +/** + * @id c/cert/wrap-functions-that-can-fail-spuriously-in-loop + * @name CON41-C: Wrap functions that can fail spuriously in a loop + * @description Failing to wrap a function that may fail spuriously may result in unreliable program + * behavior. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/cert/id/con41-c + * correctness + * concurrency + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert + +/** + * Models calls to routines in the `stdatomic` library. Note that these + * are typically implemented as macros within Clang and GCC's standard + * libraries. + */ +class SpuriouslyFailingFunctionCallType extends MacroInvocation { + SpuriouslyFailingFunctionCallType() { + getMacroName() = ["atomic_compare_exchange_weak", "atomic_compare_exchange_weak_explicit"] + } +} + +from SpuriouslyFailingFunctionCallType fc +where + not isExcluded(fc, Concurrency3Package::wrapFunctionsThatCanFailSpuriouslyInLoopQuery()) and + ( + exists(StmtParent sp | sp = fc.getStmt() and not sp.(Stmt).getParentStmt*() instanceof Loop) + or + exists(StmtParent sp | + sp = fc.getExpr() and not sp.(Expr).getEnclosingStmt().getParentStmt*() instanceof Loop + ) + ) +select fc, "Function that can spuriously fail not wrapped in a loop." From 0ae0c005e12ba0e8791b0d3221ba9cfeda543b87 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 13:46:30 -0400 Subject: [PATCH 22/42] fixup of rules --- rules.csv | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rules.csv b/rules.csv index 44d2dcf85a..49bec22760 100755 --- a/rules.csv +++ b/rules.csv @@ -485,17 +485,17 @@ 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,,Concurrency3,Very Hard, +c,CERT-C,CON30-C,Yes,Rule,,,Clean up thread-specific storage,,Concurrency4,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,,Concurrency3,Hard, +c,CERT-C,CON34-C,Yes,Rule,,,Declare objects shared between threads with appropriate storage durations,,Concurrency4,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,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,CON39-C,Yes,Rule,,,Do not join or detach a thread that was previously joined or detached,,Concurrency4,Hard, +c,CERT-C,CON40-C,Yes,Rule,,,Do not refer to an atomic variable twice in an expression,,Concurrency4,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, From 444bcedf8f4cd1e131dbcf149c29cf040bd8f8a7 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:00:54 -0400 Subject: [PATCH 23/42] moving some classes into the library --- .../src/codingstandards/cpp/Concurrency.qll | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index 9caf085325..5eae03560f 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -787,3 +787,22 @@ class DestructorMutexDestroyer extends MutexDestroyer, DestructorCall { */ override Expr getMutexExpr() { getQualifier() = result } } + +/** + * Models a conditional variable denoted by `std::condition_variable`. + */ +class ConditionalVariable extends Variable { + ConditionalVariable() { + getUnderlyingType().(Class).hasQualifiedName("std", "condition_variable") + } +} + +/** + * Models a conditional function, which is a function that depends on the value + * of a conditional variable. + */ +class ConditionalFunction extends Function { + ConditionalFunction() { + exists(ConditionalVariable cv | cv.getAnAccess().getEnclosingFunction() = this) + } +} From 9306f0a19c01963ca899f69460929bd0f50bbbb9 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:26:41 -0400 Subject: [PATCH 24/42] rf --- ...serveSafetyWhenUsingConditionVariables.md} | 0 ...eserveSafetyWhenUsingConditionVariables.ql | 23 ++++++ ...yAndLivenessWhenUsingConditionVariables.ql | 77 ------------------- 3 files changed, 23 insertions(+), 77 deletions(-) rename cpp/cert/src/rules/CON55-CPP/{PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.md => PreserveSafetyWhenUsingConditionVariables.md} (100%) create mode 100644 cpp/cert/src/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.ql delete mode 100644 cpp/cert/src/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.ql diff --git a/cpp/cert/src/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.md b/cpp/cert/src/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.md similarity index 100% rename from cpp/cert/src/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.md rename to cpp/cert/src/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.md diff --git a/cpp/cert/src/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.ql b/cpp/cert/src/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.ql new file mode 100644 index 0000000000..faec80c67d --- /dev/null +++ b/cpp/cert/src/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.ql @@ -0,0 +1,23 @@ +/** + * @id cpp/cert/preserve-safety-when-using-condition-variables + * @name CON55-CPP: Preserve thread safety and liveness when using condition variables + * @description Usage of `notify_one` within a thread can lead to potential deadlocks and liveness + * problems. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/con55-cpp + * correctness + * concurrency + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.cpp.cert +import codingstandards.cpp.rules.preservesafetywhenusingconditionvariables.PreserveSafetyWhenUsingConditionVariables + +class PreserveSafetyWhenUsingConditionVariablesQuery extends PreserveSafetyWhenUsingConditionVariablesSharedQuery { + PreserveSafetyWhenUsingConditionVariablesQuery() { + this = ConcurrencyPackage::preserveSafetyWhenUsingConditionVariablesQuery() + } +} diff --git a/cpp/cert/src/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.ql b/cpp/cert/src/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.ql deleted file mode 100644 index 6fd6605f58..0000000000 --- a/cpp/cert/src/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.ql +++ /dev/null @@ -1,77 +0,0 @@ -/** - * @id cpp/cert/preserve-thread-safety-and-liveness-when-using-condition-variables - * @name CON55-CPP: Preserve thread safety and liveness when using condition variables - * @description Usage of `notify_one` within a thread can lead to potential deadlocks and liveness - * problems. - * @kind problem - * @precision high - * @problem.severity error - * @tags external/cert/id/con55-cpp - * correctness - * concurrency - * external/cert/obligation/rule - */ - -import cpp -import codingstandards.cpp.cert -import codingstandards.cpp.Concurrency - -/** - * Models a conditional variable denoted by `std::condition_variable`. - */ -class ConditionalVariable extends Variable { - ConditionalVariable() { - getUnderlyingType().(Class).hasQualifiedName("std", "condition_variable") - } -} - -/** - * Models a notification arising from a conditional variable. - */ -class ConditionalNotification extends FunctionCall { - string name; - - ConditionalNotification() { - exists(MemberFunction mf | - mf = getTarget() and - mf.getDeclaringType().hasQualifiedName("std", "condition_variable") and - mf.getName() = name - ) - } - - predicate isNotifyOne() { name in ["notify_one"] } -} - -/** - * Models a conditional function, which is a function that depends on the value - * of a conditional variable. - */ -class ConditionalFunction extends Function { - ConditionalFunction() { - exists(ConditionalVariable cv | cv.getAnAccess().getEnclosingFunction() = this) - } -} - -/* - * This query works by looking for usages of `notify_one` in the context of a - * function that is used in a thread. - * - * To avoid this problem a programmer may use `notify_all` or use unique - * condition variables. The problem of checking for correct usage of multiple - * condition variables is especially non-trivial and thus this query - * conservatively over-approximates potential issues with condition variables. - * - * Note that the check for using conditional variables within a loop is covered - * by CON54-CPP - */ - -from ConditionalNotification cv, ThreadedFunction tf -where - not isExcluded(cv, - ConcurrencyPackage::preserveThreadSafetyAndLivenessWhenUsingConditionVariablesQuery()) and - // the problematic types of uses of conditional variables - // are the cases where `notify_one` is used. - cv.isNotifyOne() and - // to be problematic this function should actually be used in a thread - cv.getEnclosingFunction() = tf -select cv, "Possible unsafe usage of `notify_one` which can lead to deadlocking of threads." From 27874f0f5ea88babfa69e0ac6b1310bb54a6754d Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:27:06 -0400 Subject: [PATCH 25/42] rf --- .../CON55-CPP/PreserveSafetyWhenUsingConditionVariables.testref | 1 + ...veThreadSafetyAndLivenessWhenUsingConditionVariables.expected | 1 - ...serveThreadSafetyAndLivenessWhenUsingConditionVariables.qlref | 1 - 3 files changed, 1 insertion(+), 2 deletions(-) create mode 100644 cpp/cert/test/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.testref delete mode 100644 cpp/cert/test/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.expected delete mode 100644 cpp/cert/test/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.qlref diff --git a/cpp/cert/test/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.testref b/cpp/cert/test/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.testref new file mode 100644 index 0000000000..ab183ef4fe --- /dev/null +++ b/cpp/cert/test/rules/CON55-CPP/PreserveSafetyWhenUsingConditionVariables.testref @@ -0,0 +1 @@ +cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql \ No newline at end of file diff --git a/cpp/cert/test/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.expected b/cpp/cert/test/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.expected deleted file mode 100644 index 6f22d7819c..0000000000 --- a/cpp/cert/test/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.expected +++ /dev/null @@ -1 +0,0 @@ -| test.cpp:19:8:19:17 | call to notify_one | Possible unsafe usage of `notify_one` which can lead to deadlocking of threads. | diff --git a/cpp/cert/test/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.qlref b/cpp/cert/test/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.qlref deleted file mode 100644 index 8116699f43..0000000000 --- a/cpp/cert/test/rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.qlref +++ /dev/null @@ -1 +0,0 @@ -rules/CON55-CPP/PreserveThreadSafetyAndLivenessWhenUsingConditionVariables.ql \ No newline at end of file From f5d8a702d5baaad81096ab9cddf00b32dfa1b739 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:27:21 -0400 Subject: [PATCH 26/42] import --- .../cpp/exclusions/c/Concurrency3.qll | 64 ---------------- .../cpp/exclusions/cpp/Concurrency.qll | 16 ++-- ...serveSafetyWhenUsingConditionVariables.qll | 73 +++++++++++++++++++ ...SafetyWhenUsingConditionVariables.expected | 1 + ...eserveSafetyWhenUsingConditionVariables.ql | 2 + .../test.cpp | 0 6 files changed, 84 insertions(+), 72 deletions(-) create mode 100644 cpp/common/src/codingstandards/cpp/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.qll create mode 100644 cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected create mode 100644 cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql rename cpp/{cert/test/rules/CON55-CPP => common/test/rules/preservesafetywhenusingconditionvariables}/test.cpp (100%) diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency3.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency3.qll index 13ca308d1f..029d0e16e4 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency3.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency3.qll @@ -4,24 +4,12 @@ import RuleMetadata import codingstandards.cpp.exclusions.RuleMetadata newtype Concurrency3Query = - TCleanUpThreadSpecificStorageQuery() or TDoNotAllowAMutexToGoOutOfScopeWhileLockedQuery() or TDoNotDestroyAMutexWhileItIsLockedQuery() or - TDeclareThreadsWithAppropriateStorageDurationsQuery() or TPreserveSafetyWhenUsingConditionVariablesQuery() or - TThreadPreviouslyJoinedOrDetachedQuery() or - TDoNotReferToAnAtomicVariableTwiceInExpressionQuery() or TWrapFunctionsThatCanFailSpuriouslyInLoopQuery() predicate isConcurrency3QueryMetadata(Query query, string queryId, string ruleId) { - query = - // `Query` instance for the `cleanUpThreadSpecificStorage` query - Concurrency3Package::cleanUpThreadSpecificStorageQuery() and - queryId = - // `@id` for the `cleanUpThreadSpecificStorage` query - "c/cert/clean-up-thread-specific-storage" and - ruleId = "CON30-C" - or query = // `Query` instance for the `doNotAllowAMutexToGoOutOfScopeWhileLocked` query Concurrency3Package::doNotAllowAMutexToGoOutOfScopeWhileLockedQuery() and @@ -38,14 +26,6 @@ predicate isConcurrency3QueryMetadata(Query query, string queryId, string ruleId "c/cert/do-not-destroy-a-mutex-while-it-is-locked" and ruleId = "CON31-C" or - query = - // `Query` instance for the `declareThreadsWithAppropriateStorageDurations` query - Concurrency3Package::declareThreadsWithAppropriateStorageDurationsQuery() and - queryId = - // `@id` for the `declareThreadsWithAppropriateStorageDurations` query - "c/cert/declare-threads-with-appropriate-storage-durations" and - ruleId = "CON34-C" - or query = // `Query` instance for the `preserveSafetyWhenUsingConditionVariables` query Concurrency3Package::preserveSafetyWhenUsingConditionVariablesQuery() and @@ -54,22 +34,6 @@ predicate isConcurrency3QueryMetadata(Query query, string queryId, string ruleId "c/cert/preserve-safety-when-using-condition-variables" and ruleId = "CON38-C" or - query = - // `Query` instance for the `threadPreviouslyJoinedOrDetached` query - Concurrency3Package::threadPreviouslyJoinedOrDetachedQuery() and - queryId = - // `@id` for the `threadPreviouslyJoinedOrDetached` query - "c/cert/thread-previously-joined-or-detached" and - ruleId = "CON39-C" - or - query = - // `Query` instance for the `doNotReferToAnAtomicVariableTwiceInExpression` query - Concurrency3Package::doNotReferToAnAtomicVariableTwiceInExpressionQuery() and - queryId = - // `@id` for the `doNotReferToAnAtomicVariableTwiceInExpression` query - "c/cert/do-not-refer-to-an-atomic-variable-twice-in-expression" and - ruleId = "CON40-C" - or query = // `Query` instance for the `wrapFunctionsThatCanFailSpuriouslyInLoop` query Concurrency3Package::wrapFunctionsThatCanFailSpuriouslyInLoopQuery() and @@ -80,13 +44,6 @@ predicate isConcurrency3QueryMetadata(Query query, string queryId, string ruleId } module Concurrency3Package { - Query cleanUpThreadSpecificStorageQuery() { - //autogenerate `Query` type - result = - // `Query` type for `cleanUpThreadSpecificStorage` query - TQueryC(TConcurrency3PackageQuery(TCleanUpThreadSpecificStorageQuery())) - } - Query doNotAllowAMutexToGoOutOfScopeWhileLockedQuery() { //autogenerate `Query` type result = @@ -101,13 +58,6 @@ module Concurrency3Package { TQueryC(TConcurrency3PackageQuery(TDoNotDestroyAMutexWhileItIsLockedQuery())) } - Query declareThreadsWithAppropriateStorageDurationsQuery() { - //autogenerate `Query` type - result = - // `Query` type for `declareThreadsWithAppropriateStorageDurations` query - TQueryC(TConcurrency3PackageQuery(TDeclareThreadsWithAppropriateStorageDurationsQuery())) - } - Query preserveSafetyWhenUsingConditionVariablesQuery() { //autogenerate `Query` type result = @@ -115,20 +65,6 @@ module Concurrency3Package { TQueryC(TConcurrency3PackageQuery(TPreserveSafetyWhenUsingConditionVariablesQuery())) } - Query threadPreviouslyJoinedOrDetachedQuery() { - //autogenerate `Query` type - result = - // `Query` type for `threadPreviouslyJoinedOrDetached` query - TQueryC(TConcurrency3PackageQuery(TThreadPreviouslyJoinedOrDetachedQuery())) - } - - Query doNotReferToAnAtomicVariableTwiceInExpressionQuery() { - //autogenerate `Query` type - result = - // `Query` type for `doNotReferToAnAtomicVariableTwiceInExpression` query - TQueryC(TConcurrency3PackageQuery(TDoNotReferToAnAtomicVariableTwiceInExpressionQuery())) - } - Query wrapFunctionsThatCanFailSpuriouslyInLoopQuery() { //autogenerate `Query` type result = diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/Concurrency.qll index 3ae8b37cca..3a2696c880 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/cpp/Concurrency.qll @@ -10,7 +10,7 @@ newtype ConcurrencyQuery = TPreventBitFieldAccessFromMultipleThreadsQuery() or TDeadlockByLockingInPredefinedOrderQuery() or TWrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery() or - TPreserveThreadSafetyAndLivenessWhenUsingConditionVariablesQuery() or + TPreserveSafetyWhenUsingConditionVariablesQuery() or TDoNotSpeculativelyLockALockedNonRecursiveMutexQuery() or TLockedALockedNonRecursiveMutexAuditQuery() @@ -64,11 +64,11 @@ predicate isConcurrencyQueryMetadata(Query query, string queryId, string ruleId) ruleId = "CON54-CPP" or query = - // `Query` instance for the `preserveThreadSafetyAndLivenessWhenUsingConditionVariables` query - ConcurrencyPackage::preserveThreadSafetyAndLivenessWhenUsingConditionVariablesQuery() and + // `Query` instance for the `preserveSafetyWhenUsingConditionVariables` query + ConcurrencyPackage::preserveSafetyWhenUsingConditionVariablesQuery() and queryId = - // `@id` for the `preserveThreadSafetyAndLivenessWhenUsingConditionVariables` query - "cpp/cert/preserve-thread-safety-and-liveness-when-using-condition-variables" and + // `@id` for the `preserveSafetyWhenUsingConditionVariables` query + "cpp/cert/preserve-safety-when-using-condition-variables" and ruleId = "CON55-CPP" or query = @@ -131,11 +131,11 @@ module ConcurrencyPackage { TQueryCPP(TConcurrencyPackageQuery(TWrapFunctionsThatCanSpuriouslyWakeUpInLoopQuery())) } - Query preserveThreadSafetyAndLivenessWhenUsingConditionVariablesQuery() { + Query preserveSafetyWhenUsingConditionVariablesQuery() { //autogenerate `Query` type result = - // `Query` type for `preserveThreadSafetyAndLivenessWhenUsingConditionVariables` query - TQueryCPP(TConcurrencyPackageQuery(TPreserveThreadSafetyAndLivenessWhenUsingConditionVariablesQuery())) + // `Query` type for `preserveSafetyWhenUsingConditionVariables` query + TQueryCPP(TConcurrencyPackageQuery(TPreserveSafetyWhenUsingConditionVariablesQuery())) } Query doNotSpeculativelyLockALockedNonRecursiveMutexQuery() { diff --git a/cpp/common/src/codingstandards/cpp/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.qll b/cpp/common/src/codingstandards/cpp/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.qll new file mode 100644 index 0000000000..50d0f34ec7 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.qll @@ -0,0 +1,73 @@ +/** + * Provides a library which includes a `problems` predicate for reporting.... + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import codingstandards.cpp.Concurrency + +abstract class PreserveSafetyWhenUsingConditionVariablesSharedQuery extends Query { } + +Query getQuery() { result instanceof PreserveSafetyWhenUsingConditionVariablesSharedQuery } + +/** + * Models a notification arising from a conditional variable. + */ +abstract class ConditionalNotification extends FunctionCall { + abstract predicate isNotifyOne(); +} + +class CPPConditionalNotification extends ConditionalNotification { + string name; + + CPPConditionalNotification() { + exists(MemberFunction mf | + mf = getTarget() and + mf.getDeclaringType().hasQualifiedName("std", "condition_variable") and + mf.getName() = name + ) + } + + override predicate isNotifyOne() { name in ["notify_one"] } +} + +class C11ConditionalNotification extends ConditionalNotification { + string name; + + C11ConditionalNotification() { + exists(Function mf | + mf = getTarget() and + mf.getName() = ["cnd_signal", "cnd_broadcast"] and + mf.getName() = name + ) + } + + override predicate isNotifyOne() { name in ["cnd_signal"] } +} + +/* + * This query works by looking for single dispatch notifications in the context of a + * function that is used in a thread. + * + * To avoid this problem a programmer may use `notify_all` or `cnd_broadcast` or use unique + * condition variables. The problem of checking for correct usage of multiple + * condition variables is especially non-trivial and thus this query + * conservatively over-approximates potential issues with condition variables. + * + * Note that the check for using conditional variables within a loop is covered + * by CON54-CPP + */ + +query predicate problems(ConditionalNotification cn, string message) { + not isExcluded(cn, getQuery()) and + exists(ThreadedFunction tf | + // the problematic types of uses of conditional variables + // are the cases where single dispatch notification is used. + cn.isNotifyOne() and + // to be problematic this function should actually be used in a thread + cn.getEnclosingFunction() = tf + ) and + message = + "Possible unsafe usage of single dispatch notification which can lead to deadlocking of threads." +} diff --git a/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected b/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected new file mode 100644 index 0000000000..2ec1a0ac6c --- /dev/null +++ b/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected @@ -0,0 +1 @@ +No expected results have yet been specified \ No newline at end of file diff --git a/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql b/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql new file mode 100644 index 0000000000..cef4c700ab --- /dev/null +++ b/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.preservesafetywhenusingconditionvariables.PreserveSafetyWhenUsingConditionVariables diff --git a/cpp/cert/test/rules/CON55-CPP/test.cpp b/cpp/common/test/rules/preservesafetywhenusingconditionvariables/test.cpp similarity index 100% rename from cpp/cert/test/rules/CON55-CPP/test.cpp rename to cpp/common/test/rules/preservesafetywhenusingconditionvariables/test.cpp From e5e99650094b40a7bfc2f22b722d3e89b75259a0 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:27:35 -0400 Subject: [PATCH 27/42] import --- rule_packages/c/Concurrency3.json | 76 +++--------------------------- rule_packages/cpp/Concurrency.json | 3 +- 2 files changed, 8 insertions(+), 71 deletions(-) diff --git a/rule_packages/c/Concurrency3.json b/rule_packages/c/Concurrency3.json index 609674a5dd..5da16b0522 100644 --- a/rule_packages/c/Concurrency3.json +++ b/rule_packages/c/Concurrency3.json @@ -1,22 +1,5 @@ { "CERT-C": { - "CON30-C": { - "properties": { - "obligation": "rule" - }, - "queries": [ - { - "description": "", - "kind": "problem", - "name": "Clean up thread-specific storage", - "precision": "medium", - "severity": "error", - "short_name": "CleanUpThreadSpecificStorage", - "tags": [] - } - ], - "title": "Clean up thread-specific storage" - }, "CON31-C": { "properties": { "obligation": "rule" @@ -51,74 +34,27 @@ ], "title": "Do not destroy a mutex while it is locked" }, - "CON34-C": { - "properties": { - "obligation": "rule" - }, - "queries": [ - { - "description": "", - "kind": "problem", - "name": "Declare objects shared between threads with appropriate storage durations", - "precision": "high", - "severity": "error", - "short_name": "DeclareThreadsWithAppropriateStorageDurations", - "tags": [] - } - ], - "title": "Declare objects shared between threads with appropriate storage durations" - }, "CON38-C": { "properties": { "obligation": "rule" }, "queries": [ { - "description": "", + "description": "Usages of `cnd_signal` with non-unique condition variables may impact thread safety and liveness.", "kind": "problem", "name": "Preserve thread safety and liveness when using condition variables", "precision": "very-high", "severity": "error", "short_name": "PreserveSafetyWhenUsingConditionVariables", - "tags": [] + "shared_implementation_short_name": "PreserveSafetyWhenUsingConditionVariables", + "tags": [ + "correctness", + "concurrency" + ] } ], "title": "Preserve thread safety and liveness when using condition variables" }, - "CON39-C": { - "properties": { - "obligation": "rule" - }, - "queries": [ - { - "description": "", - "kind": "problem", - "name": "Do not join or detach a thread that was previously joined or detached", - "precision": "high", - "severity": "error", - "short_name": "ThreadPreviouslyJoinedOrDetached", - "tags": [] - } - ], - "title": "Do not join or detach a thread that was previously joined or detached" - }, - "CON40-C": { - "properties": { - "obligation": "rule" - }, - "queries": [ - { - "description": "", - "kind": "problem", - "name": "Do not refer to an atomic variable twice in an expression", - "precision": "very-high", - "severity": "error", - "short_name": "DoNotReferToAnAtomicVariableTwiceInExpression", - "tags": [] - } - ], - "title": "Do not refer to an atomic variable twice in an expression" - }, "CON41-C": { "properties": { "obligation": "rule" diff --git a/rule_packages/cpp/Concurrency.json b/rule_packages/cpp/Concurrency.json index e47f1004ed..6e5898ecd8 100644 --- a/rule_packages/cpp/Concurrency.json +++ b/rule_packages/cpp/Concurrency.json @@ -128,7 +128,8 @@ "name": "Preserve thread safety and liveness when using condition variables", "precision": "high", "severity": "error", - "short_name": "PreserveThreadSafetyAndLivenessWhenUsingConditionVariables", + "short_name": "PreserveSafetyWhenUsingConditionVariables", + "shared_implementation_short_name": "PreserveSafetyWhenUsingConditionVariables", "tags": [ "correctness", "concurrency" From 149e546e6dcf6663859ef9e863d8b1703710d0a2 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:27:52 -0400 Subject: [PATCH 28/42] rf --- ...SafetyWhenUsingConditionVariables.expected | 1 + ...eserveSafetyWhenUsingConditionVariables.ql | 2 + .../test.c | 52 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected create mode 100644 c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql create mode 100644 c/common/test/rules/preservesafetywhenusingconditionvariables/test.c diff --git a/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected b/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected new file mode 100644 index 0000000000..2ec1a0ac6c --- /dev/null +++ b/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected @@ -0,0 +1 @@ +No expected results have yet been specified \ No newline at end of file diff --git a/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql b/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql new file mode 100644 index 0000000000..cef4c700ab --- /dev/null +++ b/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.preservesafetywhenusingconditionvariables.PreserveSafetyWhenUsingConditionVariables diff --git a/c/common/test/rules/preservesafetywhenusingconditionvariables/test.c b/c/common/test/rules/preservesafetywhenusingconditionvariables/test.c new file mode 100644 index 0000000000..0134a1fd6d --- /dev/null +++ b/c/common/test/rules/preservesafetywhenusingconditionvariables/test.c @@ -0,0 +1,52 @@ +#include + +cnd_t condition; +void t1(void *a) { + cnd_signal(&condition); // NON_COMPLIANT +} + +void t2(void *a) { + cnd_broadcast(&condition); // COMPLIANT +} + +void t3(void *a) { + cnd_signal(&condition); // COMPLIANT (not a thread) +} + +void f1() { + thrd_t threads[5]; + + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t1, &mxl); + } + + for (size_t i = 0; i < 1; i++) { + thrd_join(threads[i], 0); + } + + mtx_destroy(&mxl); + return 0; +} + +void f2() { + thrd_t threads[5]; + + mtx_t mxl; + + mtx_init(&mxl, mtx_plain); // COMPLIANT + + for (size_t i = 0; i < 1; i++) { + thrd_create(&threads[i], t2, &mxl); + } + + for (size_t i = 0; i < 1; i++) { + thrd_join(threads[i], 0); + } + + mtx_destroy(&mxl); + return 0; +} From c295bb2aaf69fac7e684e63494c892210cd3367d Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:28:02 -0400 Subject: [PATCH 29/42] rf --- .../CON38-C/PreserveSafetyWhenUsingConditionVariables.testref | 1 + 1 file changed, 1 insertion(+) create mode 100644 c/cert/test/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.testref diff --git a/c/cert/test/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.testref b/c/cert/test/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.testref new file mode 100644 index 0000000000..235e98dd5b --- /dev/null +++ b/c/cert/test/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.testref @@ -0,0 +1 @@ +c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.ql \ No newline at end of file From 6241d5ac0150b3a5d43eb7732f84984b995d9e25 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:28:23 -0400 Subject: [PATCH 30/42] rf --- ...eserveSafetyWhenUsingConditionVariables.md | 16 +++++++++++++ ...eserveSafetyWhenUsingConditionVariables.ql | 23 +++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md create mode 100644 c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.ql diff --git a/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md new file mode 100644 index 0000000000..9d63231976 --- /dev/null +++ b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md @@ -0,0 +1,16 @@ +# CON38-C: Preserve thread safety and liveness when using condition variables + +This query implements the CERT-C rule CON38-C: + +> Preserve thread safety and liveness when using condition variables +## CERT + +** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## Implementation notes + +None + +## References + +* CERT-C: [CON38-C: Preserve thread safety and liveness when using condition variables](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.ql b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.ql new file mode 100644 index 0000000000..dad45dd592 --- /dev/null +++ b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.ql @@ -0,0 +1,23 @@ +/** + * @id c/cert/preserve-safety-when-using-condition-variables + * @name CON38-C: Preserve thread safety and liveness when using condition variables + * @description Usages of `cnd_signal` with non-unique condition variables may impact thread safety + * and liveness. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/cert/id/con38-c + * correctness + * concurrency + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.rules.preservesafetywhenusingconditionvariables.PreserveSafetyWhenUsingConditionVariables + +class PreserveSafetyWhenUsingConditionVariablesQuery extends PreserveSafetyWhenUsingConditionVariablesSharedQuery { + PreserveSafetyWhenUsingConditionVariablesQuery() { + this = Concurrency3Package::preserveSafetyWhenUsingConditionVariablesQuery() + } +} From 261fe7164882deea019b535732a7d201c3b324ad Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:30:11 -0400 Subject: [PATCH 31/42] test case --- .../PreserveSafetyWhenUsingConditionVariables.expected | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected b/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected index 2ec1a0ac6c..8b5548add9 100644 --- a/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected +++ b/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected @@ -1 +1 @@ -No expected results have yet been specified \ No newline at end of file +| test.c:5:3:5:12 | call to cnd_signal | Possible unsafe usage of single dispatch notification which can lead to deadlocking of threads. | From 6548c334a38d37ad3c027edb48c919203363a649 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:32:14 -0400 Subject: [PATCH 32/42] test case --- .../PreserveSafetyWhenUsingConditionVariables.expected | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected b/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected index 2ec1a0ac6c..986186782b 100644 --- a/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected +++ b/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected @@ -1 +1 @@ -No expected results have yet been specified \ No newline at end of file +| test.cpp:19:8:19:17 | call to notify_one | Possible unsafe usage of single dispatch notification which can lead to deadlocking of threads. | From d0b94f94d429c32681f15be4d0dab638581643e0 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 11 Aug 2022 14:35:37 -0400 Subject: [PATCH 33/42] docs --- ...eserveSafetyWhenUsingConditionVariables.md | 345 +++++++++++++++++- 1 file changed, 343 insertions(+), 2 deletions(-) diff --git a/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md index 9d63231976..03825f5376 100644 --- a/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md +++ b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md @@ -3,9 +3,350 @@ This query implements the CERT-C rule CON38-C: > Preserve thread safety and liveness when using condition variables -## CERT -** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## Description + +Both thread safety and [liveness](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-liveness) are concerns when using condition variables. The *thread-safety* property requires that all objects maintain consistent states in a multithreaded environment \[[Lea 2000](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Lea2000)\]. The *liveness* property requires that every operation or function invocation execute to completion without interruption; for example, there is no deadlock. + +Condition variables must be used inside a `while` loop. (See [CON36-C. Wrap functions that can spuriously wake up in a loop](https://wiki.sei.cmu.edu/confluence/display/c/CON36-C.+Wrap+functions+that+can+spuriously+wake+up+in+a+loop) for more information.) To guarantee liveness, programs must test the `while` loop condition before invoking the `cnd_wait()` function. This early test checks whether another thread has already satisfied the [condition predicate](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-conditionpredicate) and has sent a notification. Invoking the `cnd_wait()` function after the notification has been sent results in indefinite blocking. + +To guarantee thread safety, programs must test the `while` loop condition after returning from the `cnd_wait()` function. When a given thread invokes the `cnd_wait()` function, it will attempt to block until its condition variable is signaled by a call to `cnd_broadcast()` or to `cnd_signal()`. + +The `cnd_signal()` function unblocks one of the threads that are blocked on the specified condition variable at the time of the call. If multiple threads are waiting on the same condition variable, the scheduler can select any of those threads to be awakened (assuming that all threads have the same priority level). The `cnd_broadcast()` function unblocks all of the threads that are blocked on the specified condition variable at the time of the call. The order in which threads execute following a call to `cnd_broadcast()` is unspecified. Consequently, an unrelated thread could start executing, discover that its condition predicate is satisfied, and resume execution even though it was supposed to remain dormant. For these reasons, threads must check the condition predicate after the `cnd_wait()` function returns. A `while` loop is the best choice for checking the condition predicate both before and after invoking `cnd_wait()`. + +The use of `cnd_signal()` is safe if each thread uses a unique condition variable. If multiple threads share a condition variable, the use of `cnd_signal()` is safe only if the following conditions are met: + +* All threads must perform the same set of operations after waking up, which means that any thread can be selected to wake up and resume for a single invocation of `cnd_signal()`. +* Only one thread is required to wake upon receiving the signal. +The `cnd_broadcast()` function can be used to unblock all of the threads that are blocked on the specified condition variable if the use of `cnd_signal()` is unsafe. + +## Noncompliant Code Example (cnd_signal()) + +This noncompliant code example uses five threads that are intended to execute sequentially according to the step level assigned to each thread when it is created (serialized processing). The `current_step` variable holds the current step level and is incremented when the respective thread completes. Finally, another thread is signaled so that the next step can be executed. Each thread waits until its step level is ready, and the `cnd_wait()` function call is wrapped inside a `while` loop, in compliance with [CON36-C. Wrap functions that can spuriously wake up in a loop](https://wiki.sei.cmu.edu/confluence/display/c/CON36-C.+Wrap+functions+that+can+spuriously+wake+up+in+a+loop). + +```cpp +#include +#include + +enum { NTHREADS = 5 }; + +mtx_t mutex; +cnd_t cond; + +int run_step(void *t) { + static size_t current_step = 0; + size_t my_step = *(size_t *)t; + + if (thrd_success != mtx_lock(&mutex)) { + /* Handle error */ + } + + printf("Thread %zu has the lock\n", my_step); + while (current_step != my_step) { + printf("Thread %zu is sleeping...\n", my_step); + + if (thrd_success != cnd_wait(&cond, &mutex)) { + /* Handle error */ + } + + printf("Thread %zu woke up\n", my_step); + } + /* Do processing ... */ + printf("Thread %zu is processing...\n", my_step); + current_step++; + + /* Signal awaiting task */ + if (thrd_success != cnd_signal(&cond)) { + /* Handle error */ + } + + printf("Thread %zu is exiting...\n", my_step); + + if (thrd_success != mtx_unlock(&mutex)) { + /* Handle error */ + } + return 0; +} +int main(void) { + thrd_t threads[NTHREADS]; + size_t step[NTHREADS]; + + if (thrd_success != mtx_init(&mutex, mtx_plain)) { + /* Handle error */ + } + + if (thrd_success != cnd_init(&cond)) { + /* Handle error */ + } + + /* Create threads */ + for (size_t i = 0; i < NTHREADS; ++i) { + step[i] = i; + + if (thrd_success != thrd_create(&threads[i], run_step, + &step[i])) { + /* Handle error */ + } + } + + /* Wait for all threads to complete */ + for (size_t i = NTHREADS; i != 0; --i) { + if (thrd_success != thrd_join(threads[i-1], NULL)) { + /* Handle error */ + } + } + + mtx_destroy(&mutex); + cnd_destroy(&cond); + return 0; +} +``` +In this example, all threads share a condition variable. Each thread has its own distinct [condition predicate](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-conditionpredicate) because each thread requires `current_step` to have a different value before proceeding. When the condition variable is signaled, any of the waiting threads can wake up. + +The following table illustrates a possible scenario in which the [liveness](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-liveness) property is violated. If, by chance, the notified thread is not the thread with the next step value, that thread will wait again. No additional notifications can occur, and eventually the pool of available threads will be exhausted. + +**Deadlock: Out-of-Sequence Step Value** + +
Time Thread \# ( my_step ) current_step Action
0 3 0 Thread 3 executes first time: predicate is FALSE -> wait()
1 2 0 Thread 2 executes first time: predicate is FALSE -> wait()
2 4 0 Thread 4 executes first time: predicate is FALSE -> wait()
3 0 0 Thread 0 executes first time: predicate is TRUE -> current_step++; cnd_signal()
4 1 1 Thread 1 executes first time: predicate is TRUE -> current_step++; cnd_signal()
5 3 2 Thread 3 wakes up (scheduler choice): predicate is FALSE -> wait()
6 — — Thread exhaustion! No more threads to run, and a conditional variable signal is needed to wake up the others
+This noncompliant code example violates the [liveness](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-liveness) property. + + +## Compliant Solution (cnd_broadcast()) + +This compliant solution uses the `cnd_broadcast()` function to signal all waiting threads instead of a single random thread. Only the `run_step()` thread code from the noncompliant code example is modified, as follows: + +```cpp +#include +#include + +mtx_t mutex; +cnd_t cond; +int run_step(void *t) { + static size_t current_step = 0; + size_t my_step = *(size_t *)t; + + if (thrd_success != mtx_lock(&mutex)) { + /* Handle error */ + } + + printf("Thread %zu has the lock\n", my_step); + + while (current_step != my_step) { + printf("Thread %zu is sleeping...\n", my_step); + + if (thrd_success != cnd_wait(&cond, &mutex)) { + /* Handle error */ + } + + printf("Thread %zu woke up\n", my_step); + } + + /* Do processing ... */ + printf("Thread %zu is processing...\n", my_step); + + current_step++; + + /* Signal ALL waiting tasks */ + if (thrd_success != cnd_broadcast(&cond)) { + /* Handle error */ + } + + printf("Thread %zu is exiting...\n", my_step); + + if (thrd_success != mtx_unlock(&mutex)) { + /* Handle error */ + } + return 0; +} +``` +Awakening all threads guarantees the [liveness](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-liveness) property because each thread will execute its [condition predicate](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-conditionpredicate) test, and exactly one will succeed and continue execution. + +## Compliant Solution (Using cnd_signal() with a Unique Condition Variable per Thread) + +Another compliant solution is to use a unique condition variable for each thread (all associated with the same mutex). In this case, `cnd_signal()` wakes up only the thread that is waiting on it. This solution is more efficient than using `cnd_broadcast()` because only the desired thread is awakened. + +The [condition predicate](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-conditionpredicate) of the signaled thread must be true; otherwise, a deadlock will occur. + +```cpp +#include +#include + +enum { NTHREADS = 5 }; + +mtx_t mutex; +cnd_t cond[NTHREADS]; + +int run_step(void *t) { + static size_t current_step = 0; + size_t my_step = *(size_t *)t; + + if (thrd_success != mtx_lock(&mutex)) { + /* Handle error */ + } + + printf("Thread %zu has the lock\n", my_step); + + while (current_step != my_step) { + printf("Thread %zu is sleeping...\n", my_step); + + if (thrd_success != cnd_wait(&cond[my_step], &mutex)) { + /* Handle error */ + } + + printf("Thread %zu woke up\n", my_step); + } + + /* Do processing ... */ + printf("Thread %zu is processing...\n", my_step); + + current_step++; + + /* Signal next step thread */ + if ((my_step + 1) < NTHREADS) { + if (thrd_success != cnd_signal(&cond[my_step + 1])) { + /* Handle error */ + } + } + + printf("Thread %zu is exiting...\n", my_step); + + if (thrd_success != mtx_unlock(&mutex)) { + /* Handle error */ + } + return 0; +} + +int main(void) { + thrd_t threads[NTHREADS]; + size_t step[NTHREADS]; + + if (thrd_success != mtx_init(&mutex, mtx_plain)) { + /* Handle error */ + } + + for (size_t i = 0; i< NTHREADS; ++i) { + if (thrd_success != cnd_init(&cond[i])) { + /* Handle error */ + } + } + + /* Create threads */ + for (size_t i = 0; i < NTHREADS; ++i) { + step[i] = i; + if (thrd_success != thrd_create(&threads[i], run_step, + &step[i])) { + /* Handle error */ + } + } + + /* Wait for all threads to complete */ + for (size_t i = NTHREADS; i != 0; --i) { + if (thrd_success != thrd_join(threads[i-1], NULL)) { + /* Handle error */ + } + } + + mtx_destroy(&mutex); + + for (size_t i = 0; i < NTHREADS; ++i) { + cnd_destroy(&cond[i]); + } + return 0; +} +``` + +## Compliant Solution (Windows, Condition Variables) + +This compliant solution uses a `CONDITION_VARIABLE` object, available on Microsoft Windows (Vista and later): + +```cpp +#include +#include + +CRITICAL_SECTION lock; +CONDITION_VARIABLE cond; + +DWORD WINAPI run_step(LPVOID t) { + static size_t current_step = 0; + size_t my_step = (size_t)t; + + EnterCriticalSection(&lock); + printf("Thread %zu has the lock\n", my_step); + + while (current_step != my_step) { + printf("Thread %zu is sleeping...\n", my_step); + + if (!SleepConditionVariableCS(&cond, &lock, INFINITE)) { + /* Handle error */ + } + + printf("Thread %zu woke up\n", my_step); + } + + /* Do processing ... */ + printf("Thread %zu is processing...\n", my_step); + + current_step++; + + LeaveCriticalSection(&lock); + + /* Signal ALL waiting tasks */ + WakeAllConditionVariable(&cond); + + printf("Thread %zu is exiting...\n", my_step); + return 0; +} + +enum { NTHREADS = 5 }; + +int main(void) { + HANDLE threads[NTHREADS]; + + InitializeCriticalSection(&lock); + InitializeConditionVariable(&cond); + + /* Create threads */ + for (size_t i = 0; i < NTHREADS; ++i) { + threads[i] = CreateThread(NULL, 0, run_step, (LPVOID)i, 0, NULL); + } + + /* Wait for all threads to complete */ + WaitForMultipleObjects(NTHREADS, threads, TRUE, INFINITE); + + DeleteCriticalSection(&lock); + + return 0; +} +``` + +## Risk Assessment + +Failing to preserve the thread safety and [liveness](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-liveness) of a program when using condition variables can lead to indefinite blocking and [denial of service](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-denial-of-service) (DoS). + +
Rule Severity Likelihood Remediation Cost Priority Level
CON38-C Low Unlikely Medium P2 L3
+ + +## Automated Detection + +
Tool Version Checker Description
CodeSonar 7.0p0 CONCURRENCY.BADFUNC.CNDSIGNAL Use of Condition Variable Signal
Helix QAC 2022.2 C1778, C1779
Klocwork 2022.2 CERT.CONC.UNSAFE_COND_VAR_C
Parasoft C/C++test 2022.1 CERT_C-CON38-a Use the 'cnd_signal()' function with a unique condition variable
Polyspace Bug Finder R2022a CERT C: Rule CON38-C Checks for multiple threads waiting on same condition variable (rule fully covered)
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://www.securecoding.cert.org/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+CON38-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CERT Oracle Secure Coding Standard for Java THI02-J. Notify all waiting threads rather than a single thread Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## Bibliography + +
\[ IEEE Std 1003.1:2013 \] XSH, System Interfaces, pthread_cond_broadcast XSH, System Interfaces, pthread_cond_signal
\[ Lea 2000 \]
+ ## Implementation notes From 1a68c187c416bfa50864e93f6ec91c8088e248ab Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 12 Aug 2022 09:43:57 -0400 Subject: [PATCH 34/42] docs --- .../rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md | 1 - c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md | 1 - 2 files changed, 2 deletions(-) diff --git a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md index aaf1a5b480..bafad6e688 100644 --- a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md +++ b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule CON31-C: > Do not destroy a mutex while it is locked - ## Description Mutexes are used to protect shared data structures being concurrently accessed. If a mutex is destroyed while a thread is blocked waiting for that mutex, [critical sections](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-criticalsections) and shared data are no longer protected. diff --git a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md index aaf1a5b480..bafad6e688 100644 --- a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md +++ b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule CON31-C: > Do not destroy a mutex while it is locked - ## Description Mutexes are used to protect shared data structures being concurrently accessed. If a mutex is destroyed while a thread is blocked waiting for that mutex, [critical sections](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-criticalsections) and shared data are no longer protected. From 8357878ef7a2bbaab446c7a929664923949cd4a6 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 12 Aug 2022 09:46:02 -0400 Subject: [PATCH 35/42] docs --- .../rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md | 1 - 1 file changed, 1 deletion(-) diff --git a/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md index 419783f68d..da6cf2e1ed 100644 --- a/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md +++ b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule CON41-C: > Wrap functions that can fail spuriously in a loop - ## Description Functions that can fail spuriously should be wrapped in a loop. The `atomic_compare_exchange_weak()` and `atomic_compare_exchange_weak_explicit()` functions both attempt to set an atomic variable to a new value but only if it currently possesses a known old value. Unlike the related functions `atomic_compare_exchange_strong()` and `atomic_compare_exchange_strong_explicit()`, these functions are permitted to *fail spuriously*. This makes these functions faster on some platforms—for example, on architectures that implement compare-and-exchange using load-linked/store-conditional instructions, such as Alpha, ARM, MIPS, and PowerPC. The C Standard, 7.17.7.4, paragraph 4 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO%2FIEC9899-2011)\], describes this behavior: From 27fa710535bd22595f7ea6bddb83d15ef7dd40f0 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 12 Aug 2022 10:01:46 -0400 Subject: [PATCH 36/42] placeholders --- .../PreserveSafetyWhenUsingConditionVariables.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.qll b/cpp/common/src/codingstandards/cpp/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.qll index 50d0f34ec7..94d9d201c4 100644 --- a/cpp/common/src/codingstandards/cpp/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.qll +++ b/cpp/common/src/codingstandards/cpp/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.qll @@ -1,5 +1,6 @@ /** - * Provides a library which includes a `problems` predicate for reporting.... + * Provides a library which includes a `problems` predicate for reporting errors + * related to the use of condition variables. */ import cpp From 6164b137b919dfba1c21726b6656758ed73e5fd6 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 12 Aug 2022 10:10:37 -0400 Subject: [PATCH 37/42] test case update --- ...llowAMutexToGoOutOfScopeWhileLocked.expected | 1 + .../test.cpp | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected index 4bc23b0367..3499b51375 100644 --- a/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected +++ b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected @@ -1 +1,2 @@ | test.cpp:16:14:16:15 | call to mutex | Mutex used by thread potentially destroyed while in use. | +| test.cpp:134:14:134:15 | call to mutex | Mutex used by thread potentially destroyed while in use. | diff --git a/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp index 624aeab272..0b8d7f022b 100644 --- a/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp +++ b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp @@ -122,3 +122,20 @@ void f10() { threads[i] = std::thread(t5, i); } } + +void join_threads(std::thread *threads) { + for (int i = 0; i < 5; ++i) { + threads[i].join(); + } +} + +void f11() { + std::thread threads[5]; + std::mutex m1; // NON_COMPLIANT (FALSE POSITIVE) + + for (int i = 0; i < 5; ++i) { + threads[i] = std::thread(t1, i, &m1); + } + + join_threads(threads); +} \ No newline at end of file From 926564c91b38130567d4fca2301145415c278253 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Fri, 12 Aug 2022 16:27:54 +0200 Subject: [PATCH 38/42] Update cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp --- .../rules/donotallowamutextogooutofscopewhilelocked/test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp index 0b8d7f022b..a883a9e267 100644 --- a/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp +++ b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp @@ -131,7 +131,7 @@ void join_threads(std::thread *threads) { void f11() { std::thread threads[5]; - std::mutex m1; // NON_COMPLIANT (FALSE POSITIVE) + std::mutex m1; // COMPLIANT[FALSE_POSITIVE] for (int i = 0; i < 5; ++i) { threads[i] = std::thread(t1, i, &m1); From 2bbf844ccad78b61e47a01ff416f78dd70ea9f82 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 12 Aug 2022 11:13:21 -0400 Subject: [PATCH 39/42] scope --- rule_packages/c/Concurrency3.json | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/rule_packages/c/Concurrency3.json b/rule_packages/c/Concurrency3.json index 5da16b0522..fccfd47e9d 100644 --- a/rule_packages/c/Concurrency3.json +++ b/rule_packages/c/Concurrency3.json @@ -32,7 +32,10 @@ ] } ], - "title": "Do not destroy a mutex while it is locked" + "title": "Do not destroy a mutex while it is locked", + "implementation_scope": { + "description": "This implementation does not allow for thread synchronization to be performed in subroutines. All synchronization must be performed within the context of the other thread management functions." + } }, "CON38-C": { "properties": { @@ -53,7 +56,10 @@ ] } ], - "title": "Preserve thread safety and liveness when using condition variables" + "title": "Preserve thread safety and liveness when using condition variables", + "implementation_scope": { + "description": "This implementation does not attempt to identify unique condition variables and instead advocates for the usage of `cnd_broadcast`." + } }, "CON41-C": { "properties": { @@ -73,7 +79,10 @@ ] } ], - "title": "Wrap functions that can fail spuriously in a loop" + "title": "Wrap functions that can fail spuriously in a loop", + "implementation_scope": { + "description": "This implementation does not attempt to identify a relationship between the condition variable and the atomic operation." + } } } } \ No newline at end of file From 5d86cd802bff8e4fb44d337a9dee299ba8ece106 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 12 Aug 2022 11:15:28 -0400 Subject: [PATCH 40/42] implementation notes --- .../rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md | 2 +- c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md | 2 +- .../rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md | 2 +- .../rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md index bafad6e688..e5403d0f7a 100644 --- a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md +++ b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md @@ -165,7 +165,7 @@ CWE-667 = Union, CON31-C, POS48-C, list) where list = ## Implementation notes -None +This implementation does not allow for thread synchronization to be performed in subroutines. All synchronization must be performed within the context of the other thread management functions. ## References diff --git a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md index bafad6e688..e5403d0f7a 100644 --- a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md +++ b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md @@ -165,7 +165,7 @@ CWE-667 = Union, CON31-C, POS48-C, list) where list = ## Implementation notes -None +This implementation does not allow for thread synchronization to be performed in subroutines. All synchronization must be performed within the context of the other thread management functions. ## References diff --git a/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md index 03825f5376..17e4e95822 100644 --- a/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md +++ b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md @@ -350,7 +350,7 @@ Search for [vulnerabilities](https://www.securecoding.cert.org/confluence/displa ## Implementation notes -None +This implementation does not attempt to identify unique condition variables and instead advocates for the usage of `cnd_broadcast`. ## References diff --git a/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md index da6cf2e1ed..b176e77da2 100644 --- a/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md +++ b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md @@ -149,7 +149,7 @@ Search for [vulnerabilities](https://www.securecoding.cert.org/confluence/displa ## Implementation notes -None +This implementation does not attempt to identify a relationship between the condition variable and the atomic operation. ## References From 782e57cd9366355a17af3d7d081a86a3bcd9c11a Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 12 Aug 2022 14:13:32 -0400 Subject: [PATCH 41/42] revert --- .../rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md | 2 +- c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md index e5403d0f7a..bafad6e688 100644 --- a/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md +++ b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md @@ -165,7 +165,7 @@ CWE-667 = Union, CON31-C, POS48-C, list) where list = ## Implementation notes -This implementation does not allow for thread synchronization to be performed in subroutines. All synchronization must be performed within the context of the other thread management functions. +None ## References diff --git a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md index e5403d0f7a..bafad6e688 100644 --- a/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md +++ b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md @@ -165,7 +165,7 @@ CWE-667 = Union, CON31-C, POS48-C, list) where list = ## Implementation notes -This implementation does not allow for thread synchronization to be performed in subroutines. All synchronization must be performed within the context of the other thread management functions. +None ## References From e8776e0a41203001db4fe9d4595d924106d41448 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 12 Aug 2022 14:14:32 -0400 Subject: [PATCH 42/42] revert --- .../rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md | 2 +- .../rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md index 17e4e95822..03825f5376 100644 --- a/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md +++ b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md @@ -350,7 +350,7 @@ Search for [vulnerabilities](https://www.securecoding.cert.org/confluence/displa ## Implementation notes -This implementation does not attempt to identify unique condition variables and instead advocates for the usage of `cnd_broadcast`. +None ## References diff --git a/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md index b176e77da2..da6cf2e1ed 100644 --- a/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md +++ b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md @@ -149,7 +149,7 @@ Search for [vulnerabilities](https://www.securecoding.cert.org/confluence/displa ## Implementation notes -This implementation does not attempt to identify a relationship between the condition variable and the atomic operation. +None ## References