Skip to content

Commit f9f99a0

Browse files
jketemajketema
authored
Merge pull request #20126 from MathiasVP/fix-missing-global-flow
C++: Fix missing global variable flow
2 parents 851cb04 + c8eb1cf commit f9f99a0

File tree

16 files changed

+358
-7
lines changed

16 files changed

+358
-7
lines changed

‎cpp/ql/lib/change-notes/2025-08-11-global-variable-flow.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Improved dataflow through global variables in the new dataflow library (`semmle.code.cpp.dataflow.new.DataFlow` and `semmle.code.cpp.dataflow.new.TaintTracking`). Queries based on these libraries will produce more results on codebases with many global variables.

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,13 @@ private module IndirectInstructions {
332332

333333
import IndirectInstructions
334334

335+
predicate isPostUpdateNodeImpl(Operand operand, int indirectionIndex) {
336+
operand = any(FieldAddress fa).getObjectAddressOperand() and
337+
indirectionIndex = [0 .. Ssa::countIndirectionsForCppType(Ssa::getLanguageType(operand))]
338+
or
339+
Ssa::isModifiableByCall(operand, indirectionIndex)
340+
}
341+
335342
/** Gets the callable in which this node occurs. */
336343
DataFlowCallable nodeGetEnclosingCallable(Node n) { result = n.getEnclosingCallable() }
337344

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,7 @@ private newtype TIRDataFlowNode =
4242
[getMinIndirectionsForType(var.getUnspecifiedType()) .. SsaImpl::getMaxIndirectionsForType(var.getUnspecifiedType())]
4343
} or
4444
TPostUpdateNodeImpl(Operand operand, int indirectionIndex) {
45-
operand = any(FieldAddress fa).getObjectAddressOperand() and
46-
indirectionIndex =
47-
[0 .. SsaImpl::countIndirectionsForCppType(SsaImpl::getLanguageType(operand))]
48-
or
49-
SsaImpl::isModifiableByCall(operand, indirectionIndex)
45+
isPostUpdateNodeImpl(operand, indirectionIndex)
5046
} or
5147
TSsaSynthNode(SsaImpl::SynthNode n) or
5248
TSsaIteratorNode(IteratorFlow::IteratorFlowNode n) or

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,14 @@ private predicate isGlobalUse(
143143
min(int cand, VariableAddressInstruction vai |
144144
vai.getEnclosingIRFunction() = f and
145145
vai.getAstVariable() = v and
146-
isDef(_, _, _, vai, cand, indirectionIndex)
146+
(
147+
isDef(_, _, _, vai, cand, indirectionIndex)
148+
or
149+
exists(Operand operand |
150+
isUse(_, operand, vai, cand, indirectionIndex) and
151+
isPostUpdateNodeImpl(operand, indirectionIndex)
152+
)
153+
)
147154
|
148155
cand
149156
)

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ argHasPostUpdate
4343
| test.cpp:813:19:813:35 | * ... | ArgumentNode is missing PostUpdateNode. |
4444
| test.cpp:848:23:848:25 | rpx | ArgumentNode is missing PostUpdateNode. |
4545
| test.cpp:1093:19:1093:21 | * ... | ArgumentNode is missing PostUpdateNode. |
46+
| test.cpp:1206:19:1206:37 | * ... | ArgumentNode is missing PostUpdateNode. |
47+
| test.cpp:1207:10:1207:28 | * ... | ArgumentNode is missing PostUpdateNode. |
48+
| test.cpp:1224:19:1224:37 | * ... | ArgumentNode is missing PostUpdateNode. |
49+
| test.cpp:1225:10:1225:28 | * ... | ArgumentNode is missing PostUpdateNode. |
4650
postWithInFlow
4751
| BarrierGuard.cpp:49:6:49:6 | x [post update] | PostUpdateNode should not be the target of local flow. |
4852
| BarrierGuard.cpp:60:7:60:7 | x [post update] | PostUpdateNode should not be the target of local flow. |
@@ -193,6 +197,10 @@ postWithInFlow
193197
| test.cpp:1139:4:1139:7 | data [inner post update] | PostUpdateNode should not be the target of local flow. |
194198
| test.cpp:1153:5:1153:6 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
195199
| test.cpp:1153:6:1153:6 | p [inner post update] | PostUpdateNode should not be the target of local flow. |
200+
| test.cpp:1165:5:1165:6 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
201+
| test.cpp:1165:6:1165:6 | p [inner post update] | PostUpdateNode should not be the target of local flow. |
202+
| test.cpp:1195:5:1195:6 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
203+
| test.cpp:1195:6:1195:6 | p [inner post update] | PostUpdateNode should not be the target of local flow. |
196204
viableImplInCallContextTooLarge
197205
uniqueParameterNodeAtPosition
198206
uniqueParameterNodePosition

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/test-source-sink.expected

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,11 @@ astFlow
134134
| test.cpp:1086:12:1086:12 | a | test.cpp:1088:8:1088:9 | & ... |
135135
| test.cpp:1137:7:1137:10 | data | test.cpp:1140:8:1140:18 | * ... |
136136
| test.cpp:1138:17:1138:22 | call to source | test.cpp:1140:8:1140:18 | * ... |
137+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1170:19:1170:32 | global_int_ptr |
138+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1234:19:1234:34 | global_int_array |
139+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1239:10:1239:26 | * ... |
140+
| test.cpp:1195:10:1195:24 | call to indirect_source | test.cpp:1200:19:1200:36 | global_int_ptr_ptr |
141+
| test.cpp:1195:10:1195:24 | call to indirect_source | test.cpp:1201:10:1201:27 | global_int_ptr_ptr |
137142
| true_upon_entry.cpp:17:11:17:16 | call to source | true_upon_entry.cpp:21:8:21:8 | x |
138143
| true_upon_entry.cpp:27:9:27:14 | call to source | true_upon_entry.cpp:29:8:29:8 | x |
139144
| true_upon_entry.cpp:33:11:33:16 | call to source | true_upon_entry.cpp:39:8:39:8 | x |
@@ -327,6 +332,20 @@ irFlow
327332
| test.cpp:1117:27:1117:34 | call to source | test.cpp:1117:27:1117:34 | call to source |
328333
| test.cpp:1132:11:1132:16 | call to source | test.cpp:1121:8:1121:8 | x |
329334
| test.cpp:1138:17:1138:22 | call to source | test.cpp:1140:8:1140:18 | * ... |
335+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1170:19:1170:32 | *global_int_ptr |
336+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1175:10:1175:24 | * ... |
337+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1184:19:1184:32 | *global_int_ptr |
338+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1189:10:1189:24 | * ... |
339+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1234:19:1234:34 | *global_int_array |
340+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1239:10:1239:26 | * ... |
341+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1248:19:1248:34 | *global_int_array |
342+
| test.cpp:1165:10:1165:15 | call to source | test.cpp:1253:10:1253:26 | * ... |
343+
| test.cpp:1195:10:1195:24 | *call to indirect_source | test.cpp:1200:19:1200:36 | **global_int_ptr_ptr |
344+
| test.cpp:1195:10:1195:24 | *call to indirect_source | test.cpp:1206:19:1206:37 | ** ... |
345+
| test.cpp:1195:10:1195:24 | *call to indirect_source | test.cpp:1209:10:1209:29 | * ... |
346+
| test.cpp:1195:10:1195:24 | *call to indirect_source | test.cpp:1218:19:1218:36 | **global_int_ptr_ptr |
347+
| test.cpp:1195:10:1195:24 | *call to indirect_source | test.cpp:1224:19:1224:37 | ** ... |
348+
| test.cpp:1195:10:1195:24 | *call to indirect_source | test.cpp:1227:10:1227:29 | * ... |
330349
| true_upon_entry.cpp:9:11:9:16 | call to source | true_upon_entry.cpp:13:8:13:8 | x |
331350
| true_upon_entry.cpp:17:11:17:16 | call to source | true_upon_entry.cpp:21:8:21:8 | x |
332351
| true_upon_entry.cpp:27:9:27:14 | call to source | true_upon_entry.cpp:29:8:29:8 | x |

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp

Lines changed: 98 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1155,4 +1155,101 @@ namespace conflation_regression {
11551155
}
11561156
}
11571157

