diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 83420bb65b..f027524374 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -197,6 +197,8 @@ "Concurrency1", "Concurrency2", "Concurrency3", + "Concurrency4", + "Concurrency5", "Conditionals", "Const", "DeadCode", diff --git a/c/cert/src/rules/ARR39-C/DoNotAddOrSubtractAScaledIntegerToAPointer.qhelp b/c/cert/src/rules/ARR39-C/DoNotAddOrSubtractAScaledIntegerToAPointer.qhelp new file mode 100644 index 0000000000..6a121d3dbd --- /dev/null +++ b/c/cert/src/rules/ARR39-C/DoNotAddOrSubtractAScaledIntegerToAPointer.qhelp @@ -0,0 +1,599 @@ + + +
+

Pointer arithmetic is appropriate only when the pointer argument refers to an array (see ARR37-C. Do not add or subtract an integer to a pointer to a non-array object), including an array of bytes. When performing pointer arithmetic, the size of the value to add to or subtract from a pointer is automatically scaled to the size of the type of the referenced array object. Adding or subtracting a scaled integer value to or from a pointer is invalid because it may yield a pointer that does not point to an element within or one past the end of the array. (See ARR30-C. Do not form or use out-of-bounds pointers or array subscripts.)

+

Adding a pointer to an array of a type other than character to the result of the sizeof operator or offsetof macro, which returns a size and an offset, respectively, violates this rule. However, adding an array pointer to the number of array elements, for example, by using the arr[sizeof(arr)/sizeof(arr[0])]) idiom, is allowed provided that arr refers to an array and not a pointer.

+
+
+

In this noncompliant code example, sizeof(buf) is added to the array buf. This example is noncompliant because sizeof(buf) is scaled by int and then scaled again when added to buf.

+ enum { INTBUFSIZE = 80 }; + +extern int getdata(void); +int buf[INTBUFSIZE]; + +void func(void) { + int *buf_ptr = buf; + + while (buf_ptr < (buf + sizeof(buf))) { + *buf_ptr++ = getdata(); + } +} +
+
+

This compliant solution uses an unscaled integer to obtain a pointer to the end of the array:

+ enum { INTBUFSIZE = 80 }; + +extern int getdata(void); +int buf[INTBUFSIZE]; + +void func(void) { + int *buf_ptr = buf; + + while (buf_ptr < (buf + INTBUFSIZE)) { + *buf_ptr++ = getdata(); + } +} +
+
+

In this noncompliant code example, skip is added to the pointer s. However, skip represents the byte offset of ull_b in struct big. When added to s, skip is scaled by the size of struct big.

+ #include <string.h> +#include <stdlib.h> +#include <stddef.h> + +struct big { + unsigned long long ull_a; + unsigned long long ull_b; + unsigned long long ull_c; + int si_e; + int si_f; +}; + +void func(void) { + size_t skip = offsetof(struct big, ull_b); + struct big *s = (struct big *)malloc(sizeof(struct big)); + if (s == NULL) { + /* Handle malloc() error */ + } + + memset(s + skip, 0, sizeof(struct big) - skip); + /* ... */ + free(s); + s = NULL; +} +
+
+

This compliant solution uses an unsigned char * to calculate the offset instead of using a struct big *, which would result in scaled arithmetic:

+ #include <string.h> +#include <stdlib.h> +#include <stddef.h> + +struct big { + unsigned long long ull_a; + unsigned long long ull_b; + unsigned long long ull_c; + int si_d; + int si_e; +}; + +void func(void) { + size_t skip = offsetof(struct big, ull_b); + unsigned char *ptr = (unsigned char *)malloc( + sizeof(struct big) + ); + if (ptr == NULL) { + /* Handle malloc() error */ + } + + memset(ptr + skip, 0, sizeof(struct big) - skip); + /* ... */ + free(ptr); + ptr = NULL; +} +
+
+

In this noncompliant code example, wcslen(error_msg) * sizeof(wchar_t) bytes are scaled by the size of wchar_t when added to error_msg:

+ #include <wchar.h> +#include <stdio.h> + +enum { WCHAR_BUF = 128 }; + +void func(void) { + wchar_t error_msg[WCHAR_BUF]; + + wcscpy(error_msg, L"Error: "); + fgetws(error_msg + wcslen(error_msg) * sizeof(wchar_t), + WCHAR_BUF - 7, stdin); + /* ... */ +} +
+
+

This compliant solution does not scale the length of the string; wcslen() returns the number of characters and the addition to error_msg is scaled:

+ #include <wchar.h> +#include <stdio.h> + +enum { WCHAR_BUF = 128 }; +const wchar_t ERROR_PREFIX[7] = L"Error: "; + +void func(void) { + const size_t prefix_len = wcslen(ERROR_PREFIX); + wchar_t error_msg[WCHAR_BUF]; + + wcscpy(error_msg, ERROR_PREFIX); + fgetws(error_msg + prefix_len, + WCHAR_BUF - prefix_len, stdin); + /* ... */ +} +
+
+

Failure to understand and properly use pointer arithmetic can allow an attacker to execute arbitrary code.

+ + + + + + + + + + + + + + + + + + + +
+ Rule + + Severity + + Likelihood + + Remediation Cost + + Priority + + Level +
+ ARR39-C + + High + + Probable + + High + + P6 + + L2 +
+
+
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
+ Tool + + Version + + Checker + + Description +
+ + Astrée + + + 22.04 + + scaled-pointer-arithmetic + + Partially checked + Besides direct rule violations, Astrée reports all (resulting) out-of-bound array accesses. +
+ + Axivion Bauhaus Suite + + + 7.2.0 + + CertC-ARR39 + + Fully implemented +
+ + CodeSonar + + + 7.1p0 + + LANG.MEM.BO + LANG.MEM.BU + LANG.MEM.TBA + LANG.MEM.TO + LANG.MEM.TULANG.STRUCT.PARITH + LANG.STRUCT.PBB + LANG.STRUCT.PPE + BADFUNC.BO.* + + Buffer overrun + Buffer underrun + Tainted buffer access + Type overrun + Type underrun + Pointer Arithmetic + Pointer before beginning of object + Pointer past end of object + A collection of warning classes that report uses of library functions prone to internal buffer overflows. +
+ + Coverity + + + 2017.07 + + BAD_SIZEOF + + Partially implemented +
+ + Helix QAC + + + 2022.2 + + C4955, C4956, C4957 + C++4955, C++4956, C++4957 + +
+ + Klocwork + + + 2022.2 + + MISRA.PTR.ARITH.2012 + +
+ + LDRA tool suite + + + 9.7.1 + + 47 S, 489 S, 567 S,64 X, 66 X, 68 X,69 X, 70 X, 71 X + + Partially implemented +
+ + Parasoft C/C++test + + + 2022.1 + + CERT_C-ARR39-a + CERT_C-ARR39-b + CERT_C-ARR39-c + + Avoid accessing arrays out of bounds + Pointer arithmetic should not be used + Do not add or subtract a scaled integer to a pointer +
+ Polyspace Bug Finder + + R2022a + + + CERT C: Rule ARR39-C + + + Checks for: + Incorrect pointer scalingncorrect pointer scaling, pointer access out of boundsointer access out of bounds, possible misuse of sizeofossible misuse of sizeof. + Rule partially covered. +
+ + PRQA QA-C + + + 9.7 + + 4955, 4956, 4957 + +
+ + PRQA QA-C++ + + + 4.4 + + 4955, 4956, 4957 + +
+ + RuleChecker + + + 22.04 + + scaled-pointer-arithmetic + + Partially checked +
+ + TrustInSoft Analyzer + + + 1.38 + + index_in_address + + Exhaustively detects undefined behavior (see + + one compliant and one non-compliant example + + ). +
+
+
+

