From bb70bfce4354f7e53c153b239513f6f36c7fd144 Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Thu, 21 Nov 2024 13:11:40 +0100 Subject: [PATCH 1/3] Rust: Tweak global data flow test and add inline flow test --- .../dataflow/global/inline-flow.expected | 6 ++++ .../dataflow/global/inline-flow.ql | 12 +++++++ .../library-tests/dataflow/global/main.rs | 22 ++++++++---- .../dataflow/global/viableCallable.expected | 35 ++++++++++--------- 4 files changed, 52 insertions(+), 23 deletions(-) create mode 100644 rust/ql/test/library-tests/dataflow/global/inline-flow.expected create mode 100644 rust/ql/test/library-tests/dataflow/global/inline-flow.ql diff --git a/rust/ql/test/library-tests/dataflow/global/inline-flow.expected b/rust/ql/test/library-tests/dataflow/global/inline-flow.expected new file mode 100644 index 000000000000..4e4a41dfc62c --- /dev/null +++ b/rust/ql/test/library-tests/dataflow/global/inline-flow.expected @@ -0,0 +1,6 @@ +models +edges +nodes +subpaths +testFailures +#select diff --git a/rust/ql/test/library-tests/dataflow/global/inline-flow.ql b/rust/ql/test/library-tests/dataflow/global/inline-flow.ql new file mode 100644 index 000000000000..ad553fe548dc --- /dev/null +++ b/rust/ql/test/library-tests/dataflow/global/inline-flow.ql @@ -0,0 +1,12 @@ +/** + * @kind path-problem + */ + +import rust +import utils.InlineFlowTest +import DefaultFlowTest +import ValueFlow::PathGraph + +from ValueFlow::PathNode source, ValueFlow::PathNode sink +where ValueFlow::flowPath(source, sink) +select sink, source, sink, "$@", source, source.toString() diff --git a/rust/ql/test/library-tests/dataflow/global/main.rs b/rust/ql/test/library-tests/dataflow/global/main.rs index eb3a59856922..730ddd3cf4a2 100644 --- a/rust/ql/test/library-tests/dataflow/global/main.rs +++ b/rust/ql/test/library-tests/dataflow/global/main.rs @@ -15,11 +15,11 @@ fn get_data(n: i64) -> i64 { fn data_out_of_call() { let a = get_data(7); - sink(a); // $ hasValueFlow=n + sink(a); // $ MISSING: hasValueFlow=n } fn data_in(n: i64) { - sink(n + 7); // $ hasValueFlow + sink(n); // $ MISSING: hasValueFlow=3 } fn data_in_to_call() { @@ -34,7 +34,15 @@ fn pass_through(i: i64) -> i64 { fn data_through_call() { let a = source(1); let b = pass_through(a); - sink(b); // $ hasValueFlow=1 + sink(b); // $ MISSING: hasValueFlow=1 +} + +fn block_expression_as_argument() { + let a = pass_through({ + println!("Hello"); + source(14) + }); + sink(a); // $ MISSING: hasValueFlow=14 } // ----------------------------------------------------------------------------- @@ -46,7 +54,7 @@ struct MyFlag { impl MyFlag { fn data_in(&self, n: i64) { - sink(n); // $ hasValueFlow=1 + sink(n); // $ MISSING: hasValueFlow=1 } fn get_data(&self) -> i64 { if self.flag { @@ -67,7 +75,7 @@ impl MyFlag { fn data_out_of_method() { let mn = MyFlag { flag: true }; let a = mn.get_data(); - sink(a); + sink(a); // $ MISSING: hasValueFlow=2 } fn data_in_to_method_call() { @@ -79,8 +87,8 @@ fn data_in_to_method_call() { fn data_through_method() { let mn = MyFlag { flag: true }; let a = source(4); - mn.data_through(a); - sink(a); // $ hasValueFlow=4 + let b = mn.data_through(a); + sink(b); // $ MISSING: hasValueFlow=4 } fn main() { diff --git a/rust/ql/test/library-tests/dataflow/global/viableCallable.expected b/rust/ql/test/library-tests/dataflow/global/viableCallable.expected index 6a15bb25249a..3a1db5acf22e 100644 --- a/rust/ql/test/library-tests/dataflow/global/viableCallable.expected +++ b/rust/ql/test/library-tests/dataflow/global/viableCallable.expected @@ -1,24 +1,27 @@ | main.rs:13:5:13:13 | CallExpr | main.rs:1:1:3:1 | source | | main.rs:17:13:17:23 | CallExpr | main.rs:12:1:14:1 | get_data | | main.rs:18:5:18:11 | CallExpr | main.rs:5:1:7:1 | sink | -| main.rs:22:5:22:15 | CallExpr | main.rs:5:1:7:1 | sink | +| main.rs:22:5:22:11 | CallExpr | main.rs:5:1:7:1 | sink | | main.rs:26:13:26:21 | CallExpr | main.rs:1:1:3:1 | source | | main.rs:27:5:27:14 | CallExpr | main.rs:21:1:23:1 | data_in | | main.rs:35:13:35:21 | CallExpr | main.rs:1:1:3:1 | source | | main.rs:36:13:36:27 | CallExpr | main.rs:30:1:32:1 | pass_through | | main.rs:37:5:37:11 | CallExpr | main.rs:5:1:7:1 | sink | -| main.rs:49:9:49:15 | CallExpr | main.rs:5:1:7:1 | sink | -| main.rs:55:13:55:21 | CallExpr | main.rs:1:1:3:1 | source | -| main.rs:69:13:69:25 | ... .get_data(...) | main.rs:51:5:57:5 | get_data | -| main.rs:70:5:70:11 | CallExpr | main.rs:5:1:7:1 | sink | -| main.rs:75:13:75:21 | CallExpr | main.rs:1:1:3:1 | source | -| main.rs:76:5:76:17 | ... .data_in(...) | main.rs:48:5:50:5 | data_in | -| main.rs:81:13:81:21 | CallExpr | main.rs:1:1:3:1 | source | -| main.rs:82:5:82:22 | ... .data_through(...) | main.rs:58:5:64:5 | data_through | -| main.rs:83:5:83:11 | CallExpr | main.rs:5:1:7:1 | sink | -| main.rs:87:5:87:22 | CallExpr | main.rs:16:1:19:1 | data_out_of_call | -| main.rs:88:5:88:21 | CallExpr | main.rs:25:1:28:1 | data_in_to_call | -| main.rs:89:5:89:23 | CallExpr | main.rs:34:1:38:1 | data_through_call | -| main.rs:91:5:91:24 | CallExpr | main.rs:67:1:71:1 | data_out_of_method | -| main.rs:92:5:92:28 | CallExpr | main.rs:73:1:77:1 | data_in_to_method_call | -| main.rs:93:5:93:25 | CallExpr | main.rs:79:1:84:1 | data_through_method | +| main.rs:41:13:44:6 | CallExpr | main.rs:30:1:32:1 | pass_through | +| main.rs:43:9:43:18 | CallExpr | main.rs:1:1:3:1 | source | +| main.rs:45:5:45:11 | CallExpr | main.rs:5:1:7:1 | sink | +| main.rs:57:9:57:15 | CallExpr | main.rs:5:1:7:1 | sink | +| main.rs:63:13:63:21 | CallExpr | main.rs:1:1:3:1 | source | +| main.rs:77:13:77:25 | ... .get_data(...) | main.rs:59:5:65:5 | get_data | +| main.rs:78:5:78:11 | CallExpr | main.rs:5:1:7:1 | sink | +| main.rs:83:13:83:21 | CallExpr | main.rs:1:1:3:1 | source | +| main.rs:84:5:84:17 | ... .data_in(...) | main.rs:56:5:58:5 | data_in | +| main.rs:89:13:89:21 | CallExpr | main.rs:1:1:3:1 | source | +| main.rs:90:13:90:30 | ... .data_through(...) | main.rs:66:5:72:5 | data_through | +| main.rs:91:5:91:11 | CallExpr | main.rs:5:1:7:1 | sink | +| main.rs:95:5:95:22 | CallExpr | main.rs:16:1:19:1 | data_out_of_call | +| main.rs:96:5:96:21 | CallExpr | main.rs:25:1:28:1 | data_in_to_call | +| main.rs:97:5:97:23 | CallExpr | main.rs:34:1:38:1 | data_through_call | +| main.rs:99:5:99:24 | CallExpr | main.rs:75:1:79:1 | data_out_of_method | +| main.rs:100:5:100:28 | CallExpr | main.rs:81:1:85:1 | data_in_to_method_call | +| main.rs:101:5:101:25 | CallExpr | main.rs:87:1:92:1 | data_through_method | From fffeac6a1384358335992191baf33228436da6e2 Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Thu, 21 Nov 2024 15:11:25 +0100 Subject: [PATCH 2/3] Rust: Extend data flow library instantiation for global data flow --- .../lib/codeql/rust/controlflow/CfgNodes.qll | 2 + .../rust/dataflow/internal/DataFlowImpl.qll | 164 +++++++++++++----- .../CONSISTENCY/DataFlowConsistency.expected | 4 +- .../dataflow/barrier/inline-flow.expected | 13 ++ .../library-tests/dataflow/barrier/main.rs | 4 +- .../dataflow/global/inline-flow.expected | 69 ++++++++ .../library-tests/dataflow/global/main.rs | 12 +- .../dataflow/local/DataFlowStep.expected | 6 + 8 files changed, 219 insertions(+), 55 deletions(-) diff --git a/rust/ql/lib/codeql/rust/controlflow/CfgNodes.qll b/rust/ql/lib/codeql/rust/controlflow/CfgNodes.qll index b60a6b857b2e..3ee3a3eeb611 100644 --- a/rust/ql/lib/codeql/rust/controlflow/CfgNodes.qll +++ b/rust/ql/lib/codeql/rust/controlflow/CfgNodes.qll @@ -13,6 +13,8 @@ class AstCfgNode = CfgImpl::AstCfgNode; class ExitCfgNode = CfgImpl::ExitNode; +class AnnotatedExitCfgNode = CfgImpl::AnnotatedExitNode; + /** * An assignment expression, for example * diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index 210f12825cf2..35ab9ade7b2b 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -43,22 +43,59 @@ final class DataFlowCallable extends TDataFlowCallable { } final class DataFlowCall extends TDataFlowCall { + private CallExprBaseCfgNode call; + + DataFlowCall() { this = TCall(call) } + /** Gets the underlying call in the CFG, if any. */ - CallExprCfgNode asCallExprCfgNode() { this = TNormalCall(result) } + CallExprCfgNode asCallExprCfgNode() { result = call } - MethodCallExprCfgNode asMethodCallExprCfgNode() { this = TMethodCall(result) } + MethodCallExprCfgNode asMethodCallExprCfgNode() { result = call } - CallExprBaseCfgNode asExprCfgNode() { - result = this.asCallExprCfgNode() or result = this.asMethodCallExprCfgNode() - } + CallExprBaseCfgNode asCallBaseExprCfgNode() { result = call } DataFlowCallable getEnclosingCallable() { - result = TCfgScope(this.asExprCfgNode().getExpr().getEnclosingCfgScope()) + result = TCfgScope(call.getExpr().getEnclosingCfgScope()) + } + + string toString() { result = this.asCallBaseExprCfgNode().toString() } + + Location getLocation() { result = this.asCallBaseExprCfgNode().getLocation() } +} + +/** + * The position of a parameter or an argument in a function or call. + * + * As there is a 1-to-1 correspondence between parameter positions and + * arguments positions in Rust we use the same type for both. + */ +final class ParameterPosition extends TParameterPosition { + /** Gets the underlying integer position, if any. */ + int getPosition() { this = TPositionalParameterPosition(result) } + + /** Holds if this position represents the `self` position. */ + predicate isSelf() { this = TSelfParameterPosition() } + + /** Gets a textual representation of this position. */ + string toString() { + result = this.getPosition().toString() + or + result = "self" and this.isSelf() } - string toString() { result = this.asExprCfgNode().toString() } + AstNode getParameterIn(ParamList ps) { + result = ps.getParam(this.getPosition()) + or + result = ps.getSelfParam() and this.isSelf() + } +} - Location getLocation() { result = this.asExprCfgNode().getLocation() } +/** Holds if `arg` is an argument of `call` at the position `pos`. */ +private predicate isArgumentForCall(ExprCfgNode arg, CallExprBaseCfgNode call, ParameterPosition pos) { + arg = call.getArgument(pos.getPosition()) + or + // The self argument in a method call. + arg = call.(MethodCallExprCfgNode).getReceiver() and pos.isSelf() } module Node { @@ -93,11 +130,6 @@ module Node { * Gets this node's underlying SSA definition, if any. */ Ssa::Definition asDefinition() { none() } - - /** - * Gets the parameter that corresponds to this node, if any. - */ - Param asParameter() { none() } } /** A node type that is not implemented. */ @@ -111,7 +143,7 @@ module Node { override Location getLocation() { none() } } - /** A data flow node that corresponds to a CFG node for an AST node. */ + /** A data flow node that corresponds directly to a CFG node for an AST node. */ abstract class AstCfgFlowNode extends Node { AstCfgNode n; @@ -145,24 +177,37 @@ module Node { PatNode() { this = TPatNode(n) } - /** Gets the `Pat` in the AST that this node corresponds to. */ - Pat getPat() { result = n.getPat() } + /** Gets the `PatCfgNode` in the CFG that this node corresponds to. */ + PatCfgNode getPat() { result = n } } + abstract class ParameterNode extends AstCfgFlowNode { } + /** * The value of a parameter at function entry, viewed as a node in a data * flow graph. */ - final class ParameterNode extends AstCfgFlowNode, TParameterNode { + final class NormalParameterNode extends ParameterNode, TParameterNode { override ParamCfgNode n; - ParameterNode() { this = TParameterNode(n) } + NormalParameterNode() { this = TParameterNode(n) } /** Gets the parameter in the CFG that this node corresponds to. */ ParamCfgNode getParameter() { result = n } } - final class ArgumentNode = NaNode; + final class SelfParameterNode extends ParameterNode, TSelfParameterNode { + override SelfParamCfgNode n; + + SelfParameterNode() { this = TSelfParameterNode(n) } + + /** Gets the self parameter in the AST that this node corresponds to. */ + SelfParamCfgNode getSelfParameter() { result = n } + } + + final class ArgumentNode extends ExprNode { + ArgumentNode() { isArgumentForCall(n, _, _) } + } /** An SSA node. */ class SsaNode extends Node, TSsaNode { @@ -185,7 +230,10 @@ module Node { /** A data flow node that represents a value returned by a callable. */ final class ReturnNode extends ExprNode { - ReturnNode() { this.getCfgNode().getASuccessor() instanceof ExitCfgNode } + ReturnNode() { + this.getCfgNode().getASuccessor() instanceof ExitCfgNode or + this.getCfgNode().getASuccessor() instanceof AnnotatedExitCfgNode + } ReturnKind getKind() { any() } } @@ -197,10 +245,10 @@ module Node { } final private class ExprOutNode extends ExprNode, OutNode { - ExprOutNode() { this.asExpr() instanceof CallExprCfgNode } + ExprOutNode() { this.asExpr() instanceof CallExprBaseCfgNode } /** Gets the underlying call CFG node that includes this out node. */ - override DataFlowCall getCall() { result.asExprCfgNode() = this.getCfgNode() } + override DataFlowCall getCall() { result.asCallBaseExprCfgNode() = this.getCfgNode() } } /** @@ -214,9 +262,19 @@ module Node { * Nodes corresponding to AST elements, for example `ExprNode`, usually refer * to the value before the update. */ - final class PostUpdateNode extends Node::NaNode { + final class PostUpdateNode extends Node, TArgumentPostUpdateNode { + private ExprCfgNode n; + + PostUpdateNode() { this = TArgumentPostUpdateNode(n) } + /** Gets the node before the state update. */ - Node getPreUpdateNode() { none() } + Node getPreUpdateNode() { result = TExprNode(n) } + + final override CfgScope getCfgScope() { result = n.getAstNode().getEnclosingCfgScope() } + + final override Location getLocation() { result = n.getAstNode().getLocation() } + + final override string toString() { result = n.getAstNode().toString() } } final class CastNode = NaNode; @@ -226,25 +284,27 @@ final class Node = Node::Node; /** Provides logic related to SSA. */ module SsaFlow { - private module Impl = SsaImpl::DataFlowIntegration; + private module SsaFlow = SsaImpl::DataFlowIntegration; - private Node::ParameterNode toParameterNode(ParamCfgNode p) { result.getParameter() = p } + private Node::ParameterNode toParameterNode(ParamCfgNode p) { + result.(Node::NormalParameterNode).getParameter() = p + } /** Converts a control flow node into an SSA control flow node. */ - Impl::Node asNode(Node n) { + SsaFlow::Node asNode(Node n) { n = TSsaNode(result) or - result.(Impl::ExprNode).getExpr() = n.asExpr() + result.(SsaFlow::ExprNode).getExpr() = n.asExpr() or - n = toParameterNode(result.(Impl::ParameterNode).getParameter()) + n = toParameterNode(result.(SsaFlow::ParameterNode).getParameter()) } predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) { - Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep) + SsaFlow::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep) } predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { - Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo)) + SsaFlow::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo)) } } @@ -276,6 +336,8 @@ module LocalFlow { nodeFrom.(Node::AstCfgFlowNode).getCfgNode() = nodeTo.(Node::SsaNode).getDefinitionExt().(Ssa::WriteDefinition).getControlFlowNode() or + nodeFrom.(Node::NormalParameterNode).getParameter().getPat() = nodeTo.(Node::PatNode).getPat() + or SsaFlow::localFlowStep(_, nodeFrom, nodeTo, _) or exists(AssignmentExprCfgNode a | @@ -291,6 +353,8 @@ private class ReturnKindAlias = ReturnKind; private class DataFlowCallAlias = DataFlowCall; +private class ParameterPositionAlias = ParameterPosition; + module RustDataFlow implements InputSig { /** * An element, viewed as a node in a data flow graph. Either an expression @@ -310,9 +374,15 @@ module RustDataFlow implements InputSig { final class CastNode = Node::NaNode; - predicate isParameterNode(ParameterNode p, DataFlowCallable c, ParameterPosition pos) { none() } + /** Holds if `p` is a parameter of `c` at the position `pos`. */ + predicate isParameterNode(ParameterNode p, DataFlowCallable c, ParameterPosition pos) { + p.getCfgNode().getAstNode() = pos.getParameterIn(c.asCfgScope().(Function).getParamList()) + } - predicate isArgumentNode(ArgumentNode n, DataFlowCall call, ArgumentPosition pos) { none() } + /** Holds if `n` is an argument of `c` at the position `pos`. */ + predicate isArgumentNode(ArgumentNode n, DataFlowCall call, ArgumentPosition pos) { + isArgumentForCall(n.getCfgNode(), call.asCallBaseExprCfgNode(), pos) + } DataFlowCallable nodeGetEnclosingCallable(Node node) { result = node.getEnclosingCallable() } @@ -335,10 +405,9 @@ module RustDataFlow implements InputSig { DataFlowCallable viableCallable(DataFlowCall c) { exists(Function f, string name | result.asCfgScope() = f and name = f.getName().toString() | if f.getParamList().hasSelfParam() - then name = c.asMethodCallExprCfgNode().getMethodCallExpr().getNameRef().getText() + then name = c.asMethodCallExprCfgNode().getNameRef().getText() else - name = - c.asCallExprCfgNode().getCallExpr().getExpr().(PathExpr).getPath().getPart().toString() + name = c.asCallExprCfgNode().getExpr().getExpr().(PathExpr).getPath().getPart().toString() ) } @@ -377,19 +446,15 @@ module RustDataFlow implements InputSig { ContentApprox getContentApprox(Content c) { any() } - class ParameterPosition extends string { - ParameterPosition() { this = "pos" } - } + class ParameterPosition = ParameterPositionAlias; - class ArgumentPosition extends string { - ArgumentPosition() { this = "pos" } - } + class ArgumentPosition = ParameterPosition; /** * Holds if the parameter position `ppos` matches the argument position * `apos`. */ - predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { none() } + predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos } /** * Holds if there is a simple local flow step from `node1` to `node2`. These @@ -497,13 +562,13 @@ private module Cached { newtype TNode = TExprNode(ExprCfgNode n) or TParameterNode(ParamCfgNode p) or + TSelfParameterNode(SelfParamCfgNode p) or TPatNode(PatCfgNode p) or + TArgumentPostUpdateNode(ExprCfgNode e) { isArgumentForCall(e, _, _) } or TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node) cached - newtype TDataFlowCall = - TNormalCall(CallExprCfgNode c) or - TMethodCall(MethodCallExprCfgNode c) + newtype TDataFlowCall = TCall(CallExprBaseCfgNode c) cached newtype TOptionalContentSet = @@ -521,6 +586,13 @@ private module Cached { predicate localFlowStepImpl(Node::Node nodeFrom, Node::Node nodeTo) { LocalFlow::localFlowStepCommon(nodeFrom, nodeTo) } + + cached + newtype TParameterPosition = + TPositionalParameterPosition(int i) { + exists(any(ParamList l).getParam(i)) or exists(any(ArgList l).getArg(i)) + } or + TSelfParameterPosition() } import Cached diff --git a/rust/ql/test/extractor-tests/generated/MacroItems/CONSISTENCY/DataFlowConsistency.expected b/rust/ql/test/extractor-tests/generated/MacroItems/CONSISTENCY/DataFlowConsistency.expected index 02210855a69c..1a9588efd03a 100644 --- a/rust/ql/test/extractor-tests/generated/MacroItems/CONSISTENCY/DataFlowConsistency.expected +++ b/rust/ql/test/extractor-tests/generated/MacroItems/CONSISTENCY/DataFlowConsistency.expected @@ -1,9 +1,11 @@ uniqueNodeLocation | file://:0:0:0:0 | ... .parent(...) | Node should have one location but has 0. | +| file://:0:0:0:0 | ... .parent(...) | Node should have one location but has 0. | | file://:0:0:0:0 | ... .unwrap(...) | Node should have one location but has 0. | | file://:0:0:0:0 | BlockExpr | Node should have one location but has 0. | | file://:0:0:0:0 | Param | Node should have one location but has 0. | | file://:0:0:0:0 | path | Node should have one location but has 0. | | file://:0:0:0:0 | path | Node should have one location but has 0. | +| file://:0:0:0:0 | path | Node should have one location but has 0. | missingLocation -| Nodes without location: 6 | +| Nodes without location: 8 | diff --git a/rust/ql/test/library-tests/dataflow/barrier/inline-flow.expected b/rust/ql/test/library-tests/dataflow/barrier/inline-flow.expected index a54c75d0c17f..dd98dd9cee47 100644 --- a/rust/ql/test/library-tests/dataflow/barrier/inline-flow.expected +++ b/rust/ql/test/library-tests/dataflow/barrier/inline-flow.expected @@ -1,16 +1,29 @@ models edges +| main.rs:9:13:9:19 | Param : unit | main.rs:9:30:14:1 | BlockExpr : unit | provenance | | | main.rs:21:13:21:21 | CallExpr : unit | main.rs:22:10:22:10 | s | provenance | | +| main.rs:26:13:26:21 | CallExpr : unit | main.rs:27:22:27:22 | s : unit | provenance | | +| main.rs:27:13:27:23 | CallExpr : unit | main.rs:28:10:28:10 | s | provenance | | +| main.rs:27:22:27:22 | s : unit | main.rs:9:13:9:19 | Param : unit | provenance | | +| main.rs:27:22:27:22 | s : unit | main.rs:27:13:27:23 | CallExpr : unit | provenance | | | main.rs:32:13:32:21 | CallExpr : unit | main.rs:33:10:33:10 | s | provenance | | nodes +| main.rs:9:13:9:19 | Param : unit | semmle.label | Param : unit | +| main.rs:9:30:14:1 | BlockExpr : unit | semmle.label | BlockExpr : unit | | main.rs:17:10:17:18 | CallExpr | semmle.label | CallExpr | | main.rs:21:13:21:21 | CallExpr : unit | semmle.label | CallExpr : unit | | main.rs:22:10:22:10 | s | semmle.label | s | +| main.rs:26:13:26:21 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:27:13:27:23 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:27:22:27:22 | s : unit | semmle.label | s : unit | +| main.rs:28:10:28:10 | s | semmle.label | s | | main.rs:32:13:32:21 | CallExpr : unit | semmle.label | CallExpr : unit | | main.rs:33:10:33:10 | s | semmle.label | s | subpaths +| main.rs:27:22:27:22 | s : unit | main.rs:9:13:9:19 | Param : unit | main.rs:9:30:14:1 | BlockExpr : unit | main.rs:27:13:27:23 | CallExpr : unit | testFailures #select | main.rs:17:10:17:18 | CallExpr | main.rs:17:10:17:18 | CallExpr | main.rs:17:10:17:18 | CallExpr | $@ | main.rs:17:10:17:18 | CallExpr | CallExpr | | main.rs:22:10:22:10 | s | main.rs:21:13:21:21 | CallExpr : unit | main.rs:22:10:22:10 | s | $@ | main.rs:21:13:21:21 | CallExpr : unit | CallExpr : unit | +| main.rs:28:10:28:10 | s | main.rs:26:13:26:21 | CallExpr : unit | main.rs:28:10:28:10 | s | $@ | main.rs:26:13:26:21 | CallExpr : unit | CallExpr : unit | | main.rs:33:10:33:10 | s | main.rs:32:13:32:21 | CallExpr : unit | main.rs:33:10:33:10 | s | $@ | main.rs:32:13:32:21 | CallExpr : unit | CallExpr : unit | diff --git a/rust/ql/test/library-tests/dataflow/barrier/main.rs b/rust/ql/test/library-tests/dataflow/barrier/main.rs index 8a0a7bc2be62..14935f0f3286 100644 --- a/rust/ql/test/library-tests/dataflow/barrier/main.rs +++ b/rust/ql/test/library-tests/dataflow/barrier/main.rs @@ -9,7 +9,7 @@ fn sink(s: &str) { fn sanitize(s: &str) -> &str { match s { "dangerous" => "", - s => s + s => s, } } @@ -25,7 +25,7 @@ fn through_variable() { fn with_barrier() { let s = source(1); let s = sanitize(s); - sink(s); + sink(s); // $ SPURIOUS: hasValueFlow=1 } fn main() { diff --git a/rust/ql/test/library-tests/dataflow/global/inline-flow.expected b/rust/ql/test/library-tests/dataflow/global/inline-flow.expected index 4e4a41dfc62c..6054637c1260 100644 --- a/rust/ql/test/library-tests/dataflow/global/inline-flow.expected +++ b/rust/ql/test/library-tests/dataflow/global/inline-flow.expected @@ -1,6 +1,75 @@ models edges +| main.rs:12:28:14:1 | BlockExpr : unit | main.rs:17:13:17:23 | CallExpr : unit | provenance | | +| main.rs:13:5:13:13 | CallExpr : unit | main.rs:12:28:14:1 | BlockExpr : unit | provenance | | +| main.rs:17:13:17:23 | CallExpr : unit | main.rs:18:10:18:10 | a | provenance | | +| main.rs:21:12:21:17 | Param : unit | main.rs:22:10:22:10 | n | provenance | | +| main.rs:26:13:26:21 | CallExpr : unit | main.rs:27:13:27:13 | a : unit | provenance | | +| main.rs:27:13:27:13 | a : unit | main.rs:21:12:21:17 | Param : unit | provenance | | +| main.rs:30:17:30:22 | Param : unit | main.rs:30:32:32:1 | BlockExpr : unit | provenance | | +| main.rs:35:13:35:21 | CallExpr : unit | main.rs:36:26:36:26 | a : unit | provenance | | +| main.rs:36:13:36:27 | CallExpr : unit | main.rs:37:10:37:10 | b | provenance | | +| main.rs:36:26:36:26 | a : unit | main.rs:30:17:30:22 | Param : unit | provenance | | +| main.rs:36:26:36:26 | a : unit | main.rs:36:13:36:27 | CallExpr : unit | provenance | | +| main.rs:41:13:44:6 | CallExpr : unit | main.rs:45:10:45:10 | a | provenance | | +| main.rs:41:26:44:5 | BlockExpr : unit | main.rs:30:17:30:22 | Param : unit | provenance | | +| main.rs:41:26:44:5 | BlockExpr : unit | main.rs:41:13:44:6 | CallExpr : unit | provenance | | +| main.rs:43:9:43:18 | CallExpr : unit | main.rs:41:26:44:5 | BlockExpr : unit | provenance | | +| main.rs:56:23:56:28 | Param : unit | main.rs:57:14:57:14 | n | provenance | | +| main.rs:59:31:65:5 | BlockExpr : unit | main.rs:77:13:77:25 | ... .get_data(...) : unit | provenance | | +| main.rs:63:13:63:21 | CallExpr : unit | main.rs:59:31:65:5 | BlockExpr : unit | provenance | | +| main.rs:66:28:66:33 | Param : unit | main.rs:66:43:72:5 | BlockExpr : unit | provenance | | +| main.rs:77:13:77:25 | ... .get_data(...) : unit | main.rs:78:10:78:10 | a | provenance | | +| main.rs:83:13:83:21 | CallExpr : unit | main.rs:84:16:84:16 | a : unit | provenance | | +| main.rs:84:16:84:16 | a : unit | main.rs:56:23:56:28 | Param : unit | provenance | | +| main.rs:89:13:89:21 | CallExpr : unit | main.rs:90:29:90:29 | a : unit | provenance | | +| main.rs:90:13:90:30 | ... .data_through(...) : unit | main.rs:91:10:91:10 | b | provenance | | +| main.rs:90:29:90:29 | a : unit | main.rs:66:28:66:33 | Param : unit | provenance | | +| main.rs:90:29:90:29 | a : unit | main.rs:90:13:90:30 | ... .data_through(...) : unit | provenance | | nodes +| main.rs:12:28:14:1 | BlockExpr : unit | semmle.label | BlockExpr : unit | +| main.rs:13:5:13:13 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:17:13:17:23 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:18:10:18:10 | a | semmle.label | a | +| main.rs:21:12:21:17 | Param : unit | semmle.label | Param : unit | +| main.rs:22:10:22:10 | n | semmle.label | n | +| main.rs:26:13:26:21 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:27:13:27:13 | a : unit | semmle.label | a : unit | +| main.rs:30:17:30:22 | Param : unit | semmle.label | Param : unit | +| main.rs:30:32:32:1 | BlockExpr : unit | semmle.label | BlockExpr : unit | +| main.rs:35:13:35:21 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:36:13:36:27 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:36:26:36:26 | a : unit | semmle.label | a : unit | +| main.rs:37:10:37:10 | b | semmle.label | b | +| main.rs:41:13:44:6 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:41:26:44:5 | BlockExpr : unit | semmle.label | BlockExpr : unit | +| main.rs:43:9:43:18 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:45:10:45:10 | a | semmle.label | a | +| main.rs:56:23:56:28 | Param : unit | semmle.label | Param : unit | +| main.rs:57:14:57:14 | n | semmle.label | n | +| main.rs:59:31:65:5 | BlockExpr : unit | semmle.label | BlockExpr : unit | +| main.rs:63:13:63:21 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:66:28:66:33 | Param : unit | semmle.label | Param : unit | +| main.rs:66:43:72:5 | BlockExpr : unit | semmle.label | BlockExpr : unit | +| main.rs:77:13:77:25 | ... .get_data(...) : unit | semmle.label | ... .get_data(...) : unit | +| main.rs:78:10:78:10 | a | semmle.label | a | +| main.rs:83:13:83:21 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:84:16:84:16 | a : unit | semmle.label | a : unit | +| main.rs:89:13:89:21 | CallExpr : unit | semmle.label | CallExpr : unit | +| main.rs:90:13:90:30 | ... .data_through(...) : unit | semmle.label | ... .data_through(...) : unit | +| main.rs:90:29:90:29 | a : unit | semmle.label | a : unit | +| main.rs:91:10:91:10 | b | semmle.label | b | subpaths +| main.rs:36:26:36:26 | a : unit | main.rs:30:17:30:22 | Param : unit | main.rs:30:32:32:1 | BlockExpr : unit | main.rs:36:13:36:27 | CallExpr : unit | +| main.rs:41:26:44:5 | BlockExpr : unit | main.rs:30:17:30:22 | Param : unit | main.rs:30:32:32:1 | BlockExpr : unit | main.rs:41:13:44:6 | CallExpr : unit | +| main.rs:90:29:90:29 | a : unit | main.rs:66:28:66:33 | Param : unit | main.rs:66:43:72:5 | BlockExpr : unit | main.rs:90:13:90:30 | ... .data_through(...) : unit | testFailures +| main.rs:45:10:45:10 | a | Fixed missing result: hasValueFlow=14 | #select +| main.rs:18:10:18:10 | a | main.rs:13:5:13:13 | CallExpr : unit | main.rs:18:10:18:10 | a | $@ | main.rs:13:5:13:13 | CallExpr : unit | CallExpr : unit | +| main.rs:22:10:22:10 | n | main.rs:26:13:26:21 | CallExpr : unit | main.rs:22:10:22:10 | n | $@ | main.rs:26:13:26:21 | CallExpr : unit | CallExpr : unit | +| main.rs:37:10:37:10 | b | main.rs:35:13:35:21 | CallExpr : unit | main.rs:37:10:37:10 | b | $@ | main.rs:35:13:35:21 | CallExpr : unit | CallExpr : unit | +| main.rs:45:10:45:10 | a | main.rs:43:9:43:18 | CallExpr : unit | main.rs:45:10:45:10 | a | $@ | main.rs:43:9:43:18 | CallExpr : unit | CallExpr : unit | +| main.rs:57:14:57:14 | n | main.rs:83:13:83:21 | CallExpr : unit | main.rs:57:14:57:14 | n | $@ | main.rs:83:13:83:21 | CallExpr : unit | CallExpr : unit | +| main.rs:78:10:78:10 | a | main.rs:63:13:63:21 | CallExpr : unit | main.rs:78:10:78:10 | a | $@ | main.rs:63:13:63:21 | CallExpr : unit | CallExpr : unit | +| main.rs:91:10:91:10 | b | main.rs:89:13:89:21 | CallExpr : unit | main.rs:91:10:91:10 | b | $@ | main.rs:89:13:89:21 | CallExpr : unit | CallExpr : unit | diff --git a/rust/ql/test/library-tests/dataflow/global/main.rs b/rust/ql/test/library-tests/dataflow/global/main.rs index 730ddd3cf4a2..ebdb7b30d75e 100644 --- a/rust/ql/test/library-tests/dataflow/global/main.rs +++ b/rust/ql/test/library-tests/dataflow/global/main.rs @@ -15,11 +15,11 @@ fn get_data(n: i64) -> i64 { fn data_out_of_call() { let a = get_data(7); - sink(a); // $ MISSING: hasValueFlow=n + sink(a); // $ hasValueFlow=n } fn data_in(n: i64) { - sink(n); // $ MISSING: hasValueFlow=3 + sink(n); // $ hasValueFlow=3 } fn data_in_to_call() { @@ -34,7 +34,7 @@ fn pass_through(i: i64) -> i64 { fn data_through_call() { let a = source(1); let b = pass_through(a); - sink(b); // $ MISSING: hasValueFlow=1 + sink(b); // $ hasValueFlow=1 } fn block_expression_as_argument() { @@ -54,7 +54,7 @@ struct MyFlag { impl MyFlag { fn data_in(&self, n: i64) { - sink(n); // $ MISSING: hasValueFlow=1 + sink(n); // $ hasValueFlow=1 } fn get_data(&self) -> i64 { if self.flag { @@ -75,7 +75,7 @@ impl MyFlag { fn data_out_of_method() { let mn = MyFlag { flag: true }; let a = mn.get_data(); - sink(a); // $ MISSING: hasValueFlow=2 + sink(a); // $ hasValueFlow=2 } fn data_in_to_method_call() { @@ -88,7 +88,7 @@ fn data_through_method() { let mn = MyFlag { flag: true }; let a = source(4); let b = mn.data_through(a); - sink(b); // $ MISSING: hasValueFlow=4 + sink(b); // $ hasValueFlow=4 } fn main() { diff --git a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected index 1ccc9c219c32..86bc28d7940b 100644 --- a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected +++ b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected @@ -1,13 +1,16 @@ | main.rs:3:11:3:11 | [SSA] i | main.rs:4:12:4:12 | i | | main.rs:3:11:3:11 | i | main.rs:3:11:3:11 | [SSA] i | +| main.rs:3:11:3:16 | Param | main.rs:3:11:3:11 | i | | main.rs:4:5:4:12 | ... + ... | main.rs:3:26:5:1 | BlockExpr | | main.rs:7:9:7:9 | [SSA] s | main.rs:8:20:8:20 | s | | main.rs:7:9:7:9 | s | main.rs:7:9:7:9 | [SSA] s | +| main.rs:7:9:7:14 | Param | main.rs:7:9:7:9 | s | | main.rs:19:9:19:9 | [SSA] s | main.rs:20:10:20:10 | s | | main.rs:19:9:19:9 | s | main.rs:19:9:19:9 | [SSA] s | | main.rs:19:13:19:21 | CallExpr | main.rs:19:9:19:9 | s | | main.rs:23:18:23:21 | [SSA] cond | main.rs:26:16:26:19 | cond | | main.rs:23:18:23:21 | cond | main.rs:23:18:23:21 | [SSA] cond | +| main.rs:23:18:23:27 | Param | main.rs:23:18:23:21 | cond | | main.rs:24:9:24:9 | [SSA] a | main.rs:26:23:26:23 | a | | main.rs:24:9:24:9 | a | main.rs:24:9:24:9 | [SSA] a | | main.rs:24:13:24:21 | CallExpr | main.rs:24:9:24:9 | a | @@ -23,6 +26,7 @@ | main.rs:26:34:26:34 | b | main.rs:26:32:26:36 | BlockExpr | | main.rs:30:21:30:21 | [SSA] m | main.rs:32:19:32:19 | m | | main.rs:30:21:30:21 | m | main.rs:30:21:30:21 | [SSA] m | +| main.rs:30:21:30:34 | Param | main.rs:30:21:30:21 | m | | main.rs:31:9:31:9 | [SSA] a | main.rs:33:20:33:20 | a | | main.rs:31:9:31:9 | a | main.rs:31:9:31:9 | [SSA] a | | main.rs:31:13:31:21 | CallExpr | main.rs:31:9:31:9 | a | @@ -91,6 +95,7 @@ | main.rs:118:5:118:5 | a | main.rs:116:31:119:1 | BlockExpr | | main.rs:121:22:121:22 | [SSA] b | main.rs:123:12:123:12 | b | | main.rs:121:22:121:22 | b | main.rs:121:22:121:22 | [SSA] b | +| main.rs:121:22:121:28 | Param | main.rs:121:22:121:22 | b | | main.rs:122:9:122:9 | [SSA] a | main.rs:128:5:128:5 | a | | main.rs:122:9:122:9 | a | main.rs:122:9:122:9 | [SSA] a | | main.rs:122:13:127:5 | BlockExpr | main.rs:122:9:122:9 | a | @@ -100,6 +105,7 @@ | main.rs:128:5:128:5 | a | main.rs:121:38:129:1 | BlockExpr | | main.rs:131:22:131:22 | [SSA] b | main.rs:133:12:133:12 | b | | main.rs:131:22:131:22 | b | main.rs:131:22:131:22 | [SSA] b | +| main.rs:131:22:131:28 | Param | main.rs:131:22:131:22 | b | | main.rs:132:9:132:9 | [SSA] a | main.rs:138:5:138:5 | a | | main.rs:132:9:132:9 | a | main.rs:132:9:132:9 | [SSA] a | | main.rs:132:13:137:5 | BlockExpr | main.rs:132:9:132:9 | a | From e81c3483dbe2c93f210ecc33841fd06a2e064f9f Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Fri, 22 Nov 2024 09:43:27 +0100 Subject: [PATCH 3/3] Rust: Apply suggestions from PR comments --- .../rust/dataflow/internal/DataFlowImpl.qll | 24 +++++++++---------- .../dataflow/global/inline-flow.expected | 1 - .../library-tests/dataflow/global/main.rs | 2 +- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index 35ab9ade7b2b..1ec8a42ee7e0 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -187,10 +187,10 @@ module Node { * The value of a parameter at function entry, viewed as a node in a data * flow graph. */ - final class NormalParameterNode extends ParameterNode, TParameterNode { + final class PositionalParameterNode extends ParameterNode, TParameterNode { override ParamCfgNode n; - NormalParameterNode() { this = TParameterNode(n) } + PositionalParameterNode() { this = TParameterNode(n) } /** Gets the parameter in the CFG that this node corresponds to. */ ParamCfgNode getParameter() { result = n } @@ -230,10 +230,7 @@ module Node { /** A data flow node that represents a value returned by a callable. */ final class ReturnNode extends ExprNode { - ReturnNode() { - this.getCfgNode().getASuccessor() instanceof ExitCfgNode or - this.getCfgNode().getASuccessor() instanceof AnnotatedExitCfgNode - } + ReturnNode() { this.getCfgNode().getASuccessor() instanceof AnnotatedExitCfgNode } ReturnKind getKind() { any() } } @@ -270,11 +267,11 @@ module Node { /** Gets the node before the state update. */ Node getPreUpdateNode() { result = TExprNode(n) } - final override CfgScope getCfgScope() { result = n.getAstNode().getEnclosingCfgScope() } + final override CfgScope getCfgScope() { result = n.getScope() } - final override Location getLocation() { result = n.getAstNode().getLocation() } + final override Location getLocation() { result = n.getLocation() } - final override string toString() { result = n.getAstNode().toString() } + final override string toString() { result = n.toString() } } final class CastNode = NaNode; @@ -287,7 +284,7 @@ module SsaFlow { private module SsaFlow = SsaImpl::DataFlowIntegration; private Node::ParameterNode toParameterNode(ParamCfgNode p) { - result.(Node::NormalParameterNode).getParameter() = p + result.(Node::PositionalParameterNode).getParameter() = p } /** Converts a control flow node into an SSA control flow node. */ @@ -336,7 +333,8 @@ module LocalFlow { nodeFrom.(Node::AstCfgFlowNode).getCfgNode() = nodeTo.(Node::SsaNode).getDefinitionExt().(Ssa::WriteDefinition).getControlFlowNode() or - nodeFrom.(Node::NormalParameterNode).getParameter().getPat() = nodeTo.(Node::PatNode).getPat() + nodeFrom.(Node::PositionalParameterNode).getParameter().getPat() = + nodeTo.(Node::PatNode).getPat() or SsaFlow::localFlowStep(_, nodeFrom, nodeTo, _) or @@ -376,7 +374,7 @@ module RustDataFlow implements InputSig { /** Holds if `p` is a parameter of `c` at the position `pos`. */ predicate isParameterNode(ParameterNode p, DataFlowCallable c, ParameterPosition pos) { - p.getCfgNode().getAstNode() = pos.getParameterIn(c.asCfgScope().(Function).getParamList()) + p.getCfgNode().getAstNode() = pos.getParameterIn(c.asCfgScope().(Callable).getParamList()) } /** Holds if `n` is an argument of `c` at the position `pos`. */ @@ -590,7 +588,7 @@ private module Cached { cached newtype TParameterPosition = TPositionalParameterPosition(int i) { - exists(any(ParamList l).getParam(i)) or exists(any(ArgList l).getArg(i)) + i in [0 .. max([any(ParamList l).getNumberOfParams(), any(ArgList l).getNumberOfArgs()]) - 1] } or TSelfParameterPosition() } diff --git a/rust/ql/test/library-tests/dataflow/global/inline-flow.expected b/rust/ql/test/library-tests/dataflow/global/inline-flow.expected index 6054637c1260..8c1d90ee45a6 100644 --- a/rust/ql/test/library-tests/dataflow/global/inline-flow.expected +++ b/rust/ql/test/library-tests/dataflow/global/inline-flow.expected @@ -64,7 +64,6 @@ subpaths | main.rs:41:26:44:5 | BlockExpr : unit | main.rs:30:17:30:22 | Param : unit | main.rs:30:32:32:1 | BlockExpr : unit | main.rs:41:13:44:6 | CallExpr : unit | | main.rs:90:29:90:29 | a : unit | main.rs:66:28:66:33 | Param : unit | main.rs:66:43:72:5 | BlockExpr : unit | main.rs:90:13:90:30 | ... .data_through(...) : unit | testFailures -| main.rs:45:10:45:10 | a | Fixed missing result: hasValueFlow=14 | #select | main.rs:18:10:18:10 | a | main.rs:13:5:13:13 | CallExpr : unit | main.rs:18:10:18:10 | a | $@ | main.rs:13:5:13:13 | CallExpr : unit | CallExpr : unit | | main.rs:22:10:22:10 | n | main.rs:26:13:26:21 | CallExpr : unit | main.rs:22:10:22:10 | n | $@ | main.rs:26:13:26:21 | CallExpr : unit | CallExpr : unit | diff --git a/rust/ql/test/library-tests/dataflow/global/main.rs b/rust/ql/test/library-tests/dataflow/global/main.rs index ebdb7b30d75e..77c37a945b8f 100644 --- a/rust/ql/test/library-tests/dataflow/global/main.rs +++ b/rust/ql/test/library-tests/dataflow/global/main.rs @@ -42,7 +42,7 @@ fn block_expression_as_argument() { println!("Hello"); source(14) }); - sink(a); // $ MISSING: hasValueFlow=14 + sink(a); // $ hasValueFlow=14 } // -----------------------------------------------------------------------------