Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Improve data flow through functions being passed as function pointers.
24 changes: 14 additions & 10 deletions rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -295,13 +295,10 @@ module LocalFlow {
class LambdaCallKind = Unit;

/** Holds if `creation` is an expression that creates a lambda of kind `kind`. */
predicate lambdaCreationExpr(Expr creation, LambdaCallKind kind) {
(
creation instanceof ClosureExpr
or
creation instanceof Scope::AsyncBlockScope
) and
exists(kind)
predicate lambdaCreationExpr(Expr creation) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped the kind parameter as it was only a nuisance for most callers.

creation instanceof ClosureExpr
or
creation instanceof Scope::AsyncBlockScope
}

/**
Expand Down Expand Up @@ -810,8 +807,15 @@ module RustDataFlow implements InputSig<Location> {

/** Holds if `creation` is an expression that creates a lambda of kind `kind` for `c`. */
predicate lambdaCreation(Node creation, LambdaCallKind kind, DataFlowCallable c) {
exists(Expr e |
e = creation.asExpr().getExpr() and lambdaCreationExpr(e, kind) and e = c.asCfgScope()
exists(kind) and
exists(Expr e | e = creation.asExpr().getExpr() |
lambdaCreationExpr(e) and e = c.asCfgScope()
or
// A path expression, that resolves to a function, evaluates to a function
// pointer. Except if the path occurs directly in a call, then it's just a
// call to the function and not a function being passed as data.
Comment on lines +814 to +816
Copy link
Preview

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should clarify what resolvePath returns or link to its definition, as it's not immediately clear what entities it resolves to or how it relates to c.asCfgScope().

Suggested change
// A path expression, that resolves to a function, evaluates to a function
// pointer. Except if the path occurs directly in a call, then it's just a
// call to the function and not a function being passed as data.
// A path expression that resolves to a function evaluates to a function
// pointer. Except if the path occurs directly in a call, then it's just a
// call to the function and not a function being passed as data.
// `resolvePath` returns the entity (such as a function or variable) to which
// the path expression refers. Here, we check if the resolved entity is the same
// as `c.asCfgScope()`, which represents the callable's scope. See the definition
// of `resolvePath` in `codeql.rust.internal.PathResolution` for details.

Copilot uses AI. Check for mistakes.

resolvePath(e.(PathExpr).getPath()) = c.asCfgScope() and
not any(CallExpr call).getFunction() = e
)
}

Expand Down Expand Up @@ -931,7 +935,7 @@ module VariableCapture {
}

class ClosureExpr extends Expr instanceof ExprCfgNode {
ClosureExpr() { lambdaCreationExpr(super.getExpr(), _) }
ClosureExpr() { lambdaCreationExpr(super.getExpr()) }

predicate hasBody(Callable body) { body = super.getExpr() }

Expand Down
4 changes: 2 additions & 2 deletions rust/ql/lib/codeql/rust/dataflow/internal/Node.qll
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ newtype TNode =
or
lambdaCallExpr(_, _, e)
or
lambdaCreationExpr(e.getExpr(), _)
lambdaCreationExpr(e.getExpr())
or
// Whenever `&mut e` has a post-update node we also create one for `e`.
// E.g., for `e` in `f(..., &mut e, ...)` or `*(&mut e) = ...`.
Expand All @@ -478,5 +478,5 @@ newtype TNode =
} or
TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node) or
TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or
TClosureSelfReferenceNode(CfgScope c) { lambdaCreationExpr(c, _) } or
TClosureSelfReferenceNode(CfgScope c) { lambdaCreationExpr(c) } or
TCaptureNode(VariableCapture::Flow::SynthesizedCaptureNode cn)

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
models
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change the dir name to lambdas? I thought closure was more standard terminology in Rust.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the tests only covered closures. Now they also covers functions, which are not closures. So I though that "closure" wasn't wide enough, and that using the data flow library terminology was the best match for what's common across the tests.

edges
| main.rs:10:20:10:52 | if cond {...} else {...} | main.rs:11:10:11:16 | f(...) | provenance | |
| main.rs:10:30:10:39 | source(...) | main.rs:10:20:10:52 | if cond {...} else {...} | provenance | |
| main.rs:15:20:15:23 | ... | main.rs:17:18:17:21 | data | provenance | |
| main.rs:22:9:22:9 | a | main.rs:23:13:23:13 | a | provenance | |
| main.rs:22:13:22:22 | source(...) | main.rs:22:9:22:9 | a | provenance | |
| main.rs:23:13:23:13 | a | main.rs:15:20:15:23 | ... | provenance | |
| main.rs:27:20:27:23 | ... | main.rs:27:26:27:52 | if cond {...} else {...} | provenance | |
| main.rs:28:9:28:9 | a | main.rs:29:21:29:21 | a | provenance | |
| main.rs:28:13:28:22 | source(...) | main.rs:28:9:28:9 | a | provenance | |
| main.rs:29:9:29:9 | b | main.rs:30:10:30:10 | b | provenance | |
| main.rs:29:13:29:22 | f(...) | main.rs:29:9:29:9 | b | provenance | |
| main.rs:29:21:29:21 | a | main.rs:27:20:27:23 | ... | provenance | |
| main.rs:29:21:29:21 | a | main.rs:29:13:29:22 | f(...) | provenance | |
| main.rs:37:16:37:25 | source(...) | main.rs:39:5:39:5 | [post] f [captured capt] | provenance | |
| main.rs:39:5:39:5 | [post] f [captured capt] | main.rs:40:10:40:13 | capt | provenance | |
| main.rs:39:5:39:5 | [post] f [captured capt] | main.rs:44:5:44:5 | g [captured capt] | provenance | |
| main.rs:44:5:44:5 | g [captured capt] | main.rs:42:14:42:17 | capt | provenance | |
| main.rs:47:29:49:1 | { ... } | main.rs:57:10:57:12 | f(...) | provenance | |
| main.rs:48:5:48:14 | source(...) | main.rs:47:29:49:1 | { ... } | provenance | |
| main.rs:51:17:51:25 | ...: i64 | main.rs:52:10:52:13 | data | provenance | |
| main.rs:62:9:62:9 | a | main.rs:63:7:63:7 | a | provenance | |
| main.rs:62:13:62:22 | source(...) | main.rs:62:9:62:9 | a | provenance | |
| main.rs:63:7:63:7 | a | main.rs:51:17:51:25 | ...: i64 | provenance | |
| main.rs:66:24:66:32 | ...: i64 | main.rs:66:42:72:1 | { ... } | provenance | |
| main.rs:76:9:76:9 | a | main.rs:77:21:77:21 | a | provenance | |
| main.rs:76:13:76:22 | source(...) | main.rs:76:9:76:9 | a | provenance | |
| main.rs:77:9:77:9 | b | main.rs:78:10:78:10 | b | provenance | |
| main.rs:77:13:77:22 | f(...) | main.rs:77:9:77:9 | b | provenance | |
| main.rs:77:21:77:21 | a | main.rs:66:24:66:32 | ...: i64 | provenance | |
| main.rs:77:21:77:21 | a | main.rs:77:13:77:22 | f(...) | provenance | |
nodes
| main.rs:10:20:10:52 | if cond {...} else {...} | semmle.label | if cond {...} else {...} |
| main.rs:10:30:10:39 | source(...) | semmle.label | source(...) |
| main.rs:11:10:11:16 | f(...) | semmle.label | f(...) |
| main.rs:15:20:15:23 | ... | semmle.label | ... |
| main.rs:17:18:17:21 | data | semmle.label | data |
| main.rs:22:9:22:9 | a | semmle.label | a |
| 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:27:26:27:52 | if cond {...} else {...} | semmle.label | if cond {...} else {...} |
| main.rs:28:9:28:9 | a | semmle.label | a |
| main.rs:28:13:28:22 | source(...) | semmle.label | source(...) |
| main.rs:29:9:29:9 | b | semmle.label | b |
| main.rs:29:13:29:22 | f(...) | semmle.label | f(...) |
| main.rs:29:21:29:21 | a | semmle.label | a |
| main.rs:30:10:30:10 | b | semmle.label | b |
| main.rs:37:16:37:25 | source(...) | semmle.label | source(...) |
| main.rs:39:5:39:5 | [post] f [captured capt] | semmle.label | [post] f [captured capt] |
| main.rs:40:10:40:13 | capt | semmle.label | capt |
| main.rs:42:14:42:17 | capt | semmle.label | capt |
| main.rs:44:5:44:5 | g [captured capt] | semmle.label | g [captured capt] |
| main.rs:47:29:49:1 | { ... } | semmle.label | { ... } |
| main.rs:48:5:48:14 | source(...) | semmle.label | source(...) |
| main.rs:51:17:51:25 | ...: i64 | semmle.label | ...: i64 |
| main.rs:52:10:52:13 | data | semmle.label | data |
| main.rs:57:10:57:12 | f(...) | semmle.label | f(...) |
| main.rs:62:9:62:9 | a | semmle.label | a |
| main.rs:62:13:62:22 | source(...) | semmle.label | source(...) |
| main.rs:63:7:63:7 | a | semmle.label | a |
| main.rs:66:24:66:32 | ...: i64 | semmle.label | ...: i64 |
| main.rs:66:42:72:1 | { ... } | semmle.label | { ... } |
| main.rs:76:9:76:9 | a | semmle.label | a |
| main.rs:76:13:76:22 | source(...) | semmle.label | source(...) |
| main.rs:77:9:77:9 | b | semmle.label | b |
| main.rs:77:13:77:22 | f(...) | semmle.label | f(...) |
| main.rs:77:21:77:21 | a | semmle.label | a |
| main.rs:78:10:78:10 | b | semmle.label | b |
subpaths
| main.rs:29:21:29:21 | a | main.rs:27:20:27:23 | ... | main.rs:27:26:27:52 | if cond {...} else {...} | main.rs:29:13:29:22 | f(...) |
| main.rs:77:21:77:21 | a | main.rs:66:24:66:32 | ...: i64 | main.rs:66:42:72:1 | { ... } | main.rs:77:13:77:22 | f(...) |
testFailures
#select
| main.rs:11:10:11:16 | f(...) | main.rs:10:30:10:39 | source(...) | main.rs:11:10:11:16 | f(...) | $@ | main.rs:10:30:10:39 | source(...) | source(...) |
| main.rs:17:18:17:21 | data | main.rs:22:13:22:22 | source(...) | main.rs:17:18:17:21 | data | $@ | main.rs:22:13:22:22 | source(...) | source(...) |
| main.rs:30:10:30:10 | b | main.rs:28:13:28:22 | source(...) | main.rs:30:10:30:10 | b | $@ | main.rs:28:13:28:22 | source(...) | source(...) |
| main.rs:40:10:40:13 | capt | main.rs:37:16:37:25 | source(...) | main.rs:40:10:40:13 | capt | $@ | main.rs:37:16:37:25 | source(...) | source(...) |
| main.rs:42:14:42:17 | capt | main.rs:37:16:37:25 | source(...) | main.rs:42:14:42:17 | capt | $@ | main.rs:37:16:37:25 | source(...) | source(...) |
| main.rs:52:10:52:13 | data | main.rs:62:13:62:22 | source(...) | main.rs:52:10:52:13 | data | $@ | main.rs:62:13:62:22 | source(...) | source(...) |
| main.rs:57:10:57:12 | f(...) | main.rs:48:5:48:14 | source(...) | main.rs:57:10:57:12 | f(...) | $@ | main.rs:48:5:48:14 | source(...) | source(...) |
| main.rs:78:10:78:10 | b | main.rs:76:13:76:22 | source(...) | main.rs:78:10:78:10 | b | $@ | main.rs:76:13:76:22 | source(...) | source(...) |
Original file line number Diff line number Diff line change
Expand Up @@ -6,30 +6,25 @@ 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|
let f = |cond, data| {
if cond {
sink(data); // $ hasValueFlow=87
} else {
sink(0)
};
}
};
Comment on lines +15 to +21
Copy link
Preview

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The closure formatting is inconsistent with the rest of the file. Consider using the same single-line format as closure_flow_through() for consistency, or apply consistent multi-line formatting throughout.

