Skip to content

Commit f696594

Browse files
jbjjbj
authored
Merge pull request github#3295 from MathiasVP/field-flow-single-struct
C++: Add PostUpdateNode for updates to structs with no chi instructions
2 parents cbed175 + a49d22e commit f696594

File tree

7 files changed

+93
-6
lines changed

7 files changed

+93
-6
lines changed

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

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -180,12 +180,16 @@ private class ArrayContent extends Content, TArrayContent {
180180
override Type getType() { none() }
181181
}
182182

183-
/**
184-
* Holds if data can flow from `node1` to `node2` via an assignment to `f`.
185-
* Thus, `node2` references an object with a field `f` that contains the
186-
* value of `node1`.
187-
*/
188-
predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
183+
private predicate storeStepNoChi(Node node1, Content f, PostUpdateNode node2) {
184+
exists(FieldAddressInstruction fa, StoreInstruction store |
185+
store = node2.asInstruction() and
186+
store.getDestinationAddress() = fa and
187+
store.getSourceValue() = node1.asInstruction() and
188+
f.(FieldContent).getField() = fa.getField()
189+
)
190+
}
191+
192+
private predicate storeStepChi(Node node1, Content f, PostUpdateNode node2) {
189193
exists(FieldAddressInstruction fa, StoreInstruction store |
190194
node1.asInstruction() = store and
191195
store.getDestinationAddress() = fa and
@@ -194,6 +198,16 @@ predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
194198
)
195199
}
196200

