From 502b7cfe330bebb7ec29ef3db2a4e653def7866c Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 16 Jan 2019 09:47:58 +0100 Subject: [PATCH 1/3] C++: Don't use C-style varargs in test.cpp `sink` As we prepare to clarify how conversions are treated, we don't want a `sink(...)` declaration where it's non-obvious which conversions are applied to arguments. --- cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp index 5cd5f9e3989f..88271adf40e9 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -1,5 +1,5 @@ int source(); -void sink(...); +void sink(int); void sink(const int *); void sink(int **); void intraprocedural_with_local_flow() { int t2; From dcb24e07c3ce0a2a957819a6604d239d409b00d0 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 16 Jan 2019 13:56:52 +0100 Subject: [PATCH 2/3] C++: Remove getFullyConverted call in sink def With this change, the `IRDataflowTestCommon.qll` and `DataflowTestCommon.qll` files use the same definitions of sources and sinks. Since the IR data flow library is meant to be compatible with the AST data flow library, this is what we ought to be testing. Two alerts change but not necessarily for the right reasons. --- .../dataflow/dataflow-tests/IRDataflowTestCommon.qll | 2 +- .../library-tests/dataflow/dataflow-tests/test_diff.expected | 2 +- .../test/library-tests/dataflow/dataflow-tests/test_ir.expected | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/IRDataflowTestCommon.qll b/cpp/ql/test/library-tests/dataflow/dataflow-tests/IRDataflowTestCommon.qll index ec972d40039c..ce2eb1ac97ad 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/IRDataflowTestCommon.qll +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/IRDataflowTestCommon.qll @@ -19,7 +19,7 @@ class TestAllocationConfig extends DataFlow::Configuration { override predicate isSink(DataFlow::Node sink) { exists(FunctionCall call | call.getTarget().getName() = "sink" and - sink.asExpr() = call.getAnArgument().getFullyConverted() + sink.asExpr() = call.getAnArgument() ) } diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected index c220d3f724b8..6cd0d7a94041 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected @@ -1,5 +1,5 @@ | test.cpp:66:30:66:36 | test.cpp:71:8:71:9 | AST only | -| test.cpp:89:28:89:34 | test.cpp:90:8:90:14 | AST only | +| test.cpp:89:28:89:34 | test.cpp:92:8:92:14 | IR only | | test.cpp:100:13:100:18 | test.cpp:103:10:103:12 | AST only | | test.cpp:120:9:120:20 | test.cpp:126:8:126:19 | AST only | | test.cpp:122:18:122:30 | test.cpp:132:22:132:23 | IR only | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected index 8243f789c9e0..db429c84c73f 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_ir.expected @@ -9,6 +9,8 @@ | test.cpp:76:8:76:9 | Load: u1 | test.cpp:75:7:75:8 | Uninitialized: definition of u1 | | test.cpp:84:8:84:18 | Load: ... ? ... : ... | test.cpp:83:7:83:8 | Uninitialized: definition of u2 | | test.cpp:86:8:86:9 | Load: i1 | test.cpp:83:7:83:8 | Uninitialized: definition of u2 | +| test.cpp:90:8:90:14 | Load: source1 | test.cpp:89:28:89:34 | InitializeParameter: source1 | +| test.cpp:92:8:92:14 | Load: source1 | test.cpp:89:28:89:34 | InitializeParameter: source1 | | test.cpp:132:22:132:23 | Load: m1 | test.cpp:122:18:122:30 | InitializeParameter: sourceStruct1 | | test.cpp:140:22:140:23 | Load: m1 | test.cpp:122:18:122:30 | InitializeParameter: sourceStruct1 | | test.cpp:188:8:188:8 | Load: y | test.cpp:186:27:186:32 | Call: call to source | From 22b15037fc12d74085b0f96e4085d034d0542016 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 16 Jan 2019 14:17:57 +0100 Subject: [PATCH 3/3] C++: Split DataFlow::Node.asExpr into two The existing `Node.asExpr` predicate changes semantics so it becomes the one that most users should use when they don't want to think about `Conversion`s. A new `Node.asConvertedExpr` predicate is added and has the same semantics as the old `Node.asExpr` predicate. It's for advanced users that know about `Conversion`s and want to account for them. --- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 44 ++++++++++++++++--- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index cd9b874781bd..1568d5fbc079 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -24,8 +24,22 @@ class Node extends Instruction { result = this.getResultType() } - /** Gets the expression corresponding to this node, if any. */ - Expr asExpr() { result = this.getConvertedResultExpression() } + /** + * Gets the non-conversion expression corresponding to this node, if any. If + * this node strictly (in the sense of `asConvertedExpr`) corresponds to a + * `Conversion`, then the result is that `Conversion`'s non-`Conversion` base + * expression. + */ + Expr asExpr() { + result.getConversion*() = this.getConvertedResultExpression() and + not result instanceof Conversion + } + + /** + * Gets the expression corresponding to this node, if any. The returned + * expression may be a `Conversion`. + */ + Expr asConvertedExpr() { result = this.getConvertedResultExpression() } /** Gets the parameter corresponding to this node, if any. */ Parameter asParameter() { result = this.(InitializeParameterInstruction).getParameter() } @@ -48,10 +62,21 @@ class Node extends Instruction { * An expression, viewed as a node in a data flow graph. */ class ExprNode extends Node { - Expr expr; + ExprNode() { exists(this.asExpr()) } + + /** + * Gets the non-conversion expression corresponding to this node, if any. If + * this node strictly (in the sense of `getConvertedExpr`) corresponds to a + * `Conversion`, then the result is that `Conversion`'s non-`Conversion` base + * expression. + */ + Expr getExpr() { result = this.asExpr() } - ExprNode() { expr = this.asExpr() } - Expr getExpr() { result = expr } + /** + * Gets the expression corresponding to this node, if any. The returned + * expression may be a `Conversion`. + */ + Expr getConvertedExpr() { result = this.asConvertedExpr() } } /** @@ -98,10 +123,17 @@ abstract class PostUpdateNode extends Node { } /** - * Gets the `Node` corresponding to `e`. + * Gets a `Node` corresponding to `e` or any of its conversions. There is no + * result if `e` is a `Conversion`. */ ExprNode exprNode(Expr e) { result.getExpr() = e } +/** + * Gets the `Node` corresponding to `e`, if any. Here, `e` may be a + * `Conversion`. + */ +ExprNode convertedExprNode(Expr e) { result.getExpr() = e } + /** * Gets the `Node` corresponding to the value of `p` at function entry. */