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..bafad6e688 --- /dev/null +++ b/c/cert/src/rules/CON31-C/DoNotAllowAMutexToGoOutOfScopeWhileLocked.md @@ -0,0 +1,172 @@ +# 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 + + +## 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 + +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..bafad6e688 --- /dev/null +++ b/c/cert/src/rules/CON31-C/DoNotDestroyAMutexWhileItIsLocked.md @@ -0,0 +1,172 @@ +# 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 + + +## 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 + +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() + } +} 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..03825f5376 --- /dev/null +++ b/c/cert/src/rules/CON38-C/PreserveSafetyWhenUsingConditionVariables.md @@ -0,0 +1,357 @@ +# 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 + + +## 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 + +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() + } +} 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..da6cf2e1ed --- /dev/null +++ b/c/cert/src/rules/CON41-C/WrapFunctionsThatCanFailSpuriouslyInLoop.md @@ -0,0 +1,156 @@ +# 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." 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 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 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 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 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/c/common/test/rules/donotdestroyamutexwhileitislocked/test.c b/c/common/test/rules/donotdestroyamutexwhileitislocked/test.c new file mode 100644 index 0000000000..0d9173c5a2 --- /dev/null +++ b/c/common/test/rules/donotdestroyamutexwhileitislocked/test.c @@ -0,0 +1,92 @@ +#include +#include + +/// 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; +} + +// case 1 - correctly waiting for a well-behaved thread +int f1() { + thrd_t threads[5]; + + 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 < 1; i++) { + thrd_join(threads[i], 0); + } + + mtx_destroy(&mx1); + return 0; +} + +// case 2 - incorrectly waiting for a well behaved thread. +int f2() { + thrd_t threads[5]; + + 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(&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; +} diff --git a/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected b/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected new file mode 100644 index 0000000000..8b5548add9 --- /dev/null +++ b/c/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected @@ -0,0 +1 @@ +| test.c:5:3:5:12 | call to cnd_signal | Possible unsafe usage of single dispatch notification which can lead to deadlocking of threads. | 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; +} 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. diff --git a/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql b/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql index 716c02a06d..f14dad7091 100644 --- a/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql +++ b/cpp/cert/src/rules/CON50-CPP/DoNotAllowAMutexToGoOutOfScopeWhileLocked.ql @@ -14,33 +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 stack 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. - * - * 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." - */ - -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." +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 e1cfc98397..2f2f5a6cdb 100644 --- a/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql +++ b/cpp/cert/src/rules/CON50-CPP/DoNotDestroyAMutexWhileItIsLocked.ql @@ -13,34 +13,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.donotdestroyamutexwhileitislocked.DoNotDestroyAMutexWhileItIsLocked -/* - * 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. - * - * 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." - */ - -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())) - ) -select cc, "Mutex used by thread potentially deleted while in use." +class DoNotDestroyAMutexWhileItIsLockedQuery extends DoNotDestroyAMutexWhileItIsLockedSharedQuery { + DoNotDestroyAMutexWhileItIsLockedQuery() { + this = ConcurrencyPackage::doNotDestroyAMutexWhileItIsLockedQuery() + } +} 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/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." 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 diff --git a/cpp/cert/test/rules/CON50-CPP/test.cpp b/cpp/cert/test/rules/CON50-CPP/test.cpp deleted file mode 100644 index 241d0b08c2..0000000000 --- a/cpp/cert/test/rules/CON50-CPP/test.cpp +++ /dev/null @@ -1,77 +0,0 @@ -#include -#include - -void t1(int i, std::mutex *pm) {} -void t2(int i, std::mutex **pm) {} -void f1() { - std::thread threads[5]; - std::mutex m1; - - for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t1, i, &m1); // NON_COMPLIANT - } -} - -void f2() { - std::thread threads[5]; - std::mutex m1; - - for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t1, i, &m1); // COMPLIANT - due to check below - } - - for (int i = 0; i < 5; ++i) { - threads[i].join(); - } -} - -std::mutex m2; - -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. - } -} - -std::mutex *m3; - -void f4() { - m3 = new std::mutex(); - - std::thread threads[5]; - - for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t1, i, m3); // NON_COMPLIANT - } - - delete m3; -} - -void f5() { - m3 = new std::mutex(); - - std::thread threads[5]; - - for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t2, i, &m3); // COMPLIANT - } -} - -void f6() { - m3 = new std::mutex(); - - std::thread threads[5]; - - for (int i = 0; i < 5; ++i) { - threads[i] = std::thread(t1, i, m3); // COMPLIANT - } - - for (int i = 0; i < 5; ++i) { - threads[i].join(); - } - - delete m3; -} \ No newline at end of file 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 diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index badd7d8fea..5eae03560f 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,351 @@ class MutexDependentThreadConstructor extends ThreadConstructorCall { } /** - * Models a call to a a `std::thread` join. + * Models thread waiting functions. + */ +abstract class ThreadWait extends FunctionCall { } + +/** + * Models a call to a `std::thread` join. */ -class ThreadWait extends FunctionCall { +class CPPThreadWait extends ThreadWait { VariableAccess var; - ThreadWait() { + CPPThreadWait() { getTarget().(MemberFunction).getDeclaringType().hasQualifiedName("std", "thread") and getTarget().getName() = "join" } } + +/** + * Models a call to `thrd_join` in C11. + */ +class C11ThreadWait extends ThreadWait { + VariableAccess var; + + C11ThreadWait() { getTarget().getName() = "thrd_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. 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. + */ +abstract class ThreadDependentMutex extends DataFlow::Node { + DataFlow::Node 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() { + // 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 + // 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()) + ) + ) + } + + /** + * 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. + * 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 isSink(DataFlow::Node node) { + exists(ThreadCreationFunction f | f.getAnArgument() = node.asExpr()) + } +} + +/** + * Models expressions that destroy mutexes. + */ +abstract class MutexDestroyer extends StmtParent { + /** + * 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 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 | + // 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 + 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. + */ +class DestructorMutexDestroyer extends MutexDestroyer, DestructorCall { + DestructorMutexDestroyer() { getTarget().getDeclaringType().hasQualifiedName("std", "mutex") } + + /** + * Returns the `Expr` being deleted. + */ + 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) + } +} 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..029d0e16e4 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency3.qll @@ -0,0 +1,74 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype Concurrency3Query = + TDoNotAllowAMutexToGoOutOfScopeWhileLockedQuery() or + TDoNotDestroyAMutexWhileItIsLockedQuery() or + TPreserveSafetyWhenUsingConditionVariablesQuery() or + TWrapFunctionsThatCanFailSpuriouslyInLoopQuery() + +predicate isConcurrency3QueryMetadata(Query query, string queryId, string ruleId) { + 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 `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 `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 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 preserveSafetyWhenUsingConditionVariablesQuery() { + //autogenerate `Query` type + result = + // `Query` type for `preserveSafetyWhenUsingConditionVariables` query + TQueryC(TConcurrency3PackageQuery(TPreserveSafetyWhenUsingConditionVariablesQuery())) + } + + 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 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/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" +} 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..94d9d201c4 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.qll @@ -0,0 +1,74 @@ +/** + * Provides a library which includes a `problems` predicate for reporting errors + * related to the use of condition variables. + */ + +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/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected new file mode 100644 index 0000000000..3499b51375 --- /dev/null +++ b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/DoNotAllowAMutexToGoOutOfScopeWhileLocked.expected @@ -0,0 +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/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..a883a9e267 --- /dev/null +++ b/cpp/common/test/rules/donotallowamutextogooutofscopewhilelocked/test.cpp @@ -0,0 +1,141 @@ +#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); + } +} + +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; // 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 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/common/test/rules/donotdestroyamutexwhileitislocked/test.cpp b/cpp/common/test/rules/donotdestroyamutexwhileitislocked/test.cpp new file mode 100644 index 0000000000..f069315ebe --- /dev/null +++ b/cpp/common/test/rules/donotdestroyamutexwhileitislocked/test.cpp @@ -0,0 +1,146 @@ +#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]; + 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(); // 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); + } + + 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); + } +} + +// 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/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected b/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected new file mode 100644 index 0000000000..986186782b --- /dev/null +++ b/cpp/common/test/rules/preservesafetywhenusingconditionvariables/PreserveSafetyWhenUsingConditionVariables.expected @@ -0,0 +1 @@ +| test.cpp:19:8:19:17 | call to notify_one | Possible unsafe usage of single dispatch notification which can lead to deadlocking of threads. | 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 diff --git a/rule_packages/c/Concurrency3.json b/rule_packages/c/Concurrency3.json new file mode 100644 index 0000000000..fccfd47e9d --- /dev/null +++ b/rule_packages/c/Concurrency3.json @@ -0,0 +1,88 @@ +{ + "CERT-C": { + "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", + "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": { + "obligation": "rule" + }, + "queries": [ + { + "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", + "shared_implementation_short_name": "PreserveSafetyWhenUsingConditionVariables", + "tags": [ + "correctness", + "concurrency" + ] + } + ], + "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": { + "obligation": "rule" + }, + "queries": [ + { + "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": [ + "correctness", + "concurrency" + ] + } + ], + "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 diff --git a/rule_packages/cpp/Concurrency.json b/rule_packages/cpp/Concurrency.json index 80bd09dfd8..6e5898ecd8 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" @@ -126,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" 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,