Skip to content

Commit 74173d2

Browse files
jsingletjsinglet
committed
review fixes
1 parent 4a24cf0 commit 74173d2

File tree

7 files changed

+155
-106
lines changed

7 files changed

+155
-106
lines changed

‎c/cert/src/rules/CON30-C/CleanUpThreadSpecificStorage.ql

Lines changed: 27 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -44,48 +44,32 @@ class TssCreateToTssDeleteDataFlowConfiguration extends DataFlow::Configuration
4444
}
4545
}
4646

47-
class TssCreateToFreeDataFlowConfiguration extends DataFlow::Configuration {
48-
TssCreateToFreeDataFlowConfiguration() { this = "TssCreateToFreeDataFlowConfiguration" }
49-
50-
override predicate isSource(DataFlow::Node node) {
51-
exists(TSSCreateFunctionCall tsc, Expr e |
52-
// the only requirement of the source is that at some point
53-
// it refers to the key of a create statement
54-
e.getParent*() = tsc.getKey() and
55-
(e = node.asDefiningArgument() or e = node.asExpr())
56-
)
57-
}
58-
59-
override predicate isSink(DataFlow::Node node) {
60-
exists(TSSGetFunctionCall tsg, FreeFunctionCall ffc, Expr e |
61-
// the only requirement of a sink is that at some point
62-
// it references the key of a delete call.
63-
e.getParent*() = tsg.getKey() and
64-
(e = node.asDefiningArgument() or e = node.asExpr()) and
65-
ffc.getArgument(0) = tsg
66-
)
67-
}
68-
}
69-
70-
from TSSCreateFunctionCall tcc
47+
from TSSCreateFunctionCall tcfc
7148
where
72-
not isExcluded(tcc, Concurrency4Package::cleanUpThreadSpecificStorageQuery()) and
73-
if tcc.hasDeallocator()
74-
then
75-
// if they specify a deallocator the memory must be freed with a call to
76-
// tss_delete(key), which implies that there is dataflow from the create call
77-
// to the delete call
78-
not exists(TssCreateToTssDeleteDataFlowConfiguration config |
79-
config.hasFlow(DataFlow::definitionByReferenceNodeFromArgument(tcc.getKey()), _)
49+
not isExcluded(tcfc, Concurrency4Package::cleanUpThreadSpecificStorageQuery()) and
50+
// all calls to `tss_create` must be bookended by calls to tss_delete
51+
// even if a thread is not created.
52+
not exists(TssCreateToTssDeleteDataFlowConfiguration config |
53+
config.hasFlow(DataFlow::definitionByReferenceNodeFromArgument(tcfc.getKey()), _)
54+
or
55+
config.hasFlow(DataFlow::exprNode(tcfc.getKey()), _)
56+
)
57+
or
58+
// if a thread is created, we must check additional items
59+
exists(C11ThreadCreateCall tcc |
60+
tcfc.getASuccessor*() = tcc and
61+
if tcfc.hasDeallocator()
62+
then
63+
// if they specify a deallocator, they must wait for this thread to finish, otherwise
64+
// automatic calls to the deallocator will not work.
65+
not exists(ThreadWait tw | tcc.getASuccessor*() = tw)
8066
or
81-
config.hasFlow(DataFlow::exprNode(tcc.getKey()), _)
82-
)
83-
else
84-
// otherwise, they are required to call some kind of free on the result of
85-
// a key which has dataflow of the form tss_create -> tss_get -> free.
86-
not exists(TssCreateToFreeDataFlowConfiguration config |
87-
config.hasFlow(DataFlow::definitionByReferenceNodeFromArgument(tcc.getKey()), _)
88-
or
89-
config.hasFlow(DataFlow::exprNode(tcc.getKey()), _)
90-
)
91-
select tcc
67+
// freeing memory twice can lead to errors; because of this we report cases
68+
// where a deallocator is specified but free is called explicitly.
69+
getAThreadSpecificStorageDeallocationCall(tcc, _)
70+
else
71+
// otherwise, we require that the thread that gets called calls a free like
72+
// function with the argument of a `tss_get` call.
73+
not getAThreadSpecificStorageDeallocationCall(tcc, _)
74+
)
75+
select tcfc, "Resources used by thread specific storage may not be cleaned up."

‎c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,7 @@ import codingstandards.c.cert
1717
import codingstandards.cpp.Concurrency
1818
import semmle.code.cpp.dataflow.TaintTracking
1919
import semmle.code.cpp.dataflow.DataFlow
20-
21-
class MallocFunctionCall extends FunctionCall {
22-
MallocFunctionCall() { getTarget().getName() = "malloc" }
23-
}
20+
import semmle.code.cpp.commons.Alloc
2421

