Skip to content

Commit fafc0b5

Browse files
jketemajketema
authored
Merge pull request #10995 from MathiasVP/fix-as-expr
C++: Fix `asExpr` and `asIndirectExpr` in IR dataflow
2 parents b7e42e8 + 1722614 commit fafc0b5

24 files changed

Lines changed: 27618 additions & 27387 deletions

File tree

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

Lines changed: 114 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -142,11 +142,7 @@ class Node extends TIRDataFlowNode {
142142
* Gets the non-conversion expression that's indirectly tracked by this node
143143
* under `index` number of indirections.
144144
*/
145-
Expr asIndirectExpr(int index) {
146-
exists(Operand operand | hasOperandAndIndex(this, operand, index) |
147-
result = operand.getDef().getUnconvertedResultExpression()
148-
)
149-
}
145+
Expr asIndirectExpr(int index) { result = this.(IndirectExprNode).getExpr(index) }
150146

151147
/**
152148
* Gets the non-conversion expression that's indirectly tracked by this node
@@ -165,9 +161,7 @@ class Node extends TIRDataFlowNode {
165161
* behind `index` number of indirections.
166162
*/
167163
Expr asIndirectConvertedExpr(int index) {
168-
exists(Operand operand | hasOperandAndIndex(this, operand, index) |
169-
result = operand.getDef().getConvertedResultExpression()
170-
)
164+
result = this.(IndirectExprNode).getConvertedExpr(index)
171165
}
172166

173167
/**
@@ -309,14 +303,25 @@ class Node extends TIRDataFlowNode {
309303

310304
/** Gets a textual representation of this element. */
311305
cached
312-
final string toString() { result = this.toStringImpl() }
306+
final string toString() {
307+
result = toExprString(this)
308+
or
309+
not exists(toExprString(this)) and
310+
result = this.toStringImpl()
311+
}
313312

314313
/** INTERNAL: Do not use. */
315314
string toStringImpl() {
316315
none() // overridden by subclasses
317316
}
318317
}
319318

319+
private string toExprString(Node n) {
320+
result = n.asExpr().toString()
321+
or
322+
result = n.asIndirectExpr().toString() + " indirection"
323+
}
324+
320325
/**
321326
* An instruction, viewed as a node in a data flow graph.
322327
*/
@@ -738,6 +743,19 @@ private predicate exprNodeShouldBeIndirectOperand(IndirectOperand node, Expr e,
738743
not convertedExprMustBeOperand(e)
739744
}
740745