201+
/**
202+
* Holds if data can flow from `node1` to `node2` via an assignment to `f`.
203+
* Thus, `node2` references an object with a field `f` that contains the
204+
* value of `node1`.
205+
*/
206+
predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
207+
storeStepNoChi(node1, f, node2) or
208+
storeStepChi(node1, f, node2)
209+
}
210+
197211
/**
198212
* Holds if data can flow from `node1` to `node2` via a read of `f`.
199213
* Thus, `node1` references an object with a field `f` whose value ends up in

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,25 @@ private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
270270
override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() }
271271
}
272272

273+
/**
274+
* Not every store instruction generates a chi instruction that we can attach a PostUpdateNode to.
275+
* For instance, an update to a field of a struct containing only one field. For these cases we
276+
* attach the PostUpdateNode to the store instruction. There's no obvious pre update node for this case
277+
* (as the entire memory is updated), so `getPreUpdateNode` is implemented as `none()`.
278+
*/
279+
private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNode {
280+
override StoreInstruction instr;
281+
282+
ExplicitSingleFieldStoreQualifierNode() {
283+
exists(FieldAddressInstruction field |
284+
field = instr.getDestinationAddress() and
285+
not exists(ChiInstruction chi | chi.getPartial() = instr)
286+
)
287+
}
288+
289+
override Node getPreUpdateNode() { none() }
290+
}
291+
273292
/**
274293
* A node that represents the value of a variable after a function call that
275294
* may have changed the variable because it's passed by reference.
@@ -404,6 +423,15 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
404423
simpleInstructionLocalFlowStep(nodeFrom.asInstruction(), nodeTo.asInstruction())
405424
}
406425

426+
pragma[noinline]
427+
private predicate getFieldSizeOfClass(Class c, Type type, int size) {
428+
exists(Field f |
429+
f.getDeclaringType() = c and
430+
f.getType() = type and
431+
type.getSize() = size
432+
)
433+
}
434+
407435
cached
408436
private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction iTo) {
409437
iTo.(CopyInstruction).getSourceValue() = iFrom
@@ -452,6 +480,14 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
452480
iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = chi
453481
)
454482
or
483+
// Flow from stores to structs with a single field to a load of that field.
484+
iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = iFrom and
485+
exists(int size, Type type |
486+
type = iFrom.getResultType() and
487+
iTo.getResultType().getSize() = size and
488+
getFieldSizeOfClass(iTo.getResultType(), type, size)
489+
)
490+
or
455491
// Flow through modeled functions
456492
modelFlow(iFrom, iTo)
457493
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ unreachableNodeCCtx
1919
localCallNodes
2020
postIsNotPre
2121
postHasUniquePre
22+
| simple.cpp:65:5:65:22 | Store | PostUpdateNode should have one pre-update node but has 0. |
2223
uniquePostUpdate
2324
postIsInSameCallable
2425
reverseRead

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,10 @@ edges
221221
| simple.cpp:48:9:48:9 | g [b_] | simple.cpp:26:15:26:15 | f [b_] |
222222
| simple.cpp:51:9:51:9 | h [a_] | simple.cpp:26:15:26:15 | f [a_] |
223223
| simple.cpp:51:9:51:9 | h [b_] | simple.cpp:26:15:26:15 | f [b_] |
224+
| simple.cpp:65:5:65:5 | a [post update] [i] | simple.cpp:67:10:67:11 | a2 [i] |
225+
| simple.cpp:65:5:65:22 | ... = ... | simple.cpp:65:5:65:5 | a [post update] [i] |
226+
| simple.cpp:65:11:65:20 | call to user_input | simple.cpp:65:5:65:22 | ... = ... |
227+
| simple.cpp:67:10:67:11 | a2 [i] | simple.cpp:67:13:67:13 | i |
224228
| struct_init.c:14:24:14:25 | ab [a] | struct_init.c:15:8:15:9 | ab [a] |
225229
| struct_init.c:15:8:15:9 | ab [a] | struct_init.c:15:12:15:12 | a |
226230
| struct_init.c:20:17:20:36 | {...} [a] | struct_init.c:22:8:22:9 | ab [a] |
@@ -504,6 +508,11 @@ nodes
504508
| simple.cpp:48:9:48:9 | g [b_] | semmle.label | g [b_] |
505509
| simple.cpp:51:9:51:9 | h [a_] | semmle.label | h [a_] |
506510
| simple.cpp:51:9:51:9 | h [b_] | semmle.label | h [b_] |
511+
| simple.cpp:65:5:65:5 | a [post update] [i] | semmle.label | a [post update] [i] |
512+
| simple.cpp:65:5:65:22 | ... = ... | semmle.label | ... = ... |
513+
| simple.cpp:65:11:65:20 | call to user_input | semmle.label | call to user_input |
514+
| simple.cpp:67:10:67:11 | a2 [i] | semmle.label | a2 [i] |
515+
| simple.cpp:67:13:67:13 | i | semmle.label | i |
507516
| struct_init.c:14:24:14:25 | ab [a] | semmle.label | ab [a] |
508517
| struct_init.c:15:8:15:9 | ab [a] | semmle.label | ab [a] |
509518
| struct_init.c:15:12:15:12 | a | semmle.label | a |
@@ -580,6 +589,7 @@ nodes
580589
| simple.cpp:28:12:28:12 | call to a | simple.cpp:41:12:41:21 | call to user_input | simple.cpp:28:12:28:12 | call to a | call to a flows from $@ | simple.cpp:41:12:41:21 | call to user_input | call to user_input |
581590
| simple.cpp:29:12:29:12 | call to b | simple.cpp:40:12:40:21 | call to user_input | simple.cpp:29:12:29:12 | call to b | call to b flows from $@ | simple.cpp:40:12:40:21 | call to user_input | call to user_input |
582591
| simple.cpp:29:12:29:12 | call to b | simple.cpp:42:12:42:21 | call to user_input | simple.cpp:29:12:29:12 | call to b | call to b flows from $@ | simple.cpp:42:12:42:21 | call to user_input | call to user_input |
592+
| simple.cpp:67:13:67:13 | i | simple.cpp:65:11:65:20 | call to user_input | simple.cpp:67:13:67:13 | i | i flows from $@ | simple.cpp:65:11:65:20 | call to user_input | call to user_input |
583593
| struct_init.c:15:12:15:12 | a | struct_init.c:20:20:20:29 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:20:20:20:29 | call to user_input | call to user_input |
584594
| struct_init.c:15:12:15:12 | a | struct_init.c:27:7:27:16 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:27:7:27:16 | call to user_input | call to user_input |
585595
| struct_init.c:15:12:15:12 | a | struct_init.c:40:20:40:29 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:40:20:40:29 | call to user_input | call to user_input |

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ edges
2525
| aliasing.cpp:79:11:79:20 | call to user_input | aliasing.cpp:80:12:80:13 | m1 |
2626
| aliasing.cpp:86:10:86:19 | call to user_input | aliasing.cpp:87:12:87:13 | m1 |
2727
| aliasing.cpp:92:12:92:21 | call to user_input | aliasing.cpp:93:12:93:13 | m1 |
28+
| simple.cpp:65:5:65:22 | Store [i] | simple.cpp:66:12:66:12 | Store [i] |
29+
| simple.cpp:65:11:65:20 | call to user_input | simple.cpp:65:5:65:22 | Store [i] |
30+
| simple.cpp:66:12:66:12 | Store [i] | simple.cpp:67:13:67:13 | i |
2831
| struct_init.c:20:20:20:29 | call to user_input | struct_init.c:22:11:22:11 | a |
2932
| struct_init.c:27:7:27:16 | call to user_input | struct_init.c:31:23:31:23 | a |
3033
nodes
@@ -63,6 +66,10 @@ nodes
6366
| aliasing.cpp:87:12:87:13 | m1 | semmle.label | m1 |
6467
| aliasing.cpp:92:12:92:21 | call to user_input | semmle.label | call to user_input |
6568
| aliasing.cpp:93:12:93:13 | m1 | semmle.label | m1 |
69+
| simple.cpp:65:5:65:22 | Store [i] | semmle.label | Store [i] |
70+
| simple.cpp:65:11:65:20 | call to user_input | semmle.label | call to user_input |
71+
| simple.cpp:66:12:66:12 | Store [i] | semmle.label | Store [i] |
72+
| simple.cpp:67:13:67:13 | i | semmle.label | i |
6673
| struct_init.c:20:20:20:29 | call to user_input | semmle.label | call to user_input |
6774
| struct_init.c:22:11:22:11 | a | semmle.label | a |
6875
| struct_init.c:27:7:27:16 | call to user_input | semmle.label | call to user_input |
@@ -78,5 +85,6 @@ nodes
7885
| aliasing.cpp:80:12:80:13 | m1 | aliasing.cpp:79:11:79:20 | call to user_input | aliasing.cpp:80:12:80:13 | m1 | m1 flows from $@ | aliasing.cpp:79:11:79:20 | call to user_input | call to user_input |
7986
| aliasing.cpp:87:12:87:13 | m1 | aliasing.cpp:86:10:86:19 | call to user_input | aliasing.cpp:87:12:87:13 | m1 | m1 flows from $@ | aliasing.cpp:86:10:86:19 | call to user_input | call to user_input |
8087
| aliasing.cpp:93:12:93:13 | m1 | aliasing.cpp:92:12:92:21 | call to user_input | aliasing.cpp:93:12:93:13 | m1 | m1 flows from $@ | aliasing.cpp:92:12:92:21 | call to user_input | call to user_input |
88+
| simple.cpp:67:13:67:13 | i | simple.cpp:65:11:65:20 | call to user_input | simple.cpp:67:13:67:13 | i | i flows from $@ | simple.cpp:65:11:65:20 | call to user_input | call to user_input |
8189
| struct_init.c:22:11:22:11 | a | struct_init.c:20:20:20:29 | call to user_input | struct_init.c:22:11:22:11 | a | a flows from $@ | struct_init.c:20:20:20:29 | call to user_input | call to user_input |
8290
| struct_init.c:31:23:31:23 | a | struct_init.c:27:7:27:16 | call to user_input | struct_init.c:31:23:31:23 | a | a flows from $@ | struct_init.c:27:7:27:16 | call to user_input | call to user_input |

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,4 +53,18 @@ void foo()
5353
// Nothing should alert
5454
bar(i);
5555
}
56+
57+
struct A
58+
{
59+
int i;
60+
};
61+
62+
void single_field_test()
63+
{
64+
A a;
65+
a.i = user_input();
66+
A a2 = a;
67+
sink(a2.i);
68+
}
69+
5670
} // namespace Simple

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -788,6 +788,10 @@ unreachableNodeCCtx
788788
localCallNodes
789789
postIsNotPre
790790
postHasUniquePre
791+
| assignexpr.cpp:9:2:9:12 | Store | PostUpdateNode should have one pre-update node but has 0. |
792+
| bad_asts.cpp:15:10:15:12 | Store | PostUpdateNode should have one pre-update node but has 0. |
793+
| file://:0:0:0:0 | Store | PostUpdateNode should have one pre-update node but has 0. |
794+
| ir.cpp:531:14:531:14 | Store | PostUpdateNode should have one pre-update node but has 0. |
791795
uniquePostUpdate
792796
postIsInSameCallable
793797
reverseRead

0 commit comments

Comments
 (0)