Suggested change
let f = |cond, data| {
if cond {
sink(data); // $ hasValueFlow=87
} else {
sink(0)
};
}
};
let f = |cond, data| sink(if cond { data } else { 0 }); // $ hasValueFlow=87

Copilot uses AI. Check for mistakes.

Comment on lines +15 to +21
Copy link
Preview

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The closure formatting is inconsistent with the rest of the file. Consider using the same single-line format as closure_flow_through() for consistency, or apply consistent multi-line formatting throughout.

Suggested change
let f = |cond, data| {
if cond {
sink(data); // $ hasValueFlow=87
} else {
sink(0)
};
}
};
let f = |cond, data| if cond { sink(data); /* $ hasValueFlow=87 */ } else { sink(0) };

Copilot uses AI. Check for mistakes.

let a = source(87);
f(true, a);
}

fn closure_flow_through() {
let f = |cond, data|
if cond {
data
} else {
0
};
let f = |cond, data| if cond { data } else { 0 };
let a = source(43);
let b = f(true, a);
sink(b); // $ hasValueFlow=43
Expand All @@ -49,9 +44,46 @@ fn closure_captured_variable() {
g();
}

fn get_from_source() -> i64 {
source(93)
}

fn pass_to_sink(data: i64) {
sink(data); // $ hasValueFlow=34
}

fn function_flow_out() {
let f = get_from_source;
sink(f()); // $ hasValueFlow=93
}

fn function_flow_in() {
let f = pass_to_sink;
let a = source(34);
f(a);
}

fn get_arg(cond: bool, data: i64) -> i64 {
if cond {
data
} else {
0
}
}

fn function_flows_through() {
let f = get_arg;
let a = source(56);
let b = f(true, a);
sink(b); // $ hasValueFlow=56
}

fn main() {
closure_flow_out();
closure_flow_in();
closure_flow_through();
closure_captured_variable();
function_flow_in();
function_flow_out();
function_flows_through();
}