Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes foundational interprocedural call resolution, warranting final human validation of precision and performance.
Review effort: Balanced
Findings: None
What changed in this PR
Models awaited Rust async blocks as lambda-style calls in data flow.
Changes:
- Adds await expressions as data-flow calls with argument and return nodes.
- Adds async-block flow and capture tests.
- Updates expected flow results and test dependencies.
| File | Description |
|---|---|
rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll |
Integrates await expressions into lambda-call resolution. |
rust/ql/lib/codeql/rust/dataflow/internal/Node.qll |
Adds await argument and output nodes. |
rust/ql/test/library-tests/dataflow/lambdas/main.rs |
Tests async-block flows and captures. |
rust/ql/test/library-tests/dataflow/lambdas/options.yml |
Enables Cargo checking with futures. |
rust/ql/test/library-tests/dataflow/lambdas/Cargo.lock |
Locks the new test dependency. |
rust/ql/test/library-tests/dataflow/lambdas/inline-flow.expected |
Records expected async-block flow paths. |
rust/ql/test/library-tests/dataflow/global/main.rs |
Marks async-block await flow as supported. |
rust/ql/test/library-tests/dataflow/global/inline-flow.expected |
Updates global flow expectations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Looks clean and effective. 👍
| Stages::DataFlowStage::ref() and | ||
| call.hasEnclosingCfgScope() | ||
| } or | ||
| TAwaitExpr(AwaitExpr await) { await.hasEnclosingCfgScope() } or |
There was a problem hiding this comment.
What would an AwaitExpr without an enclosing scope look like? Or is this check just defensive programming?
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
We currently model x.await as reading FutureContent, so I wonder if we should:
- For
asyncfunctions, add implicitFutureContentstore steps from returned values to a synthetic return node. - For
asyncblocks, do the same. - For
x.await, in addition to considering it a read step, also consider it a call to a synthetic callable with flow summaryinput: Argument[self].ReturnValue.FutureContentandoutput: ReturnValue.
|
With that approach, how does the body of the async block fit into the CFG? There'd be no "call" to the block at It seems to me that only "calling" the block at the let value = Cell::new("not secret");
let block = async {
sink(value.get());
};
value.set(source());
block.await;should have an alert at |
With this PR we model
.awaitas calls to async blocks.DCA seems fine. A few more query results.