746+
/** Holds if `node` should be an `IndirectOperand` that maps `node.asIndirectExpr()` to `e`. */
747+
private predicate indirectExprNodeShouldBeIndirectOperand(IndirectOperand node, Expr e) {
748+
exists(Instruction instr |
749+
instr = node.getOperand().getDef() and
750+
not node instanceof ExprNode
751+
|
752+
e = instr.(VariableAddressInstruction).getAst().(Expr).getFullyConverted()
753+
or
754+
not instr instanceof VariableAddressInstruction and
755+
e = instr.getConvertedResultExpression()
756+
)
757+
}
758+
741759
private predicate exprNodeShouldBeIndirectOutNode(IndirectArgumentOutNode node, Expr e) {
742760
exists(CallInstruction call |
743761
call.getStaticCallTarget() instanceof Constructor and
@@ -754,18 +772,35 @@ predicate exprNodeShouldBeInstruction(Node node, Expr e) {
754772
not exprNodeShouldBeIndirectOutNode(_, e)
755773
}
756774

757-
private class ExprNodeBase extends Node {
758-
Expr getConvertedExpr() { none() } // overridden by subclasses
775+
/** Holds if `node` should be an `IndirectInstruction` that maps `node.asIndirectExpr()` to `e`. */
776+
predicate indirectExprNodeShouldBeIndirectInstruction(IndirectInstruction node, Expr e) {
777+
exists(Instruction instr |
778+
instr = node.getInstruction() and not indirectExprNodeShouldBeIndirectOperand(_, e)
779+
|
780+
e = instr.(VariableAddressInstruction).getAst().(Expr).getFullyConverted()
781+
or
782+
not instr instanceof VariableAddressInstruction and
783+
e = instr.getConvertedResultExpression()
784+
)
785+
}
786+
787+
abstract private class ExprNodeBase extends Node {
788+
/**
789+
* Gets the expression corresponding to this node, if any. The returned
790+
* expression may be a `Conversion`.
791+
*/
792+
abstract Expr getConvertedExpr();
759793

760-
Expr getExpr() { none() } // overridden by subclasses
794+
/** Gets the non-conversion expression corresponding to this node, if any. */
795+
abstract Expr getExpr();
761796
}
762797

763798
private class InstructionExprNode extends ExprNodeBase, InstructionNode {
764799
InstructionExprNode() { exprNodeShouldBeInstruction(this, _) }
765800

766801
final override Expr getConvertedExpr() { exprNodeShouldBeInstruction(this, result) }
767802

768-
final override Expr getExpr() { result = this.getInstruction().getUnconvertedResultExpression() }
803+
final override Expr getExpr() { result = this.getConvertedExpr().getUnconverted() }
769804

770805
final override string toStringImpl() { result = this.getConvertedExpr().toString() }
771806
}
@@ -775,9 +810,7 @@ private class OperandExprNode extends ExprNodeBase, OperandNode {
775810

776811
final override Expr getConvertedExpr() { exprNodeShouldBeOperand(this, result) }
777812

778-
final override Expr getExpr() {
779-
result = this.getOperand().getDef().getUnconvertedResultExpression()
780-
}
813+
final override Expr getExpr() { result = this.getConvertedExpr().getUnconverted() }
781814

782815
final override string toStringImpl() {
783816
// Avoid generating multiple `toString` results for `ArgumentNode`s
@@ -790,17 +823,58 @@ private class OperandExprNode extends ExprNodeBase, OperandNode {
790823
}
791824

792825
private class IndirectOperandExprNode extends ExprNodeBase, IndirectOperand {
793-
LoadInstruction load;
826+
IndirectOperandExprNode() { exprNodeShouldBeIndirectOperand(this, _, _) }
794827

795-
IndirectOperandExprNode() { exprNodeShouldBeIndirectOperand(this, _, load) }
828+
final override Expr getConvertedExpr() { exprNodeShouldBeIndirectOperand(this, result, _) }
796829

797-
final override Expr getConvertedExpr() { exprNodeShouldBeIndirectOperand(this, result, load) }
798-
799-
final override Expr getExpr() { result = load.getUnconvertedResultExpression() }
830+
final override Expr getExpr() { result = this.getConvertedExpr().getUnconverted() }
800831

801832
final override string toStringImpl() { result = this.getConvertedExpr().toString() }
802833
}
803834

835+
abstract private class IndirectExprNodeBase extends Node {
836+
/**
837+
* Gets the expression corresponding to this node, if any. The returned
838+
* expression may be a `Conversion`.
839+
*/
840+
abstract Expr getConvertedExpr(int indirectionIndex);
841+
842+
/** Gets the non-conversion expression corresponding to this node, if any. */
843+
abstract Expr getExpr(int indirectionIndex);
844+
}
845+
846+
private class IndirectOperandIndirectExprNode extends IndirectExprNodeBase, IndirectOperand {
847+
IndirectOperandIndirectExprNode() { indirectExprNodeShouldBeIndirectOperand(this, _) }
848+
849+
final override Expr getConvertedExpr(int index) {
850+
this.getIndirectionIndex() = index and
851+
indirectExprNodeShouldBeIndirectOperand(this, result)
852+
}
853+
854+
final override Expr getExpr(int index) {
855+
this.getIndirectionIndex() = index and
856+
result = this.getConvertedExpr(index).getUnconverted()
857+
}
858+
859+
final override string toStringImpl() { result = super.toStringImpl() }
860+
}
861+
862+
private class IndirectInstructionIndirectExprNode extends IndirectExprNodeBase, IndirectInstruction {
863+
IndirectInstructionIndirectExprNode() { indirectExprNodeShouldBeIndirectInstruction(this, _) }
864+
865+
final override Expr getConvertedExpr(int index) {
866+
this.getIndirectionIndex() = index and
867+
indirectExprNodeShouldBeIndirectInstruction(this, result)
868+
}
869+
870+
final override Expr getExpr(int index) {
871+
this.getIndirectionIndex() = index and
872+
result = this.getConvertedExpr(index).getUnconverted()
873+
}
874+
875+
final override string toStringImpl() { result = super.toStringImpl() }
876+
}
877+
804878
private class IndirectArgumentOutExprNode extends ExprNodeBase, IndirectArgumentOutNode {
805879
IndirectArgumentOutExprNode() { exprNodeShouldBeIndirectOutNode(this, _) }
806880

@@ -830,6 +904,25 @@ class ExprNode extends Node instanceof ExprNodeBase {
830904
Expr getConvertedExpr() { result = super.getConvertedExpr() }
831905
}
832906

907+
/**
908+
* An indirect expression, viewed as a node in a data flow graph.
909+
*/
910+
class IndirectExprNode extends Node instanceof IndirectExprNodeBase {
911+
/**
912+
* Gets the non-conversion expression corresponding to this node, if any. If
913+
* this node strictly (in the sense of `getConvertedExpr`) corresponds to a
914+
* `Conversion`, then the result is that `Conversion`'s non-`Conversion` base
915+
* expression.
916+
*/
917+
Expr getExpr(int indirectionIndex) { result = super.getExpr(indirectionIndex) }
918+
919+
/**
920+
* Gets the expression corresponding to this node, if any. The returned
921+
* expression may be a `Conversion`.
922+
*/
923+
Expr getConvertedExpr(int indirectionIndex) { result = super.getConvertedExpr(indirectionIndex) }
924+
}
925+
833926
/**
834927
* The value of a parameter at function entry, viewed as a node in a data
835928
* flow graph. This includes both explicit parameters such as `x` in `f(x)`

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/AllocMultiplicationOverflow.expected‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
11
edges
22
| test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... |
3-
| test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | (unsigned long)... |
3+
| test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... |
44
| test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... |
5-
| test.cpp:22:17:22:21 | (size_t)... | test.cpp:23:33:23:37 | size1 |
5+
| test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 |
66
| test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 |
77
nodes
88
| test.cpp:13:33:13:37 | ... * ... | semmle.label | ... * ... |
99
| test.cpp:13:33:13:37 | ... * ... | semmle.label | ... * ... |
1010
| test.cpp:13:33:13:37 | ... * ... | semmle.label | ... * ... |
11-
| test.cpp:15:31:15:35 | (unsigned long)... | semmle.label | (unsigned long)... |
11+
| test.cpp:15:31:15:35 | ... * ... | semmle.label | ... * ... |
1212
| test.cpp:15:31:15:35 | ... * ... | semmle.label | ... * ... |
1313
| test.cpp:15:31:15:35 | ... * ... | semmle.label | ... * ... |
1414
| test.cpp:19:34:19:38 | ... * ... | semmle.label | ... * ... |
1515
| test.cpp:19:34:19:38 | ... * ... | semmle.label | ... * ... |
1616
| test.cpp:19:34:19:38 | ... * ... | semmle.label | ... * ... |
17-
| test.cpp:22:17:22:21 | (size_t)... | semmle.label | (size_t)... |
17+
| test.cpp:22:17:22:21 | ... * ... | semmle.label | ... * ... |
1818
| test.cpp:22:17:22:21 | ... * ... | semmle.label | ... * ... |
1919
| test.cpp:23:33:23:37 | size1 | semmle.label | size1 |
2020
| test.cpp:30:27:30:31 | ... * ... | semmle.label | ... * ... |
@@ -24,13 +24,13 @@ subpaths
2424
| test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:13:33:13:37 | ... * ... | multiplication |
2525
| test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:13:33:13:37 | ... * ... | multiplication |
2626
| test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:13:33:13:37 | ... * ... | multiplication |
27-
| test.cpp:15:31:15:35 | (unsigned long)... | test.cpp:15:31:15:35 | (unsigned long)... | test.cpp:15:31:15:35 | (unsigned long)... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:15:31:15:35 | (unsigned long)... | multiplication |
28-
| test.cpp:15:31:15:35 | (unsigned long)... | test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | (unsigned long)... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:15:31:15:35 | ... * ... | multiplication |
27+
| test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:15:31:15:35 | ... * ... | multiplication |
28+
| test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:15:31:15:35 | ... * ... | multiplication |
2929
| test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:15:31:15:35 | ... * ... | multiplication |
3030
| test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:19:34:19:38 | ... * ... | multiplication |
3131
| test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:19:34:19:38 | ... * ... | multiplication |
3232
| test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:19:34:19:38 | ... * ... | multiplication |
33-
| test.cpp:23:33:23:37 | size1 | test.cpp:22:17:22:21 | (size_t)... | test.cpp:23:33:23:37 | size1 | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:22:17:22:21 | (size_t)... | multiplication |
33+
| test.cpp:23:33:23:37 | size1 | test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:22:17:22:21 | ... * ... | multiplication |
3434
| test.cpp:23:33:23:37 | size1 | test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:22:17:22:21 | ... * ... | multiplication |
3535
| test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:30:27:30:31 | ... * ... | multiplication |
3636
| test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:31:27:31:31 | ... * ... | multiplication |

0 commit comments

Comments
 (0)