1158-
int recursion = (sink(recursion), source()); // clean
1158+
int recursion = (sink(recursion), source()); // clean
1159+
1160+
1161+
namespace globals_without_explicit_def {
1162+
int* global_int_ptr;
1163+
1164+
void set(int* p) { // $ ast-def=p ir-def=*p
1165+
*p = source();
1166+
}
1167+
1168+
void test1() {
1169+
set(global_int_ptr);
1170+
indirect_sink(global_int_ptr); // $ ir,ast
1171+
}
1172+
1173+
void test2() {
1174+
set(global_int_ptr);
1175+
sink(*global_int_ptr); // $ ir MISSING: ast
1176+
}
1177+
1178+
void calls_set() {
1179+
set(global_int_ptr);
1180+
}
1181+
1182+
void test3() {
1183+
calls_set();
1184+
indirect_sink(global_int_ptr); // $ ir MISSING: ast
1185+
}
1186+
1187+
void test4() {
1188+
calls_set();
1189+
sink(*global_int_ptr); // $ ir MISSING: ast
1190+
}
1191+
1192+
int** global_int_ptr_ptr;
1193+
1194+
void set_indirect(int** p) { // $ ast-def=p ir-def=*p ir-def=**p
1195+
*p = indirect_source();
1196+
}
1197+
1198+
void test5() {
1199+
set_indirect(global_int_ptr_ptr);
1200+
indirect_sink(global_int_ptr_ptr); // $ ir,ast
1201+
sink(global_int_ptr_ptr); // $ SPURIOUS: ast
1202+
}
1203+
1204+
void test6() {
1205+
set_indirect(global_int_ptr_ptr);
1206+
indirect_sink(*global_int_ptr_ptr); // $ ir MISSING: ast
1207+
sink(*global_int_ptr_ptr);
1208+
indirect_sink(**global_int_ptr_ptr);
1209+
sink(**global_int_ptr_ptr); // $ ir
1210+
}
1211+
1212+
void calls_set_indirect() {
1213+
set_indirect(global_int_ptr_ptr);
1214+
}
1215+
1216+
void test7() {
1217+
calls_set_indirect();
1218+
indirect_sink(global_int_ptr_ptr); // $ ir MISSING: ast
1219+
sink(global_int_ptr_ptr); // $ MISSING: ast
1220+
}
1221+
1222+
void test8() {
1223+
calls_set_indirect();
1224+
indirect_sink(*global_int_ptr_ptr); // $ ir MISSING: ast
1225+
sink(*global_int_ptr_ptr);
1226+
indirect_sink(**global_int_ptr_ptr);
1227+
sink(**global_int_ptr_ptr); // $ ir MISSING: ast
1228+
}
1229+
1230+
int global_int_array[10];
1231+
1232+
void test9() {
1233+
set(global_int_array);
1234+
indirect_sink(global_int_array); // $ ir,ast
1235+
}
1236+
1237+
void test10() {
1238+
set(global_int_array);
1239+
sink(*global_int_array); // $ ir,ast
1240+
}
1241+
1242+
void calls_set_array() {
1243+
set(global_int_array);
1244+
}
1245+
1246+
void test11() {
1247+
calls_set_array();
1248+
indirect_sink(global_int_array); // $ ir MISSING: ast
1249+
}
1250+
1251+
void test12() {
1252+
calls_set_array();
1253+
sink(*global_int_array); // $ ir MISSING: ast
1254+
}
1255+
}