Search for vulnerabilities resulting from the violation of this rule on the CERT website.

+
+
+

Key here (explains table format and definitions)

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
+ Taxonomy + + Taxonomy item + + Relationship +
+ + CERT C Secure Coding Standard + + + + ARR30-C. Do not form or use out-of-bounds pointers or array subscripts + + + Prior to 2018-01-12: CERT: Unspecified Relationship +
+ + CERT C Secure Coding Standard + + + + ARR37-C. Do not add or subtract an integer to a pointer to a non-array object + + + Prior to 2018-01-12: CERT: Unspecified Relationship +
+ + ISO/IEC TR 24772:2013 + + + Pointer Casting and Pointer Type Changes [HFC] + + Prior to 2018-01-12: CERT: Unspecified Relationship +
+ + ISO/IEC TR 24772:2013 + + + Pointer Arithmetic [RVG] + + Prior to 2018-01-12: CERT: Unspecified Relationship +
+ + MISRA C:2012 + + + Rule 18.1 (required) + + Prior to 2018-01-12: CERT: Unspecified Relationship +
+ + MISRA C:2012 + + + Rule 18.2 (required) + + Prior to 2018-01-12: CERT: Unspecified Relationship +
+ + MISRA C:2012 + + + Rule 18.3 (required) + + Prior to 2018-01-12: CERT: Unspecified Relationship +
+ + MISRA C:2012 + + + Rule 18.4 (advisory) + + Prior to 2018-01-12: CERT: Unspecified Relationship +
+ + CWE 2.11 + + + + CWE-468 + + , Incorrect Pointer Scaling + + 2017-07-07: CERT: Exact +
+
+
+ + + + + + + + + + + +
+ [ + + Dowd 2006 + + ] + + Chapter 6, "C Language Issues" +
+ [ + + Murenin 07 + + ] + +
+
+
\ No newline at end of file diff --git a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md new file mode 100644 index 0000000000..8f4dec4c3e --- /dev/null +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md @@ -0,0 +1,180 @@ +# CON30-C: Clean up thread-specific storage + +This query implements the CERT-C rule CON30-C: + +> Clean up thread-specific storage + + +## Description + +The `tss_create()` function creates a thread-specific storage pointer identified by a key. Threads can allocate thread-specific storage and associate the storage with a key that uniquely identifies the storage by calling the `tss_set()` function. If not properly freed, this memory may be leaked. Ensure that thread-specific storage is freed. + +## Noncompliant Code Example + +In this noncompliant code example, each thread dynamically allocates storage in the `get_data()` function, which is then associated with the global key by the call to `tss_set()` in the `add_data()` function. This memory is subsequently leaked when the threads terminate. + +```cpp +#include +#include + +/* Global key to the thread-specific storage */ +tss_t key; +enum { MAX_THREADS = 3 }; + +int *get_data(void) { + int *arr = (int *)malloc(2 * sizeof(int)); + if (arr == NULL) { + return arr; /* Report error */ + } + arr[0] = 10; + arr[1] = 42; + return arr; +} + +int add_data(void) { + int *data = get_data(); + if (data == NULL) { + return -1; /* Report error */ + } + + if (thrd_success != tss_set(key, (void *)data)) { + /* Handle error */ + } + return 0; +} + +void print_data(void) { + /* Get this thread's global data from key */ + int *data = tss_get(key); + + if (data != NULL) { + /* Print data */ + } +} + +int function(void *dummy) { + if (add_data() != 0) { + return -1; /* Report error */ + } + print_data(); + return 0; +} + +int main(void) { + thrd_t thread_id[MAX_THREADS]; + + /* Create the key before creating the threads */ + if (thrd_success != tss_create(&key, NULL)) { + /* Handle error */ + } + + /* Create threads that would store specific storage */ + for (size_t i = 0; i < MAX_THREADS; i++) { + if (thrd_success != thrd_create(&thread_id[i], function, NULL)) { + /* Handle error */ + } + } + + for (size_t i = 0; i < MAX_THREADS; i++) { + if (thrd_success != thrd_join(thread_id[i], NULL)) { + /* Handle error */ + } + } + + tss_delete(key); + return 0; +} + +``` + +## Compliant Solution + +In this compliant solution, each thread explicitly frees the thread-specific storage returned by the `tss_get()` function before terminating: + +```cpp +#include +#include + +/* Global key to the thread-specific storage */ +tss_t key; + +int function(void *dummy) { + if (add_data() != 0) { + return -1; /* Report error */ + } + print_data(); + free(tss_get(key)); + return 0; +} + +/* ... Other functions are unchanged */ + +``` + +## Compliant Solution + +This compliant solution invokes a destructor function registered during the call to `tss_create()` to automatically free any thread-specific storage: + +```cpp +#include +#include + +/* Global key to the thread-specific storage */ +tss_t key; +enum { MAX_THREADS = 3 }; + +/* ... Other functions are unchanged */ + +void destructor(void *data) { + free(data); +} + +int main(void) { + thrd_t thread_id[MAX_THREADS]; + + /* Create the key before creating the threads */ + if (thrd_success != tss_create(&key, destructor)) { + /* Handle error */ + } + + /* Create threads that would store specific storage */ + for (size_t i = 0; i < MAX_THREADS; i++) { + if (thrd_success != thrd_create(&thread_id[i], function, NULL)) { + /* Handle error */ + } + } + + for (size_t i = 0; i < MAX_THREADS; i++) { + if (thrd_success != thrd_join(thread_id[i], NULL)) { + /* Handle error */ + } + } + + tss_delete(key); + return 0; +} +``` + +## Risk Assessment + +Failing to free thread-specific objects results in memory leaks and could result in a [denial-of-service attack](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-denial-of-service). + +
Rule Severity Likelihood Remediation Cost Priority Level
CON30-C Medium Unlikely Medium P4 L3
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 Supported, but no explicit checker
CodeSonar 7.1p0 ALLOC.LEAK Leak
Coverity 2017.07 ALLOC_FREE_MISMATCH Partially implemented, correct implementation is more involved
Helix QAC 2022.2 C1780, C1781, C1782, C1783, C1784
Parasoft C/C++test 2022.1 CERT_C-CON30-a Ensure resources are freed
Polyspace Bug Finder R2022a CERT C: Rule CON30-C Checks for thread-specific memory leak (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+CON30-C). + +## Implementation notes + +This query does not attempt to ensure that the deallocation function in fact deallocates memory and instead assumes the contract is valid. Additionally, this query requires that all `tss_create` calls are bookended by calls to `tss_delete`, even if a thread is not created. + +## References + +* CERT-C: [CON30-C: Clean up thread-specific storage](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql new file mode 100644 index 0000000000..55f4afe7d8 --- /dev/null +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql @@ -0,0 +1,71 @@ +/** + * @id c/cert/clean-up-thread-specific-storage + * @name CON30-C: Clean up thread-specific storage + * @description Failing to clean up thread-specific resources can lead to unpredictable program + * behavior. + * @kind problem + * @precision medium + * @problem.severity error + * @tags external/cert/id/con30-c + * correctness + * concurrency + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.Concurrency +import semmle.code.cpp.dataflow.TaintTracking +import semmle.code.cpp.dataflow.DataFlow + +class TssCreateToTssDeleteDataFlowConfiguration extends DataFlow::Configuration { + TssCreateToTssDeleteDataFlowConfiguration() { this = "TssCreateToTssDeleteDataFlowConfiguration" } + + override predicate isSource(DataFlow::Node node) { + exists(TSSCreateFunctionCall tsc, Expr e | + // the only requirement of the source is that at some point + // it refers to the key of a create statement + e.getParent*() = tsc.getKey() and + (e = node.asDefiningArgument() or e = node.asExpr()) + ) + } + + override predicate isSink(DataFlow::Node node) { + exists(TSSDeleteFunctionCall tsd, Expr e | + // the only requirement of a sink is that at some point + // it references the key of a delete call. + e.getParent*() = tsd.getKey() and + (e = node.asDefiningArgument() or e = node.asExpr()) + ) + } +} + +from TSSCreateFunctionCall tcfc +where + not isExcluded(tcfc, Concurrency4Package::cleanUpThreadSpecificStorageQuery()) and + // all calls to `tss_create` must be bookended by calls to tss_delete + // even if a thread is not created. + not exists(TssCreateToTssDeleteDataFlowConfiguration config | + config.hasFlow(DataFlow::definitionByReferenceNodeFromArgument(tcfc.getKey()), _) + or + config.hasFlow(DataFlow::exprNode(tcfc.getKey()), _) + ) + or + // if a thread is created, we must check additional items + exists(C11ThreadCreateCall tcc | + tcfc.getASuccessor*() = tcc and + if tcfc.hasDeallocator() + then + // if they specify a deallocator, they must wait for this thread to finish, otherwise + // automatic calls to the deallocator will not work. + not exists(ThreadWait tw | tcc.getASuccessor*() = tw) + or + // freeing memory twice can lead to errors; because of this we report cases + // where a deallocator is specified but free is called explicitly. + getAThreadSpecificStorageDeallocationCall(tcc, _) + else + // otherwise, we require that the thread that gets called calls a free like + // function with the argument of a `tss_get` call. + not getAThreadSpecificStorageDeallocationCall(tcc, _) + ) +select tcfc, "Resources used by thread specific storage may not be cleaned up." diff --git a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md new file mode 100644 index 0000000000..68fe49222d --- /dev/null +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md @@ -0,0 +1,393 @@ +# CON34-C: Declare objects shared between threads with appropriate storage durations + +This query implements the CERT-C rule CON34-C: + +> Declare objects shared between threads with appropriate storage durations + + +## Description + +Accessing the automatic or thread-local variables of one thread from another thread is [implementation-defined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior) and can cause invalid memory accesses because the execution of threads can be interwoven within the constraints of the synchronization model. As a result, the referenced stack frame or thread-local variable may no longer be valid when another thread tries to access it. Shared static variables can be protected by thread synchronization mechanisms. + +However, automatic (local) variables cannot be shared in the same manner because the referenced stack frame's thread would need to stop executing, or some other mechanism must be employed to ensure that the referenced stack frame is still valid. Do not access automatic or thread-local objects from a thread other than the one with which the object is associated. See [DCL30-C. Declare objects with appropriate storage durations](https://wiki.sei.cmu.edu/confluence/display/c/DCL30-C.+Declare+objects+with+appropriate+storage+durations) for information on how to declare objects with appropriate storage durations when data is not being shared between threads. + +Noncompliant Code Example (Automatic Storage Duration) + +This noncompliant code example passes the address of a variable to a child thread, which prints it out. The variable has automatic storage duration. Depending on the execution order, the child thread might reference the variable after the variable's lifetime in the parent thread. This would cause the child thread to access an invalid memory location. + +```cpp +#include +#include + +int child_thread(void *val) { + int *res = (int *)val; + printf("Result: %d\n", *res); + return 0; +} + +void create_thread(thrd_t *tid) { + int val = 1; + if (thrd_success != thrd_create(tid, child_thread, &val)) { + /* Handle error */ + } +} + +int main(void) { + thrd_t tid; + create_thread(&tid); + + if (thrd_success != thrd_join(tid, NULL)) { + /* Handle error */ + } + return 0; +} + +``` + +## Noncompliant Code Example (Automatic Storage Duration) + +One practice is to ensure that all objects with automatic storage duration shared between threads are declared such that their lifetime extends past the lifetime of the threads. This can be accomplished using a thread synchronization mechanism, such as `thrd_join()`. In this code example, `val` is declared in `main()`, where `thrd_join()` is called. Because the parent thread waits until the child thread completes before continuing its execution, the shared objects have a lifetime at least as great as the thread. + +```cpp +#include +#include + +int child_thread(void *val) { + int *result = (int *)val; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return 0; +} + +void create_thread(thrd_t *tid, int *val) { + if (thrd_success != thrd_create(tid, child_thread, val)) { + /* Handle error */ + } +} + +int main(void) { + int val = 1; + thrd_t tid; + create_thread(&tid, &val); + if (thrd_success != thrd_join(tid, NULL)) { + /* Handle error */ + } + return 0; +} +``` + +## + +However, the C Standard, 6.2.4 paragraphs 4 and 5 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography)\], states: + +> The result of attempting to indirectly access an object with thread storage duration from a thread other than the one with which the object is associated is implementation-defined. . . . + + +The result of attempting to indirectly access an object with automatic storage duration from a thread other than the one with which the object is associated is implementation-defined. + +Therefore this example relies on [implementation-defined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior) and is nonportable. + +## Compliant Solution (Static Storage Duration) + +This compliant solution stores the value in an object having static storage duration. The lifetime of this object is the entire execution of the program; consequently, it can be safely accessed by any thread. + +```cpp +#include +#include + +int child_thread(void *v) { + int *result = (int *)v; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return 0; +} + +void create_thread(thrd_t *tid) { + static int val = 1; + if (thrd_success != thrd_create(tid, child_thread, &val)) { + /* Handle error */ + } +} + +int main(void) { + thrd_t tid; + create_thread(&tid); + if (thrd_success != thrd_join(tid, NULL)) { + /* Handle error */ + } + return 0; +} + +``` + +## Compliant Solution (Allocated Storage Duration) + +This compliant solution stores the value passed to the child thread in a dynamically allocated object. Because this object will persist until explicitly freed, the child thread can safely access its value. + +```cpp +#include +#include +#include + +int child_thread(void *val) { + int *result = (int *)val; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return 0; +} + +void create_thread(thrd_t *tid, int *value) { + *value = 1; + if (thrd_success != thrd_create(tid, child_thread, + value)) { + /* Handle error */ + } +} + +int main(void) { + thrd_t tid; + int *value = (int *)malloc(sizeof(int)); + if (!value) { + /* Handle error */ + } + create_thread(&tid, value); + if (thrd_success != thrd_join(tid, NULL)) { + /* Handle error */ + } + free(value); + return 0; +} + +``` + +## Noncompliant Code Example (Thread-Specific Storage) + +In this noncompliant code example, the value is stored in thread-specific storage of the parent thread. However, because thread-specific data is available only to the thread that stores it, the `child_thread()` function will set `result` to a null value. + +```cpp +#include +#include +#include + +static tss_t key; + +int child_thread(void *v) { + void *result = tss_get(*(tss_t *)v); + printf("Result: %d\n", *(int *)result); + return 0; +} + +int create_thread(void *thrd) { + int *val = (int *)malloc(sizeof(int)); + if (val == NULL) { + /* Handle error */ + } + *val = 1; + if (thrd_success != tss_set(key, val)) { + /* Handle error */ + } + if (thrd_success != thrd_create((thrd_t *)thrd, + child_thread, &key)) { + /* Handle error */ + } + return 0; +} + +int main(void) { + thrd_t parent_tid, child_tid; + + if (thrd_success != tss_create(&key, free)) { + /* Handle error */ + } + if (thrd_success != thrd_create(&parent_tid, create_thread, + &child_tid)) { + /* Handle error */ + } + if (thrd_success != thrd_join(parent_tid, NULL)) { + /* Handle error */ + } + if (thrd_success != thrd_join(child_tid, NULL)) { + /* Handle error */ + } + tss_delete(key); + return 0; +} +``` + +## Compliant Solution (Thread-Specific Storage) + +This compliant solution illustrates how thread-specific storage can be combined with a call to a thread synchronization mechanism, such as `thrd_join()`. Because the parent thread waits until the child thread completes before continuing its execution, the child thread is guaranteed to access a valid live object. + +```cpp +#include +#include +#include + +static tss_t key; + +int child_thread(void *v) { + int *result = v; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return 0; +} + +int create_thread(void *thrd) { + int *val = (int *)malloc(sizeof(int)); + if (val == NULL) { + /* Handle error */ + } + *val = 1; + if (thrd_success != tss_set(key, val)) { + /* Handle error */ + } + /* ... */ + void *v = tss_get(key); + if (thrd_success != thrd_create((thrd_t *)thrd, + child_thread, v)) { + /* Handle error */ + } + return 0; +} + +int main(void) { + thrd_t parent_tid, child_tid; + + if (thrd_success != tss_create(&key, free)) { + /* Handle error */ + } + if (thrd_success != thrd_create(&parent_tid, create_thread, + &child_tid)) { + /* Handle error */ + } + if (thrd_success != thrd_join(parent_tid, NULL)) { + /* Handle error */ + } + if (thrd_success != thrd_join(child_tid, NULL)) { + /* Handle error */ + } + tss_delete(key); +return 0; +} +``` +This compliant solution uses pointer-to-integer and integer-to-pointer conversions, which have implementation-defined behavior. (See [INT36-C. Converting a pointer to integer or integer to pointer](https://wiki.sei.cmu.edu/confluence/display/c/INT36-C.+Converting+a+pointer+to+integer+or+integer+to+pointer).) + +## Compliant Solution (Thread-Local Storage, Windows, Visual Studio) + +Similar to the preceding compliant solution, this compliant solution uses thread-local storage combined with thread synchronization to ensure the child thread is accessing a valid live object. It uses the Visual Studio–specific [__declspec(thread)](http://msdn.microsoft.com/en-us/library/9w1sdazb.aspx) language extension to provide the thread-local storage and the `[WaitForSingleObject()](http://msdn.microsoft.com/en-us/library/windows/desktop/ms687032(v=vs.85).aspx)` API to provide the synchronization. + +```cpp +#include +#include + +DWORD WINAPI child_thread(LPVOID v) { + int *result = (int *)v; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return NULL; +} + +int create_thread(HANDLE *tid) { + /* Declare val as a thread-local value */ + __declspec(thread) int val = 1; + *tid = create_thread(NULL, 0, child_thread, &val, 0, NULL); + return *tid == NULL; +} + +int main(void) { + HANDLE tid; + + if (create_thread(&tid)) { + /* Handle error */ + } + + if (WAIT_OBJECT_0 != WaitForSingleObject(tid, INFINITE)) { + /* Handle error */ + } + CloseHandle(tid); + + return 0; +} + +``` + +## Noncompliant Code Example (OpenMP, parallel) + +It is important to note that local data can be used securely with threads when using other thread interfaces, so the programmer need not always copy data into nonlocal memory when sharing data with threads. For example, the `shared` keyword in *®The OpenMP API Specification for Parallel Programming* \[[OpenMP](http://openmp.org/wp/)\] can be used in combination with OpenMP's threading interface to share local memory without having to worry about whether local automatic variables remain valid. + +In this noncompliant code example, a variable `j` is declared outside a `parallel` `#pragma` and not listed as a private variable. In OpenMP, variables outside a `parallel #pragma` are shared unless designated as `private`. + +```cpp +#include +#include + +int main(void) { + int j = 0; + #pragma omp parallel + { + int t = omp_get_thread_num(); + printf("Running thread - %d\n", t); + for (int i = 0; i < 5050; i++) { + j++; /* j not private; could be a race condition */ + } + printf("Just ran thread - %d\n", t); + printf("loop count %d\n", j); + } +return 0; +} +``` + +## Compliant Solution (OpenMP, parallel, private) + +In this compliant solution, the variable `j` is declared outside of the `parallel` `#pragma` but is explicitly labeled as `private`: + +```cpp +#include +#include + +int main(void) { + int j = 0; + #pragma omp parallel private(j) + { + int t = omp_get_thread_num(); + printf("Running thread - %d\n", t); + for (int i = 0; i < 5050; i++) { + j++; + } + printf("Just ran thread - %d\n", t); + printf("loop count %d\n", j); + } +return 0; +} +``` + +## Risk Assessment + +Threads that reference the stack of other threads can potentially overwrite important information on the stack, such as function pointers and return addresses. The compiler may not generate warnings if the programmer allows one thread to access another thread's local variables, so a programmer may not catch a potential error at compile time. The remediation cost for this error is high because analysis tools have difficulty diagnosing problems with concurrency and race conditions. + +
Recommendation Severity Likelihood Remediation Cost Priority Level
CON34-C Medium Probable High P4 L3
+ + +## Automated Detection + +
Tool Version Checker Description
CodeSonar 7.1p0 CONCURRENCY.LOCALARG Local Variable Passed to Thread
Helix QAC 2022.2 C4926, C4927, C4928
Parasoft C/C++test 2022.1 CERT_C-CON34-a Declare objects shared between POSIX threads with appropriate storage durations
Polyspace Bug Finder R2022a CERT C: Rule CON34-C Checks for automatic or thread local variable escaping from a C11 thread (rule fully covered)
PRQA QA-C 9.7 4926, 4927, 4928 Enforced by QAC
+ + +## 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+CON34-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 C Secure Coding Standard DCL30-C. Declare objects with appropriate storage durations Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] 6.2.4, "Storage Durations of Objects"
\[ OpenMP \] ® The OpenMP API Specification for Parallel Programming
+ + +## Implementation notes + +This query does not consider Windows implementations or OpenMP implementations. This query is primarily about excluding cases wherein the storage duration of a variable is appropriate. As such, this query is not concerned if the appropriate synchronization mechanisms are used, such as sequencing calls to `thrd_join` and `free`. An audit query is supplied to handle some of those cases. + +## References + +* CERT-C: [CON34-C: Declare objects shared between threads with appropriate storage durations](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql new file mode 100644 index 0000000000..71138f4ff8 --- /dev/null +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql @@ -0,0 +1,43 @@ +/** + * @id c/cert/appropriate-thread-object-storage-durations + * @name CON34-C: Declare objects shared between threads with appropriate storage durations + * @description Accessing thread-local variables with automatic storage durations can lead to + * unpredictable program behavior. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/con34-c + * correctness + * concurrency + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.Concurrency +import semmle.code.cpp.dataflow.TaintTracking +import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.commons.Alloc + +from C11ThreadCreateCall tcc, StackVariable sv, Expr arg, Expr acc +where + not isExcluded(tcc, Concurrency4Package::appropriateThreadObjectStorageDurationsQuery()) and + tcc.getArgument(2) = arg and + sv.getAnAccess() = acc and + // a stack variable that is given as an argument to a thread + TaintTracking::localTaint(DataFlow::exprNode(acc), DataFlow::exprNode(arg)) and + // or isn't one of the allowed usage patterns + not exists(Expr mfc | + isAllocationExpr(mfc) and + sv.getAnAssignedValue() = mfc and + acc.getAPredecessor*() = mfc + ) and + not exists(TSSGetFunctionCall tsg, TSSSetFunctionCall tss, DataFlow::Node src | + sv.getAnAssignedValue() = tsg and + acc.getAPredecessor*() = tsg and + // there should be dataflow from somewhere (the same somewhere) + // into each of the first arguments + DataFlow::localFlow(src, DataFlow::exprNode(tsg.getArgument(0))) and + DataFlow::localFlow(src, DataFlow::exprNode(tss.getArgument(0))) + ) +select tcc, "$@ not declared with appropriate storage duration", arg, "Shared object" diff --git a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md new file mode 100644 index 0000000000..75ca7635c6 --- /dev/null +++ b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md @@ -0,0 +1,393 @@ +# CON34-C: (Audit) Declare objects shared between threads with appropriate storage durations + +This query implements the CERT-C rule CON34-C: + +> Declare objects shared between threads with appropriate storage durations + + +## Description + +Accessing the automatic or thread-local variables of one thread from another thread is [implementation-defined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior) and can cause invalid memory accesses because the execution of threads can be interwoven within the constraints of the synchronization model. As a result, the referenced stack frame or thread-local variable may no longer be valid when another thread tries to access it. Shared static variables can be protected by thread synchronization mechanisms. + +However, automatic (local) variables cannot be shared in the same manner because the referenced stack frame's thread would need to stop executing, or some other mechanism must be employed to ensure that the referenced stack frame is still valid. Do not access automatic or thread-local objects from a thread other than the one with which the object is associated. See [DCL30-C. Declare objects with appropriate storage durations](https://wiki.sei.cmu.edu/confluence/display/c/DCL30-C.+Declare+objects+with+appropriate+storage+durations) for information on how to declare objects with appropriate storage durations when data is not being shared between threads. + +Noncompliant Code Example (Automatic Storage Duration) + +This noncompliant code example passes the address of a variable to a child thread, which prints it out. The variable has automatic storage duration. Depending on the execution order, the child thread might reference the variable after the variable's lifetime in the parent thread. This would cause the child thread to access an invalid memory location. + +```cpp +#include +#include + +int child_thread(void *val) { + int *res = (int *)val; + printf("Result: %d\n", *res); + return 0; +} + +void create_thread(thrd_t *tid) { + int val = 1; + if (thrd_success != thrd_create(tid, child_thread, &val)) { + /* Handle error */ + } +} + +int main(void) { + thrd_t tid; + create_thread(&tid); + + if (thrd_success != thrd_join(tid, NULL)) { + /* Handle error */ + } + return 0; +} + +``` + +## Noncompliant Code Example (Automatic Storage Duration) + +One practice is to ensure that all objects with automatic storage duration shared between threads are declared such that their lifetime extends past the lifetime of the threads. This can be accomplished using a thread synchronization mechanism, such as `thrd_join()`. In this code example, `val` is declared in `main()`, where `thrd_join()` is called. Because the parent thread waits until the child thread completes before continuing its execution, the shared objects have a lifetime at least as great as the thread. + +```cpp +#include +#include + +int child_thread(void *val) { + int *result = (int *)val; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return 0; +} + +void create_thread(thrd_t *tid, int *val) { + if (thrd_success != thrd_create(tid, child_thread, val)) { + /* Handle error */ + } +} + +int main(void) { + int val = 1; + thrd_t tid; + create_thread(&tid, &val); + if (thrd_success != thrd_join(tid, NULL)) { + /* Handle error */ + } + return 0; +} +``` + +## + +However, the C Standard, 6.2.4 paragraphs 4 and 5 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography)\], states: + +> The result of attempting to indirectly access an object with thread storage duration from a thread other than the one with which the object is associated is implementation-defined. . . . + + +The result of attempting to indirectly access an object with automatic storage duration from a thread other than the one with which the object is associated is implementation-defined. + +Therefore this example relies on [implementation-defined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior) and is nonportable. + +## Compliant Solution (Static Storage Duration) + +This compliant solution stores the value in an object having static storage duration. The lifetime of this object is the entire execution of the program; consequently, it can be safely accessed by any thread. + +```cpp +#include +#include + +int child_thread(void *v) { + int *result = (int *)v; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return 0; +} + +void create_thread(thrd_t *tid) { + static int val = 1; + if (thrd_success != thrd_create(tid, child_thread, &val)) { + /* Handle error */ + } +} + +int main(void) { + thrd_t tid; + create_thread(&tid); + if (thrd_success != thrd_join(tid, NULL)) { + /* Handle error */ + } + return 0; +} + +``` + +## Compliant Solution (Allocated Storage Duration) + +This compliant solution stores the value passed to the child thread in a dynamically allocated object. Because this object will persist until explicitly freed, the child thread can safely access its value. + +```cpp +#include +#include +#include + +int child_thread(void *val) { + int *result = (int *)val; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return 0; +} + +void create_thread(thrd_t *tid, int *value) { + *value = 1; + if (thrd_success != thrd_create(tid, child_thread, + value)) { + /* Handle error */ + } +} + +int main(void) { + thrd_t tid; + int *value = (int *)malloc(sizeof(int)); + if (!value) { + /* Handle error */ + } + create_thread(&tid, value); + if (thrd_success != thrd_join(tid, NULL)) { + /* Handle error */ + } + free(value); + return 0; +} + +``` + +## Noncompliant Code Example (Thread-Specific Storage) + +In this noncompliant code example, the value is stored in thread-specific storage of the parent thread. However, because thread-specific data is available only to the thread that stores it, the `child_thread()` function will set `result` to a null value. + +```cpp +#include +#include +#include + +static tss_t key; + +int child_thread(void *v) { + void *result = tss_get(*(tss_t *)v); + printf("Result: %d\n", *(int *)result); + return 0; +} + +int create_thread(void *thrd) { + int *val = (int *)malloc(sizeof(int)); + if (val == NULL) { + /* Handle error */ + } + *val = 1; + if (thrd_success != tss_set(key, val)) { + /* Handle error */ + } + if (thrd_success != thrd_create((thrd_t *)thrd, + child_thread, &key)) { + /* Handle error */ + } + return 0; +} + +int main(void) { + thrd_t parent_tid, child_tid; + + if (thrd_success != tss_create(&key, free)) { + /* Handle error */ + } + if (thrd_success != thrd_create(&parent_tid, create_thread, + &child_tid)) { + /* Handle error */ + } + if (thrd_success != thrd_join(parent_tid, NULL)) { + /* Handle error */ + } + if (thrd_success != thrd_join(child_tid, NULL)) { + /* Handle error */ + } + tss_delete(key); + return 0; +} +``` + +## Compliant Solution (Thread-Specific Storage) + +This compliant solution illustrates how thread-specific storage can be combined with a call to a thread synchronization mechanism, such as `thrd_join()`. Because the parent thread waits until the child thread completes before continuing its execution, the child thread is guaranteed to access a valid live object. + +```cpp +#include +#include +#include + +static tss_t key; + +int child_thread(void *v) { + int *result = v; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return 0; +} + +int create_thread(void *thrd) { + int *val = (int *)malloc(sizeof(int)); + if (val == NULL) { + /* Handle error */ + } + *val = 1; + if (thrd_success != tss_set(key, val)) { + /* Handle error */ + } + /* ... */ + void *v = tss_get(key); + if (thrd_success != thrd_create((thrd_t *)thrd, + child_thread, v)) { + /* Handle error */ + } + return 0; +} + +int main(void) { + thrd_t parent_tid, child_tid; + + if (thrd_success != tss_create(&key, free)) { + /* Handle error */ + } + if (thrd_success != thrd_create(&parent_tid, create_thread, + &child_tid)) { + /* Handle error */ + } + if (thrd_success != thrd_join(parent_tid, NULL)) { + /* Handle error */ + } + if (thrd_success != thrd_join(child_tid, NULL)) { + /* Handle error */ + } + tss_delete(key); +return 0; +} +``` +This compliant solution uses pointer-to-integer and integer-to-pointer conversions, which have implementation-defined behavior. (See [INT36-C. Converting a pointer to integer or integer to pointer](https://wiki.sei.cmu.edu/confluence/display/c/INT36-C.+Converting+a+pointer+to+integer+or+integer+to+pointer).) + +## Compliant Solution (Thread-Local Storage, Windows, Visual Studio) + +Similar to the preceding compliant solution, this compliant solution uses thread-local storage combined with thread synchronization to ensure the child thread is accessing a valid live object. It uses the Visual Studio–specific [__declspec(thread)](http://msdn.microsoft.com/en-us/library/9w1sdazb.aspx) language extension to provide the thread-local storage and the `[WaitForSingleObject()](http://msdn.microsoft.com/en-us/library/windows/desktop/ms687032(v=vs.85).aspx)` API to provide the synchronization. + +```cpp +#include +#include + +DWORD WINAPI child_thread(LPVOID v) { + int *result = (int *)v; + printf("Result: %d\n", *result); /* Correctly prints 1 */ + return NULL; +} + +int create_thread(HANDLE *tid) { + /* Declare val as a thread-local value */ + __declspec(thread) int val = 1; + *tid = create_thread(NULL, 0, child_thread, &val, 0, NULL); + return *tid == NULL; +} + +int main(void) { + HANDLE tid; + + if (create_thread(&tid)) { + /* Handle error */ + } + + if (WAIT_OBJECT_0 != WaitForSingleObject(tid, INFINITE)) { + /* Handle error */ + } + CloseHandle(tid); + + return 0; +} + +``` + +## Noncompliant Code Example (OpenMP, parallel) + +It is important to note that local data can be used securely with threads when using other thread interfaces, so the programmer need not always copy data into nonlocal memory when sharing data with threads. For example, the `shared` keyword in *®The OpenMP API Specification for Parallel Programming* \[[OpenMP](http://openmp.org/wp/)\] can be used in combination with OpenMP's threading interface to share local memory without having to worry about whether local automatic variables remain valid. + +In this noncompliant code example, a variable `j` is declared outside a `parallel` `#pragma` and not listed as a private variable. In OpenMP, variables outside a `parallel #pragma` are shared unless designated as `private`. + +```cpp +#include +#include + +int main(void) { + int j = 0; + #pragma omp parallel + { + int t = omp_get_thread_num(); + printf("Running thread - %d\n", t); + for (int i = 0; i < 5050; i++) { + j++; /* j not private; could be a race condition */ + } + printf("Just ran thread - %d\n", t); + printf("loop count %d\n", j); + } +return 0; +} +``` + +## Compliant Solution (OpenMP, parallel, private) + +In this compliant solution, the variable `j` is declared outside of the `parallel` `#pragma` but is explicitly labeled as `private`: + +```cpp +#include +#include + +int main(void) { + int j = 0; + #pragma omp parallel private(j) + { + int t = omp_get_thread_num(); + printf("Running thread - %d\n", t); + for (int i = 0; i < 5050; i++) { + j++; + } + printf("Just ran thread - %d\n", t); + printf("loop count %d\n", j); + } +return 0; +} +``` + +## Risk Assessment + +Threads that reference the stack of other threads can potentially overwrite important information on the stack, such as function pointers and return addresses. The compiler may not generate warnings if the programmer allows one thread to access another thread's local variables, so a programmer may not catch a potential error at compile time. The remediation cost for this error is high because analysis tools have difficulty diagnosing problems with concurrency and race conditions. + +
Recommendation Severity Likelihood Remediation Cost Priority Level
CON34-C Medium Probable High P4 L3
+ + +## Automated Detection + +
Tool Version Checker Description
CodeSonar 7.1p0 CONCURRENCY.LOCALARG Local Variable Passed to Thread
Helix QAC 2022.2 C4926, C4927, C4928
Parasoft C/C++test 2022.1 CERT_C-CON34-a Declare objects shared between POSIX threads with appropriate storage durations
Polyspace Bug Finder R2022a CERT C: Rule CON34-C Checks for automatic or thread local variable escaping from a C11 thread (rule fully covered)
PRQA QA-C 9.7 4926, 4927, 4928 Enforced by QAC
+ + +## 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+CON34-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 C Secure Coding Standard DCL30-C. Declare objects with appropriate storage durations Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] 6.2.4, "Storage Durations of Objects"
\[ OpenMP \] ® The OpenMP API Specification for Parallel Programming
+ + +## Implementation notes + +None + +## References + +* CERT-C: [CON34-C: Declare objects shared between threads with appropriate storage durations](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql new file mode 100644 index 0000000000..ddcddb8dc5 --- /dev/null +++ b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql @@ -0,0 +1,35 @@ +/** + * @id c/cert/thread-object-storage-durations-not-initialized + * @name CON34-C: (Audit) Declare objects shared between threads with appropriate storage durations + * @description Storage durations not correctly initialized can cause unpredictable program + * behavior. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/con34-c + * external/cert/audit + * correctness + * concurrency + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.Concurrency +import semmle.code.cpp.dataflow.TaintTracking +import semmle.code.cpp.dataflow.DataFlow + +from TSSGetFunctionCall tsg, ThreadedFunction tf +where + not isExcluded(tsg, Concurrency4Package::threadObjectStorageDurationsNotInitializedQuery()) and + // from within a threaded function there is a call to tsg + tsg.getEnclosingFunction() = tf and + // however, there does not exist a proper sequencing. + not exists(TSSSetFunctionCall tss, DataFlow::Node src | + // there should be dataflow from somewhere (the same somewhere) + // into each of the first arguments + DataFlow::localFlow(src, DataFlow::exprNode(tsg.getArgument(0))) and + DataFlow::localFlow(src, DataFlow::exprNode(tss.getArgument(0))) + ) +select tsg, + "Call to a thread specific storage function from within a threaded context on an object that may not be owned by this thread." diff --git a/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected new file mode 100644 index 0000000000..e03b665a1c --- /dev/null +++ b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected @@ -0,0 +1,11 @@ +| test.c:27:3:27:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:49:3:49:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:71:3:71:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:87:3:87:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:95:3:95:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:135:3:135:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:139:3:139:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:143:3:143:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:147:3:147:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:151:3:151:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| test.c:155:3:155:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | diff --git a/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.qlref b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.qlref new file mode 100644 index 0000000000..da70a8d136 --- /dev/null +++ b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.qlref @@ -0,0 +1 @@ +rules/CON30-C/CleanUpThreadSpecificStorage.ql \ No newline at end of file diff --git a/c/cert/test/rules/CON30-C/test.c b/c/cert/test/rules/CON30-C/test.c new file mode 100644 index 0000000000..13d802388d --- /dev/null +++ b/c/cert/test/rules/CON30-C/test.c @@ -0,0 +1,156 @@ +#include +#include +#include + +static tss_t k; + +void t1(void *data) {} +void t2(void *data) { free(tss_get(k)); } +void t3(void *data) { + void *p = tss_get(k); + free(p); +} + +void do_free(void *d) { free(d); } +void maybe_free(void *d) {} + +void m1() { + thrd_t id; + tss_create(&k, free); // COMPLIANT + thrd_create(&id, t1, NULL); + thrd_join(id, NULL); + tss_delete(k); +} + +void m1a() { + thrd_t id; + tss_create(&k, free); // NON_COMPLIANT - Doesn't wait for thread to cleanup + // resources; if tss_delete is called prior to thread + // termination the destructor won't be called. + thrd_create(&id, t1, NULL); + tss_delete(k); +} + +void m1b() { + tss_create(&k, free); // COMPLIANT - No threads created. + tss_delete(k); +} + +void m2() { + thrd_t id; + tss_create(&k, do_free); // COMPLIANT + thrd_create(&id, t1, NULL); + thrd_join(id, NULL); + tss_delete(k); +} + +void m2a() { + thrd_t id; + tss_create(&k, do_free); // NON_COMPLIANT - Doesn't wait for thread to cleanup + // resources; if tss_delete is called prior to thread + // termination the destructor won't be called. + thrd_create(&id, t1, NULL); + tss_delete(k); +} + +void m2b() { + tss_create(&k, do_free); // COMPLIANT - No threads created. + tss_delete(k); +} + +void m3() { + thrd_t id; + tss_create(&k, maybe_free); // COMPLIANT + thrd_create(&id, t1, NULL); + thrd_join(id, NULL); + tss_delete(k); +} + +void m3a() { + thrd_t id; + tss_create(&k, + maybe_free); // NON_COMPLIANT - Doesn't wait for thread to cleanup + // resources; if tss_delete is called prior to thread + // termination the destructor won't be called. + thrd_create(&id, t1, NULL); + tss_delete(k); +} + +void m3b() { + tss_create(&k, maybe_free); // COMPLIANT - No threads created. + tss_delete(k); +} + +void m4() { + thrd_t id; + + tss_create(&k, free); // NON_COMPLIANT - The memory is deallocated, but the + // usage pattern is non-standard and may lead to errors. + thrd_create(&id, t2, NULL); + thrd_join(id, NULL); + tss_delete(k); +} + +void m5() { + tss_create(&k, NULL); // NON_COMPLIANT - `tss_delete` should be called. +} + +void m5a() { + thrd_t id; + + tss_create(&k, NULL); // COMPLIANT + thrd_create(&id, t2, NULL); + thrd_join(id, NULL); + tss_delete(k); +} + +void m5aa() { + thrd_t id; + + tss_create(&k, NULL); // COMPLIANT + thrd_create(&id, t3, NULL); + thrd_join(id, NULL); + tss_delete(k); +} + +void m5b() { + thrd_t id; + + tss_create(&k, NULL); // COMPLIANT - Cleanup can happen before OR after + // `tss_delete` is called; so there is no need to wait. + thrd_create(&id, t2, NULL); + tss_delete(k); +} + +void m5bb() { + thrd_t id; + + tss_create(&k, NULL); // COMPLIANT - Cleanup can happen before OR after + // `tss_delete` is called; so there is no need to wait. + thrd_create(&id, t3, NULL); + tss_delete(k); +} + +void m6() { + tss_create(&k, free); // NON_COMPLIANT +} + +void m7() { + tss_create(&k, do_free); // NON_COMPLIANT +} + +void m8() { + tss_create(&k, maybe_free); // NON_COMPLIANT +} + +void m9() { + tss_create(&k, NULL); // NON_COMPLIANT +} + +void m10() { + tss_create(&k, NULL); // NON_COMPLIANT +} + +void m11() { + tss_create(&k, NULL); // NON_COMPLIANT +} diff --git a/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected b/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected new file mode 100644 index 0000000000..c3cdc8bd7b --- /dev/null +++ b/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected @@ -0,0 +1,4 @@ +| test.c:23:3:23:13 | call to thrd_create | $@ not declared with appropriate storage duration | test.c:23:24:23:29 | & ... | Shared object | +| test.c:74:3:74:13 | call to thrd_create | $@ not declared with appropriate storage duration | test.c:74:24:74:24 | p | Shared object | +| test.c:85:3:85:13 | call to thrd_create | $@ not declared with appropriate storage duration | test.c:85:24:85:24 | p | Shared object | +| test.c:94:3:94:13 | call to thrd_create | $@ not declared with appropriate storage duration | test.c:94:24:94:24 | p | Shared object | diff --git a/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.qlref b/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.qlref new file mode 100644 index 0000000000..94c30180c3 --- /dev/null +++ b/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.qlref @@ -0,0 +1 @@ +rules/CON34-C/AppropriateThreadObjectStorageDurations.ql \ No newline at end of file diff --git a/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.expected b/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.expected new file mode 100644 index 0000000000..95d0a20041 --- /dev/null +++ b/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.expected @@ -0,0 +1 @@ +| test.c:14:7:14:13 | call to tss_get | Call to a thread specific storage function from within a threaded context on an object that may not be owned by this thread. | diff --git a/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.qlref b/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.qlref new file mode 100644 index 0000000000..b15d1b589c --- /dev/null +++ b/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.qlref @@ -0,0 +1 @@ +rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql \ No newline at end of file diff --git a/c/cert/test/rules/CON34-C/test.c b/c/cert/test/rules/CON34-C/test.c new file mode 100644 index 0000000000..2b5e62d5a6 --- /dev/null +++ b/c/cert/test/rules/CON34-C/test.c @@ -0,0 +1,116 @@ +#include +#include +#include + +static tss_t k; + +void t1(void *v) { + int *value = (int *)v; + int a = *value + 1; +} + +void t2(void *v) { + int *value = + tss_get(k); // NON_COMPLIANT (AUDIT) - A threaded function without a + // `tss_set` should be considered suspicious. + int a = *value + 1; +} + +void m1() { + thrd_t id; + int value; + + thrd_create(&id, t1, &value); // NON_COMPLIANT +} + +void m2() { + thrd_t id; + int *value = (int *)malloc(sizeof(int)); + + thrd_create(&id, t1, value); // COMPLIANT - free is never called +} + +void m3() { + thrd_t id; + int *value = (int *)malloc(sizeof(int)); + + thrd_create(&id, t1, + value); // COMPLIANT - free is called without synchronization, + // however this is beyond the scope of this query. + free(value); +} + +void m4() { + thrd_t id; + int *value = (int *)realloc(NULL, sizeof(int)); + + thrd_create(&id, t1, value); // COMPLIANT + + thrd_join(id, NULL); + + free(value); +} + +void m5() { + thrd_t id; + int *value = (int *)malloc(sizeof(int)); + + tss_create(&k, free); + tss_set(k, value); + + void *p = tss_get(k); + + thrd_create(&id, t1, p); // COMPLIANT +} + +void m5a() { + thrd_t id; + int *value = (int *)malloc(sizeof(int)); + + tss_set(k, value); + + void *p = tss_get(k); + + thrd_create(&id, t1, p); // NON_COMPLIANT - k not initialized. +} + +void m6() { + thrd_t id; + int *value = (int *)malloc(sizeof(int)); + + tss_create(&k, free); + + void *p = tss_get(k); + + thrd_create(&id, t1, p); // NON_COMPLIANT -- get without set +} + +void m6a() { + thrd_t id; + int *value = (int *)malloc(sizeof(int)); + + void *p = tss_get(k); + + thrd_create(&id, t1, p); // NON_COMPLIANT -- get without set +} + +void m7(void *v) { + int *value = + tss_get(k); // COMPLIANT (AUDIT) - A non-threaded function without a + // `tss_set` should not be considered suspicious. + int a = *value + 1; +} + +void m8() { + thrd_t id; + int *value = (int *)malloc(sizeof(int)); + thrd_create(&id, t2, + value); // COMPLIANT - note that t2 (which is now a threaded + // function) is NON_COMPLIANT in an audit query. +} + +void m9() { + thrd_t id; + static int value = 100; + thrd_create(&id, t1, &value); // COMPLIANT - compliant for static values. +} \ 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 5eae03560f..9994a79150 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -806,3 +806,59 @@ class ConditionalFunction extends Function { exists(ConditionalVariable cv | cv.getAnAccess().getEnclosingFunction() = this) } } + +/** + * Models calls to thread specific storage function calls. + */ +abstract class ThreadSpecificStorageFunctionCall extends FunctionCall { + /** + * Gets the key to which this call references. + */ + Expr getKey() { getArgument(0) = result } +} + +/** + * Models calls to `tss_get`. + */ +class TSSGetFunctionCall extends ThreadSpecificStorageFunctionCall { + TSSGetFunctionCall() { getTarget().getName() = "tss_get" } +} + +/** + * Models calls to `tss_set`. + */ +class TSSSetFunctionCall extends ThreadSpecificStorageFunctionCall { + TSSSetFunctionCall() { getTarget().getName() = "tss_set" } +} + +/** + * Models calls to `tss_create` + */ +class TSSCreateFunctionCall extends ThreadSpecificStorageFunctionCall { + TSSCreateFunctionCall() { getTarget().getName() = "tss_create" } + + predicate hasDeallocator() { + not exists(MacroInvocation mi, NULLMacro nm | + getArgument(1) = mi.getExpr() and + mi = nm.getAnInvocation() + ) + } +} + +/** + * Models calls to `tss_delete` + */ +class TSSDeleteFunctionCall extends ThreadSpecificStorageFunctionCall { + TSSDeleteFunctionCall() { getTarget().getName() = "tss_delete" } +} + +/** + * Gets a call to `DeallocationExpr` that deallocates memory owned by thread specific + * storage. + */ +predicate getAThreadSpecificStorageDeallocationCall(C11ThreadCreateCall tcc, DeallocationExpr dexp) { + exists(TSSGetFunctionCall tsg | + tcc.getFunction().getEntryPoint().getASuccessor*() = tsg and + DataFlow::localFlow(DataFlow::exprNode(tsg), DataFlow::exprNode(dexp.getFreedExpr())) + ) +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll new file mode 100644 index 0000000000..43faee8521 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll @@ -0,0 +1,58 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype Concurrency4Query = + TCleanUpThreadSpecificStorageQuery() or + TAppropriateThreadObjectStorageDurationsQuery() or + TThreadObjectStorageDurationsNotInitializedQuery() + +predicate isConcurrency4QueryMetadata(Query query, string queryId, string ruleId) { + query = + // `Query` instance for the `cleanUpThreadSpecificStorage` query + Concurrency4Package::cleanUpThreadSpecificStorageQuery() and + queryId = + // `@id` for the `cleanUpThreadSpecificStorage` query + "c/cert/clean-up-thread-specific-storage" and + ruleId = "CON30-C" + or + query = + // `Query` instance for the `appropriateThreadObjectStorageDurations` query + Concurrency4Package::appropriateThreadObjectStorageDurationsQuery() and + queryId = + // `@id` for the `appropriateThreadObjectStorageDurations` query + "c/cert/appropriate-thread-object-storage-durations" and + ruleId = "CON34-C" + or + query = + // `Query` instance for the `threadObjectStorageDurationsNotInitialized` query + Concurrency4Package::threadObjectStorageDurationsNotInitializedQuery() and + queryId = + // `@id` for the `threadObjectStorageDurationsNotInitialized` query + "c/cert/thread-object-storage-durations-not-initialized" and + ruleId = "CON34-C" +} + +module Concurrency4Package { + Query cleanUpThreadSpecificStorageQuery() { + //autogenerate `Query` type + result = + // `Query` type for `cleanUpThreadSpecificStorage` query + TQueryC(TConcurrency4PackageQuery(TCleanUpThreadSpecificStorageQuery())) + } + + Query appropriateThreadObjectStorageDurationsQuery() { + //autogenerate `Query` type + result = + // `Query` type for `appropriateThreadObjectStorageDurations` query + TQueryC(TConcurrency4PackageQuery(TAppropriateThreadObjectStorageDurationsQuery())) + } + + Query threadObjectStorageDurationsNotInitializedQuery() { + //autogenerate `Query` type + result = + // `Query` type for `threadObjectStorageDurationsNotInitialized` query + TQueryC(TConcurrency4PackageQuery(TThreadObjectStorageDurationsNotInitializedQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index d00f1a65cf..24a7851467 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -6,6 +6,7 @@ import Banned import Concurrency1 import Concurrency2 import Concurrency3 +import Concurrency4 import Contracts1 import Declarations1 import Declarations2 @@ -35,6 +36,7 @@ newtype TCQuery = TConcurrency1PackageQuery(Concurrency1Query q) or TConcurrency2PackageQuery(Concurrency2Query q) or TConcurrency3PackageQuery(Concurrency3Query q) or + TConcurrency4PackageQuery(Concurrency4Query q) or TContracts1PackageQuery(Contracts1Query q) or TDeclarations1PackageQuery(Declarations1Query q) or TDeclarations2PackageQuery(Declarations2Query q) or @@ -64,6 +66,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId) { isConcurrency1QueryMetadata(query, queryId, ruleId) or isConcurrency2QueryMetadata(query, queryId, ruleId) or isConcurrency3QueryMetadata(query, queryId, ruleId) or + isConcurrency4QueryMetadata(query, queryId, ruleId) or isContracts1QueryMetadata(query, queryId, ruleId) or isDeclarations1QueryMetadata(query, queryId, ruleId) or isDeclarations2QueryMetadata(query, queryId, ruleId) or diff --git a/rule_packages/c/Concurrency4.json b/rule_packages/c/Concurrency4.json new file mode 100644 index 0000000000..65a17ed2d7 --- /dev/null +++ b/rule_packages/c/Concurrency4.json @@ -0,0 +1,64 @@ +{ + "CERT-C": { + "CON30-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Failing to clean up thread-specific resources can lead to unpredictable program behavior.", + "kind": "problem", + "name": "Clean up thread-specific storage", + "precision": "medium", + "severity": "error", + "short_name": "CleanUpThreadSpecificStorage", + "tags": [ + "correctness", + "concurrency" + ], + "implementation_scope": { + "description": "This query does not attempt to ensure that the deallocation function in fact deallocates memory and instead assumes the contract is valid. Additionally, this query requires that all `tss_create` calls are bookended by calls to `tss_delete`, even if a thread is not created." + } + + } + ], + "title": "Clean up thread-specific storage" + }, + "CON34-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Accessing thread-local variables with automatic storage durations can lead to unpredictable program behavior.", + "kind": "problem", + "name": "Declare objects shared between threads with appropriate storage durations", + "precision": "high", + "severity": "error", + "short_name": "AppropriateThreadObjectStorageDurations", + "tags": [ + "correctness", + "concurrency" + ], + "implementation_scope": { + "description": "This query does not consider Windows implementations or OpenMP implementations. This query is primarily about excluding cases wherein the storage duration of a variable is appropriate. As such, this query is not concerned if the appropriate synchronization mechanisms are used, such as sequencing calls to `thrd_join` and `free`. An audit query is supplied to handle some of those cases." + } + }, + { + "description": "Storage durations not correctly initialized can cause unpredictable program behavior.", + "kind": "problem", + "name": "(Audit) Declare objects shared between threads with appropriate storage durations", + "precision": "high", + "severity": "error", + "short_name": "ThreadObjectStorageDurationsNotInitialized", + "tags": [ + "external/cert/audit", + "correctness", + "concurrency" + ] + } + ], + "title": "Declare objects shared between threads with appropriate storage durations" + } + } +} \ No newline at end of file diff --git a/rules.csv b/rules.csv index 673f116f19..0a97b03044 100755 --- a/rules.csv +++ b/rules.csv @@ -494,8 +494,8 @@ c,CERT-C,CON35-C,Yes,Rule,,,Avoid deadlock by locking in a predefined order,CON5 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,,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,CON39-C,Yes,Rule,,,Do not join or detach a thread that was previously joined or detached,,Concurrency5,Hard, +c,CERT-C,CON40-C,Yes,Rule,,,Do not refer to an atomic variable twice in an expression,,Concurrency5,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,