From e8357a648d986f593e71aa743711a492b67434fe Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Wed, 11 Dec 2024 14:45:36 +0100 Subject: [PATCH 1/9] Rust: Add additional data flow tests --- .../dataflow/closures/inline-flow.expected | 33 ++++++++++ .../dataflow/closures/inline-flow.ql | 12 ++++ .../library-tests/dataflow/closures/main.rs | 57 +++++++++++++++++ .../dataflow/local/DataFlowStep.expected | 61 +++++-------------- .../dataflow/local/inline-flow.expected | 27 -------- .../test/library-tests/dataflow/local/main.rs | 41 +++++-------- 6 files changed, 132 insertions(+), 99 deletions(-) create mode 100644 rust/ql/test/library-tests/dataflow/closures/inline-flow.expected create mode 100644 rust/ql/test/library-tests/dataflow/closures/inline-flow.ql create mode 100644 rust/ql/test/library-tests/dataflow/closures/main.rs diff --git a/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected b/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected new file mode 100644 index 000000000000..316642d60364 --- /dev/null +++ b/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected @@ -0,0 +1,33 @@ +models +edges +| main.rs:11:20:11:52 | if cond {...} else {...} | main.rs:12:10:12:16 | f(...) | provenance | | +| main.rs:11:30:11:39 | source(...) | main.rs:11:20:11:52 | if cond {...} else {...} | provenance | | +| main.rs:16:20:16:23 | ... | main.rs:18:18:18:21 | data | provenance | | +| main.rs:22:13:22:22 | source(...) | main.rs:23:13:23:13 | a | provenance | | +| main.rs:23:13:23:13 | a | main.rs:16:20:16:23 | ... | provenance | | +| main.rs:27:20:27:23 | ... | main.rs:28:9:32:9 | if cond {...} else {...} | provenance | | +| main.rs:33:13:33:22 | source(...) | main.rs:34:21:34:21 | a | provenance | | +| main.rs:34:13:34:22 | f(...) | main.rs:35:10:35:10 | b | provenance | | +| main.rs:34:21:34:21 | a | main.rs:27:20:27:23 | ... | provenance | | +| main.rs:34:21:34:21 | a | main.rs:34:13:34:22 | f(...) | provenance | | +nodes +| main.rs:11:20:11:52 | if cond {...} else {...} | semmle.label | if cond {...} else {...} | +| main.rs:11:30:11:39 | source(...) | semmle.label | source(...) | +| main.rs:12:10:12:16 | f(...) | semmle.label | f(...) | +| main.rs:16:20:16:23 | ... | semmle.label | ... | +| main.rs:18:18:18:21 | data | semmle.label | data | +| main.rs:22:13:22:22 | source(...) | semmle.label | source(...) | +| main.rs:23:13:23:13 | a | semmle.label | a | +| main.rs:27:20:27:23 | ... | semmle.label | ... | +| main.rs:28:9:32:9 | if cond {...} else {...} | semmle.label | if cond {...} else {...} | +| main.rs:33:13:33:22 | source(...) | semmle.label | source(...) | +| main.rs:34:13:34:22 | f(...) | semmle.label | f(...) | +| main.rs:34:21:34:21 | a | semmle.label | a | +| main.rs:35:10:35:10 | b | semmle.label | b | +subpaths +| main.rs:34:21:34:21 | a | main.rs:27:20:27:23 | ... | main.rs:28:9:32:9 | if cond {...} else {...} | main.rs:34:13:34:22 | f(...) | +testFailures +#select +| main.rs:12:10:12:16 | f(...) | main.rs:11:30:11:39 | source(...) | main.rs:12:10:12:16 | f(...) | $@ | main.rs:11:30:11:39 | source(...) | source(...) | +| main.rs:18:18:18:21 | data | main.rs:22:13:22:22 | source(...) | main.rs:18:18:18:21 | data | $@ | main.rs:22:13:22:22 | source(...) | source(...) | +| main.rs:35:10:35:10 | b | main.rs:33:13:33:22 | source(...) | main.rs:35:10:35:10 | b | $@ | main.rs:33:13:33:22 | source(...) | source(...) | diff --git a/rust/ql/test/library-tests/dataflow/closures/inline-flow.ql b/rust/ql/test/library-tests/dataflow/closures/inline-flow.ql new file mode 100644 index 000000000000..ad553fe548dc --- /dev/null +++ b/rust/ql/test/library-tests/dataflow/closures/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/closures/main.rs b/rust/ql/test/library-tests/dataflow/closures/main.rs new file mode 100644 index 000000000000..d5a67525119c --- /dev/null +++ b/rust/ql/test/library-tests/dataflow/closures/main.rs @@ -0,0 +1,57 @@ +fn source(i: i64) -> i64 { + 1000 + i +} + +fn sink(s: i64) { + println!("{}", s); +} + + +fn closure_flow_out() { + let f = |cond| if cond { source(92) } else { 0 }; + sink(f(true)); // $ hasValueFlow=92 +} + +fn closure_flow_in() { + let f = |cond, data| + if cond { + sink(data); // $ hasValueFlow=87 + } else { + sink(0) + }; + let a = source(87); + f(true, a); +} + +fn closure_flow_through() { + let f = |cond, data| + if cond { + data + } else { + 0 + }; + let a = source(43); + let b = f(true, a); + sink(b); // $ hasValueFlow=43 +} + +fn closure_captured_variable() { + let mut capt = 1; + sink(capt); + let mut f = || { + capt = source(73); + }; + f(); + sink(capt); // $ MISSING: hasValueFlow=73 + let g = || { + sink(capt); // $ MISSING: hasValueFlow=73 + }; + g(); +} + +fn main() { + closure_flow_out(); + closure_flow_in(); + closure_flow_through(); + closure_captured_variable(); +} diff --git a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected index f637b80dac28..d5376353d83e 100644 --- a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected +++ b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected @@ -427,51 +427,20 @@ localStep | main.rs:377:13:377:19 | [post] mut_arr | main.rs:379:10:379:16 | mut_arr | | main.rs:377:13:377:19 | mut_arr | main.rs:379:10:379:16 | mut_arr | | main.rs:377:13:377:22 | mut_arr[1] | main.rs:377:9:377:9 | d | -| main.rs:383:9:383:9 | [SSA] f | main.rs:384:10:384:10 | f | -| main.rs:383:9:383:9 | f | main.rs:383:9:383:9 | [SSA] f | -| main.rs:383:13:383:52 | \|...\| ... | main.rs:383:9:383:9 | f | -| main.rs:383:14:383:17 | ... | main.rs:383:14:383:17 | cond | -| main.rs:383:14:383:17 | [SSA] cond | main.rs:383:23:383:26 | cond | -| main.rs:383:14:383:17 | cond | main.rs:383:14:383:17 | [SSA] cond | -| main.rs:383:28:383:41 | { ... } | main.rs:383:20:383:52 | if cond {...} else {...} | -| main.rs:383:30:383:39 | source(...) | main.rs:383:28:383:41 | { ... } | -| main.rs:383:48:383:52 | { ... } | main.rs:383:20:383:52 | if cond {...} else {...} | -| main.rs:383:50:383:50 | 0 | main.rs:383:48:383:52 | { ... } | -| main.rs:388:9:388:9 | [SSA] f | main.rs:395:5:395:5 | f | -| main.rs:388:9:388:9 | f | main.rs:388:9:388:9 | [SSA] f | -| main.rs:388:13:393:9 | \|...\| ... | main.rs:388:9:388:9 | f | -| main.rs:388:14:388:17 | ... | main.rs:388:14:388:17 | cond | -| main.rs:388:14:388:17 | [SSA] cond | main.rs:389:12:389:15 | cond | -| main.rs:388:14:388:17 | cond | main.rs:388:14:388:17 | [SSA] cond | -| main.rs:388:20:388:23 | ... | main.rs:388:20:388:23 | data | -| main.rs:388:20:388:23 | [SSA] data | main.rs:390:18:390:21 | data | -| main.rs:388:20:388:23 | data | main.rs:388:20:388:23 | [SSA] data | -| main.rs:389:17:391:9 | { ... } | main.rs:389:9:393:9 | if cond {...} else {...} | -| main.rs:391:16:393:9 | { ... } | main.rs:389:9:393:9 | if cond {...} else {...} | -| main.rs:392:13:392:19 | sink(...) | main.rs:391:16:393:9 | { ... } | -| main.rs:394:9:394:9 | [SSA] a | main.rs:395:13:395:13 | a | -| main.rs:394:9:394:9 | a | main.rs:394:9:394:9 | [SSA] a | -| main.rs:394:13:394:22 | source(...) | main.rs:394:9:394:9 | a | -| main.rs:399:9:399:9 | [SSA] f | main.rs:406:13:406:13 | f | -| main.rs:399:9:399:9 | f | main.rs:399:9:399:9 | [SSA] f | -| main.rs:399:13:404:9 | \|...\| ... | main.rs:399:9:399:9 | f | -| main.rs:399:14:399:17 | ... | main.rs:399:14:399:17 | cond | -| main.rs:399:14:399:17 | [SSA] cond | main.rs:400:12:400:15 | cond | -| main.rs:399:14:399:17 | cond | main.rs:399:14:399:17 | [SSA] cond | -| main.rs:399:20:399:23 | ... | main.rs:399:20:399:23 | data | -| main.rs:399:20:399:23 | [SSA] data | main.rs:401:13:401:16 | data | -| main.rs:399:20:399:23 | data | main.rs:399:20:399:23 | [SSA] data | -| main.rs:400:17:402:9 | { ... } | main.rs:400:9:404:9 | if cond {...} else {...} | -| main.rs:401:13:401:16 | data | main.rs:400:17:402:9 | { ... } | -| main.rs:402:16:404:9 | { ... } | main.rs:400:9:404:9 | if cond {...} else {...} | -| main.rs:403:13:403:13 | 0 | main.rs:402:16:404:9 | { ... } | -| main.rs:405:9:405:9 | [SSA] a | main.rs:406:21:406:21 | a | -| main.rs:405:9:405:9 | a | main.rs:405:9:405:9 | [SSA] a | -| main.rs:405:13:405:22 | source(...) | main.rs:405:9:405:9 | a | -| main.rs:406:9:406:9 | [SSA] b | main.rs:407:10:407:10 | b | -| main.rs:406:9:406:9 | b | main.rs:406:9:406:9 | [SSA] b | -| main.rs:406:13:406:22 | f(...) | main.rs:406:9:406:9 | b | -| main.rs:431:13:431:33 | result_questionmark(...) | main.rs:431:9:431:9 | _ | +| main.rs:386:9:386:9 | a | main.rs:386:9:386:9 | [SSA] a | +| main.rs:386:13:386:22 | source(...) | main.rs:386:9:386:9 | a | +| main.rs:387:9:387:9 | [SSA] b | main.rs:388:14:388:14 | b | +| main.rs:387:9:387:9 | b | main.rs:387:9:387:9 | [SSA] b | +| main.rs:387:13:387:14 | &a | main.rs:387:9:387:9 | b | +| main.rs:388:9:388:9 | [SSA] c | main.rs:389:10:389:10 | c | +| main.rs:388:9:388:9 | c | main.rs:388:9:388:9 | [SSA] c | +| main.rs:388:13:388:14 | * ... | main.rs:388:9:388:9 | c | +| main.rs:393:17:393:17 | 1 | main.rs:393:9:393:13 | a | +| main.rs:395:9:395:9 | [SSA] b | main.rs:396:6:396:6 | b | +| main.rs:395:9:395:9 | b | main.rs:395:9:395:9 | [SSA] b | +| main.rs:395:13:395:18 | &mut a | main.rs:395:9:395:9 | b | +| main.rs:396:10:396:19 | source(...) | main.rs:396:5:396:6 | * ... | +| main.rs:421:13:421:33 | result_questionmark(...) | main.rs:421:9:421:9 | _ | storeStep | main.rs:94:14:94:22 | source(...) | tuple.0 | main.rs:94:13:94:26 | TupleExpr | | main.rs:94:25:94:25 | 2 | tuple.1 | main.rs:94:13:94:26 | TupleExpr | @@ -540,7 +509,7 @@ storeStep | main.rs:373:27:373:27 | 2 | array[] | main.rs:373:23:373:31 | [...] | | main.rs:373:30:373:30 | 3 | array[] | main.rs:373:23:373:31 | [...] | | main.rs:376:18:376:27 | source(...) | array[] | main.rs:376:5:376:11 | [post] mut_arr | -| main.rs:414:27:414:27 | 0 | Some | main.rs:414:22:414:28 | Some(...) | +| main.rs:404:27:404:27 | 0 | Some | main.rs:404:22:404:28 | Some(...) | readStep | file://:0:0:0:0 | [summary param] self in lang:core::_::::unwrap | Some | file://:0:0:0:0 | [summary] read: Argument[self].Variant[crate::option::Option::Some(0)] in lang:core::_::::unwrap | | main.rs:33:9:33:15 | Some(...) | Some | main.rs:33:14:33:14 | _ | 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 d464f5625817..185bea1394fb 100644 --- a/rust/ql/test/library-tests/dataflow/local/inline-flow.expected +++ b/rust/ql/test/library-tests/dataflow/local/inline-flow.expected @@ -99,16 +99,6 @@ edges | main.rs:377:13:377:19 | mut_arr [array[]] | main.rs:377:13:377:22 | mut_arr[1] | provenance | | | main.rs:377:13:377:22 | mut_arr[1] | main.rs:378:10:378:10 | d | provenance | | | main.rs:379:10:379:16 | mut_arr [array[]] | main.rs:379:10:379:19 | mut_arr[0] | provenance | | -| main.rs:383:20:383:52 | if cond {...} else {...} | main.rs:384:10:384:16 | f(...) | provenance | | -| main.rs:383:30:383:39 | source(...) | main.rs:383:20:383:52 | if cond {...} else {...} | provenance | | -| main.rs:388:20:388:23 | ... | main.rs:390:18:390:21 | data | provenance | | -| main.rs:394:13:394:22 | source(...) | main.rs:395:13:395:13 | a | provenance | | -| main.rs:395:13:395:13 | a | main.rs:388:20:388:23 | ... | provenance | | -| main.rs:399:20:399:23 | ... | main.rs:400:9:404:9 | if cond {...} else {...} | provenance | | -| main.rs:405:13:405:22 | source(...) | main.rs:406:21:406:21 | a | provenance | | -| main.rs:406:13:406:22 | f(...) | main.rs:407:10:407:10 | b | provenance | | -| main.rs:406:21:406:21 | a | main.rs:399:20:399:23 | ... | provenance | | -| main.rs:406:21:406:21 | a | main.rs:406:13:406:22 | f(...) | provenance | | nodes | main.rs:15:10:15:18 | source(...) | semmle.label | source(...) | | main.rs:19:13:19:21 | source(...) | semmle.label | source(...) | @@ -233,21 +223,7 @@ nodes | main.rs:378:10:378:10 | d | semmle.label | d | | main.rs:379:10:379:16 | mut_arr [array[]] | semmle.label | mut_arr [array[]] | | main.rs:379:10:379:19 | mut_arr[0] | semmle.label | mut_arr[0] | -| main.rs:383:20:383:52 | if cond {...} else {...} | semmle.label | if cond {...} else {...} | -| main.rs:383:30:383:39 | source(...) | semmle.label | source(...) | -| main.rs:384:10:384:16 | f(...) | semmle.label | f(...) | -| main.rs:388:20:388:23 | ... | semmle.label | ... | -| main.rs:390:18:390:21 | data | semmle.label | data | -| main.rs:394:13:394:22 | source(...) | semmle.label | source(...) | -| main.rs:395:13:395:13 | a | semmle.label | a | -| main.rs:399:20:399:23 | ... | semmle.label | ... | -| main.rs:400:9:404:9 | if cond {...} else {...} | semmle.label | if cond {...} else {...} | -| main.rs:405:13:405:22 | source(...) | semmle.label | source(...) | -| main.rs:406:13:406:22 | f(...) | semmle.label | f(...) | -| main.rs:406:21:406:21 | a | semmle.label | a | -| main.rs:407:10:407:10 | b | semmle.label | b | subpaths -| main.rs:406:21:406:21 | a | main.rs:399:20:399:23 | ... | main.rs:400:9:404:9 | if cond {...} else {...} | main.rs:406:13:406:22 | f(...) | testFailures #select | main.rs:15:10:15:18 | source(...) | main.rs:15:10:15:18 | source(...) | main.rs:15:10:15:18 | source(...) | $@ | main.rs:15:10:15:18 | source(...) | source(...) | @@ -282,6 +258,3 @@ testFailures | main.rs:367:18:367:18 | c | main.rs:362:23:362:32 | source(...) | main.rs:367:18:367:18 | c | $@ | main.rs:362:23:362:32 | source(...) | source(...) | | main.rs:378:10:378:10 | d | main.rs:376:18:376:27 | source(...) | main.rs:378:10:378:10 | d | $@ | main.rs:376:18:376:27 | source(...) | source(...) | | main.rs:379:10:379:19 | mut_arr[0] | main.rs:376:18:376:27 | source(...) | main.rs:379:10:379:19 | mut_arr[0] | $@ | main.rs:376:18:376:27 | source(...) | source(...) | -| main.rs:384:10:384:16 | f(...) | main.rs:383:30:383:39 | source(...) | main.rs:384:10:384:16 | f(...) | $@ | main.rs:383:30:383:39 | source(...) | source(...) | -| main.rs:390:18:390:21 | data | main.rs:394:13:394:22 | source(...) | main.rs:390:18:390:21 | data | $@ | main.rs:394:13:394:22 | source(...) | source(...) | -| main.rs:407:10:407:10 | b | main.rs:405:13:405:22 | source(...) | main.rs:407:10:407:10 | b | $@ | main.rs:405:13:405:22 | 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 0dbc48732f6f..819f68bc0498 100644 --- a/rust/ql/test/library-tests/dataflow/local/main.rs +++ b/rust/ql/test/library-tests/dataflow/local/main.rs @@ -379,32 +379,22 @@ fn array_assignment() { sink(mut_arr[0]); // $ SPURIOUS: hasValueFlow=55 } -fn closure_flow_out() { - let f = |cond| if cond { source(92) } else { 0 }; - sink(f(true)); // $ hasValueFlow=92 -} +// ----------------------------------------------------------------------------- +// Data flow through mutable borrows -fn closure_flow_in() { - let f = |cond, data| - if cond { - sink(data); // $ hasValueFlow=87 - } else { - sink(0) - }; - let a = source(87); - f(true, a); +fn read_through_borrow() { + let a = source(21); + let b = &a; + let c = *b; + sink(c); // $ MISSING: hasValueFlow=21 } -fn closure_flow_through() { - let f = |cond, data| - if cond { - data - } else { - 0 - }; - let a = source(43); - let b = f(true, a); - sink(b); // $ hasValueFlow=43 +fn write_through_borrow() { + let mut a = 1; + sink(a); + let b = &mut a; + *b = source(39); + sink(a); // $ MISSING: hasValueFlow=39 } fn main() { @@ -440,7 +430,6 @@ fn main() { array_for_loop(); array_slice_pattern(); array_assignment(); - closure_flow_out(); - closure_flow_in(); - closure_flow_through(); + read_through_borrow(); + write_through_borrow(); } From 94b037fad1b3f44c59650164dbda2d1994d63aac Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Wed, 11 Dec 2024 15:01:47 +0100 Subject: [PATCH 2/9] Rust: Instantiate variable capture library for data flow --- rust/ql/lib/codeql/rust/dataflow/Ssa.qll | 60 +++- .../rust/dataflow/internal/DataFlowImpl.qll | 287 +++++++++++++++++- .../codeql/rust/dataflow/internal/SsaImpl.qll | 2 +- .../dataflow/closures/inline-flow.expected | 9 + .../library-tests/dataflow/closures/main.rs | 4 +- .../test/library-tests/variables/Ssa.expected | 60 +++- 6 files changed, 387 insertions(+), 35 deletions(-) diff --git a/rust/ql/lib/codeql/rust/dataflow/Ssa.qll b/rust/ql/lib/codeql/rust/dataflow/Ssa.qll index 5b452a329fa9..8fe460ce8ae3 100644 --- a/rust/ql/lib/codeql/rust/dataflow/Ssa.qll +++ b/rust/ql/lib/codeql/rust/dataflow/Ssa.qll @@ -210,23 +210,22 @@ module Ssa { final CfgNode getWriteAccess() { result = write } /** - * Holds if this SSA definition assigns `value` to the underlying variable. + * Holds if this SSA definition assigns `value` to the underlying + * variable. * - * This is either a direct assignment, `x = value`, or an assignment via - * simple pattern matching - * - * ```rb - * case value - * in Foo => x then ... - * in y => then ... - * end - * ``` + * This is either the value in a direct assignment, `x = value`, or in a + * `let` statement, `let x = value`. Note that patterns on the rhs. are + * currently not supported. */ predicate assigns(ExprCfgNode value) { - exists(AssignmentExprCfgNode ae, BasicBlock bb, int i | - this.definesAt(_, bb, i) and - ae.getLhs() = bb.getNode(i) and - value = ae.getRhs() + exists(AssignmentExprCfgNode ae | + ae.getLhs() = write and + ae.getRhs() = value + ) + or + exists(LetStmtCfgNode ls | + ls.getPat() = write and + ls.getInitializer() = value ) } @@ -338,4 +337,37 @@ module Ssa { override Location getLocation() { result = this.getBasicBlock().getLocation() } } + + /** + * An SSA definition inserted at a call that may update the value of a captured + * variable. For example, in + * + * ```rb + * fn capture_mut() { + * let mut y = 0; + * (0..5).for_each(|| { + * y += x + * }); + * y + * } + * ``` + * + * a definition for `y` is inserted at the call to `for_each`. + */ + class CapturedCallDefinition extends Definition, SsaImpl::UncertainWriteDefinition { + CapturedCallDefinition() { + exists(Variable v, BasicBlock bb, int i | + this.definesAt(v, bb, i) and + SsaImpl::capturedCallWrite(_, bb, i, v) + ) + } + + /** + * Gets the immediately preceding definition. Since this update is uncertain, + * the value from the preceding definition might still be valid. + */ + final Definition getPriorDefinition() { result = SsaImpl::uncertainWriteDefinitionInput(this) } + + override string toString() { result = " " + this.getSourceVariable() } + } } diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index 068429ce09b0..098056773190 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -101,11 +101,19 @@ final class ParameterPosition extends TParameterPosition { /** Holds if this position represents the `self` position. */ predicate isSelf() { this = TSelfParameterPosition() } + /** + * Holds if this position represents a reference to a lambda itself. Only + * used for tracking flow through captured variables. + */ + predicate isLambdaSelf() { this = TLambdaSelfParameterPosition() } + /** Gets a textual representation of this position. */ string toString() { result = this.getPosition().toString() or result = "self" and this.isSelf() + or + result = "lambda self" and this.isLambdaSelf() } ParamBase getParameterIn(ParamList ps) { @@ -264,6 +272,26 @@ module Node { } } + /** + * The run-time representation of a closure itself at function entry, viewed + * as a node in a data flow graph. + */ + final class ClosureParameterNode extends ParameterNode, TLambdaSelfReferenceNode { + private CfgScope cfgScope; + + ClosureParameterNode() { this = TLambdaSelfReferenceNode(cfgScope) } + + final override CfgScope getCfgScope() { result = cfgScope } + + override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) { + cfgScope = c.asCfgScope() and pos.isLambdaSelf() + } + + override Location getLocation() { result = cfgScope.getLocation() } + + override string toString() { result = "lambda self in " + cfgScope } + } + abstract class ArgumentNode extends Node { abstract predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos); } @@ -292,6 +320,21 @@ module Node { } } + /** + * A data flow node that represents the run-time representation of a closure + * passed into the closure body at an invocation. + */ + final class ClosureArgumentNode extends ArgumentNode, ExprNode { + private CallExprCfgNode call_; + + ClosureArgumentNode() { lambdaCallExpr(call_, _, this.asExpr()) } + + override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) { + call.asCallExprCfgNode() = call_ and + pos.isLambdaSelf() + } + } + /** An SSA node. */ class SsaNode extends Node, TSsaNode { SsaImpl::DataFlowIntegration::SsaNode node; @@ -402,6 +445,37 @@ module Node { final override string toString() { result = PostUpdateNode.super.toString() } } + private class CapturePostUpdateNode extends PostUpdateNode, CaptureNode { + private CaptureNode pre; + + CapturePostUpdateNode() { + VariableCapture::Flow::capturePostUpdateNode(this.getSynthesizedCaptureNode(), + pre.getSynthesizedCaptureNode()) + } + + override Node getPreUpdateNode() { result = pre } + + final override string toString() { result = PostUpdateNode.super.toString() } + } + + /** + * A synthesized data flow node representing a closure object that tracks + * captured variables. + */ + class CaptureNode extends Node, TCaptureNode { + private VariableCapture::Flow::SynthesizedCaptureNode cn; + + CaptureNode() { this = TCaptureNode(cn) } + + VariableCapture::Flow::SynthesizedCaptureNode getSynthesizedCaptureNode() { result = cn } + + override CfgScope getCfgScope() { result = cn.getEnclosingCallable() } + + override Location getLocation() { result = cn.getLocation() } + + override string toString() { result = cn.toString() } + } + final class CastNode = NaNode; } @@ -625,6 +699,18 @@ private class StructFieldContent extends Content, TStructFieldContent { override string toString() { result = s.toString() + "." + field_.toString() } } +/** A captured variable. */ +private class CapturedVariableContent extends Content, TCapturedVariableContent { + private Variable v; + + CapturedVariableContent() { this = TCapturedVariableContent(v) } + + /** Gets the captured variable. */ + Variable getVariable() { result = v } + + override string toString() { result = "captured " + v } +} + /** * An element in an array. */ @@ -681,6 +767,26 @@ final class SingletonContentSet extends ContentSet, TSingletonContentSet { override Content getAReadContent() { result = c } } +class LambdaCallKind = Unit; + +/** Holds if `creation` is an expression that creates a lambda of kind `kind` for `c`. */ +private predicate lambdaCreationExpr(Expr creation, LambdaCallKind kind) { + creation instanceof ClosureExpr and exists(kind) +} + +/** + * Holds if `call` is a lambda call of kind `kind` where `receiver` is the + * invoked expression. + */ +predicate lambdaCallExpr(CallExprCfgNode call, LambdaCallKind kind, ExprCfgNode receiver) { + receiver = call.getFunction() and + // All calls to complex expressions and local variable accesses are lambda call. + exists(Expr f | f = receiver.getExpr() | + f instanceof PathExpr implies f = any(Variable v).getAnAccess() + ) and + exists(kind) +} + // Defines a set of aliases needed for the `RustDataFlow` module private module Aliases { class DataFlowCallableAlias = DataFlowCallable; @@ -694,6 +800,8 @@ private module Aliases { class ContentAlias = Content; class ContentSetAlias = ContentSet; + + class LambdaCallKindAlias = LambdaCallKind; } module RustDataFlow implements InputSig { @@ -735,6 +843,12 @@ module RustDataFlow implements InputSig { node instanceof Node::SsaNode or node instanceof Node::FlowSummaryNode + or + node instanceof Node::CaptureNode + or + node instanceof Node::ClosureParameterNode + or + node instanceof Node::ClosureArgumentNode } class DataFlowExpr = ExprCfgNode; @@ -775,6 +889,8 @@ module RustDataFlow implements InputSig { class ContentSet = ContentSetAlias; + class LambdaCallKind = LambdaCallKindAlias; + predicate forceHighPrecision(Content c) { none() } final class ContentApprox = Content; // TODO: Implement if needed @@ -799,12 +915,17 @@ module RustDataFlow implements InputSig { ( LocalFlow::localFlowStepCommon(nodeFrom, nodeTo) or - exists(boolean isUseStep | SsaFlow::localFlowStep(_, nodeFrom, nodeTo, isUseStep) | + exists(SsaImpl::DefinitionExt def, boolean isUseStep | + SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep) and + not def instanceof VariableCapture::CapturedSsaDefinitionExt + | isUseStep = false or isUseStep = true and not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _) ) + or + VariableCapture::localFlowStep(nodeFrom, nodeTo) ) and model = "" or @@ -914,6 +1035,8 @@ module RustDataFlow implements InputSig { c.(VariantPositionContent).getVariantCanonicalPath(0).getExtendedCanonicalPath() = ["crate::option::Option::Some", "crate::result::Result::Ok"] ) + or + VariableCapture::readStep(node1, c, node2) ) or FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(Node::FlowSummaryNode).getSummaryNode(), @@ -995,6 +1118,8 @@ module RustDataFlow implements InputSig { node1.asExpr() = assignment.getRhs() and node2.(PostUpdateNode).getPreUpdateNode().asExpr() = index.getBase() ) + or + VariableCapture::storeStep(node1, c, node2) ) or FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(Node::FlowSummaryNode).getSummaryNode(), @@ -1011,6 +1136,8 @@ module RustDataFlow implements InputSig { or FlowSummaryImpl::Private::Steps::summaryClearsContent(n.(Node::FlowSummaryNode).getSummaryNode(), cs) + or + VariableCapture::clearsContent(n, cs.(SingletonContentSet).getContent()) } /** @@ -1045,6 +1172,9 @@ module RustDataFlow implements InputSig { p.isParameterOf(c, pos) and FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(c.asLibraryCallable(), pos) ) + or + VariableCapture::Flow::heuristicAllowInstanceParameterReturnInSelf(p.(Node::ClosureParameterNode) + .getCfgScope()) } /** @@ -1064,15 +1194,11 @@ module RustDataFlow implements InputSig { .getSummaryNode(), node2.(Node::FlowSummaryNode).getSummaryNode()) } - class LambdaCallKind = Unit; - /** Holds if `creation` is an expression that creates a lambda of kind `kind` for `c`. */ predicate lambdaCreation(Node creation, LambdaCallKind kind, DataFlowCallable c) { - exists(ClosureExpr cl | - cl = creation.asExpr().getExpr() and - cl = c.asCfgScope() - ) and - exists(kind) + exists(Expr e | + e = creation.asExpr().getExpr() and lambdaCreationExpr(e, kind) and e = c.asCfgScope() + ) } /** @@ -1098,6 +1224,141 @@ module RustDataFlow implements InputSig { class DataFlowSecondLevelScope = Void; } +/** Provides logic related to captured variables. */ +module VariableCapture { + private import codeql.dataflow.VariableCapture as SharedVariableCapture + + private predicate closureFlowStep(ExprCfgNode e1, ExprCfgNode e2) { + e1 = getALastEvalNode(e2) + or + exists(Ssa::Definition def | + def.getARead() = e2 and + def.getAnUltimateDefinition().(Ssa::WriteDefinition).assigns(e1) + ) + } + + private module CaptureInput implements SharedVariableCapture::InputSig { + private import rust as Ast + private import codeql.rust.controlflow.BasicBlocks as BasicBlocks + private import codeql.rust.elements.Variable as Variable + + class BasicBlock extends BasicBlocks::BasicBlock { + Callable getEnclosingCallable() { result = this.getScope() } + } + + class ControlFlowNode = CfgNode; + + BasicBlock getImmediateBasicBlockDominator(BasicBlock bb) { + result = bb.getImmediateDominator() + } + + BasicBlock getABasicBlockSuccessor(BasicBlock bb) { result = bb.getASuccessor() } + + class CapturedVariable extends Variable { + CapturedVariable() { this.isCaptured() } + + Callable getCallable() { result = this.getEnclosingCfgScope() } + } + + final class CapturedParameter extends CapturedVariable { + ParamBase p; + + CapturedParameter() { p = this.getParameter() } + + Node::SourceParameterNode getParameterNode() { result.getParameter().getParamBase() = p } + } + + class Expr extends CfgNode { + predicate hasCfgNode(BasicBlock bb, int i) { this = bb.getNode(i) } + } + + class VariableWrite extends Expr { + ExprCfgNode source; + CapturedVariable v; + + VariableWrite() { + exists(AssignmentExprCfgNode assign, Variable::VariableWriteAccess write | + this = assign and + v = write.getVariable() and + assign.getLhs().getExpr() = write and + assign.getRhs() = source + ) + or + exists(LetStmtCfgNode ls | + this = ls and + v.getPat() = ls.getPat().getPat() and + ls.getInitializer() = source + ) + } + + CapturedVariable getVariable() { result = v } + + ExprCfgNode getSource() { result = source } + } + + class VariableRead extends Expr instanceof ExprCfgNode { + CapturedVariable v; + + VariableRead() { + exists(VariableReadAccess read | this.getExpr() = read and v = read.getVariable()) + } + + CapturedVariable getVariable() { result = v } + } + + class ClosureExpr extends Expr instanceof ExprCfgNode { + ClosureExpr() { lambdaCreationExpr(super.getExpr(), _) } + + predicate hasBody(Callable body) { body = super.getExpr() } + + predicate hasAliasedAccess(Expr f) { closureFlowStep+(this, f) and not closureFlowStep(f, _) } + } + + class Callable extends CfgScope { + predicate isConstructor() { none() } + } + } + + class CapturedVariable = CaptureInput::CapturedVariable; + + module Flow = SharedVariableCapture::Flow; + + private Flow::ClosureNode asClosureNode(Node n) { + result = n.(Node::CaptureNode).getSynthesizedCaptureNode() + or + result.(Flow::ExprNode).getExpr() = n.asExpr() + or + result.(Flow::VariableWriteSourceNode).getVariableWrite().getSource() = n.asExpr() + or + result.(Flow::ExprPostUpdateNode).getExpr() = + n.(Node::PostUpdateNode).getPreUpdateNode().asExpr() + or + result.(Flow::ParameterNode).getParameter().getParameterNode() = n + or + result.(Flow::ThisParameterNode).getCallable() = n.(Node::ClosureParameterNode).getCfgScope() + } + + predicate storeStep(Node node1, CapturedVariableContent c, Node node2) { + Flow::storeStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2)) + } + + predicate readStep(Node node1, CapturedVariableContent c, Node node2) { + Flow::readStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2)) + } + + predicate localFlowStep(Node node1, Node node2) { + Flow::localFlowStep(asClosureNode(node1), asClosureNode(node2)) + } + + predicate clearsContent(Node node, CapturedVariableContent c) { + Flow::clearsContent(asClosureNode(node), c.getVariable()) + } + + class CapturedSsaDefinitionExt extends SsaImpl::DefinitionExt { + CapturedSsaDefinitionExt() { this.getSourceVariable() instanceof CapturedVariable } + } +} + import MakeImpl /** A collection of cached types and predicates to be evaluated in the same stage. */ @@ -1112,6 +1373,8 @@ private module Cached { TPatNode(PatCfgNode p) or TExprPostUpdateNode(ExprCfgNode e) { isArgumentForCall(e, _, _) or + lambdaCallExpr(_, _, e) or + lambdaCreationExpr(e.getExpr(), _) or e = [ any(IndexExprCfgNode i).getBase(), any(FieldExprCfgNode access).getExpr(), @@ -1119,7 +1382,9 @@ private module Cached { ] } or TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node) or - TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) + TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or + TLambdaSelfReferenceNode(CfgScope c) { lambdaCreationExpr(c, _) } or + TCaptureNode(VariableCapture::Flow::SynthesizedCaptureNode cn) cached newtype TDataFlowCall = @@ -1156,6 +1421,7 @@ private module Cached { or FlowSummaryImpl::ParsePositions::isParsedParameterPosition(_, i) } or + TLambdaSelfParameterPosition() or TSelfParameterPosition() cached @@ -1211,7 +1477,8 @@ private module Cached { } or TStructFieldContent(StructCanonicalPath s, string field) { field = s.getStruct().getFieldList().(RecordFieldList).getAField().getName().getText() - } + } or + TCapturedVariableContent(VariableCapture::CapturedVariable v) cached newtype TContentSet = TSingletonContentSet(Content c) diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll index 64e2d54722d8..75b52b16c707 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll @@ -335,7 +335,7 @@ private module Cached { /** * Holds if `v` is written at index `i` in basic block `bb`, and the corresponding - * AST write access is `write`. + * write access node in the CFG is `write`. */ cached predicate variableWriteActual(BasicBlock bb, int i, Variable v, CfgNode write) { diff --git a/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected b/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected index 316642d60364..166cdae7d36f 100644 --- a/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected +++ b/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected @@ -10,6 +10,9 @@ edges | main.rs:34:13:34:22 | f(...) | main.rs:35:10:35:10 | b | provenance | | | main.rs:34:21:34:21 | a | main.rs:27:20:27:23 | ... | provenance | | | main.rs:34:21:34:21 | a | main.rs:34:13:34:22 | f(...) | provenance | | +| main.rs:42:16:42:25 | source(...) | main.rs:44:5:44:5 | [post] f [captured capt] | provenance | | +| main.rs:44:5:44:5 | [post] f [captured capt] | main.rs:45:10:45:13 | capt | provenance | | +| main.rs:44:5:44:5 | [post] f [captured capt] | main.rs:47:14:47:17 | capt | provenance | | nodes | main.rs:11:20:11:52 | if cond {...} else {...} | semmle.label | if cond {...} else {...} | | main.rs:11:30:11:39 | source(...) | semmle.label | source(...) | @@ -24,6 +27,10 @@ nodes | main.rs:34:13:34:22 | f(...) | semmle.label | f(...) | | main.rs:34:21:34:21 | a | semmle.label | a | | main.rs:35:10:35:10 | b | semmle.label | b | +| main.rs:42:16:42:25 | source(...) | semmle.label | source(...) | +| main.rs:44:5:44:5 | [post] f [captured capt] | semmle.label | [post] f [captured capt] | +| main.rs:45:10:45:13 | capt | semmle.label | capt | +| main.rs:47:14:47:17 | capt | semmle.label | capt | subpaths | main.rs:34:21:34:21 | a | main.rs:27:20:27:23 | ... | main.rs:28:9:32:9 | if cond {...} else {...} | main.rs:34:13:34:22 | f(...) | testFailures @@ -31,3 +38,5 @@ testFailures | main.rs:12:10:12:16 | f(...) | main.rs:11:30:11:39 | source(...) | main.rs:12:10:12:16 | f(...) | $@ | main.rs:11:30:11:39 | source(...) | source(...) | | main.rs:18:18:18:21 | data | main.rs:22:13:22:22 | source(...) | main.rs:18:18:18:21 | data | $@ | main.rs:22:13:22:22 | source(...) | source(...) | | main.rs:35:10:35:10 | b | main.rs:33:13:33:22 | source(...) | main.rs:35:10:35:10 | b | $@ | main.rs:33:13:33:22 | source(...) | source(...) | +| main.rs:45:10:45:13 | capt | main.rs:42:16:42:25 | source(...) | main.rs:45:10:45:13 | capt | $@ | main.rs:42:16:42:25 | source(...) | source(...) | +| main.rs:47:14:47:17 | capt | main.rs:42:16:42:25 | source(...) | main.rs:47:14:47:17 | capt | $@ | main.rs:42:16:42:25 | source(...) | source(...) | diff --git a/rust/ql/test/library-tests/dataflow/closures/main.rs b/rust/ql/test/library-tests/dataflow/closures/main.rs index d5a67525119c..66ce59a3b041 100644 --- a/rust/ql/test/library-tests/dataflow/closures/main.rs +++ b/rust/ql/test/library-tests/dataflow/closures/main.rs @@ -42,9 +42,9 @@ fn closure_captured_variable() { capt = source(73); }; f(); - sink(capt); // $ MISSING: hasValueFlow=73 + sink(capt); // $ hasValueFlow=73 let g = || { - sink(capt); // $ MISSING: hasValueFlow=73 + sink(capt); // $ hasValueFlow=73 }; g(); } diff --git a/rust/ql/test/library-tests/variables/Ssa.expected b/rust/ql/test/library-tests/variables/Ssa.expected index e126ca45c3a2..13e7f1e4ce38 100644 --- a/rust/ql/test/library-tests/variables/Ssa.expected +++ b/rust/ql/test/library-tests/variables/Ssa.expected @@ -120,12 +120,12 @@ definition | variables.rs:418:9:418:13 | y | variables.rs:418:13:418:13 | y | | variables.rs:420:9:420:20 | closure2 | variables.rs:420:13:420:20 | closure2 | | variables.rs:421:9:421:9 | y | variables.rs:418:13:418:13 | y | -| variables.rs:423:5:423:14 | closure2(...) | variables.rs:418:13:418:13 | y | +| variables.rs:423:5:423:14 | y | variables.rs:418:13:418:13 | y | | variables.rs:428:9:428:20 | closure3 | variables.rs:428:13:428:20 | closure3 | | variables.rs:436:9:436:13 | i | variables.rs:436:13:436:13 | i | | variables.rs:437:9:437:13 | block | variables.rs:437:9:437:13 | block | | variables.rs:438:9:438:9 | i | variables.rs:436:13:436:13 | i | -| variables.rs:441:5:441:15 | await block | variables.rs:436:13:436:13 | i | +| variables.rs:441:5:441:15 | i | variables.rs:436:13:436:13 | i | | variables.rs:445:8:445:8 | b | variables.rs:445:8:445:8 | b | | variables.rs:446:9:446:13 | x | variables.rs:446:13:446:13 | x | | variables.rs:449:5:457:5 | phi | variables.rs:446:13:446:13 | x | @@ -248,10 +248,10 @@ read | variables.rs:412:9:412:16 | closure1 | variables.rs:412:9:412:16 | closure1 | variables.rs:415:5:415:12 | closure1 | | variables.rs:412:20:414:5 | x | variables.rs:410:13:410:13 | x | variables.rs:413:19:413:19 | x | | variables.rs:420:9:420:20 | closure2 | variables.rs:420:13:420:20 | closure2 | variables.rs:423:5:423:12 | closure2 | -| variables.rs:423:5:423:14 | closure2(...) | variables.rs:418:13:418:13 | y | variables.rs:424:15:424:15 | y | +| variables.rs:423:5:423:14 | y | variables.rs:418:13:418:13 | y | variables.rs:424:15:424:15 | y | | variables.rs:428:9:428:20 | closure3 | variables.rs:428:13:428:20 | closure3 | variables.rs:431:5:431:12 | closure3 | | variables.rs:437:9:437:13 | block | variables.rs:437:9:437:13 | block | variables.rs:441:5:441:9 | block | -| variables.rs:441:5:441:15 | await block | variables.rs:436:13:436:13 | i | variables.rs:442:15:442:15 | i | +| variables.rs:441:5:441:15 | i | variables.rs:436:13:436:13 | i | variables.rs:442:15:442:15 | i | | variables.rs:445:8:445:8 | b | variables.rs:445:8:445:8 | b | variables.rs:449:8:449:8 | b | | variables.rs:446:9:446:13 | x | variables.rs:446:13:446:13 | x | variables.rs:447:15:447:15 | x | | variables.rs:446:9:446:13 | x | variables.rs:446:13:446:13 | x | variables.rs:448:15:448:15 | x | @@ -363,10 +363,10 @@ firstRead | variables.rs:412:9:412:16 | closure1 | variables.rs:412:9:412:16 | closure1 | variables.rs:415:5:415:12 | closure1 | | variables.rs:412:20:414:5 | x | variables.rs:410:13:410:13 | x | variables.rs:413:19:413:19 | x | | variables.rs:420:9:420:20 | closure2 | variables.rs:420:13:420:20 | closure2 | variables.rs:423:5:423:12 | closure2 | -| variables.rs:423:5:423:14 | closure2(...) | variables.rs:418:13:418:13 | y | variables.rs:424:15:424:15 | y | +| variables.rs:423:5:423:14 | y | variables.rs:418:13:418:13 | y | variables.rs:424:15:424:15 | y | | variables.rs:428:9:428:20 | closure3 | variables.rs:428:13:428:20 | closure3 | variables.rs:431:5:431:12 | closure3 | | variables.rs:437:9:437:13 | block | variables.rs:437:9:437:13 | block | variables.rs:441:5:441:9 | block | -| variables.rs:441:5:441:15 | await block | variables.rs:436:13:436:13 | i | variables.rs:442:15:442:15 | i | +| variables.rs:441:5:441:15 | i | variables.rs:436:13:436:13 | i | variables.rs:442:15:442:15 | i | | variables.rs:445:8:445:8 | b | variables.rs:445:8:445:8 | b | variables.rs:449:8:449:8 | b | | variables.rs:446:9:446:13 | x | variables.rs:446:13:446:13 | x | variables.rs:447:15:447:15 | x | | variables.rs:449:5:457:5 | phi | variables.rs:446:13:446:13 | x | variables.rs:458:15:458:15 | x | @@ -471,10 +471,10 @@ lastRead | variables.rs:412:9:412:16 | closure1 | variables.rs:412:9:412:16 | closure1 | variables.rs:415:5:415:12 | closure1 | | variables.rs:412:20:414:5 | x | variables.rs:410:13:410:13 | x | variables.rs:413:19:413:19 | x | | variables.rs:420:9:420:20 | closure2 | variables.rs:420:13:420:20 | closure2 | variables.rs:423:5:423:12 | closure2 | -| variables.rs:423:5:423:14 | closure2(...) | variables.rs:418:13:418:13 | y | variables.rs:424:15:424:15 | y | +| variables.rs:423:5:423:14 | y | variables.rs:418:13:418:13 | y | variables.rs:424:15:424:15 | y | | variables.rs:428:9:428:20 | closure3 | variables.rs:428:13:428:20 | closure3 | variables.rs:431:5:431:12 | closure3 | | variables.rs:437:9:437:13 | block | variables.rs:437:9:437:13 | block | variables.rs:441:5:441:9 | block | -| variables.rs:441:5:441:15 | await block | variables.rs:436:13:436:13 | i | variables.rs:442:15:442:15 | i | +| variables.rs:441:5:441:15 | i | variables.rs:436:13:436:13 | i | variables.rs:442:15:442:15 | i | | variables.rs:445:8:445:8 | b | variables.rs:445:8:445:8 | b | variables.rs:449:8:449:8 | b | | variables.rs:446:9:446:13 | x | variables.rs:446:13:446:13 | x | variables.rs:448:15:448:15 | x | | variables.rs:449:5:457:5 | phi | variables.rs:446:13:446:13 | x | variables.rs:458:15:458:15 | x | @@ -592,10 +592,54 @@ ultimateDef | variables.rs:449:5:457:5 | phi | variables.rs:450:9:450:9 | x | | variables.rs:449:5:457:5 | phi | variables.rs:454:9:454:9 | x | assigns +| variables.rs:16:9:16:10 | x1 | variables.rs:16:14:16:16 | "a" | +| variables.rs:21:9:21:14 | x2 | variables.rs:21:18:21:18 | 4 | | variables.rs:23:5:23:6 | x2 | variables.rs:23:10:23:10 | 5 | +| variables.rs:28:9:28:13 | x | variables.rs:28:17:28:17 | 1 | | variables.rs:30:5:30:5 | x | variables.rs:30:9:30:9 | 2 | +| variables.rs:35:9:35:10 | x3 | variables.rs:35:14:35:14 | 1 | +| variables.rs:37:9:37:10 | x3 | variables.rs:38:9:38:14 | ... + ... | +| variables.rs:43:9:43:10 | x4 | variables.rs:43:14:43:16 | "a" | +| variables.rs:46:13:46:14 | x4 | variables.rs:46:18:46:20 | "b" | +| variables.rs:75:9:75:10 | p1 | variables.rs:75:14:75:37 | Point {...} | +| variables.rs:85:9:85:10 | s1 | variables.rs:85:14:85:41 | Some(...) | +| variables.rs:102:9:102:10 | s1 | variables.rs:102:14:102:41 | Some(...) | +| variables.rs:111:9:111:10 | x6 | variables.rs:111:14:111:20 | Some(...) | +| variables.rs:112:9:112:10 | y1 | variables.rs:112:14:112:15 | 10 | +| variables.rs:128:9:128:15 | numbers | variables.rs:128:19:128:35 | TupleExpr | +| variables.rs:155:9:155:10 | p2 | variables.rs:155:14:155:37 | Point {...} | +| variables.rs:169:9:169:11 | msg | variables.rs:169:15:169:38 | ...::Hello {...} | +| variables.rs:189:9:189:14 | either | variables.rs:189:18:189:33 | ...::Left(...) | +| variables.rs:203:9:203:10 | tv | variables.rs:203:14:203:36 | ...::Second(...) | +| variables.rs:219:9:219:14 | either | variables.rs:219:18:219:33 | ...::Left(...) | +| variables.rs:229:9:229:14 | either | variables.rs:229:18:229:33 | ...::Left(...) | +| variables.rs:253:9:253:10 | fv | variables.rs:253:14:253:35 | ...::Second(...) | +| variables.rs:315:9:315:23 | example_closure | variables.rs:316:9:317:9 | \|...\| x | +| variables.rs:318:9:318:10 | n1 | variables.rs:319:9:319:26 | example_closure(...) | +| variables.rs:323:9:323:26 | immutable_variable | variables.rs:324:9:325:9 | \|...\| x | +| variables.rs:326:9:326:10 | n2 | variables.rs:327:9:327:29 | immutable_variable(...) | +| variables.rs:332:9:332:9 | v | variables.rs:332:13:332:41 | &... | +| variables.rs:350:9:350:13 | ref_i | variables.rs:351:9:351:14 | &mut i | +| variables.rs:373:9:373:9 | y | variables.rs:374:9:374:28 | mutate_param(...) | +| variables.rs:380:9:380:9 | w | variables.rs:381:9:381:19 | &mut ... | +| variables.rs:393:9:393:9 | y | variables.rs:394:9:394:14 | &mut x | +| variables.rs:400:9:400:9 | x | variables.rs:400:13:400:15 | 100 | +| variables.rs:402:9:402:11 | cap | variables.rs:402:15:404:5 | \|...\| ... | +| variables.rs:410:9:410:13 | x | variables.rs:410:17:410:17 | 1 | +| variables.rs:412:9:412:16 | closure1 | variables.rs:412:20:414:5 | \|...\| ... | +| variables.rs:418:9:418:13 | y | variables.rs:418:17:418:17 | 2 | +| variables.rs:420:9:420:20 | closure2 | variables.rs:420:24:422:5 | \|...\| ... | | variables.rs:421:9:421:9 | y | variables.rs:421:13:421:13 | 3 | +| variables.rs:428:9:428:20 | closure3 | variables.rs:428:24:430:5 | \|...\| ... | +| variables.rs:436:9:436:13 | i | variables.rs:436:22:436:22 | 0 | +| variables.rs:437:9:437:13 | block | variables.rs:437:17:439:5 | { ... } | | variables.rs:438:9:438:9 | i | variables.rs:438:13:438:13 | 1 | +| variables.rs:446:9:446:13 | x | variables.rs:446:17:446:17 | 1 | | variables.rs:450:9:450:9 | x | variables.rs:450:13:450:13 | 2 | | variables.rs:454:9:454:9 | x | variables.rs:454:13:454:13 | 3 | +| variables.rs:462:9:462:9 | x | variables.rs:462:13:462:13 | 1 | +| variables.rs:491:13:491:17 | f | variables.rs:491:21:494:9 | \|...\| ... | +| variables.rs:510:9:510:13 | a | variables.rs:510:17:510:25 | [...] | | variables.rs:514:5:514:5 | a | variables.rs:514:9:514:17 | [...] | +| variables.rs:519:9:519:9 | x | variables.rs:519:13:519:14 | 16 | +| variables.rs:523:9:523:9 | z | variables.rs:523:13:523:14 | 17 | From 59f3f1f1e995e6fe5cc81bc5ed99e5e741a46cb2 Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Thu, 12 Dec 2024 08:58:35 +0100 Subject: [PATCH 3/9] Apply suggestions from code review Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> --- rust/ql/lib/codeql/rust/dataflow/Ssa.qll | 2 +- rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/ql/lib/codeql/rust/dataflow/Ssa.qll b/rust/ql/lib/codeql/rust/dataflow/Ssa.qll index 8fe460ce8ae3..92ea1944b42d 100644 --- a/rust/ql/lib/codeql/rust/dataflow/Ssa.qll +++ b/rust/ql/lib/codeql/rust/dataflow/Ssa.qll @@ -342,7 +342,7 @@ module Ssa { * An SSA definition inserted at a call that may update the value of a captured * variable. For example, in * - * ```rb + * ```rust * fn capture_mut() { * let mut y = 0; * (0..5).for_each(|| { diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index 098056773190..ac4867fa424f 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -769,7 +769,7 @@ final class SingletonContentSet extends ContentSet, TSingletonContentSet { class LambdaCallKind = Unit; -/** Holds if `creation` is an expression that creates a lambda of kind `kind` for `c`. */ +/** Holds if `creation` is an expression that creates a lambda of kind `kind`. */ private predicate lambdaCreationExpr(Expr creation, LambdaCallKind kind) { creation instanceof ClosureExpr and exists(kind) } From 2cf043cfbcd1d57d34b761f5b17a88ffd7ec84c5 Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Thu, 12 Dec 2024 10:19:53 +0100 Subject: [PATCH 4/9] Rust: Address PR comments --- rust/ql/lib/codeql/rust/dataflow/Ssa.qll | 35 +----------- .../rust/dataflow/internal/DataFlowImpl.qll | 56 +++++++++---------- .../codeql/rust/dataflow/internal/SsaImpl.qll | 33 +++++++++++ 3 files changed, 61 insertions(+), 63 deletions(-) diff --git a/rust/ql/lib/codeql/rust/dataflow/Ssa.qll b/rust/ql/lib/codeql/rust/dataflow/Ssa.qll index 92ea1944b42d..ac9d71b13d79 100644 --- a/rust/ql/lib/codeql/rust/dataflow/Ssa.qll +++ b/rust/ql/lib/codeql/rust/dataflow/Ssa.qll @@ -214,7 +214,7 @@ module Ssa { * variable. * * This is either the value in a direct assignment, `x = value`, or in a - * `let` statement, `let x = value`. Note that patterns on the rhs. are + * `let` statement, `let x = value`. Note that patterns on the lhs. are * currently not supported. */ predicate assigns(ExprCfgNode value) { @@ -337,37 +337,4 @@ module Ssa { override Location getLocation() { result = this.getBasicBlock().getLocation() } } - - /** - * An SSA definition inserted at a call that may update the value of a captured - * variable. For example, in - * - * ```rust - * fn capture_mut() { - * let mut y = 0; - * (0..5).for_each(|| { - * y += x - * }); - * y - * } - * ``` - * - * a definition for `y` is inserted at the call to `for_each`. - */ - class CapturedCallDefinition extends Definition, SsaImpl::UncertainWriteDefinition { - CapturedCallDefinition() { - exists(Variable v, BasicBlock bb, int i | - this.definesAt(v, bb, i) and - SsaImpl::capturedCallWrite(_, bb, i, v) - ) - } - - /** - * Gets the immediately preceding definition. Since this update is uncertain, - * the value from the preceding definition might still be valid. - */ - final Definition getPriorDefinition() { result = SsaImpl::uncertainWriteDefinitionInput(this) } - - override string toString() { result = " " + this.getSourceVariable() } - } } diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index ac4867fa424f..12783cd3b71c 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -102,10 +102,10 @@ final class ParameterPosition extends TParameterPosition { predicate isSelf() { this = TSelfParameterPosition() } /** - * Holds if this position represents a reference to a lambda itself. Only + * Holds if this position represents a reference to a closure itself. Only * used for tracking flow through captured variables. */ - predicate isLambdaSelf() { this = TLambdaSelfParameterPosition() } + predicate isClosureSelf() { this = TClosureSelfParameterPosition() } /** Gets a textual representation of this position. */ string toString() { @@ -113,7 +113,7 @@ final class ParameterPosition extends TParameterPosition { or result = "self" and this.isSelf() or - result = "lambda self" and this.isLambdaSelf() + result = "closure self" and this.isClosureSelf() } ParamBase getParameterIn(ParamList ps) { @@ -276,15 +276,15 @@ module Node { * The run-time representation of a closure itself at function entry, viewed * as a node in a data flow graph. */ - final class ClosureParameterNode extends ParameterNode, TLambdaSelfReferenceNode { + final class ClosureParameterNode extends ParameterNode, TClosureSelfReferenceNode { private CfgScope cfgScope; - ClosureParameterNode() { this = TLambdaSelfReferenceNode(cfgScope) } + ClosureParameterNode() { this = TClosureSelfReferenceNode(cfgScope) } final override CfgScope getCfgScope() { result = cfgScope } override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) { - cfgScope = c.asCfgScope() and pos.isLambdaSelf() + cfgScope = c.asCfgScope() and pos.isClosureSelf() } override Location getLocation() { result = cfgScope.getLocation() } @@ -331,7 +331,7 @@ module Node { override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) { call.asCallExprCfgNode() = call_ and - pos.isLambdaSelf() + pos.isClosureSelf() } } @@ -403,6 +403,24 @@ module Node { override DataFlowCall getCall(ReturnKind kind) { result = call and kind = kind_ } } + /** + * A synthesized data flow node representing a closure object that tracks + * captured variables. + */ + class CaptureNode extends Node, TCaptureNode { + private VariableCapture::Flow::SynthesizedCaptureNode cn; + + CaptureNode() { this = TCaptureNode(cn) } + + VariableCapture::Flow::SynthesizedCaptureNode getSynthesizedCaptureNode() { result = cn } + + override CfgScope getCfgScope() { result = cn.getEnclosingCallable() } + + override Location getLocation() { result = cn.getLocation() } + + override string toString() { result = cn.toString() } + } + /** * A node associated with an object after an operation that might have * changed its state. @@ -458,24 +476,6 @@ module Node { final override string toString() { result = PostUpdateNode.super.toString() } } - /** - * A synthesized data flow node representing a closure object that tracks - * captured variables. - */ - class CaptureNode extends Node, TCaptureNode { - private VariableCapture::Flow::SynthesizedCaptureNode cn; - - CaptureNode() { this = TCaptureNode(cn) } - - VariableCapture::Flow::SynthesizedCaptureNode getSynthesizedCaptureNode() { result = cn } - - override CfgScope getCfgScope() { result = cn.getEnclosingCallable() } - - override Location getLocation() { result = cn.getLocation() } - - override string toString() { result = cn.toString() } - } - final class CastNode = NaNode; } @@ -847,8 +847,6 @@ module RustDataFlow implements InputSig { node instanceof Node::CaptureNode or node instanceof Node::ClosureParameterNode - or - node instanceof Node::ClosureArgumentNode } class DataFlowExpr = ExprCfgNode; @@ -1383,7 +1381,7 @@ private module Cached { } or TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node) or TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or - TLambdaSelfReferenceNode(CfgScope c) { lambdaCreationExpr(c, _) } or + TClosureSelfReferenceNode(CfgScope c) { lambdaCreationExpr(c, _) } or TCaptureNode(VariableCapture::Flow::SynthesizedCaptureNode cn) cached @@ -1421,7 +1419,7 @@ private module Cached { or FlowSummaryImpl::ParsePositions::isParsedParameterPosition(_, i) } or - TLambdaSelfParameterPosition() or + TClosureSelfParameterPosition() or TSelfParameterPosition() cached diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll index 75b52b16c707..e0ede89b9c4b 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll @@ -467,6 +467,39 @@ class PhiReadNode extends DefinitionExt, Impl::PhiReadNode { override Location getLocation() { result = Impl::PhiReadNode.super.getLocation() } } +/** + * An SSA definition inserted at a call that may update the value of a captured + * variable. For example, in + * + * ```rust + * fn capture_mut() { + * let mut y = 0; + * (0..5).for_each(|x| { + * y += x + * }); + * y + * } + * ``` + * + * a definition for `y` is inserted at the call to `for_each`. + */ +class CapturedCallDefinition extends Definition, Impl::UncertainWriteDefinition { + CapturedCallDefinition() { + exists(Variable v, BasicBlock bb, int i | + this.definesAt(v, bb, i) and + capturedCallWrite(_, bb, i, v) + ) + } + + /** + * Gets the immediately preceding definition. Since this update is uncertain, + * the value from the preceding definition might still be valid. + */ + final Definition getPriorDefinition() { result = uncertainWriteDefinitionInput(this) } + + override string toString() { result = " " + this.getSourceVariable() } +} + private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig { class Expr extends CfgNodes::AstCfgNode { predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } From 9fe7bb3e2bc3a56a1df49ecac4c668aec4c4f3e6 Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Thu, 12 Dec 2024 11:19:14 +0100 Subject: [PATCH 5/9] Rust: Address PR comments --- rust/ql/lib/codeql/rust/dataflow/Ssa.qll | 33 +++++++++++++++++++ .../rust/dataflow/internal/DataFlowImpl.qll | 2 +- .../codeql/rust/dataflow/internal/SsaImpl.qll | 33 ------------------- 3 files changed, 34 insertions(+), 34 deletions(-) diff --git a/rust/ql/lib/codeql/rust/dataflow/Ssa.qll b/rust/ql/lib/codeql/rust/dataflow/Ssa.qll index ac9d71b13d79..128b9ccc141d 100644 --- a/rust/ql/lib/codeql/rust/dataflow/Ssa.qll +++ b/rust/ql/lib/codeql/rust/dataflow/Ssa.qll @@ -337,4 +337,37 @@ module Ssa { override Location getLocation() { result = this.getBasicBlock().getLocation() } } + + /** + * An SSA definition inserted at a call that may update the value of a captured + * variable. For example, in + * + * ```rust + * fn capture_mut() { + * let mut y = 0; + * (0..5).for_each(|x| { + * y += x + * }); + * y + * } + * ``` + * + * a definition for `y` is inserted at the call to `for_each`. + */ + private class CapturedCallDefinition extends Definition, SsaImpl::UncertainWriteDefinition { + CapturedCallDefinition() { + exists(Variable v, BasicBlock bb, int i | + this.definesAt(v, bb, i) and + SsaImpl::capturedCallWrite(_, bb, i, v) + ) + } + + /** + * Gets the immediately preceding definition. Since this update is uncertain, + * the value from the preceding definition might still be valid. + */ + final Definition getPriorDefinition() { result = SsaImpl::uncertainWriteDefinitionInput(this) } + + override string toString() { result = " " + this.getSourceVariable() } + } } diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index 12783cd3b71c..14595c5095af 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -289,7 +289,7 @@ module Node { override Location getLocation() { result = cfgScope.getLocation() } - override string toString() { result = "lambda self in " + cfgScope } + override string toString() { result = "closure self in " + cfgScope } } abstract class ArgumentNode extends Node { diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll index e0ede89b9c4b..75b52b16c707 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll @@ -467,39 +467,6 @@ class PhiReadNode extends DefinitionExt, Impl::PhiReadNode { override Location getLocation() { result = Impl::PhiReadNode.super.getLocation() } } -/** - * An SSA definition inserted at a call that may update the value of a captured - * variable. For example, in - * - * ```rust - * fn capture_mut() { - * let mut y = 0; - * (0..5).for_each(|x| { - * y += x - * }); - * y - * } - * ``` - * - * a definition for `y` is inserted at the call to `for_each`. - */ -class CapturedCallDefinition extends Definition, Impl::UncertainWriteDefinition { - CapturedCallDefinition() { - exists(Variable v, BasicBlock bb, int i | - this.definesAt(v, bb, i) and - capturedCallWrite(_, bb, i, v) - ) - } - - /** - * Gets the immediately preceding definition. Since this update is uncertain, - * the value from the preceding definition might still be valid. - */ - final Definition getPriorDefinition() { result = uncertainWriteDefinitionInput(this) } - - override string toString() { result = " " + this.getSourceVariable() } -} - private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig { class Expr extends CfgNodes::AstCfgNode { predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } From 16dcc5c27842bb90a4f008768faf7854900d8306 Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Thu, 12 Dec 2024 11:23:22 +0100 Subject: [PATCH 6/9] Rust: Add variable capture consistency query --- rust/ql/consistency-queries/VariableCaptureConsistency.ql | 8 ++++++++ .../CONSISTENCY/VariableCaptureConsistency.expected | 5 +++++ .../CONSISTENCY/VariableCaptureConsistency.expected | 4 ++++ 3 files changed, 17 insertions(+) create mode 100644 rust/ql/consistency-queries/VariableCaptureConsistency.ql create mode 100644 rust/ql/test/library-tests/controlflow/CONSISTENCY/VariableCaptureConsistency.expected create mode 100644 rust/ql/test/library-tests/variables/CONSISTENCY/VariableCaptureConsistency.expected diff --git a/rust/ql/consistency-queries/VariableCaptureConsistency.ql b/rust/ql/consistency-queries/VariableCaptureConsistency.ql new file mode 100644 index 000000000000..894d654c0291 --- /dev/null +++ b/rust/ql/consistency-queries/VariableCaptureConsistency.ql @@ -0,0 +1,8 @@ +/** + * @name Variable capture data flow inconsistencies + * @description Lists the variable capture data flow inconsistencies in the database. This query is intended for internal use. + * @kind table + * @id rust/diagnostics/variable-capture-data-flow-consistency + */ + +import codeql.rust.dataflow.internal.DataFlowImpl::VariableCapture::Flow::ConsistencyChecks diff --git a/rust/ql/test/library-tests/controlflow/CONSISTENCY/VariableCaptureConsistency.expected b/rust/ql/test/library-tests/controlflow/CONSISTENCY/VariableCaptureConsistency.expected new file mode 100644 index 000000000000..b768e7abeae8 --- /dev/null +++ b/rust/ql/test/library-tests/controlflow/CONSISTENCY/VariableCaptureConsistency.expected @@ -0,0 +1,5 @@ +variableAccessAstNesting +| test.rs:396:26:396:26 | b | test.rs:409:28:414:9 | { ... } | CapturedVariable access is not nested in the defining callable | +| test.rs:409:23:409:25 | foo | test.rs:409:28:414:9 | { ... } | CapturedVariable access is not nested in the defining callable | +consistencyOverview +| CapturedVariable access is not nested in the defining callable | 2 | diff --git a/rust/ql/test/library-tests/variables/CONSISTENCY/VariableCaptureConsistency.expected b/rust/ql/test/library-tests/variables/CONSISTENCY/VariableCaptureConsistency.expected new file mode 100644 index 000000000000..2cd30314cac8 --- /dev/null +++ b/rust/ql/test/library-tests/variables/CONSISTENCY/VariableCaptureConsistency.expected @@ -0,0 +1,4 @@ +variableAccessAstNesting +| variables.rs:436:13:436:13 | i | variables.rs:437:17:439:5 | { ... } | CapturedVariable access is not nested in the defining callable | +consistencyOverview +| CapturedVariable access is not nested in the defining callable | 1 | From 0fa40fcdcc4169fa460c9b46a85438b6eff07888 Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Thu, 12 Dec 2024 16:28:19 +0100 Subject: [PATCH 7/9] Rust: Fix captured variable data flow inconsistency --- .../ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll | 8 +++++++- .../CONSISTENCY/VariableCaptureConsistency.expected | 5 ----- .../CONSISTENCY/VariableCaptureConsistency.expected | 4 ---- 3 files changed, 7 insertions(+), 10 deletions(-) delete mode 100644 rust/ql/test/library-tests/controlflow/CONSISTENCY/VariableCaptureConsistency.expected delete mode 100644 rust/ql/test/library-tests/variables/CONSISTENCY/VariableCaptureConsistency.expected diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index 14595c5095af..f0022a82a573 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -8,6 +8,7 @@ private import codeql.dataflow.DataFlow private import codeql.dataflow.internal.DataFlowImpl private import rust private import SsaImpl as SsaImpl +private import codeql.rust.controlflow.internal.Scope as Scope private import codeql.rust.controlflow.ControlFlowGraph private import codeql.rust.controlflow.CfgNodes private import codeql.rust.dataflow.Ssa @@ -771,7 +772,12 @@ class LambdaCallKind = Unit; /** Holds if `creation` is an expression that creates a lambda of kind `kind`. */ private predicate lambdaCreationExpr(Expr creation, LambdaCallKind kind) { - creation instanceof ClosureExpr and exists(kind) + ( + creation instanceof ClosureExpr + or + creation instanceof Scope::AsyncBlockScope + ) and + exists(kind) } /** diff --git a/rust/ql/test/library-tests/controlflow/CONSISTENCY/VariableCaptureConsistency.expected b/rust/ql/test/library-tests/controlflow/CONSISTENCY/VariableCaptureConsistency.expected deleted file mode 100644 index b768e7abeae8..000000000000 --- a/rust/ql/test/library-tests/controlflow/CONSISTENCY/VariableCaptureConsistency.expected +++ /dev/null @@ -1,5 +0,0 @@ -variableAccessAstNesting -| test.rs:396:26:396:26 | b | test.rs:409:28:414:9 | { ... } | CapturedVariable access is not nested in the defining callable | -| test.rs:409:23:409:25 | foo | test.rs:409:28:414:9 | { ... } | CapturedVariable access is not nested in the defining callable | -consistencyOverview -| CapturedVariable access is not nested in the defining callable | 2 | diff --git a/rust/ql/test/library-tests/variables/CONSISTENCY/VariableCaptureConsistency.expected b/rust/ql/test/library-tests/variables/CONSISTENCY/VariableCaptureConsistency.expected deleted file mode 100644 index 2cd30314cac8..000000000000 --- a/rust/ql/test/library-tests/variables/CONSISTENCY/VariableCaptureConsistency.expected +++ /dev/null @@ -1,4 +0,0 @@ -variableAccessAstNesting -| variables.rs:436:13:436:13 | i | variables.rs:437:17:439:5 | { ... } | CapturedVariable access is not nested in the defining callable | -consistencyOverview -| CapturedVariable access is not nested in the defining callable | 1 | From 1d8e7fd9ea22dd158a80070785d9eacb3d26da3a Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Thu, 12 Dec 2024 16:47:51 +0100 Subject: [PATCH 8/9] Rust: Accept differences --- .../test/library-tests/dataflow/closures/inline-flow.expected | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected b/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected index 166cdae7d36f..0c58b547208c 100644 --- a/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected +++ b/rust/ql/test/library-tests/dataflow/closures/inline-flow.expected @@ -12,7 +12,8 @@ edges | main.rs:34:21:34:21 | a | main.rs:34:13:34:22 | f(...) | provenance | | | main.rs:42:16:42:25 | source(...) | main.rs:44:5:44:5 | [post] f [captured capt] | provenance | | | main.rs:44:5:44:5 | [post] f [captured capt] | main.rs:45:10:45:13 | capt | provenance | | -| main.rs:44:5:44:5 | [post] f [captured capt] | main.rs:47:14:47:17 | capt | provenance | | +| main.rs:44:5:44:5 | [post] f [captured capt] | main.rs:49:5:49:5 | g [captured capt] | provenance | | +| main.rs:49:5:49:5 | g [captured capt] | main.rs:47:14:47:17 | capt | provenance | | nodes | main.rs:11:20:11:52 | if cond {...} else {...} | semmle.label | if cond {...} else {...} | | main.rs:11:30:11:39 | source(...) | semmle.label | source(...) | @@ -31,6 +32,7 @@ nodes | main.rs:44:5:44:5 | [post] f [captured capt] | semmle.label | [post] f [captured capt] | | main.rs:45:10:45:13 | capt | semmle.label | capt | | main.rs:47:14:47:17 | capt | semmle.label | capt | +| main.rs:49:5:49:5 | g [captured capt] | semmle.label | g [captured capt] | subpaths | main.rs:34:21:34:21 | a | main.rs:27:20:27:23 | ... | main.rs:28:9:32:9 | if cond {...} else {...} | main.rs:34:13:34:22 | f(...) | testFailures From 9da5d7128bb62410f4416f25b62d14f39991d6cf Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Mon, 16 Dec 2024 09:40:13 +0100 Subject: [PATCH 9/9] Rust: Add test with data flow inconsistency --- .../CONSISTENCY/DataFlowConsistency.expected | 2 ++ .../dataflow/local/DataFlowStep.expected | 29 +++++++++++++++++-- .../test/library-tests/dataflow/local/main.rs | 14 +++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected diff --git a/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected b/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected new file mode 100644 index 000000000000..19ff1527e16e --- /dev/null +++ b/rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected @@ -0,0 +1,2 @@ +identityLocalStep +| main.rs:404:7:404:18 | 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 d5376353d83e..1ec24c96b486 100644 --- a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected +++ b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected @@ -440,7 +440,28 @@ localStep | main.rs:395:9:395:9 | b | main.rs:395:9:395:9 | [SSA] b | | main.rs:395:13:395:18 | &mut a | main.rs:395:9:395:9 | b | | main.rs:396:10:396:19 | source(...) | main.rs:396:5:396:6 | * ... | -| main.rs:421:13:421:33 | result_questionmark(...) | main.rs:421:9:421:9 | _ | +| main.rs:402:39:402:43 | [SSA] names | main.rs:404:23:404:27 | names | +| main.rs:402:39:402:43 | names | main.rs:402:39:402:43 | [SSA] names | +| main.rs:402:39:402:72 | ...: Vec::<...> | main.rs:402:39:402:43 | names | +| main.rs:403:7:403:18 | [SSA] default_name | main.rs:404:23:404:27 | [SSA] [input] SSA phi read(default_name) | +| main.rs:403:7:403:18 | default_name | main.rs:403:7:403:18 | [SSA] default_name | +| main.rs:403:22:403:43 | ... .to_string(...) | main.rs:403:7:403:18 | default_name | +| main.rs:404:3:410:3 | for ... in ... { ... } | main.rs:402:75:411:1 | { ... } | +| main.rs:404:7:404:18 | [SSA] SSA phi read(default_name) | main.rs:404:29:410:3 | [SSA] [input] SSA phi read(default_name) | +| main.rs:404:7:404:18 | [SSA] SSA phi read(default_name) | main.rs:408:7:408:14 | [SSA] [input] SSA phi read(default_name) | +| main.rs:404:8:404:11 | [SSA] cond | main.rs:405:8:405:11 | cond | +| main.rs:404:8:404:11 | cond | main.rs:404:8:404:11 | [SSA] cond | +| main.rs:404:14:404:17 | [SSA] name | main.rs:406:15:406:18 | name | +| main.rs:404:14:404:17 | name | main.rs:404:14:404:17 | [SSA] name | +| main.rs:404:23:404:27 | [SSA] [input] SSA phi read(default_name) | main.rs:404:7:404:18 | [SSA] SSA phi read(default_name) | +| main.rs:404:29:410:3 | [SSA] [input] SSA phi read(default_name) | main.rs:404:7:404:18 | [SSA] SSA phi read(default_name) | +| main.rs:405:5:409:5 | if cond {...} | main.rs:404:29:410:3 | { ... } | +| main.rs:406:11:406:11 | [SSA] n | main.rs:407:12:407:12 | n | +| main.rs:406:11:406:11 | n | main.rs:406:11:406:11 | [SSA] n | +| main.rs:406:15:406:62 | name.unwrap_or_else(...) | main.rs:406:11:406:11 | n | +| main.rs:406:35:406:61 | [SSA] default_name | main.rs:406:38:406:49 | default_name | +| main.rs:408:7:408:14 | [SSA] [input] SSA phi read(default_name) | main.rs:404:7:404:18 | [SSA] SSA phi read(default_name) | +| main.rs:434:13:434:33 | result_questionmark(...) | main.rs:434:9:434:9 | _ | storeStep | main.rs:94:14:94:22 | source(...) | tuple.0 | main.rs:94:13:94:26 | TupleExpr | | main.rs:94:25:94:25 | 2 | tuple.1 | main.rs:94:13:94:26 | TupleExpr | @@ -509,7 +530,8 @@ storeStep | main.rs:373:27:373:27 | 2 | array[] | main.rs:373:23:373:31 | [...] | | main.rs:373:30:373:30 | 3 | array[] | main.rs:373:23:373:31 | [...] | | main.rs:376:18:376:27 | source(...) | array[] | main.rs:376:5:376:11 | [post] mut_arr | -| main.rs:404:27:404:27 | 0 | Some | main.rs:404:22:404:28 | Some(...) | +| main.rs:406:35:406:61 | default_name | captured default_name | main.rs:406:35:406:61 | \|...\| ... | +| main.rs:417:27:417:27 | 0 | Some | main.rs:417:22:417:28 | Some(...) | readStep | file://:0:0:0:0 | [summary param] self in lang:core::_::::unwrap | Some | file://:0:0:0:0 | [summary] read: Argument[self].Variant[crate::option::Option::Some(0)] in lang:core::_::::unwrap | | main.rs:33:9:33:15 | Some(...) | Some | main.rs:33:14:33:14 | _ | @@ -579,3 +601,6 @@ readStep | main.rs:376:5:376:11 | mut_arr | array[] | main.rs:376:5:376:14 | mut_arr[1] | | main.rs:377:13:377:19 | mut_arr | array[] | main.rs:377:13:377:22 | mut_arr[1] | | main.rs:379:10:379:16 | mut_arr | array[] | main.rs:379:10:379:19 | mut_arr[0] | +| main.rs:404:23:404:27 | names | array[] | main.rs:404:7:404:18 | TuplePat | +| main.rs:406:35:406:61 | [post] \|...\| ... | captured default_name | main.rs:406:35:406:61 | [post] default_name | +| main.rs:406:38:406:49 | this | captured default_name | main.rs:406:38:406:49 | default_name | diff --git a/rust/ql/test/library-tests/dataflow/local/main.rs b/rust/ql/test/library-tests/dataflow/local/main.rs index 819f68bc0498..57757324542d 100644 --- a/rust/ql/test/library-tests/dataflow/local/main.rs +++ b/rust/ql/test/library-tests/dataflow/local/main.rs @@ -397,6 +397,19 @@ fn write_through_borrow() { sink(a); // $ MISSING: hasValueFlow=39 } +// 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; + } + } +} + fn main() { direct(); variable_usage(); @@ -432,4 +445,5 @@ fn main() { array_assignment(); read_through_borrow(); write_through_borrow(); + captured_variable_and_continue(vec![]); }