From 98667ece34be9e3f71e6ae6fa83c6e63ea6fa0ee Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 21 Sep 2022 17:44:58 -0400 Subject: [PATCH 01/24] work --- .vscode/tasks.json | 1 + ...AppropriateThreadObjectStorageDurations.md | 18 +++++ ...AppropriateThreadObjectStorageDurations.ql | 65 ++++++++++++++++ ...riateThreadObjectStorageDurations.expected | 1 + ...ropriateThreadObjectStorageDurations.qlref | 1 + .../cpp/exclusions/c/Concurrency4.qll | 74 ++++++++++++++++++ .../cpp/exclusions/c/RuleMetadata.qll | 3 + rule_packages/c/Concurrency4.json | 78 +++++++++++++++++++ 8 files changed, 241 insertions(+) create mode 100644 c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md create mode 100644 c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql create mode 100644 c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected create mode 100644 c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.qlref create mode 100644 cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll create mode 100644 rule_packages/c/Concurrency4.json diff --git a/.vscode/tasks.json b/.vscode/tasks.json index b7e907a3bc..52ab0e8ff0 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -199,6 +199,7 @@ "Concurrency1", "Concurrency2", "Concurrency3", + "Concurrency4", "Conditionals", "Const", "DeadCode", 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..b5beea9e3c --- /dev/null +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md @@ -0,0 +1,18 @@ +# 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 + + +## CERT + +** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## 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/AppropriateThreadObjectStorageDurations.ql b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql new file mode 100644 index 0000000000..aae206e9d0 --- /dev/null +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql @@ -0,0 +1,65 @@ +/** + * @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 + + +/// anything from tss_get is ok. the tss get must obtain the value from the +// context that called tss_set NOT in the thread. +/// anything that was static is ok +/// anything dynamically allocated is ok. + +// THREAD LOCAL IS NOT OK -- EG STACK VARIBLES ARE NEVER OK + +// we can make this really simple -- just look for thread create functions +// wherein you pass in a variable created on the stack that is a) not static or +// b) not created via malloc and c) not obtained from tss_get. +// we should do something more to determine if tss_get is wrongly called from a +// thread context without a matching tss_get as another query. That should be an +// audit. tss get without set. +// tss_get in the parent thread should maybe be followed by a thread_join +// function +// +// + +// IN THE PARENT -- a call to tss_set MUST be followed by a thread_join. +// Without this, it is possible the context isn't valid anymore. + +// It's important to note -- tss_get set DOES NOT require a call to delete +// to require the wait. It just requires the wait if it is used at all. + +class MallocFunctionCall extends FunctionCall { + MallocFunctionCall(){ + getTarget().getName() = "malloc" + } +} + +from MallocFunctionCall fc, StackVariable sv, Expr e +where not isExcluded(fc) +and TaintTracking::localTaint(DataFlow::exprNode(fc), DataFlow::exprNode(e)) +select fc, e + + +// from C11ThreadCreateCall tcc, StackVariable sv, Expr arg +// where +// not isExcluded(tcc, Concurrency4Package::appropriateThreadObjectStorageDurationsQuery()) and +// tcc.getArgument(2) = arg and +// // a stack variable that is given as an argument to a thread +// TaintTracking::localTaint(DataFlow::exprNode(sv.getAnAccess()), DataFlow::exprNode(arg)) +// // that isn't one of the allowed usage patterns +// TaintTracking::localTaint(DataFlow::exprNode(sv.getAnAccess()), DataFlow::exprNode(arg)) +// select tcc, "$@ not declared with appropriate storage duration", arg, "Shared object" 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..2ec1a0ac6c --- /dev/null +++ b/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected @@ -0,0 +1 @@ +No expected results have yet been specified \ No newline at end of file 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/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..acea7a5fe5 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll @@ -0,0 +1,74 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype Concurrency4Query = + TCleanUpThreadSpecificStorageQuery() or + TAppropriateThreadObjectStorageDurationsQuery() or + TThreadWasPreviouslyJoinedOrDetachedQuery() or + TDoNotReferToAnAtomicVariableTwiceInExpressionQuery() + +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 `threadWasPreviouslyJoinedOrDetached` query + Concurrency4Package::threadWasPreviouslyJoinedOrDetachedQuery() and + queryId = + // `@id` for the `threadWasPreviouslyJoinedOrDetached` query + "c/cert/thread-was-previously-joined-or-detached" and + ruleId = "CON39-C" + or + query = + // `Query` instance for the `doNotReferToAnAtomicVariableTwiceInExpression` query + Concurrency4Package::doNotReferToAnAtomicVariableTwiceInExpressionQuery() and + queryId = + // `@id` for the `doNotReferToAnAtomicVariableTwiceInExpression` query + "c/cert/do-not-refer-to-an-atomic-variable-twice-in-expression" and + ruleId = "CON40-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 threadWasPreviouslyJoinedOrDetachedQuery() { + //autogenerate `Query` type + result = + // `Query` type for `threadWasPreviouslyJoinedOrDetached` query + TQueryC(TConcurrency4PackageQuery(TThreadWasPreviouslyJoinedOrDetachedQuery())) + } + + Query doNotReferToAnAtomicVariableTwiceInExpressionQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotReferToAnAtomicVariableTwiceInExpression` query + TQueryC(TConcurrency4PackageQuery(TDoNotReferToAnAtomicVariableTwiceInExpressionQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index 6b6915ad3b..6099cc37b7 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 IO1 @@ -31,6 +32,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 TIO1PackageQuery(IO1Query q) or @@ -56,6 +58,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 isIO1QueryMetadata(query, queryId, ruleId) or diff --git a/rule_packages/c/Concurrency4.json b/rule_packages/c/Concurrency4.json new file mode 100644 index 0000000000..71df30243b --- /dev/null +++ b/rule_packages/c/Concurrency4.json @@ -0,0 +1,78 @@ +{ + "CERT-C": { + "CON30-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "", + "kind": "problem", + "name": "Clean up thread-specific storage", + "precision": "medium", + "severity": "error", + "short_name": "CleanUpThreadSpecificStorage", + "tags": [] + } + ], + "title": "Clean up thread-specific storage" + }, + "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 those cases." + } + } + ], + "title": "Declare objects shared between threads with appropriate storage durations" + }, + "CON39-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "", + "kind": "problem", + "name": "Do not join or detach a thread that was previously joined or detached", + "precision": "high", + "severity": "error", + "short_name": "ThreadWasPreviouslyJoinedOrDetached", + "tags": [] + } + ], + "title": "Do not join or detach a thread that was previously joined or detached" + }, + "CON40-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "", + "kind": "problem", + "name": "Do not refer to an atomic variable twice in an expression", + "precision": "very-high", + "severity": "error", + "short_name": "DoNotReferToAnAtomicVariableTwiceInExpression", + "tags": [] + } + ], + "title": "Do not refer to an atomic variable twice in an expression" + } + } +} \ No newline at end of file From 409ff609d062e2803a2e84138a76bff6dff8b3e6 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 21 Sep 2022 17:45:15 -0400 Subject: [PATCH 02/24] work --- c/cert/test/rules/CON34-C/main.c | 78 ++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 c/cert/test/rules/CON34-C/main.c diff --git a/c/cert/test/rules/CON34-C/main.c b/c/cert/test/rules/CON34-C/main.c new file mode 100644 index 0000000000..36ea6823eb --- /dev/null +++ b/c/cert/test/rules/CON34-C/main.c @@ -0,0 +1,78 @@ +#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 *)malloc(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_set(k, value); + + void *p = tss_get(k); + + thrd_create(&id, t1, p); // COMPLIANT +} + +void m6(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 m7() { + 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. +} From 9243c935b9f8b0c60be12e990c8f5a4191f96c35 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 22 Sep 2022 13:11:26 -0400 Subject: [PATCH 03/24] work --- .../src/codingstandards/cpp/Concurrency.qll | 19 +++++++++++++++++++ .../cpp/exclusions/c/Concurrency4.qll | 16 ++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index 5eae03560f..09447557ea 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -806,3 +806,22 @@ class ConditionalFunction extends Function { exists(ConditionalVariable cv | cv.getAnAccess().getEnclosingFunction() = this) } } + +/** + * Models calls to thread specific storage function calls. + */ +abstract class ThreadSpecificStorageFunctionCall extends FunctionCall { } + +/** + * 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" } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll index acea7a5fe5..4c566be220 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll @@ -6,6 +6,7 @@ import codingstandards.cpp.exclusions.RuleMetadata newtype Concurrency4Query = TCleanUpThreadSpecificStorageQuery() or TAppropriateThreadObjectStorageDurationsQuery() or + TThreadObjectStorageDurationsNotInitializedQuery() or TThreadWasPreviouslyJoinedOrDetachedQuery() or TDoNotReferToAnAtomicVariableTwiceInExpressionQuery() @@ -26,6 +27,14 @@ predicate isConcurrency4QueryMetadata(Query query, string queryId, string ruleId "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" + or query = // `Query` instance for the `threadWasPreviouslyJoinedOrDetached` query Concurrency4Package::threadWasPreviouslyJoinedOrDetachedQuery() and @@ -58,6 +67,13 @@ module Concurrency4Package { TQueryC(TConcurrency4PackageQuery(TAppropriateThreadObjectStorageDurationsQuery())) } + Query threadObjectStorageDurationsNotInitializedQuery() { + //autogenerate `Query` type + result = + // `Query` type for `threadObjectStorageDurationsNotInitialized` query + TQueryC(TConcurrency4PackageQuery(TThreadObjectStorageDurationsNotInitializedQuery())) + } + Query threadWasPreviouslyJoinedOrDetachedQuery() { //autogenerate `Query` type result = From 40cbfc705fabd8d924f30f0b9538c51bea227039 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 22 Sep 2022 13:11:41 -0400 Subject: [PATCH 04/24] work --- ...riateThreadObjectStorageDurations.expected | 5 ++- ...ectStorageDurationsNotInitialized.expected | 1 + ...ObjectStorageDurationsNotInitialized.qlref | 1 + c/cert/test/rules/CON34-C/main.c | 42 ++++++++++++++++++- 4 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.expected create mode 100644 c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.qlref diff --git a/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected b/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected index 2ec1a0ac6c..2d867fd55a 100644 --- a/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected +++ b/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected @@ -1 +1,4 @@ -No expected results have yet been specified \ No newline at end of file +| main.c:23:3:23:13 | call to thrd_create | $@ not declared with appropriate storage duration | main.c:23:24:23:29 | & ... | Shared object | +| main.c:74:3:74:13 | call to thrd_create | $@ not declared with appropriate storage duration | main.c:74:24:74:24 | p | Shared object | +| main.c:85:3:85:13 | call to thrd_create | $@ not declared with appropriate storage duration | main.c:85:24:85:24 | p | Shared object | +| main.c:94:3:94:13 | call to thrd_create | $@ not declared with appropriate storage duration | main.c:94:24:94:24 | p | Shared object | 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..87d3ced28c --- /dev/null +++ b/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.expected @@ -0,0 +1 @@ +| main.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/main.c b/c/cert/test/rules/CON34-C/main.c index 36ea6823eb..c53640f6ab 100644 --- a/c/cert/test/rules/CON34-C/main.c +++ b/c/cert/test/rules/CON34-C/main.c @@ -55,6 +55,7 @@ void m5() { thrd_t id; int *value = (int *)malloc(sizeof(int)); + tss_create(&k, free); tss_set(k, value); void *p = tss_get(k); @@ -62,17 +63,54 @@ void m5() { thrd_create(&id, t1, p); // COMPLIANT } -void m6(void *v) { +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 m7() { +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 From 1dead2e026310fd71696a73926d4f1029662ebd6 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 22 Sep 2022 13:11:52 -0400 Subject: [PATCH 05/24] work --- ...AppropriateThreadObjectStorageDurations.md | 2 +- ...AppropriateThreadObjectStorageDurations.ql | 67 +++++++------------ ...eadObjectStorageDurationsNotInitialized.md | 16 +++++ ...eadObjectStorageDurationsNotInitialized.ql | 35 ++++++++++ 4 files changed, 76 insertions(+), 44 deletions(-) create mode 100644 c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md create mode 100644 c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql diff --git a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md index b5beea9e3c..5cc33408c2 100644 --- a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md @@ -11,7 +11,7 @@ This query implements the CERT-C rule CON34-C: ## Implementation notes -None +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 those cases. ## References diff --git a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql index aae206e9d0..352d868a31 100644 --- a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql @@ -16,50 +16,31 @@ import cpp import codingstandards.c.cert import codingstandards.cpp.Concurrency import semmle.code.cpp.dataflow.TaintTracking - - -/// anything from tss_get is ok. the tss get must obtain the value from the -// context that called tss_set NOT in the thread. -/// anything that was static is ok -/// anything dynamically allocated is ok. - -// THREAD LOCAL IS NOT OK -- EG STACK VARIBLES ARE NEVER OK - -// we can make this really simple -- just look for thread create functions -// wherein you pass in a variable created on the stack that is a) not static or -// b) not created via malloc and c) not obtained from tss_get. -// we should do something more to determine if tss_get is wrongly called from a -// thread context without a matching tss_get as another query. That should be an -// audit. tss get without set. -// tss_get in the parent thread should maybe be followed by a thread_join -// function -// -// - -// IN THE PARENT -- a call to tss_set MUST be followed by a thread_join. -// Without this, it is possible the context isn't valid anymore. - -// It's important to note -- tss_get set DOES NOT require a call to delete -// to require the wait. It just requires the wait if it is used at all. +import semmle.code.cpp.dataflow.DataFlow class MallocFunctionCall extends FunctionCall { - MallocFunctionCall(){ - getTarget().getName() = "malloc" - } + MallocFunctionCall() { getTarget().getName() = "malloc" } } -from MallocFunctionCall fc, StackVariable sv, Expr e -where not isExcluded(fc) -and TaintTracking::localTaint(DataFlow::exprNode(fc), DataFlow::exprNode(e)) -select fc, e - - -// from C11ThreadCreateCall tcc, StackVariable sv, Expr arg -// where -// not isExcluded(tcc, Concurrency4Package::appropriateThreadObjectStorageDurationsQuery()) and -// tcc.getArgument(2) = arg and -// // a stack variable that is given as an argument to a thread -// TaintTracking::localTaint(DataFlow::exprNode(sv.getAnAccess()), DataFlow::exprNode(arg)) -// // that isn't one of the allowed usage patterns -// TaintTracking::localTaint(DataFlow::exprNode(sv.getAnAccess()), DataFlow::exprNode(arg)) -// select tcc, "$@ not declared with appropriate storage duration", arg, "Shared object" +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 + // it's either not static + not sv.isStatic() and + // or isn't one of the allowed usage patterns + not exists(MallocFunctionCall mfc | + 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..fbc4ed40af --- /dev/null +++ b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md @@ -0,0 +1,16 @@ +# 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 +## CERT + +** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## 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..b6a5b8cb32 --- /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/autosar/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." From bf07368303ce53f45ab3fd33d58ff753cfdd92f9 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Thu, 22 Sep 2022 13:12:11 -0400 Subject: [PATCH 06/24] work --- rule_packages/c/Concurrency4.json | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/rule_packages/c/Concurrency4.json b/rule_packages/c/Concurrency4.json index 71df30243b..db3b37cd93 100644 --- a/rule_packages/c/Concurrency4.json +++ b/rule_packages/c/Concurrency4.json @@ -19,7 +19,7 @@ }, "CON34-C": { "properties": { - "obligation": "rule" + "obligation": "rule" }, "queries": [ { @@ -34,8 +34,21 @@ "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 those cases." + "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/autosar/audit", + "correctness", + "concurrency" + ] } ], "title": "Declare objects shared between threads with appropriate storage durations" From 94293ec0f90b331578cd27a695174a6150cb1a9d Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 11:48:19 -0400 Subject: [PATCH 07/24] concurrency5 --- .vscode/tasks.json | 1 + 1 file changed, 1 insertion(+) diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 3a58968037..4018d2d75f 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -198,6 +198,7 @@ "Concurrency2", "Concurrency3", "Concurrency4", + "Concurrency5", "Conditionals", "Const", "DeadCode", From 9288fd80c24b1333685925a9cbaa9748cc905494 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 11:48:28 -0400 Subject: [PATCH 08/24] edit package --- rule_packages/c/Concurrency4.json | 34 ------------------------------- 1 file changed, 34 deletions(-) diff --git a/rule_packages/c/Concurrency4.json b/rule_packages/c/Concurrency4.json index db3b37cd93..995a285db6 100644 --- a/rule_packages/c/Concurrency4.json +++ b/rule_packages/c/Concurrency4.json @@ -52,40 +52,6 @@ } ], "title": "Declare objects shared between threads with appropriate storage durations" - }, - "CON39-C": { - "properties": { - "obligation": "rule" - }, - "queries": [ - { - "description": "", - "kind": "problem", - "name": "Do not join or detach a thread that was previously joined or detached", - "precision": "high", - "severity": "error", - "short_name": "ThreadWasPreviouslyJoinedOrDetached", - "tags": [] - } - ], - "title": "Do not join or detach a thread that was previously joined or detached" - }, - "CON40-C": { - "properties": { - "obligation": "rule" - }, - "queries": [ - { - "description": "", - "kind": "problem", - "name": "Do not refer to an atomic variable twice in an expression", - "precision": "very-high", - "severity": "error", - "short_name": "DoNotReferToAnAtomicVariableTwiceInExpression", - "tags": [] - } - ], - "title": "Do not refer to an atomic variable twice in an expression" } } } \ No newline at end of file From 8a7f9e3b9fbed0bb44303a8db7c706d1107f3a58 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 15:40:09 -0400 Subject: [PATCH 09/24] checkpoint --- .../CleanUpThreadSpecificStorage.expected | 1 + .../CleanUpThreadSpecificStorage.qlref | 1 + c/cert/test/rules/CON30-C/main.c | 93 +++++++++++++++++++ 3 files changed, 95 insertions(+) create mode 100644 c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected create mode 100644 c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.qlref create mode 100644 c/cert/test/rules/CON30-C/main.c 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..2ec1a0ac6c --- /dev/null +++ b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected @@ -0,0 +1 @@ +No expected results have yet been specified \ No newline at end of file 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/main.c b/c/cert/test/rules/CON30-C/main.c new file mode 100644 index 0000000000..d666a43f9f --- /dev/null +++ b/c/cert/test/rules/CON30-C/main.c @@ -0,0 +1,93 @@ +#include +#include +#include + +static tss_t k; + +void do_free(void *d) { free(d); } + +void maybe_free(void *d) {} + +void m1() { + tss_create(&k, free); // COMPLIANT + tss_delete(k); +} + +void m2() { + tss_create(&k, do_free); // COMPLIANT + tss_delete(k); +} + +void m3() { + tss_create(&k, maybe_free); // COMPLIANT + tss_delete(k); +} + +void m1a() { + tss_create(&k, free); // COMPLIANT + free(tss_get(k)); +} + +void m2a() { + tss_create(&k, do_free); // COMPLIANT + free(tss_get(k)); +} + +void m3a() { + tss_create(&k, maybe_free); // COMPLIANT + free(tss_get(k)); +} + +void m1b() { + tss_create(&k, NULL); // COMPLIANT + free(tss_get(k)); +} + +void m2b() { + tss_create(&k, NULL); // COMPLIANT + free(tss_get(k)); +} + +void m3b() { + tss_create(&k, NULL); // COMPLIANT + free(tss_get(k)); +} + +void m4() { + tss_create(&k, free); // NON_COMPLIANT +} + +void m5() { + tss_create(&k, do_free); // NON_COMPLIANT +} + +void m6() { + tss_create(&k, maybe_free); // NON_COMPLIANT +} + +void m4a() { + tss_create(&k, NULL); // NON_COMPLIANT +} + +void m5a() { + tss_create(&k, NULL); // NON_COMPLIANT +} + +void m6a() { + tss_create(&k, NULL); // NON_COMPLIANT +} + +void m4b() { + tss_create(&k, NULL); // NON_COMPLIANT + tss_delete(k); +} + +void m5b() { + tss_create(&k, NULL); // NON_COMPLIANT + tss_delete(k); +} + +void m6b() { + tss_create(&k, NULL); // NON_COMPLIANT + tss_delete(k); +} From c537f5aa52ba8b8c6dbda9e6e5563a590bbb3e07 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 15:40:31 -0400 Subject: [PATCH 10/24] chcekpoint --- .../CON30-C/CleanUpThreadSpecificStorage.md | 18 +++++++++++ .../CON30-C/CleanUpThreadSpecificStorage.ql | 31 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md create mode 100644 c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql 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..e0b429d17d --- /dev/null +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md @@ -0,0 +1,18 @@ +# CON30-C: Clean up thread-specific storage + +This query implements the CERT-C rule CON30-C: + +> Clean up thread-specific storage + + +## CERT + +** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## Implementation notes + +This query does not attempt to ensure that the deallocation function in fact deallocates memory and instead assumes the contract is valid. + +## 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..dc127817de --- /dev/null +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql @@ -0,0 +1,31 @@ +/** + * @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 + +// there are two safe patterns. +// 1) They call free(tss_get(key)) +// 2) They call tss_create(key, destructor) -- we don't make an attempt to +// understand what the function is. They must also call tss_delete(key) +// THAT MEANS there is dataflow from tss_create -> tss_delete +// OR there is dataflow from tss_create -> tss_delete +// we just make sure in one arg version it's wrapped in a call to free. +// That IS there is taint from tss_create -> free(); + +from Function f +where + not isExcluded(f, Concurrency4Package::cleanUpThreadSpecificStorageQuery()) + and nm +select mi.getExpr() \ No newline at end of file From 6a52db9987e8aa5836f92012538a17f2259318a2 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 15:40:43 -0400 Subject: [PATCH 11/24] checkpoint --- .../rules/CON34-C/AppropriateThreadObjectStorageDurations.md | 2 +- .../rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md index 5cc33408c2..68d7c5654a 100644 --- a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md @@ -11,7 +11,7 @@ This query implements the CERT-C rule CON34-C: ## 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 those cases. +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 diff --git a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md index fbc4ed40af..b67de57556 100644 --- a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md +++ b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md @@ -3,6 +3,8 @@ This query implements the CERT-C rule CON34-C: > Declare objects shared between threads with appropriate storage durations + + ## CERT ** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** From ebb05f4a959e390cdc69eb91e553761bd3f2222c Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 15:40:59 -0400 Subject: [PATCH 12/24] checkpoint --- .../src/codingstandards/cpp/Concurrency.qll | 21 +++++++++++- .../cpp/exclusions/c/Concurrency4.qll | 34 +------------------ 2 files changed, 21 insertions(+), 34 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index 09447557ea..6c00bf6f44 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -810,7 +810,14 @@ class ConditionalFunction extends Function { /** * Models calls to thread specific storage function calls. */ -abstract class ThreadSpecificStorageFunctionCall extends FunctionCall { } +abstract class ThreadSpecificStorageFunctionCall extends FunctionCall { + /** + * Gets the key to which this call references. + */ + Expr getKey(){ + getArgument(0) = result + } +} /** * Models calls to `tss_get`. @@ -825,3 +832,15 @@ class TSSGetFunctionCall extends ThreadSpecificStorageFunctionCall { 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(){ + // getArgument(0) instanceof NULLMacro + // } + +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll index 4c566be220..43faee8521 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Concurrency4.qll @@ -6,9 +6,7 @@ import codingstandards.cpp.exclusions.RuleMetadata newtype Concurrency4Query = TCleanUpThreadSpecificStorageQuery() or TAppropriateThreadObjectStorageDurationsQuery() or - TThreadObjectStorageDurationsNotInitializedQuery() or - TThreadWasPreviouslyJoinedOrDetachedQuery() or - TDoNotReferToAnAtomicVariableTwiceInExpressionQuery() + TThreadObjectStorageDurationsNotInitializedQuery() predicate isConcurrency4QueryMetadata(Query query, string queryId, string ruleId) { query = @@ -34,22 +32,6 @@ predicate isConcurrency4QueryMetadata(Query query, string queryId, string ruleId // `@id` for the `threadObjectStorageDurationsNotInitialized` query "c/cert/thread-object-storage-durations-not-initialized" and ruleId = "CON34-C" - or - query = - // `Query` instance for the `threadWasPreviouslyJoinedOrDetached` query - Concurrency4Package::threadWasPreviouslyJoinedOrDetachedQuery() and - queryId = - // `@id` for the `threadWasPreviouslyJoinedOrDetached` query - "c/cert/thread-was-previously-joined-or-detached" and - ruleId = "CON39-C" - or - query = - // `Query` instance for the `doNotReferToAnAtomicVariableTwiceInExpression` query - Concurrency4Package::doNotReferToAnAtomicVariableTwiceInExpressionQuery() and - queryId = - // `@id` for the `doNotReferToAnAtomicVariableTwiceInExpression` query - "c/cert/do-not-refer-to-an-atomic-variable-twice-in-expression" and - ruleId = "CON40-C" } module Concurrency4Package { @@ -73,18 +55,4 @@ module Concurrency4Package { // `Query` type for `threadObjectStorageDurationsNotInitialized` query TQueryC(TConcurrency4PackageQuery(TThreadObjectStorageDurationsNotInitializedQuery())) } - - Query threadWasPreviouslyJoinedOrDetachedQuery() { - //autogenerate `Query` type - result = - // `Query` type for `threadWasPreviouslyJoinedOrDetached` query - TQueryC(TConcurrency4PackageQuery(TThreadWasPreviouslyJoinedOrDetachedQuery())) - } - - Query doNotReferToAnAtomicVariableTwiceInExpressionQuery() { - //autogenerate `Query` type - result = - // `Query` type for `doNotReferToAnAtomicVariableTwiceInExpression` query - TQueryC(TConcurrency4PackageQuery(TDoNotReferToAnAtomicVariableTwiceInExpressionQuery())) - } } From 2f44d8337deb55def6b6aefa2a98a414340adc47 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 15:41:04 -0400 Subject: [PATCH 13/24] checkpoint --- rule_packages/c/Concurrency4.json | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/rule_packages/c/Concurrency4.json b/rule_packages/c/Concurrency4.json index 995a285db6..5ee1061a48 100644 --- a/rule_packages/c/Concurrency4.json +++ b/rule_packages/c/Concurrency4.json @@ -6,13 +6,20 @@ }, "queries": [ { - "description": "", + "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": [] + "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." + } + } ], "title": "Clean up thread-specific storage" From 47845180f2efb6ff0905e870f62e7f7ddeb4ed09 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 15:41:29 -0400 Subject: [PATCH 14/24] checkpoint --- rules.csv | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules.csv b/rules.csv index 02191e5626..b81910af4f 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, From 911ba8f5697398a076c96708a3a8975df7f87e86 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 15:47:29 -0400 Subject: [PATCH 15/24] checkpoint --- c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql index dc127817de..c3359400af 100644 --- a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql @@ -24,7 +24,7 @@ import codingstandards.c.cert // we just make sure in one arg version it's wrapped in a call to free. // That IS there is taint from tss_create -> free(); -from Function f +from Function f where not isExcluded(f, Concurrency4Package::cleanUpThreadSpecificStorageQuery()) and nm From 31d223cf9e9e84a5e0cfb9c7538e9924ca0491ab Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 18:21:05 -0400 Subject: [PATCH 16/24] work --- ...ddOrSubtractAScaledIntegerToAPointer.qhelp | 599 ++++++++++++++++++ .../CON30-C/CleanUpThreadSpecificStorage.md | 167 ++++- .../CON30-C/CleanUpThreadSpecificStorage.ql | 84 ++- ...AppropriateThreadObjectStorageDurations.md | 380 ++++++++++- ...eadObjectStorageDurationsNotInitialized.md | 380 ++++++++++- c/cert/test/rules/CON30-C/main.c | 11 +- .../src/codingstandards/cpp/Concurrency.qll | 25 +- 7 files changed, 1616 insertions(+), 30 deletions(-) create mode 100644 c/cert/src/rules/ARR39-C/DoNotAddOrSubtractAScaledIntegerToAPointer.qhelp 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 index e0b429d17d..cdb9c44ff8 100644 --- a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md @@ -5,9 +5,172 @@ This query implements the CERT-C rule CON30-C: > Clean up thread-specific storage -## CERT -** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** +## 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 diff --git a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql index c3359400af..206737a35b 100644 --- a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql @@ -14,18 +14,78 @@ import cpp import codingstandards.c.cert +import codingstandards.cpp.Concurrency +import semmle.code.cpp.dataflow.TaintTracking +import semmle.code.cpp.dataflow.DataFlow -// there are two safe patterns. -// 1) They call free(tss_get(key)) -// 2) They call tss_create(key, destructor) -- we don't make an attempt to -// understand what the function is. They must also call tss_delete(key) -// THAT MEANS there is dataflow from tss_create -> tss_delete -// OR there is dataflow from tss_create -> tss_delete -// we just make sure in one arg version it's wrapped in a call to free. -// That IS there is taint from tss_create -> free(); +class FreeFunctionCall extends FunctionCall { + FreeFunctionCall() { getTarget().getName() = "free" } +} -from Function f +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()) + ) + } +} + +class TssCreateToFreeDataFlowConfiguration extends DataFlow::Configuration { + TssCreateToFreeDataFlowConfiguration() { this = "TssCreateToFreeDataFlowConfiguration" } + + 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(TSSGetFunctionCall tsg, FreeFunctionCall ffc, Expr e | + // the only requirement of a sink is that at some point + // it references the key of a delete call. + e.getParent*() = tsg.getKey() and + (e = node.asDefiningArgument() or e = node.asExpr()) and + ffc.getArgument(0) = tsg + ) + } +} + +from TSSCreateFunctionCall tcc where - not isExcluded(f, Concurrency4Package::cleanUpThreadSpecificStorageQuery()) - and nm -select mi.getExpr() \ No newline at end of file + not isExcluded(tcc, Concurrency4Package::cleanUpThreadSpecificStorageQuery()) and + if tcc.hasDeallocator() + then + // if they specify a deallocator the memory must be freed with a call to + // tss_delete(key), which implies that there is dataflow from the create call + // to the delete call + not exists(TssCreateToTssDeleteDataFlowConfiguration config | + config.hasFlow(DataFlow::definitionByReferenceNodeFromArgument(tcc.getKey()), _) + or + config.hasFlow(DataFlow::exprNode(tcc.getKey()), _) + ) + else + // otherwise, they are required to call some kind of free on the result of + // a key which has dataflow of the form tss_create -> tss_get -> free. + not exists(TssCreateToFreeDataFlowConfiguration config | + config.hasFlow(DataFlow::definitionByReferenceNodeFromArgument(tcc.getKey()), _) + or + config.hasFlow(DataFlow::exprNode(tcc.getKey()), _) + ) +select tcc diff --git a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md index 68d7c5654a..7af91eb7e7 100644 --- a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md @@ -5,9 +5,385 @@ This query implements the CERT-C rule CON34-C: > Declare objects shared between threads with appropriate storage durations -## CERT -** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** +## 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 diff --git a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md index b67de57556..598aa21f1c 100644 --- a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md +++ b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md @@ -5,9 +5,385 @@ This query implements the CERT-C rule CON34-C: > Declare objects shared between threads with appropriate storage durations -## CERT -** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** +## 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 diff --git a/c/cert/test/rules/CON30-C/main.c b/c/cert/test/rules/CON30-C/main.c index d666a43f9f..a8961fa54d 100644 --- a/c/cert/test/rules/CON30-C/main.c +++ b/c/cert/test/rules/CON30-C/main.c @@ -24,17 +24,22 @@ void m3() { } void m1a() { - tss_create(&k, free); // COMPLIANT + tss_create(&k, free); // NON_COMPLIANT - The memory is deallocated, but the + // usage pattern is non-standard and may lead to errors. free(tss_get(k)); } void m2a() { - tss_create(&k, do_free); // COMPLIANT + tss_create(&k, + do_free); // NON_COMPLIANT - The memory is deallocated, but the + // usage pattern is non-standard and may lead to errors. free(tss_get(k)); } void m3a() { - tss_create(&k, maybe_free); // COMPLIANT + tss_create( + &k, maybe_free); // NON_COMPLIANT - The memory is deallocated, but the + // usage pattern is non-standard and may lead to errors. free(tss_get(k)); } diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index 6c00bf6f44..fb07829b7a 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -810,13 +810,11 @@ class ConditionalFunction extends Function { /** * Models calls to thread specific storage function calls. */ -abstract class ThreadSpecificStorageFunctionCall extends FunctionCall { +abstract class ThreadSpecificStorageFunctionCall extends FunctionCall { /** * Gets the key to which this call references. */ - Expr getKey(){ - getArgument(0) = result - } + Expr getKey() { getArgument(0) = result } } /** @@ -838,9 +836,18 @@ class TSSSetFunctionCall extends ThreadSpecificStorageFunctionCall { */ class TSSCreateFunctionCall extends ThreadSpecificStorageFunctionCall { TSSCreateFunctionCall() { getTarget().getName() = "tss_create" } - - // predicate hasDeallocator(){ - // getArgument(0) instanceof NULLMacro - // } - + + 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" } } From a58e8110e553830d1a039ea4457e7c0644cd4922 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Fri, 23 Sep 2022 18:23:40 -0400 Subject: [PATCH 17/24] Results --- .../CON30-C/CleanUpThreadSpecificStorage.expected | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected index 2ec1a0ac6c..a59afbb608 100644 --- a/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected +++ b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected @@ -1 +1,12 @@ -No expected results have yet been specified \ No newline at end of file +| main.c:27:3:27:12 | call to tss_create | +| main.c:33:3:33:12 | call to tss_create | +| main.c:40:3:40:12 | call to tss_create | +| main.c:62:3:62:12 | call to tss_create | +| main.c:66:3:66:12 | call to tss_create | +| main.c:70:3:70:12 | call to tss_create | +| main.c:74:3:74:12 | call to tss_create | +| main.c:78:3:78:12 | call to tss_create | +| main.c:82:3:82:12 | call to tss_create | +| main.c:86:3:86:12 | call to tss_create | +| main.c:91:3:91:12 | call to tss_create | +| main.c:96:3:96:12 | call to tss_create | From 4a24cf0d5d5fd83bbe8d67b3e096c3e434d1bcd1 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Mon, 26 Sep 2022 11:20:28 -0400 Subject: [PATCH 18/24] docs --- c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md | 1 - .../src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md | 1 - .../rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md | 1 - 3 files changed, 3 deletions(-) diff --git a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md index cdb9c44ff8..b16aa677a3 100644 --- a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md @@ -5,7 +5,6 @@ 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. diff --git a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md index 7af91eb7e7..68fe49222d 100644 --- a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.md @@ -5,7 +5,6 @@ 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. diff --git a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md index 598aa21f1c..75ca7635c6 100644 --- a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md +++ b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.md @@ -5,7 +5,6 @@ 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. From a40600098c592da660ed7aa00618fd89ed668c81 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 26 Sep 2022 13:36:04 -0400 Subject: [PATCH 19/24] Concurrency 4: rename test files --- .../CleanUpThreadSpecificStorage.expected | 24 +++++++++---------- c/cert/test/rules/CON30-C/{main.c => test.c} | 0 ...riateThreadObjectStorageDurations.expected | 8 +++---- ...ectStorageDurationsNotInitialized.expected | 2 +- c/cert/test/rules/CON34-C/{main.c => test.c} | 0 5 files changed, 17 insertions(+), 17 deletions(-) rename c/cert/test/rules/CON30-C/{main.c => test.c} (100%) rename c/cert/test/rules/CON34-C/{main.c => test.c} (100%) diff --git a/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected index a59afbb608..71253bc490 100644 --- a/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected +++ b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected @@ -1,12 +1,12 @@ -| main.c:27:3:27:12 | call to tss_create | -| main.c:33:3:33:12 | call to tss_create | -| main.c:40:3:40:12 | call to tss_create | -| main.c:62:3:62:12 | call to tss_create | -| main.c:66:3:66:12 | call to tss_create | -| main.c:70:3:70:12 | call to tss_create | -| main.c:74:3:74:12 | call to tss_create | -| main.c:78:3:78:12 | call to tss_create | -| main.c:82:3:82:12 | call to tss_create | -| main.c:86:3:86:12 | call to tss_create | -| main.c:91:3:91:12 | call to tss_create | -| main.c:96:3:96:12 | call to tss_create | +| test.c:27:3:27:12 | call to tss_create | +| test.c:33:3:33:12 | call to tss_create | +| test.c:40:3:40:12 | call to tss_create | +| test.c:62:3:62:12 | call to tss_create | +| test.c:66:3:66:12 | call to tss_create | +| test.c:70:3:70:12 | call to tss_create | +| test.c:74:3:74:12 | call to tss_create | +| test.c:78:3:78:12 | call to tss_create | +| test.c:82:3:82:12 | call to tss_create | +| test.c:86:3:86:12 | call to tss_create | +| test.c:91:3:91:12 | call to tss_create | +| test.c:96:3:96:12 | call to tss_create | diff --git a/c/cert/test/rules/CON30-C/main.c b/c/cert/test/rules/CON30-C/test.c similarity index 100% rename from c/cert/test/rules/CON30-C/main.c rename to c/cert/test/rules/CON30-C/test.c diff --git a/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected b/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected index 2d867fd55a..c3cdc8bd7b 100644 --- a/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected +++ b/c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected @@ -1,4 +1,4 @@ -| main.c:23:3:23:13 | call to thrd_create | $@ not declared with appropriate storage duration | main.c:23:24:23:29 | & ... | Shared object | -| main.c:74:3:74:13 | call to thrd_create | $@ not declared with appropriate storage duration | main.c:74:24:74:24 | p | Shared object | -| main.c:85:3:85:13 | call to thrd_create | $@ not declared with appropriate storage duration | main.c:85:24:85:24 | p | Shared object | -| main.c:94:3:94:13 | call to thrd_create | $@ not declared with appropriate storage duration | main.c:94:24:94:24 | p | Shared object | +| 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/ThreadObjectStorageDurationsNotInitialized.expected b/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.expected index 87d3ced28c..95d0a20041 100644 --- a/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.expected +++ b/c/cert/test/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.expected @@ -1 +1 @@ -| main.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. | +| 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/main.c b/c/cert/test/rules/CON34-C/test.c similarity index 100% rename from c/cert/test/rules/CON34-C/main.c rename to c/cert/test/rules/CON34-C/test.c From 74173d29e4a50cd5b72ee934156ef2514dd6bd74 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Tue, 27 Sep 2022 17:31:28 -0400 Subject: [PATCH 20/24] review fixes --- .../CON30-C/CleanUpThreadSpecificStorage.ql | 70 ++++----- ...AppropriateThreadObjectStorageDurations.ql | 13 +- .../CleanUpThreadSpecificStorage.expected | 23 ++- c/cert/test/rules/CON30-C/main.c | 140 +++++++++++++----- c/cert/test/rules/CON34-C/main.c | 2 +- .../src/codingstandards/cpp/Concurrency.qll | 11 ++ rule_packages/c/Concurrency4.json | 2 +- 7 files changed, 155 insertions(+), 106 deletions(-) diff --git a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql index 206737a35b..50db1a76ac 100644 --- a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql @@ -44,48 +44,32 @@ class TssCreateToTssDeleteDataFlowConfiguration extends DataFlow::Configuration } } -class TssCreateToFreeDataFlowConfiguration extends DataFlow::Configuration { - TssCreateToFreeDataFlowConfiguration() { this = "TssCreateToFreeDataFlowConfiguration" } - - 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(TSSGetFunctionCall tsg, FreeFunctionCall ffc, Expr e | - // the only requirement of a sink is that at some point - // it references the key of a delete call. - e.getParent*() = tsg.getKey() and - (e = node.asDefiningArgument() or e = node.asExpr()) and - ffc.getArgument(0) = tsg - ) - } -} - -from TSSCreateFunctionCall tcc +from TSSCreateFunctionCall tcfc where - not isExcluded(tcc, Concurrency4Package::cleanUpThreadSpecificStorageQuery()) and - if tcc.hasDeallocator() - then - // if they specify a deallocator the memory must be freed with a call to - // tss_delete(key), which implies that there is dataflow from the create call - // to the delete call - not exists(TssCreateToTssDeleteDataFlowConfiguration config | - config.hasFlow(DataFlow::definitionByReferenceNodeFromArgument(tcc.getKey()), _) + 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 - config.hasFlow(DataFlow::exprNode(tcc.getKey()), _) - ) - else - // otherwise, they are required to call some kind of free on the result of - // a key which has dataflow of the form tss_create -> tss_get -> free. - not exists(TssCreateToFreeDataFlowConfiguration config | - config.hasFlow(DataFlow::definitionByReferenceNodeFromArgument(tcc.getKey()), _) - or - config.hasFlow(DataFlow::exprNode(tcc.getKey()), _) - ) -select tcc + // 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.ql b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql index 352d868a31..71138f4ff8 100644 --- a/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql +++ b/c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql @@ -17,10 +17,7 @@ import codingstandards.c.cert import codingstandards.cpp.Concurrency import semmle.code.cpp.dataflow.TaintTracking import semmle.code.cpp.dataflow.DataFlow - -class MallocFunctionCall extends FunctionCall { - MallocFunctionCall() { getTarget().getName() = "malloc" } -} +import semmle.code.cpp.commons.Alloc from C11ThreadCreateCall tcc, StackVariable sv, Expr arg, Expr acc where @@ -29,11 +26,11 @@ where 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 - // it's either not static - not sv.isStatic() and // or isn't one of the allowed usage patterns - not exists(MallocFunctionCall mfc | - sv.getAnAssignedValue() = mfc and acc.getAPredecessor*() = mfc + 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 diff --git a/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected index a59afbb608..cc85cc979e 100644 --- a/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected +++ b/c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected @@ -1,12 +1,11 @@ -| main.c:27:3:27:12 | call to tss_create | -| main.c:33:3:33:12 | call to tss_create | -| main.c:40:3:40:12 | call to tss_create | -| main.c:62:3:62:12 | call to tss_create | -| main.c:66:3:66:12 | call to tss_create | -| main.c:70:3:70:12 | call to tss_create | -| main.c:74:3:74:12 | call to tss_create | -| main.c:78:3:78:12 | call to tss_create | -| main.c:82:3:82:12 | call to tss_create | -| main.c:86:3:86:12 | call to tss_create | -| main.c:91:3:91:12 | call to tss_create | -| main.c:96:3:96:12 | call to tss_create | +| main.c:27:3:27:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.c:49:3:49:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.c:71:3:71:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.c:87:3:87:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.c:95:3:95:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.c:135:3:135:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.c:139:3:139:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.c:143:3:143:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.c:147:3:147:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.c:151:3:151:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. | +| main.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/main.c b/c/cert/test/rules/CON30-C/main.c index a8961fa54d..13d802388d 100644 --- a/c/cert/test/rules/CON30-C/main.c +++ b/c/cert/test/rules/CON30-C/main.c @@ -4,95 +4,153 @@ static tss_t k; -void do_free(void *d) { free(d); } +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 m1a() { - tss_create(&k, free); // NON_COMPLIANT - The memory is deallocated, but the - // usage pattern is non-standard and may lead to errors. - free(tss_get(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 m2a() { - tss_create(&k, - do_free); // NON_COMPLIANT - The memory is deallocated, but the - // usage pattern is non-standard and may lead to errors. - free(tss_get(k)); +void m3b() { + tss_create(&k, maybe_free); // COMPLIANT - No threads created. + tss_delete(k); } -void m3a() { - tss_create( - &k, maybe_free); // NON_COMPLIANT - The memory is deallocated, but the - // usage pattern is non-standard and may lead to errors. - free(tss_get(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 m1b() { - tss_create(&k, NULL); // COMPLIANT - free(tss_get(k)); +void m5() { + tss_create(&k, NULL); // NON_COMPLIANT - `tss_delete` should be called. } -void m2b() { +void m5a() { + thrd_t id; + tss_create(&k, NULL); // COMPLIANT - free(tss_get(k)); + thrd_create(&id, t2, NULL); + thrd_join(id, NULL); + tss_delete(k); } -void m3b() { +void m5aa() { + thrd_t id; + tss_create(&k, NULL); // COMPLIANT - free(tss_get(k)); + thrd_create(&id, t3, NULL); + thrd_join(id, NULL); + tss_delete(k); } -void m4() { - tss_create(&k, free); // NON_COMPLIANT -} +void m5b() { + thrd_t id; -void m5() { - tss_create(&k, do_free); // NON_COMPLIANT + 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 m6() { - tss_create(&k, maybe_free); // NON_COMPLIANT +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 m4a() { - tss_create(&k, NULL); // NON_COMPLIANT +void m6() { + tss_create(&k, free); // NON_COMPLIANT } -void m5a() { - tss_create(&k, NULL); // NON_COMPLIANT +void m7() { + tss_create(&k, do_free); // NON_COMPLIANT } -void m6a() { - tss_create(&k, NULL); // NON_COMPLIANT +void m8() { + tss_create(&k, maybe_free); // NON_COMPLIANT } -void m4b() { +void m9() { tss_create(&k, NULL); // NON_COMPLIANT - tss_delete(k); } -void m5b() { +void m10() { tss_create(&k, NULL); // NON_COMPLIANT - tss_delete(k); } -void m6b() { +void m11() { tss_create(&k, NULL); // NON_COMPLIANT - tss_delete(k); } diff --git a/c/cert/test/rules/CON34-C/main.c b/c/cert/test/rules/CON34-C/main.c index c53640f6ab..11f24ef694 100644 --- a/c/cert/test/rules/CON34-C/main.c +++ b/c/cert/test/rules/CON34-C/main.c @@ -42,7 +42,7 @@ void m3() { void m4() { thrd_t id; - int *value = (int *)malloc(sizeof(int)); + int *value = (int *)realloc(NULL, sizeof(int)); thrd_create(&id, t1, value); // COMPLIANT diff --git a/cpp/common/src/codingstandards/cpp/Concurrency.qll b/cpp/common/src/codingstandards/cpp/Concurrency.qll index fb07829b7a..9994a79150 100644 --- a/cpp/common/src/codingstandards/cpp/Concurrency.qll +++ b/cpp/common/src/codingstandards/cpp/Concurrency.qll @@ -851,3 +851,14 @@ class TSSCreateFunctionCall extends ThreadSpecificStorageFunctionCall { 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/rule_packages/c/Concurrency4.json b/rule_packages/c/Concurrency4.json index 5ee1061a48..34de9536e3 100644 --- a/rule_packages/c/Concurrency4.json +++ b/rule_packages/c/Concurrency4.json @@ -52,7 +52,7 @@ "severity": "error", "short_name": "ThreadObjectStorageDurationsNotInitialized", "tags": [ - "external/autosar/audit", + "external/cert/audit", "correctness", "concurrency" ] From c4a5084e2a876f6e7a78c93fdd26b3123892df08 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Tue, 27 Sep 2022 17:37:15 -0400 Subject: [PATCH 21/24] format --- c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql | 4 ---- 1 file changed, 4 deletions(-) diff --git a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql index 50db1a76ac..55f4afe7d8 100644 --- a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql @@ -18,10 +18,6 @@ import codingstandards.cpp.Concurrency import semmle.code.cpp.dataflow.TaintTracking import semmle.code.cpp.dataflow.DataFlow -class FreeFunctionCall extends FunctionCall { - FreeFunctionCall() { getTarget().getName() = "free" } -} - class TssCreateToTssDeleteDataFlowConfiguration extends DataFlow::Configuration { TssCreateToTssDeleteDataFlowConfiguration() { this = "TssCreateToTssDeleteDataFlowConfiguration" } From 508b37a60e9c8894dd6b8602706186688c1c7aba Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 28 Sep 2022 09:18:20 -0400 Subject: [PATCH 22/24] metadata --- .../rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql index b6a5b8cb32..ddcddb8dc5 100644 --- a/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql +++ b/c/cert/src/rules/CON34-C/ThreadObjectStorageDurationsNotInitialized.ql @@ -7,7 +7,7 @@ * @precision high * @problem.severity error * @tags external/cert/id/con34-c - * external/autosar/audit + * external/cert/audit * correctness * concurrency * external/cert/obligation/rule From 2e9d5ccb3edc4a649c30f714d2501f43a53d2b74 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 28 Sep 2022 11:59:29 -0400 Subject: [PATCH 23/24] formatting --- c/cert/test/rules/CON34-C/test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c/cert/test/rules/CON34-C/test.c b/c/cert/test/rules/CON34-C/test.c index 11f24ef694..2b5e62d5a6 100644 --- a/c/cert/test/rules/CON34-C/test.c +++ b/c/cert/test/rules/CON34-C/test.c @@ -42,7 +42,7 @@ void m3() { void m4() { thrd_t id; - int *value = (int *)realloc(NULL, sizeof(int)); + int *value = (int *)realloc(NULL, sizeof(int)); thrd_create(&id, t1, value); // COMPLIANT From 2cb76bbf21819a4017c0133452192dfbbb418727 Mon Sep 17 00:00:00 2001 From: "John L. Singleton" Date: Wed, 28 Sep 2022 12:02:04 -0400 Subject: [PATCH 24/24] docs --- c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md | 2 +- rule_packages/c/Concurrency4.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md index b16aa677a3..8f4dec4c3e 100644 --- a/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md +++ b/c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.md @@ -173,7 +173,7 @@ Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+D ## Implementation notes -This query does not attempt to ensure that the deallocation function in fact deallocates memory and instead assumes the contract is valid. +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 diff --git a/rule_packages/c/Concurrency4.json b/rule_packages/c/Concurrency4.json index 34de9536e3..65a17ed2d7 100644 --- a/rule_packages/c/Concurrency4.json +++ b/rule_packages/c/Concurrency4.json @@ -17,7 +17,7 @@ "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." + "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." } }