Skip to content

Commit df8a92c

Browse files
geoffw0geoffw0
authored
Merge pull request #6 from hvitved/expect
Rust: Fix data flow through callbacks passed to library functions
2 parents f2564c3 + 8b82eaa commit df8a92c

File tree

5 files changed

+56
-51
lines changed

5 files changed

+56
-51
lines changed

‎rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,12 @@ final class DataFlowCallable extends TDataFlowCallable {
4949
}
5050

5151
final class DataFlowCall extends TDataFlowCall {
52-
private CallExprBaseCfgNode call;
53-
54-
DataFlowCall() { this = TCall(call) }
55-
5652
/** Gets the underlying call in the CFG, if any. */
57-
CallExprCfgNode asCallExprCfgNode() { result = call }
53+
CallExprCfgNode asCallExprCfgNode() { result = this.asCallBaseExprCfgNode() }
5854

59-
MethodCallExprCfgNode asMethodCallExprCfgNode() { result = call }
55+
MethodCallExprCfgNode asMethodCallExprCfgNode() { result = this.asCallBaseExprCfgNode() }
6056

61-
CallExprBaseCfgNode asCallBaseExprCfgNode() { result = call }
57+
CallExprBaseCfgNode asCallBaseExprCfgNode() { this = TCall(result) }
6258

6359
predicate isSummaryCall(
6460
FlowSummaryImpl::Public::SummarizedCallable c, FlowSummaryImpl::Private::SummaryNode receiver
@@ -67,7 +63,7 @@ final class DataFlowCall extends TDataFlowCall {
6763
}
6864

6965
DataFlowCallable getEnclosingCallable() {
70-
result = TCfgScope(call.getExpr().getEnclosingCfgScope())
66+
result = TCfgScope(this.asCallBaseExprCfgNode().getExpr().getEnclosingCfgScope())
7167
or
7268
exists(FlowSummaryImpl::Public::SummarizedCallable c |
7369
this.isSummaryCall(c, _) and
@@ -1298,10 +1294,14 @@ module RustDataFlow implements InputSig<Location> {
12981294
* invoked expression.
12991295
*/
13001296
predicate lambdaCall(DataFlowCall call, LambdaCallKind kind, Node receiver) {
1301-
receiver.asExpr() = call.asCallExprCfgNode().getFunction() and
1302-
// All calls to complex expressions and local variable accesses are lambda call.
1303-
exists(Expr f | f = receiver.asExpr().getExpr() |
1304-
f instanceof PathExpr implies f = any(Variable v).getAnAccess()
1297+
(
1298+
receiver.asExpr() = call.asCallExprCfgNode().getFunction() and
1299+
// All calls to complex expressions and local variable accesses are lambda call.
1300+
exists(Expr f | f = receiver.asExpr().getExpr() |
1301+
f instanceof PathExpr implies f = any(Variable v).getAnAccess()
1302+
)
1303+
or
1304+
call.isSummaryCall(_, receiver.(Node::FlowSummaryNode).getSummaryNode())
13051305
) and
13061306
exists(kind)
13071307
}

‎rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
11
identityLocalStep
2-
| main.rs:412:7:412:18 | phi(default_name) | Node steps to itself |
2+
| main.rs:412:9:412:20 | phi(default_name) | Node steps to itself |

‎rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -471,26 +471,26 @@ localStep
471471
| main.rs:403:13:403:19 | [post] mut_arr | main.rs:405:10:405:16 | mut_arr | |
472472
| main.rs:403:13:403:19 | mut_arr | main.rs:405:10:405:16 | mut_arr | |
473473
| main.rs:403:13:403:22 | mut_arr[1] | main.rs:403:9:403:9 | d | |
474-
| main.rs:410:39:410:43 | [SSA] names | main.rs:412:23:412:27 | names | |
474+
| main.rs:410:39:410:43 | [SSA] names | main.rs:412:25:412:29 | names | |
475475
| main.rs:410:39:410:43 | names | main.rs:410:39:410:43 | [SSA] names | |
476476
| main.rs:410:39:410:72 | ...: Vec::<...> | main.rs:410:39:410:43 | names | |
477-
| main.rs:411:7:411:18 | default_name | main.rs:411:7:411:18 | [SSA] default_name | |
478-
| main.rs:411:22:411:43 | ... .to_string(...) | main.rs:411:7:411:18 | default_name | |
479-
| main.rs:411:22:411:43 | ... .to_string(...) | main.rs:412:7:412:18 | phi(default_name) | |
480-
| main.rs:412:3:418:3 | for ... in ... { ... } | main.rs:410:75:419:1 | { ... } | |
481-
| main.rs:412:7:412:18 | phi(default_name) | main.rs:412:7:412:18 | phi(default_name) | |
482-
| main.rs:412:7:412:18 | phi(default_name) | main.rs:414:35:414:61 | default_name | |
483-
| main.rs:412:8:412:11 | [SSA] cond | main.rs:413:8:413:11 | cond | |
484-
| main.rs:412:8:412:11 | cond | main.rs:412:8:412:11 | [SSA] cond | |
485-
| main.rs:412:14:412:17 | [SSA] name | main.rs:414:15:414:18 | name | |
486-
| main.rs:412:14:412:17 | name | main.rs:412:14:412:17 | [SSA] name | |
487-
| main.rs:413:5:417:5 | if cond {...} | main.rs:412:29:418:3 | { ... } | |
488-
| main.rs:414:11:414:11 | [SSA] n | main.rs:415:12:415:12 | n | |
489-
| main.rs:414:11:414:11 | n | main.rs:414:11:414:11 | [SSA] n | |
490-
| main.rs:414:15:414:62 | name.unwrap_or_else(...) | main.rs:414:11:414:11 | n | |
491-
| main.rs:414:35:414:61 | [post] default_name | main.rs:412:7:412:18 | phi(default_name) | |
492-
| main.rs:414:35:414:61 | closure self in \|...\| ... | main.rs:414:38:414:49 | this | |
493-
| main.rs:414:35:414:61 | default_name | main.rs:412:7:412:18 | phi(default_name) | |
477+
| main.rs:411:9:411:20 | default_name | main.rs:411:9:411:20 | [SSA] default_name | |
478+
| main.rs:411:24:411:45 | ... .to_string(...) | main.rs:411:9:411:20 | default_name | |
479+
| main.rs:411:24:411:45 | ... .to_string(...) | main.rs:412:9:412:20 | phi(default_name) | |
480+
| main.rs:412:5:418:5 | for ... in ... { ... } | main.rs:410:75:419:1 | { ... } | |
481+
| main.rs:412:9:412:20 | phi(default_name) | main.rs:412:9:412:20 | phi(default_name) | |
482+
| main.rs:412:9:412:20 | phi(default_name) | main.rs:414:41:414:67 | default_name | |
483+
| main.rs:412:10:412:13 | [SSA] cond | main.rs:413:12:413:15 | cond | |
484+
| main.rs:412:10:412:13 | cond | main.rs:412:10:412:13 | [SSA] cond | |
485+
| main.rs:412:16:412:19 | [SSA] name | main.rs:414:21:414:24 | name | |
486+
| main.rs:412:16:412:19 | name | main.rs:412:16:412:19 | [SSA] name | |
487+
| main.rs:413:9:417:9 | if cond {...} | main.rs:412:31:418:5 | { ... } | |
488+
| main.rs:414:17:414:17 | [SSA] n | main.rs:415:18:415:18 | n | |
489+
| main.rs:414:17:414:17 | n | main.rs:414:17:414:17 | [SSA] n | |
490+
| main.rs:414:21:414:68 | name.unwrap_or_else(...) | main.rs:414:17:414:17 | n | |
491+
| main.rs:414:41:414:67 | [post] default_name | main.rs:412:9:412:20 | phi(default_name) | |
492+
| main.rs:414:41:414:67 | closure self in \|...\| ... | main.rs:414:44:414:55 | this | |
493+
| main.rs:414:41:414:67 | default_name | main.rs:412:9:412:20 | phi(default_name) | |
494494
| main.rs:428:9:428:9 | [SSA] s | main.rs:429:10:429:10 | s | |
495495
| main.rs:428:9:428:9 | s | main.rs:428:9:428:9 | [SSA] s | |
496496
| main.rs:428:13:428:27 | MacroExpr | main.rs:428:9:428:9 | s | |
@@ -600,7 +600,7 @@ storeStep
600600
| main.rs:399:27:399:27 | 2 | element | main.rs:399:23:399:31 | [...] |
601601
| main.rs:399:30:399:30 | 3 | element | main.rs:399:23:399:31 | [...] |
602602
| main.rs:402:18:402:27 | source(...) | element | main.rs:402:5:402:11 | [post] mut_arr |
603-
| main.rs:414:35:414:61 | default_name | captured default_name | main.rs:414:35:414:61 | \|...\| ... |
603+
| main.rs:414:41:414:67 | default_name | captured default_name | main.rs:414:41:414:67 | \|...\| ... |
604604
| main.rs:436:27:436:27 | 0 | Some | main.rs:436:22:436:28 | Some(...) |
605605
readStep
606606
| file://:0:0:0:0 | [summary param] self in lang:core::_::<crate::option::Option>::expect | Some | file://:0:0:0:0 | [summary] read: Argument[self].Variant[crate::option::Option::Some(0)] in lang:core::_::<crate::option::Option>::expect |
@@ -689,8 +689,8 @@ readStep
689689
| main.rs:402:5:402:11 | mut_arr | element | main.rs:402:5:402:14 | mut_arr[1] |
690690
| main.rs:403:13:403:19 | mut_arr | element | main.rs:403:13:403:22 | mut_arr[1] |
691691
| main.rs:405:10:405:16 | mut_arr | element | main.rs:405:10:405:19 | mut_arr[0] |
692-
| main.rs:412:7:412:18 | TuplePat | tuple.0 | main.rs:412:8:412:11 | cond |
693-
| main.rs:412:7:412:18 | TuplePat | tuple.1 | main.rs:412:14:412:17 | name |
694-
| main.rs:412:23:412:27 | names | element | main.rs:412:7:412:18 | TuplePat |
695-
| main.rs:414:35:414:61 | [post] \|...\| ... | captured default_name | main.rs:414:35:414:61 | [post] default_name |
696-
| main.rs:414:38:414:49 | this | captured default_name | main.rs:414:38:414:49 | default_name |
692+
| main.rs:412:9:412:20 | TuplePat | tuple.0 | main.rs:412:10:412:13 | cond |
693+
| main.rs:412:9:412:20 | TuplePat | tuple.1 | main.rs:412:16:412:19 | name |
694+
| main.rs:412:25:412:29 | names | element | main.rs:412:9:412:20 | TuplePat |
695+
| main.rs:414:41:414:67 | [post] \|...\| ... | captured default_name | main.rs:414:41:414:67 | [post] default_name |
696+
| main.rs:414:44:414:55 | this | captured default_name | main.rs:414:44:414:55 | default_name |

‎rust/ql/test/library-tests/dataflow/local/inline-flow.expected

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@ models
22
| 1 | Summary: lang:core; <crate::option::Option>::unwrap; Argument[self].Variant[crate::option::Option::Some(0)]; ReturnValue; value |
33
| 2 | Summary: lang:core; <crate::option::Option>::unwrap_or; Argument[0]; ReturnValue; value |
44
| 3 | Summary: lang:core; <crate::option::Option>::unwrap_or; Argument[self].Variant[crate::option::Option::Some(0)]; ReturnValue; value |
5-
| 4 | Summary: lang:core; <crate::option::Option>::unwrap_or_else; Argument[self].Variant[crate::option::Option::Some(0)]; ReturnValue; value |
6-
| 5 | Summary: lang:core; <crate::result::Result>::expect; Argument[self].Variant[crate::result::Result::Ok(0)]; ReturnValue; value |
7-
| 6 | Summary: lang:core; <crate::result::Result>::expect_err; Argument[self].Variant[crate::result::Result::Err(0)]; ReturnValue; value |
5+
| 4 | Summary: lang:core; <crate::option::Option>::unwrap_or_else; Argument[0].ReturnValue; ReturnValue; value |
6+
| 5 | Summary: lang:core; <crate::option::Option>::unwrap_or_else; Argument[self].Variant[crate::option::Option::Some(0)]; ReturnValue; value |
7+
| 6 | Summary: lang:core; <crate::result::Result>::expect; Argument[self].Variant[crate::result::Result::Ok(0)]; ReturnValue; value |
8+
| 7 | Summary: lang:core; <crate::result::Result>::expect_err; Argument[self].Variant[crate::result::Result::Err(0)]; ReturnValue; value |
89
edges
910
| main.rs:19:9:19:9 | s | main.rs:20:10:20:10 | s | provenance | |
1011
| main.rs:19:13:19:21 | source(...) | main.rs:19:9:19:9 | s | provenance | |
@@ -72,7 +73,8 @@ edges
7273
| main.rs:237:9:237:10 | s1 [Some] | main.rs:238:10:238:11 | s1 [Some] | provenance | |
7374
| main.rs:237:14:237:29 | Some(...) [Some] | main.rs:237:9:237:10 | s1 [Some] | provenance | |
7475
| main.rs:237:19:237:28 | source(...) | main.rs:237:14:237:29 | Some(...) [Some] | provenance | |
75-
| main.rs:238:10:238:11 | s1 [Some] | main.rs:238:10:238:32 | s1.unwrap_or_else(...) | provenance | MaD:4 |
76+
| main.rs:238:10:238:11 | s1 [Some] | main.rs:238:10:238:32 | s1.unwrap_or_else(...) | provenance | MaD:5 |
77+
| main.rs:241:31:241:40 | source(...) | main.rs:241:10:241:41 | s2.unwrap_or_else(...) | provenance | MaD:4 |
7678
| main.rs:245:9:245:10 | s1 [Some] | main.rs:247:14:247:15 | s1 [Some] | provenance | |
7779
| main.rs:245:14:245:29 | Some(...) [Some] | main.rs:245:9:245:10 | s1 [Some] | provenance | |
7880
| main.rs:245:19:245:28 | source(...) | main.rs:245:14:245:29 | Some(...) [Some] | provenance | |
@@ -88,11 +90,11 @@ edges
8890
| main.rs:267:9:267:10 | s1 [Ok] | main.rs:268:10:268:11 | s1 [Ok] | provenance | |
8991
| main.rs:267:32:267:45 | Ok(...) [Ok] | main.rs:267:9:267:10 | s1 [Ok] | provenance | |
9092
| main.rs:267:35:267:44 | source(...) | main.rs:267:32:267:45 | Ok(...) [Ok] | provenance | |
91-
| main.rs:268:10:268:11 | s1 [Ok] | main.rs:268:10:268:22 | s1.expect(...) | provenance | MaD:5 |
93+
| main.rs:268:10:268:11 | s1 [Ok] | main.rs:268:10:268:22 | s1.expect(...) | provenance | MaD:6 |
9294
| main.rs:271:9:271:10 | s2 [Err] | main.rs:273:10:273:11 | s2 [Err] | provenance | |
9395
| main.rs:271:32:271:46 | Err(...) [Err] | main.rs:271:9:271:10 | s2 [Err] | provenance | |
9496
| main.rs:271:36:271:45 | source(...) | main.rs:271:32:271:46 | Err(...) [Err] | provenance | |
95-
| main.rs:273:10:273:11 | s2 [Err] | main.rs:273:10:273:26 | s2.expect_err(...) | provenance | MaD:6 |
97+
| main.rs:273:10:273:11 | s2 [Err] | main.rs:273:10:273:26 | s2.expect_err(...) | provenance | MaD:7 |
9698
| main.rs:282:9:282:10 | s1 [A] | main.rs:284:11:284:12 | s1 [A] | provenance | |
9799
| main.rs:282:14:282:39 | ...::A(...) [A] | main.rs:282:9:282:10 | s1 [A] | provenance | |
98100
| main.rs:282:29:282:38 | source(...) | main.rs:282:14:282:39 | ...::A(...) [A] | provenance | |
@@ -255,6 +257,8 @@ nodes
255257
| main.rs:237:19:237:28 | source(...) | semmle.label | source(...) |
256258
| main.rs:238:10:238:11 | s1 [Some] | semmle.label | s1 [Some] |
257259
| main.rs:238:10:238:32 | s1.unwrap_or_else(...) | semmle.label | s1.unwrap_or_else(...) |
260+
| main.rs:241:10:241:41 | s2.unwrap_or_else(...) | semmle.label | s2.unwrap_or_else(...) |
261+
| main.rs:241:31:241:40 | source(...) | semmle.label | source(...) |
258262
| main.rs:245:9:245:10 | s1 [Some] | semmle.label | s1 [Some] |
259263
| main.rs:245:14:245:29 | Some(...) [Some] | semmle.label | Some(...) [Some] |
260264
| main.rs:245:19:245:28 | source(...) | semmle.label | source(...) |
@@ -386,6 +390,7 @@ testFailures
386390
| 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(...) |
387391
| 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(...) |
388392
| 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(...) |
393+
| 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(...) |
389394
| 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(...) |
390395
| 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(...) |
391396
| 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(...) |

‎rust/ql/test/library-tests/dataflow/local/main.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ fn option_unwrap_or_else() {
238238
sink(s1.unwrap_or_else(|| 0)); // $ hasValueFlow=47
239239

240240
let s2 = None;
241-
sink(s2.unwrap_or_else(|| source(48))); // $ MISSING: hasValueFlow=48
241+
sink(s2.unwrap_or_else(|| source(48))); // $ hasValueFlow=48
242242
}
243243

244244
fn option_questionmark() -> Option<i64> {
@@ -408,14 +408,14 @@ fn array_assignment() {
408408
// Test data flow inconsistency occuring with captured variables and `continue`
409409
// in a loop.
410410
pub fn captured_variable_and_continue(names: Vec<(bool, Option<String>)>) {
411-
let default_name = source(83).to_string();
412-
for (cond, name) in names {
413-
if cond {
414-
let n = name.unwrap_or_else(|| default_name.to_string());
415-
sink(n.len() as i64);
416-
continue;
411+
let default_name = source(83).to_string();
412+
for (cond, name) in names {
413+
if cond {
414+
let n = name.unwrap_or_else(|| default_name.to_string());
415+
sink(n.len() as i64);
416+
continue;
417+
}
417418
}
418-
}
419419
}
420420

421421
macro_rules! get_source {

0 commit comments

Comments
 (0)