Skip to content

Commit 30c67ba

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#15040 from MathiasVP/fewer-dataflow-branches
C++: Fix dataflow inconsistencies
2 parents 1dc0a06 + 90a62b2 commit 30c67ba

File tree

11 files changed

+518
-598
lines changed

11 files changed

+518
-598
lines changed

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

Lines changed: 44 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -18,17 +18,17 @@ private import codeql.util.Unit
1818

1919
/**
2020
* The IR dataflow graph consists of the following nodes:
21-
* - `Node0`, which injects most instructions and operands directly into the dataflow graph.
21+
* - `Node0`, which injects most instructions and operands directly into the
22+
* dataflow graph.
2223
* - `VariableNode`, which is used to model flow through global variables.
23-
* - `PostFieldUpdateNode`, which is used to model the state of a field after a value has been stored
24-
* into an address after a number of loads.
25-
* - `SsaPhiNode`, which represents phi nodes as computed by the shared SSA library.
26-
* - `IndirectArgumentOutNode`, which represents the value of an argument (and its indirections) after
27-
* it leaves a function call.
28-
* - `RawIndirectOperand`, which represents the value of `operand` after loading the address a number
29-
* of times.
30-
* - `RawIndirectInstruction`, which represents the value of `instr` after loading the address a number
31-
* of times.
24+
* - `PostUpdateNodeImpl`, which is used to model the state of an object after
25+
* an update after a number of loads.
26+
* - `SsaPhiNode`, which represents phi nodes as computed by the shared SSA
27+
* library.
28+
* - `RawIndirectOperand`, which represents the value of `operand` after
29+
* loading the address a number of times.
30+
* - `RawIndirectInstruction`, which represents the value of `instr` after
31+
* loading the address a number of times.
3232
*/
3333
cached
3434
private newtype TIRDataFlowNode =
@@ -37,14 +37,13 @@ private newtype TIRDataFlowNode =
3737
indirectionIndex =
3838
[getMinIndirectionsForType(var.getUnspecifiedType()) .. Ssa::getMaxIndirectionsForType(var.getUnspecifiedType())]
3939
} or
40-
TPostFieldUpdateNode(FieldAddress operand, int indirectionIndex) {
41-
indirectionIndex =
42-
[1 .. Ssa::countIndirectionsForCppType(operand.getObjectAddress().getResultLanguageType())]
43-
} or
44-
TSsaPhiNode(Ssa::PhiNode phi) or
45-
TIndirectArgumentOutNode(ArgumentOperand operand, int indirectionIndex) {
40+
TPostUpdateNodeImpl(Operand operand, int indirectionIndex) {
41+
operand = any(FieldAddress fa).getObjectAddressOperand() and
42+
indirectionIndex = [1 .. Ssa::countIndirectionsForCppType(Ssa::getLanguageType(operand))]
43+
or
4644
Ssa::isModifiableByCall(operand, indirectionIndex)
4745
} or
46+
TSsaPhiNode(Ssa::PhiNode phi) or
4847
TRawIndirectOperand0(Node0Impl node, int indirectionIndex) {
4948
Ssa::hasRawIndirectOperand(node.asOperand(), indirectionIndex)
5049
} or
@@ -84,7 +83,7 @@ private predicate parameterIsRedefined(Parameter p) {
8483
class FieldAddress extends Operand {
8584
FieldAddressInstruction fai;
8685

87-
FieldAddress() { fai = this.getDef() }
86+
FieldAddress() { fai = this.getDef() and not Ssa::ignoreOperand(this) }
8887

8988
/** Gets the field associated with this instruction. */
9089
Field getField() { result = fai.getField() }
@@ -550,37 +549,44 @@ Type stripPointer(Type t) {
550549
result = t.(FunctionPointerIshType).getBaseType()
551550
}
552551

553-
/**
554-
* INTERNAL: do not use.
555-
*
556-
* The node representing the value of a field after it has been updated.
557-
*/
558-
class PostFieldUpdateNode extends TPostFieldUpdateNode, PartialDefinitionNode {
552+
private class PostUpdateNodeImpl extends PartialDefinitionNode, TPostUpdateNodeImpl {
559553
int indirectionIndex;
560-
FieldAddress fieldAddress;
554+
Operand operand;
561555

562-
PostFieldUpdateNode() { this = TPostFieldUpdateNode(fieldAddress, indirectionIndex) }
556+
PostUpdateNodeImpl() { this = TPostUpdateNodeImpl(operand, indirectionIndex) }
563557

564-
override Declaration getFunction() { result = fieldAddress.getUse().getEnclosingFunction() }
558+
override Declaration getFunction() { result = operand.getUse().getEnclosingFunction() }
565559

566560
override Declaration getEnclosingCallable() { result = this.getFunction() }
567561

568-
FieldAddress getFieldAddress() { result = fieldAddress }
569-
570-
Field getUpdatedField() { result = fieldAddress.getField() }
562+
/** Gets the operand associated with this node. */
563+
Operand getOperand() { result = operand }
571564

565+
/** Gets the indirection index associated with this node. */
572566
int getIndirectionIndex() { result = indirectionIndex }
573567

574-
override Node getPreUpdateNode() {
575-
hasOperandAndIndex(result, pragma[only_bind_into](fieldAddress).getObjectAddressOperand(),
576-
indirectionIndex)
577-
}
568+
override Location getLocationImpl() { result = operand.getLocation() }
578569

579-
override Expr getDefinedExpr() {
580-
result = fieldAddress.getObjectAddress().getUnconvertedResultExpression()
570+
final override Node getPreUpdateNode() { hasOperandAndIndex(result, operand, indirectionIndex) }
571+
572+
final override Expr getDefinedExpr() {
573+
result = operand.getDef().getUnconvertedResultExpression()
581574
}
575+
}
576+
577+
/**
578+
* INTERNAL: do not use.
579+
*
580+
* The node representing the value of a field after it has been updated.
581+
*/
582+
class PostFieldUpdateNode extends PostUpdateNodeImpl {
583+
FieldAddress fieldAddress;
582584

583-
override Location getLocationImpl() { result = fieldAddress.getLocation() }
585+
PostFieldUpdateNode() { operand = fieldAddress.getObjectAddressOperand() }
586+
587+
FieldAddress getFieldAddress() { result = fieldAddress }
588+
589+
Field getUpdatedField() { result = this.getFieldAddress().getField() }
584590

585591
override string toStringImpl() { result = this.getPreUpdateNode() + " [post update]" }
586592
}
@@ -816,13 +822,8 @@ class IndirectReturnNode extends Node {
816822
* A node representing the indirection of a value after it
817823
* has been returned from a function.
818824
*/
819-
class IndirectArgumentOutNode extends Node, TIndirectArgumentOutNode, PartialDefinitionNode {
820-
ArgumentOperand operand;
821-
int indirectionIndex;
822-
823-
IndirectArgumentOutNode() { this = TIndirectArgumentOutNode(operand, indirectionIndex) }
824-
825-
int getIndirectionIndex() { result = indirectionIndex }
825+
class IndirectArgumentOutNode extends PostUpdateNodeImpl {
826+
override ArgumentOperand operand;
826827

827828
int getArgumentIndex() {
828829
exists(CallInstruction call | call.getArgumentOperand(result) = operand)
@@ -834,12 +835,6 @@ class IndirectArgumentOutNode extends Node, TIndirectArgumentOutNode, PartialDef
834835

835836
Function getStaticCallTarget() { result = this.getCallInstruction().getStaticCallTarget() }
836837

837-
override Declaration getEnclosingCallable() { result = this.getFunction() }
838-
839-
override Declaration getFunction() { result = this.getCallInstruction().getEnclosingFunction() }
840-
841-
override Node getPreUpdateNode() { hasOperandAndIndex(result, operand, indirectionIndex) }
842-
843838
override string toStringImpl() {
844839
// This string should be unique enough to be helpful but common enough to
845840
// avoid storing too many different strings.
@@ -848,10 +843,6 @@ class IndirectArgumentOutNode extends Node, TIndirectArgumentOutNode, PartialDef
848843
not exists(this.getStaticCallTarget()) and
849844
result = "output argument"
850845
}
851-
852-
override Location getLocationImpl() { result = operand.getLocation() }
853-
854-
override Expr getDefinedExpr() { result = operand.getDef().getUnconvertedResultExpression() }
855846
}
856847

857848
/**

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/array-access/ArrayAccessProductFlow.expected

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ edges
33
| test.cpp:4:17:4:22 | call to malloc | test.cpp:10:9:10:11 | arr |
44
| test.cpp:19:9:19:16 | mk_array indirection [p] | test.cpp:28:19:28:26 | call to mk_array [p] |
55
| test.cpp:19:9:19:16 | mk_array indirection [p] | test.cpp:50:18:50:25 | call to mk_array [p] |
6-
| test.cpp:21:5:21:24 | ... = ... | test.cpp:21:9:21:9 | arr indirection [post update] [p] |
7-
| test.cpp:21:9:21:9 | arr indirection [post update] [p] | test.cpp:22:5:22:7 | arr indirection [p] |
6+
| test.cpp:21:5:21:7 | arr indirection [post update] [p] | test.cpp:22:5:22:7 | arr indirection [p] |
7+
| test.cpp:21:5:21:24 | ... = ... | test.cpp:21:5:21:7 | arr indirection [post update] [p] |
88
| test.cpp:21:13:21:18 | call to malloc | test.cpp:21:5:21:24 | ... = ... |
99
| test.cpp:22:5:22:7 | arr indirection [p] | test.cpp:19:9:19:16 | mk_array indirection [p] |
1010
| test.cpp:28:19:28:26 | call to mk_array [p] | test.cpp:31:9:31:11 | arr indirection [p] |
@@ -16,17 +16,17 @@ edges
1616
| test.cpp:41:9:41:11 | arr indirection [p] | test.cpp:41:13:41:13 | p |
1717
| test.cpp:45:9:45:11 | arr indirection [p] | test.cpp:45:13:45:13 | p |
1818
| test.cpp:50:18:50:25 | call to mk_array [p] | test.cpp:39:27:39:29 | arr [p] |
19-
| test.cpp:55:5:55:24 | ... = ... | test.cpp:55:9:55:9 | arr indirection [post update] [p] |
20-
| test.cpp:55:9:55:9 | arr indirection [post update] [p] | test.cpp:56:5:56:7 | arr indirection [p] |
19+
| test.cpp:55:5:55:7 | arr indirection [post update] [p] | test.cpp:56:5:56:7 | arr indirection [p] |
20+
| test.cpp:55:5:55:24 | ... = ... | test.cpp:55:5:55:7 | arr indirection [post update] [p] |
2121
| test.cpp:55:13:55:18 | call to malloc | test.cpp:55:5:55:24 | ... = ... |
2222
| test.cpp:56:5:56:7 | arr indirection [p] | test.cpp:59:9:59:11 | arr indirection [p] |
2323
| test.cpp:56:5:56:7 | arr indirection [p] | test.cpp:63:9:63:11 | arr indirection [p] |
2424
| test.cpp:59:9:59:11 | arr indirection [p] | test.cpp:59:13:59:13 | p |
2525
| test.cpp:63:9:63:11 | arr indirection [p] | test.cpp:63:13:63:13 | p |
2626
| test.cpp:67:10:67:19 | mk_array_p indirection [p] | test.cpp:76:20:76:29 | call to mk_array_p indirection [p] |
2727
| test.cpp:67:10:67:19 | mk_array_p indirection [p] | test.cpp:98:18:98:27 | call to mk_array_p indirection [p] |
28-
| test.cpp:69:5:69:25 | ... = ... | test.cpp:69:10:69:10 | arr indirection [post update] [p] |
29-
| test.cpp:69:10:69:10 | arr indirection [post update] [p] | test.cpp:70:5:70:7 | arr indirection [p] |
28+
| test.cpp:69:5:69:7 | arr indirection [post update] [p] | test.cpp:70:5:70:7 | arr indirection [p] |
29+
| test.cpp:69:5:69:25 | ... = ... | test.cpp:69:5:69:7 | arr indirection [post update] [p] |
3030
| test.cpp:69:14:69:19 | call to malloc | test.cpp:69:5:69:25 | ... = ... |
3131
| test.cpp:70:5:70:7 | arr indirection [p] | test.cpp:67:10:67:19 | mk_array_p indirection [p] |
3232
| test.cpp:76:20:76:29 | call to mk_array_p indirection [p] | test.cpp:79:9:79:11 | arr indirection [p] |
@@ -43,8 +43,8 @@ nodes
4343
| test.cpp:6:9:6:11 | arr | semmle.label | arr |
4444
| test.cpp:10:9:10:11 | arr | semmle.label | arr |
4545
| test.cpp:19:9:19:16 | mk_array indirection [p] | semmle.label | mk_array indirection [p] |
46+
| test.cpp:21:5:21:7 | arr indirection [post update] [p] | semmle.label | arr indirection [post update] [p] |
4647
| test.cpp:21:5:21:24 | ... = ... | semmle.label | ... = ... |
47-
| test.cpp:21:9:21:9 | arr indirection [post update] [p] | semmle.label | arr indirection [post update] [p] |
4848
| test.cpp:21:13:21:18 | call to malloc | semmle.label | call to malloc |
4949
| test.cpp:22:5:22:7 | arr indirection [p] | semmle.label | arr indirection [p] |
5050
| test.cpp:28:19:28:26 | call to mk_array [p] | semmle.label | call to mk_array [p] |
@@ -58,17 +58,17 @@ nodes
5858
| test.cpp:45:9:45:11 | arr indirection [p] | semmle.label | arr indirection [p] |
5959
| test.cpp:45:13:45:13 | p | semmle.label | p |
6060
| test.cpp:50:18:50:25 | call to mk_array [p] | semmle.label | call to mk_array [p] |
61+
| test.cpp:55:5:55:7 | arr indirection [post update] [p] | semmle.label | arr indirection [post update] [p] |
6162
| test.cpp:55:5:55:24 | ... = ... | semmle.label | ... = ... |
62-
| test.cpp:55:9:55:9 | arr indirection [post update] [p] | semmle.label | arr indirection [post update] [p] |
6363
| test.cpp:55:13:55:18 | call to malloc | semmle.label | call to malloc |
6464
| test.cpp:56:5:56:7 | arr indirection [p] | semmle.label | arr indirection [p] |
6565
| test.cpp:59:9:59:11 | arr indirection [p] | semmle.label | arr indirection [p] |
6666
| test.cpp:59:13:59:13 | p | semmle.label | p |
6767
| test.cpp:63:9:63:11 | arr indirection [p] | semmle.label | arr indirection [p] |
6868
| test.cpp:63:13:63:13 | p | semmle.label | p |
6969
| test.cpp:67:10:67:19 | mk_array_p indirection [p] | semmle.label | mk_array_p indirection [p] |
70+
| test.cpp:69:5:69:7 | arr indirection [post update] [p] | semmle.label | arr indirection [post update] [p] |
7071
| test.cpp:69:5:69:25 | ... = ... | semmle.label | ... = ... |
71-
| test.cpp:69:10:69:10 | arr indirection [post update] [p] | semmle.label | arr indirection [post update] [p] |
7272
| test.cpp:69:14:69:19 | call to malloc | semmle.label | call to malloc |
7373
| test.cpp:70:5:70:7 | arr indirection [p] | semmle.label | arr indirection [p] |
7474
| test.cpp:76:20:76:29 | call to mk_array_p indirection [p] | semmle.label | call to mk_array_p indirection [p] |

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ postIsNotPre
1515
| flowOut.cpp:84:3:84:14 | access to array indirection | PostUpdateNode should not equal its pre-update node. |
1616
postHasUniquePre
1717
uniquePostUpdate
18-
| example.c:24:13:24:18 | coords indirection | Node has multiple PostUpdateNodes. |
1918
postIsInSameCallable
2019
reverseRead
2120
argHasPostUpdate

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

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -14,31 +14,6 @@ localCallNodes
1414
postIsNotPre
1515
postHasUniquePre
1616
uniquePostUpdate
17-
| aliasing.cpp:70:11:70:11 | definition of w indirection | Node has multiple PostUpdateNodes. |
18-
| aliasing.cpp:77:11:77:11 | definition of w indirection | Node has multiple PostUpdateNodes. |
19-
| aliasing.cpp:84:11:84:11 | definition of w indirection | Node has multiple PostUpdateNodes. |
20-
| aliasing.cpp:91:11:91:11 | definition of w indirection | Node has multiple PostUpdateNodes. |
21-
| clearning.cpp:54:3:54:3 | s indirection | Node has multiple PostUpdateNodes. |
22-
| clearning.cpp:61:3:61:3 | s indirection | Node has multiple PostUpdateNodes. |
23-
| clearning.cpp:90:3:90:3 | s indirection | Node has multiple PostUpdateNodes. |
24-
| clearning.cpp:104:2:104:2 | s indirection | Node has multiple PostUpdateNodes. |
25-
| clearning.cpp:111:4:111:4 | s indirection | Node has multiple PostUpdateNodes. |
26-
| clearning.cpp:118:2:118:2 | s indirection | Node has multiple PostUpdateNodes. |
27-
| clearning.cpp:125:2:125:2 | s indirection | Node has multiple PostUpdateNodes. |
28-
| clearning.cpp:132:2:132:2 | s indirection | Node has multiple PostUpdateNodes. |
29-
| clearning.cpp:139:4:139:4 | s indirection | Node has multiple PostUpdateNodes. |
30-
| clearning.cpp:165:3:165:3 | s indirection | Node has multiple PostUpdateNodes. |
31-
| clearning.cpp:172:3:172:3 | s indirection | Node has multiple PostUpdateNodes. |
32-
| complex.cpp:22:3:22:5 | this indirection | Node has multiple PostUpdateNodes. |
33-
| complex.cpp:25:7:25:7 | this indirection | Node has multiple PostUpdateNodes. |
34-
| complex.cpp:42:10:42:14 | inner indirection | Node has multiple PostUpdateNodes. |
35-
| complex.cpp:43:10:43:14 | inner indirection | Node has multiple PostUpdateNodes. |
36-
| complex.cpp:53:6:53:10 | inner indirection | Node has multiple PostUpdateNodes. |
37-
| complex.cpp:54:6:54:10 | inner indirection | Node has multiple PostUpdateNodes. |
38-
| complex.cpp:55:6:55:10 | inner indirection | Node has multiple PostUpdateNodes. |
39-
| complex.cpp:56:6:56:10 | inner indirection | Node has multiple PostUpdateNodes. |
40-
| struct_init.c:26:16:26:20 | definition of outer indirection | Node has multiple PostUpdateNodes. |
41-
| struct_init.c:41:16:41:20 | definition of outer indirection | Node has multiple PostUpdateNodes. |
4217
postIsInSameCallable
4318
reverseRead
4419
argHasPostUpdate

0 commit comments

Comments
 (0)