From 8b82eaa6335cee2aec153da156d90e5cd02bafb1 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 28 Jan 2025 13:44:27 +0100 Subject: [PATCH] Rust: Fix data flow through callbacks passed to library functions --- .../rust/dataflow/internal/DataFlowImpl.qll | 24 +++++----- .../CONSISTENCY/DataFlowConsistency.expected | 2 +- .../dataflow/local/DataFlowStep.expected | 48 +++++++++---------- .../dataflow/local/inline-flow.expected | 17 ++++--- .../test/library-tests/dataflow/local/main.rs | 16 +++---- 5 files changed, 56 insertions(+), 51 deletions(-) diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index 19da3ae509a..7f6f7b58268 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -49,16 +49,12 @@ 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() { result = call } + CallExprCfgNode asCallExprCfgNode() { result = this.asCallBaseExprCfgNode() } - MethodCallExprCfgNode asMethodCallExprCfgNode() { result = call } + MethodCallExprCfgNode asMethodCallExprCfgNode() { result = this.asCallBaseExprCfgNode() } - CallExprBaseCfgNode asCallBaseExprCfgNode() { result = call } + CallExprBaseCfgNode asCallBaseExprCfgNode() { this = TCall(result) } predicate isSummaryCall( FlowSummaryImpl::Public::SummarizedCallable c, FlowSummaryImpl::Private::SummaryNode receiver @@ -67,7 +63,7 @@ final class DataFlowCall extends TDataFlowCall { } DataFlowCallable getEnclosingCallable() { - result = TCfgScope(call.getExpr().getEnclosingCfgScope()) + result = TCfgScope(this.asCallBaseExprCfgNode().getExpr().getEnclosingCfgScope()) or exists(FlowSummaryImpl::Public::SummarizedCallable c | this.isSummaryCall(c, _) and @@ -1298,10 +1294,14 @@ module RustDataFlow implements InputSig { * invoked expression. */ predicate lambdaCall(DataFlowCall call, LambdaCallKind kind, Node receiver) { - receiver.asExpr() = call.asCallExprCfgNode().getFunction() and - // All calls to complex expressions and local variable accesses are lambda call. - exists(Expr f | f = receiver.asExpr().getExpr() | - f instanceof PathExpr implies f = any(Variable v).getAnAccess() + ( + receiver.asExpr() = call.asCallExprCfgNode().getFunction() and + // All calls to complex expressions and local variable accesses are lambda call. + exists(Expr f | f = receiver.asExpr().getExpr() | + f instanceof PathExpr implies f = any(Variable v).getAnAccess() + ) + or + call.isSummaryCall(_, receiver.(Node::FlowSummaryNode).getSummaryNode()) ) and exists(kind) } diff --git a/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected b/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected index 1eef1f6e8ab..ed95b9e1208 100644 --- a/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected +++ b/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected @@ -1,2 +1,2 @@ identityLocalStep -| main.rs:412:7:412:18 | phi(default_name) | Node steps to itself | +| main.rs:412:9:412:20 | phi(default_name) | Node steps to itself | diff --git a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected index 79bfd4bc05b..5acd1e8126c 100644 --- a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected +++ b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected @@ -471,26 +471,26 @@ localStep | main.rs:403:13:403:19 | [post] mut_arr | main.rs:405:10:405:16 | mut_arr | | | main.rs:403:13:403:19 | mut_arr | main.rs:405:10:405:16 | mut_arr | | | main.rs:403:13:403:22 | mut_arr[1] | main.rs:403:9:403:9 | d | | -| main.rs:410:39:410:43 | [SSA] names | main.rs:412:23:412:27 | names | | +| main.rs:410:39:410:43 | [SSA] names | main.rs:412:25:412:29 | names | | | main.rs:410:39:410:43 | names | main.rs:410:39:410:43 | [SSA] names | | | main.rs:410:39:410:72 | ...: Vec::<...> | main.rs:410:39:410:43 | names | | -| main.rs:411:7:411:18 | default_name | main.rs:411:7:411:18 | [SSA] default_name | | -| main.rs:411:22:411:43 | ... .to_string(...) | main.rs:411:7:411:18 | default_name | | -| main.rs:411:22:411:43 | ... .to_string(...) | main.rs:412:7:412:18 | phi(default_name) | | -| main.rs:412:3:418:3 | for ... in ... { ... } | main.rs:410:75:419:1 | { ... } | | -| main.rs:412:7:412:18 | phi(default_name) | main.rs:412:7:412:18 | phi(default_name) | | -| main.rs:412:7:412:18 | phi(default_name) | main.rs:414:35:414:61 | default_name | | -| main.rs:412:8:412:11 | [SSA] cond | main.rs:413:8:413:11 | cond | | -| main.rs:412:8:412:11 | cond | main.rs:412:8:412:11 | [SSA] cond | | -| main.rs:412:14:412:17 | [SSA] name | main.rs:414:15:414:18 | name | | -| main.rs:412:14:412:17 | name | main.rs:412:14:412:17 | [SSA] name | | -| main.rs:413:5:417:5 | if cond {...} | main.rs:412:29:418:3 | { ... } | | -| main.rs:414:11:414:11 | [SSA] n | main.rs:415:12:415:12 | n | | -| main.rs:414:11:414:11 | n | main.rs:414:11:414:11 | [SSA] n | | -| main.rs:414:15:414:62 | name.unwrap_or_else(...) | main.rs:414:11:414:11 | n | | -| main.rs:414:35:414:61 | [post] default_name | main.rs:412:7:412:18 | phi(default_name) | | -| main.rs:414:35:414:61 | closure self in \|...\| ... | main.rs:414:38:414:49 | this | | -| main.rs:414:35:414:61 | default_name | main.rs:412:7:412:18 | phi(default_name) | | +| main.rs:411:9:411:20 | default_name | main.rs:411:9:411:20 | [SSA] default_name | | +| main.rs:411:24:411:45 | ... .to_string(...) | main.rs:411:9:411:20 | default_name | | +| main.rs:411:24:411:45 | ... .to_string(...) | main.rs:412:9:412:20 | phi(default_name) | | +| main.rs:412:5:418:5 | for ... in ... { ... } | main.rs:410:75:419:1 | { ... } | | +| main.rs:412:9:412:20 | phi(default_name) | main.rs:412:9:412:20 | phi(default_name) | | +| main.rs:412:9:412:20 | phi(default_name) | main.rs:414:41:414:67 | default_name | | +| main.rs:412:10:412:13 | [SSA] cond | main.rs:413:12:413:15 | cond | | +| main.rs:412:10:412:13 | cond | main.rs:412:10:412:13 | [SSA] cond | | +| main.rs:412:16:412:19 | [SSA] name | main.rs:414:21:414:24 | name | | +| main.rs:412:16:412:19 | name | main.rs:412:16:412:19 | [SSA] name | | +| main.rs:413:9:417:9 | if cond {...} | main.rs:412:31:418:5 | { ... } | | +| main.rs:414:17:414:17 | [SSA] n | main.rs:415:18:415:18 | n | | +| main.rs:414:17:414:17 | n | main.rs:414:17:414:17 | [SSA] n | | +| main.rs:414:21:414:68 | name.unwrap_or_else(...) | main.rs:414:17:414:17 | n | | +| main.rs:414:41:414:67 | [post] default_name | main.rs:412:9:412:20 | phi(default_name) | | +| main.rs:414:41:414:67 | closure self in \|...\| ... | main.rs:414:44:414:55 | this | | +| main.rs:414:41:414:67 | default_name | main.rs:412:9:412:20 | phi(default_name) | | | main.rs:428:9:428:9 | [SSA] s | main.rs:429:10:429:10 | s | | | main.rs:428:9:428:9 | s | main.rs:428:9:428:9 | [SSA] s | | | main.rs:428:13:428:27 | MacroExpr | main.rs:428:9:428:9 | s | | @@ -600,7 +600,7 @@ storeStep | main.rs:399:27:399:27 | 2 | element | main.rs:399:23:399:31 | [...] | | main.rs:399:30:399:30 | 3 | element | main.rs:399:23:399:31 | [...] | | main.rs:402:18:402:27 | source(...) | element | main.rs:402:5:402:11 | [post] mut_arr | -| main.rs:414:35:414:61 | default_name | captured default_name | main.rs:414:35:414:61 | \|...\| ... | +| main.rs:414:41:414:67 | default_name | captured default_name | main.rs:414:41:414:67 | \|...\| ... | | main.rs:436:27:436:27 | 0 | Some | main.rs:436:22:436:28 | Some(...) | readStep | file://:0:0:0:0 | [summary param] self in lang:core::_::::expect | Some | file://:0:0:0:0 | [summary] read: Argument[self].Variant[crate::option::Option::Some(0)] in lang:core::_::::expect | @@ -689,8 +689,8 @@ readStep | main.rs:402:5:402:11 | mut_arr | element | main.rs:402:5:402:14 | mut_arr[1] | | main.rs:403:13:403:19 | mut_arr | element | main.rs:403:13:403:22 | mut_arr[1] | | main.rs:405:10:405:16 | mut_arr | element | main.rs:405:10:405:19 | mut_arr[0] | -| main.rs:412:7:412:18 | TuplePat | tuple.0 | main.rs:412:8:412:11 | cond | -| main.rs:412:7:412:18 | TuplePat | tuple.1 | main.rs:412:14:412:17 | name | -| main.rs:412:23:412:27 | names | element | main.rs:412:7:412:18 | TuplePat | -| main.rs:414:35:414:61 | [post] \|...\| ... | captured default_name | main.rs:414:35:414:61 | [post] default_name | -| main.rs:414:38:414:49 | this | captured default_name | main.rs:414:38:414:49 | default_name | +| main.rs:412:9:412:20 | TuplePat | tuple.0 | main.rs:412:10:412:13 | cond | +| main.rs:412:9:412:20 | TuplePat | tuple.1 | main.rs:412:16:412:19 | name | +| main.rs:412:25:412:29 | names | element | main.rs:412:9:412:20 | TuplePat | +| main.rs:414:41:414:67 | [post] \|...\| ... | captured default_name | main.rs:414:41:414:67 | [post] default_name | +| main.rs:414:44:414:55 | this | captured default_name | main.rs:414:44:414:55 | default_name | diff --git a/rust/ql/test/library-tests/dataflow/local/inline-flow.expected b/rust/ql/test/library-tests/dataflow/local/inline-flow.expected index 972b140623f..b52569042ed 100644 --- a/rust/ql/test/library-tests/dataflow/local/inline-flow.expected +++ b/rust/ql/test/library-tests/dataflow/local/inline-flow.expected @@ -2,9 +2,10 @@ models | 1 | Summary: lang:core; ::unwrap; Argument[self].Variant[crate::option::Option::Some(0)]; ReturnValue; value | | 2 | Summary: lang:core; ::unwrap_or; Argument[0]; ReturnValue; value | | 3 | Summary: lang:core; ::unwrap_or; Argument[self].Variant[crate::option::Option::Some(0)]; ReturnValue; value | -| 4 | Summary: lang:core; ::unwrap_or_else; Argument[self].Variant[crate::option::Option::Some(0)]; ReturnValue; value | -| 5 | Summary: lang:core; ::expect; Argument[self].Variant[crate::result::Result::Ok(0)]; ReturnValue; value | -| 6 | Summary: lang:core; ::expect_err; Argument[self].Variant[crate::result::Result::Err(0)]; ReturnValue; value | +| 4 | Summary: lang:core; ::unwrap_or_else; Argument[0].ReturnValue; ReturnValue; value | +| 5 | Summary: lang:core; ::unwrap_or_else; Argument[self].Variant[crate::option::Option::Some(0)]; ReturnValue; value | +| 6 | Summary: lang:core; ::expect; Argument[self].Variant[crate::result::Result::Ok(0)]; ReturnValue; value | +| 7 | Summary: lang:core; ::expect_err; Argument[self].Variant[crate::result::Result::Err(0)]; ReturnValue; value | edges | main.rs:19:9:19:9 | s | main.rs:20:10:20:10 | s | provenance | | | main.rs:19:13:19:21 | source(...) | main.rs:19:9:19:9 | s | provenance | | @@ -72,7 +73,8 @@ edges | main.rs:237:9:237:10 | s1 [Some] | main.rs:238:10:238:11 | s1 [Some] | provenance | | | main.rs:237:14:237:29 | Some(...) [Some] | main.rs:237:9:237:10 | s1 [Some] | provenance | | | main.rs:237:19:237:28 | source(...) | main.rs:237:14:237:29 | Some(...) [Some] | provenance | | -| main.rs:238:10:238:11 | s1 [Some] | main.rs:238:10:238:32 | s1.unwrap_or_else(...) | provenance | MaD:4 | +| main.rs:238:10:238:11 | s1 [Some] | main.rs:238:10:238:32 | s1.unwrap_or_else(...) | provenance | MaD:5 | +| main.rs:241:31:241:40 | source(...) | main.rs:241:10:241:41 | s2.unwrap_or_else(...) | provenance | MaD:4 | | main.rs:245:9:245:10 | s1 [Some] | main.rs:247:14:247:15 | s1 [Some] | provenance | | | main.rs:245:14:245:29 | Some(...) [Some] | main.rs:245:9:245:10 | s1 [Some] | provenance | | | main.rs:245:19:245:28 | source(...) | main.rs:245:14:245:29 | Some(...) [Some] | provenance | | @@ -88,11 +90,11 @@ edges | main.rs:267:9:267:10 | s1 [Ok] | main.rs:268:10:268:11 | s1 [Ok] | provenance | | | main.rs:267:32:267:45 | Ok(...) [Ok] | main.rs:267:9:267:10 | s1 [Ok] | provenance | | | main.rs:267:35:267:44 | source(...) | main.rs:267:32:267:45 | Ok(...) [Ok] | provenance | | -| main.rs:268:10:268:11 | s1 [Ok] | main.rs:268:10:268:22 | s1.expect(...) | provenance | MaD:5 | +| main.rs:268:10:268:11 | s1 [Ok] | main.rs:268:10:268:22 | s1.expect(...) | provenance | MaD:6 | | main.rs:271:9:271:10 | s2 [Err] | main.rs:273:10:273:11 | s2 [Err] | provenance | | | main.rs:271:32:271:46 | Err(...) [Err] | main.rs:271:9:271:10 | s2 [Err] | provenance | | | main.rs:271:36:271:45 | source(...) | main.rs:271:32:271:46 | Err(...) [Err] | provenance | | -| main.rs:273:10:273:11 | s2 [Err] | main.rs:273:10:273:26 | s2.expect_err(...) | provenance | MaD:6 | +| main.rs:273:10:273:11 | s2 [Err] | main.rs:273:10:273:26 | s2.expect_err(...) | provenance | MaD:7 | | main.rs:282:9:282:10 | s1 [A] | main.rs:284:11:284:12 | s1 [A] | provenance | | | main.rs:282:14:282:39 | ...::A(...) [A] | main.rs:282:9:282:10 | s1 [A] | provenance | | | main.rs:282:29:282:38 | source(...) | main.rs:282:14:282:39 | ...::A(...) [A] | provenance | | @@ -255,6 +257,8 @@ nodes | main.rs:237:19:237:28 | source(...) | semmle.label | source(...) | | main.rs:238:10:238:11 | s1 [Some] | semmle.label | s1 [Some] | | main.rs:238:10:238:32 | s1.unwrap_or_else(...) | semmle.label | s1.unwrap_or_else(...) | +| main.rs:241:10:241:41 | s2.unwrap_or_else(...) | semmle.label | s2.unwrap_or_else(...) | +| main.rs:241:31:241:40 | source(...) | semmle.label | source(...) | | main.rs:245:9:245:10 | s1 [Some] | semmle.label | s1 [Some] | | main.rs:245:14:245:29 | Some(...) [Some] | semmle.label | Some(...) [Some] | | main.rs:245:19:245:28 | source(...) | semmle.label | source(...) | @@ -386,6 +390,7 @@ testFailures | main.rs:230:10:230:24 | s1.unwrap_or(...) | main.rs:229:19:229:28 | source(...) | main.rs:230:10:230:24 | s1.unwrap_or(...) | $@ | main.rs:229:19:229:28 | source(...) | source(...) | | main.rs:233:10:233:33 | s2.unwrap_or(...) | main.rs:233:23:233:32 | source(...) | main.rs:233:10:233:33 | s2.unwrap_or(...) | $@ | main.rs:233:23:233:32 | source(...) | source(...) | | main.rs:238:10:238:32 | s1.unwrap_or_else(...) | main.rs:237:19:237:28 | source(...) | main.rs:238:10:238:32 | s1.unwrap_or_else(...) | $@ | main.rs:237:19:237:28 | source(...) | source(...) | +| main.rs:241:10:241:41 | s2.unwrap_or_else(...) | main.rs:241:31:241:40 | source(...) | main.rs:241:10:241:41 | s2.unwrap_or_else(...) | $@ | main.rs:241:31:241:40 | source(...) | source(...) | | main.rs:248:10:248:11 | i1 | main.rs:245:19:245:28 | source(...) | main.rs:248:10:248:11 | i1 | $@ | main.rs:245:19:245:28 | source(...) | source(...) | | main.rs:259:10:259:11 | i1 | main.rs:254:35:254:44 | source(...) | main.rs:259:10:259:11 | i1 | $@ | main.rs:254:35:254:44 | source(...) | source(...) | | main.rs:268:10:268:22 | s1.expect(...) | main.rs:267:35:267:44 | source(...) | main.rs:268:10:268:22 | s1.expect(...) | $@ | main.rs:267:35:267:44 | source(...) | source(...) | diff --git a/rust/ql/test/library-tests/dataflow/local/main.rs b/rust/ql/test/library-tests/dataflow/local/main.rs index c4d90f3d728..e2c1a76b142 100644 --- a/rust/ql/test/library-tests/dataflow/local/main.rs +++ b/rust/ql/test/library-tests/dataflow/local/main.rs @@ -238,7 +238,7 @@ fn option_unwrap_or_else() { sink(s1.unwrap_or_else(|| 0)); // $ hasValueFlow=47 let s2 = None; - sink(s2.unwrap_or_else(|| source(48))); // $ MISSING: hasValueFlow=48 + sink(s2.unwrap_or_else(|| source(48))); // $ hasValueFlow=48 } fn option_questionmark() -> Option { @@ -408,14 +408,14 @@ fn array_assignment() { // Test data flow inconsistency occuring with captured variables and `continue` // in a loop. pub fn captured_variable_and_continue(names: Vec<(bool, Option)>) { - let default_name = source(83).to_string(); - for (cond, name) in names { - if cond { - let n = name.unwrap_or_else(|| default_name.to_string()); - sink(n.len() as i64); - continue; + let default_name = source(83).to_string(); + for (cond, name) in names { + if cond { + let n = name.unwrap_or_else(|| default_name.to_string()); + sink(n.len() as i64); + continue; + } } - } } macro_rules! get_source {