‎cpp/ql/test/library-tests/dataflow/fields/aliasing.cpp

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,4 +204,65 @@ void deep_member_field_arrow(S2 *ps2) {
204204
void deep_member_field_arrow_different_fields(S2 *ps2) {
205205
taint_a_ptr(&ps2->s.m1);
206206
sink(ps2->s.m2);
207+
}
208+
209+
210+
namespace GlobalFieldFlow {
211+
S global_s;
212+
S2 global_s2;
213+
214+
void set_field() {
215+
global_s.m1 = user_input();
216+
}
217+
218+
void read_field() {
219+
sink(global_s.m1); // $ ir MISSING: ast
220+
}
221+
222+
void set_nested_field() {
223+
global_s2.s.m1 = user_input();
224+
}
225+
226+
void read_nested_field() {
227+
sink(global_s2.s.m1); // $ ir MISSING: ast
228+
}
229+
230+
S* global_s_ptr;
231+
S2* global_s2_ptr;
232+
233+
void set_field_ptr() {
234+
global_s_ptr->m1 = user_input();
235+
}
236+
237+
void read_field_ptr() {
238+
sink(global_s_ptr->m1); // $ ir MISSING: ast
239+
}
240+
241+
void set_nested_field_ptr() {
242+
global_s2_ptr->s.m1 = user_input();
243+
}
244+
245+
void read_nested_field_ptr() {
246+
sink(global_s2_ptr->s.m1); // $ ir MISSING: ast
247+
}
248+
249+
S_with_pointer global_s_with_pointer;
250+
251+
void set_field_indirect() {
252+
*global_s_with_pointer.data = user_input();
253+
}
254+
255+
void read_field_indirect() {
256+
sink(*global_s_with_pointer.data); // $ ir MISSING: ast
257+
}
258+
259+
S_with_array global_s_with_array;
260+
261+
void set_field_array() {
262+
*global_s_with_array.data = user_input();
263+
}
264+
265+
void read_field_array() {
266+
sink(*global_s_with_array.data); // $ ir MISSING: ast
267+
}
207268
}

‎cpp/ql/test/library-tests/dataflow/fields/dataflow-consistency.expected

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,14 @@ postWithInFlow
9595
| aliasing.cpp:194:21:194:22 | m1 [inner post update] | PostUpdateNode should not be the target of local flow. |
9696
| aliasing.cpp:200:23:200:24 | m1 [inner post update] | PostUpdateNode should not be the target of local flow. |
9797
| aliasing.cpp:205:23:205:24 | m1 [inner post update] | PostUpdateNode should not be the target of local flow. |
98+
| aliasing.cpp:215:14:215:15 | m1 [post update] | PostUpdateNode should not be the target of local flow. |
99+
| aliasing.cpp:223:17:223:18 | m1 [post update] | PostUpdateNode should not be the target of local flow. |
100+
| aliasing.cpp:234:19:234:20 | m1 [post update] | PostUpdateNode should not be the target of local flow. |
101+
| aliasing.cpp:242:22:242:23 | m1 [post update] | PostUpdateNode should not be the target of local flow. |
102+
| aliasing.cpp:252:5:252:31 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
103+
| aliasing.cpp:252:28:252:31 | data [inner post update] | PostUpdateNode should not be the target of local flow. |
104+
| aliasing.cpp:262:5:262:29 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
105+
| aliasing.cpp:262:26:262:29 | data [inner post update] | PostUpdateNode should not be the target of local flow. |
98106
| arrays.cpp:6:3:6:5 | arr [inner post update] | PostUpdateNode should not be the target of local flow. |
99107
| arrays.cpp:6:3:6:8 | access to array [post update] | PostUpdateNode should not be the target of local flow. |
100108
| arrays.cpp:15:3:15:10 | * ... [post update] | PostUpdateNode should not be the target of local flow. |

0 commit comments

Comments
 (0)