2522
from C11ThreadCreateCall tcc, StackVariable sv, Expr arg, Expr acc
2623
where
@@ -29,11 +26,11 @@ where
2926
sv.getAnAccess() = acc and
3027
// a stack variable that is given as an argument to a thread
3128
TaintTracking::localTaint(DataFlow::exprNode(acc), DataFlow::exprNode(arg)) and
32-
// it's either not static
33-
not sv.isStatic() and
3429
// or isn't one of the allowed usage patterns
35-
not exists(MallocFunctionCall mfc |
36-
sv.getAnAssignedValue() = mfc and acc.getAPredecessor*() = mfc
30+
not exists(Expr mfc |
31+
isAllocationExpr(mfc) and
32+
sv.getAnAssignedValue() = mfc and
33+
acc.getAPredecessor*() = mfc
3734
) and
3835
not exists(TSSGetFunctionCall tsg, TSSSetFunctionCall tss, DataFlow::Node src |
3936
sv.getAnAssignedValue() = tsg and

‎c/cert/test/rules/CON30-C/CleanUpThreadSpecificStorage.expected

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
1-
| main.c:27:3:27:12 | call to tss_create |
2-
| main.c:33:3:33:12 | call to tss_create |
3-
| main.c:40:3:40:12 | call to tss_create |
4-
| main.c:62:3:62:12 | call to tss_create |
5-
| main.c:66:3:66:12 | call to tss_create |
6-
| main.c:70:3:70:12 | call to tss_create |
7-
| main.c:74:3:74:12 | call to tss_create |
8-
| main.c:78:3:78:12 | call to tss_create |
9-
| main.c:82:3:82:12 | call to tss_create |
10-
| main.c:86:3:86:12 | call to tss_create |
11-
| main.c:91:3:91:12 | call to tss_create |
12-
| main.c:96:3:96:12 | call to tss_create |
1+
| main.c:27:3:27:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
2+
| main.c:49:3:49:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
3+
| main.c:71:3:71:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
4+
| main.c:87:3:87:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
5+
| main.c:95:3:95:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
6+
| main.c:135:3:135:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
7+
| main.c:139:3:139:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
8+
| main.c:143:3:143:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
9+
| main.c:147:3:147:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
10+
| main.c:151:3:151:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |
11+
| main.c:155:3:155:12 | call to tss_create | Resources used by thread specific storage may not be cleaned up. |

‎c/cert/test/rules/CON30-C/main.c

Lines changed: 99 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -4,95 +4,153 @@
44

55
static tss_t k;
66

7-
void do_free(void *d) { free(d); }
7+
void t1(void *data) {}
8+
void t2(void *data) { free(tss_get(k)); }
9+
void t3(void *data) {
10+
void *p = tss_get(k);
11+
free(p);
12+
}
813

14+
void do_free(void *d) { free(d); }
915
void maybe_free(void *d) {}
1016

1117
void m1() {
18+
thrd_t id;
1219
tss_create(&k, free); // COMPLIANT
20+
thrd_create(&id, t1, NULL);
21+
thrd_join(id, NULL);
22+
tss_delete(k);
23+
}
24+
25+
void m1a() {
26+
thrd_t id;
27+
tss_create(&k, free); // NON_COMPLIANT - Doesn't wait for thread to cleanup
28+
// resources; if tss_delete is called prior to thread
29+
// termination the destructor won't be called.
30+
thrd_create(&id, t1, NULL);
31+
tss_delete(k);
32+
}
33+
34+
void m1b() {
35+
tss_create(&k, free); // COMPLIANT - No threads created.
1336
tss_delete(k);
1437
}
1538

1639
void m2() {
40+
thrd_t id;
1741
tss_create(&k, do_free); // COMPLIANT
42+
thrd_create(&id, t1, NULL);
43+
thrd_join(id, NULL);
44+
tss_delete(k);
45+
}
46+
47+
void m2a() {
48+
thrd_t id;
49+
tss_create(&k, do_free); // NON_COMPLIANT - Doesn't wait for thread to cleanup
50+
// resources; if tss_delete is called prior to thread
51+
// termination the destructor won't be called.
52+
thrd_create(&id, t1, NULL);
53+
tss_delete(k);
54+
}
55+
56+
void m2b() {
57+
tss_create(&k, do_free); // COMPLIANT - No threads created.
1858
tss_delete(k);
1959
}
2060

2161
void m3() {
62+
thrd_t id;
2263
tss_create(&k, maybe_free); // COMPLIANT
64+
thrd_create(&id, t1, NULL);
65+
thrd_join(id, NULL);
2366
tss_delete(k);
2467
}
2568

26-
void m1a() {
27-
tss_create(&k, free); // NON_COMPLIANT - The memory is deallocated, but the
28-
// usage pattern is non-standard and may lead to errors.
29-
free(tss_get(k));
69+
void m3a() {
70+
thrd_t id;
71+
tss_create(&k,
72+
maybe_free); // NON_COMPLIANT - Doesn't wait for thread to cleanup
73+
// resources; if tss_delete is called prior to thread
74+
// termination the destructor won't be called.
75+
thrd_create(&id, t1, NULL);
76+
tss_delete(k);
3077
}
3178

32-
void m2a() {
33-
tss_create(&k,
34-
do_free); // NON_COMPLIANT - The memory is deallocated, but the
35-
// usage pattern is non-standard and may lead to errors.
36-
free(tss_get(k));
79+
void m3b() {
80+
tss_create(&k, maybe_free); // COMPLIANT - No threads created.
81+
tss_delete(k);
3782
}
3883

39-
void m3a() {
40-
tss_create(
41-
&k, maybe_free); // NON_COMPLIANT - The memory is deallocated, but the
42-
// usage pattern is non-standard and may lead to errors.
43-
free(tss_get(k));
84+
void m4() {
85+
thrd_t id;
86+
87+
tss_create(&k, free); // NON_COMPLIANT - The memory is deallocated, but the
88+
// usage pattern is non-standard and may lead to errors.
89+
thrd_create(&id, t2, NULL);
90+
thrd_join(id, NULL);
91+
tss_delete(k);
4492
}
4593

46-
void m1b() {
47-
tss_create(&k, NULL); // COMPLIANT
48-
free(tss_get(k));
94+
void m5() {
95+
tss_create(&k, NULL); // NON_COMPLIANT - `tss_delete` should be called.
4996
}
5097

51-
void m2b() {
98+
void m5a() {
99+
thrd_t id;
100+
52101
tss_create(&k, NULL); // COMPLIANT
53-
free(tss_get(k));
102+
thrd_create(&id, t2, NULL);
103+
thrd_join(id, NULL);
104+
tss_delete(k);
54105
}
55106

56-
void m3b() {
107+
void m5aa() {
108+
thrd_t id;
109+
57110
tss_create(&k, NULL); // COMPLIANT
58-
free(tss_get(k));
111+
thrd_create(&id, t3, NULL);
112+
thrd_join(id, NULL);
113+
tss_delete(k);
59114
}
60115

61-
void m4() {
62-
tss_create(&k, free); // NON_COMPLIANT
63-
}
116+
void m5b() {
117+
thrd_t id;
64118

65-
void m5() {
66-
tss_create(&k, do_free); // NON_COMPLIANT
119+
tss_create(&k, NULL); // COMPLIANT - Cleanup can happen before OR after
120+
// `tss_delete` is called; so there is no need to wait.
121+
thrd_create(&id, t2, NULL);
122+
tss_delete(k);
67123
}
68124

69-
void m6() {
70-
tss_create(&k, maybe_free); // NON_COMPLIANT
125+
void m5bb() {
126+
thrd_t id;
127+
128+
tss_create(&k, NULL); // COMPLIANT - Cleanup can happen before OR after
129+
// `tss_delete` is called; so there is no need to wait.
130+
thrd_create(&id, t3, NULL);
131+
tss_delete(k);
71132
}
72133

73-
void m4a() {
74-
tss_create(&k, NULL); // NON_COMPLIANT
134+
void m6() {
135+
tss_create(&k, free); // NON_COMPLIANT
75136
}
76137

77-
void m5a() {
78-
tss_create(&k, NULL); // NON_COMPLIANT
138+
void m7() {
139+
tss_create(&k, do_free); // NON_COMPLIANT
79140
}
80141

81-
void m6a() {
82-
tss_create(&k, NULL); // NON_COMPLIANT
142+
void m8() {
143+
tss_create(&k, maybe_free); // NON_COMPLIANT
83144
}
84145

85-
void m4b() {
146+
void m9() {
86147
tss_create(&k, NULL); // NON_COMPLIANT
87-
tss_delete(k);
88148
}
89149

90-
void m5b() {
150+
void m10() {
91151
tss_create(&k, NULL); // NON_COMPLIANT
92-
tss_delete(k);
93152
}
94153

95-
void m6b() {
154+
void m11() {
96155
tss_create(&k, NULL); // NON_COMPLIANT
97-
tss_delete(k);
98156
}

‎c/cert/test/rules/CON34-C/main.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ void m3() {
4242

4343
void m4() {
4444
thrd_t id;
45-
int *value = (int *)malloc(sizeof(int));
45+
int *value = (int *)realloc(NULL, sizeof(int));
4646

4747
thrd_create(&id, t1, value); // COMPLIANT
4848

‎cpp/common/src/codingstandards/cpp/Concurrency.qll

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -851,3 +851,14 @@ class TSSCreateFunctionCall extends ThreadSpecificStorageFunctionCall {
851851
class TSSDeleteFunctionCall extends ThreadSpecificStorageFunctionCall {
852852
TSSDeleteFunctionCall() { getTarget().getName() = "tss_delete" }
853853
}
854+
855+
/**
856+
* Gets a call to `DeallocationExpr` that deallocates memory owned by thread specific
857+
* storage.
858+
*/
859+
predicate getAThreadSpecificStorageDeallocationCall(C11ThreadCreateCall tcc, DeallocationExpr dexp) {
860+
exists(TSSGetFunctionCall tsg |
861+
tcc.getFunction().getEntryPoint().getASuccessor*() = tsg and
862+
DataFlow::localFlow(DataFlow::exprNode(tsg), DataFlow::exprNode(dexp.getFreedExpr()))
863+
)
864+
}

‎rule_packages/c/Concurrency4.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
"severity": "error",
5353
"short_name": "ThreadObjectStorageDurationsNotInitialized",
5454
"tags": [
55-
"external/autosar/audit",
55+
"external/cert/audit",
5656
"correctness",
5757
"concurrency"
5858
]

0 commit comments

Comments
 (0)