-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rust: Speedup AccessAfterLifetime.ql
#21071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bfcbf51 to
2f04451
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR optimizes the AccessAfterLifetime.ql query to significantly improve performance by replacing a recursive transitive closure calculation with doubleBoundedFastTC. The optimization was motivated by a 30% reduction in total analysis time on Rust code after a previous change (PR #20966) caused performance regression by computing transitive closure over the entire virtual dispatch graph.
Key Changes
- Introduces a parameterized module pattern using a signature for dataflow candidate pairs
- Replaces recursive
mayEncloseOnStackpredicate with optimized transitive closure usingdoubleBoundedFastTC - Refactors the logic into a
DereferenceAfterLifetimemodule that takes a flow predicate as a parameter
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql |
Updates the call to dereferenceAfterLifetime to use the new parameterized module, passing AccessAfterLifetimeFlow::flow/2 as the flow predicate |
rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll |
Refactors dereferenceAfterLifetime into a parameterized module with optimized transitive closure using doubleBoundedFastTC, introduces TcNode type to unify sources, sinks, and block expressions for efficient graph traversal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll
Outdated
Show resolved
Hide resolved
paldepind
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice speedup!
The query started regressing with 108db75, which effectively computed the transitive closure over the entire virtual dispatch graph.
Why was that commit the culprit? Isn't it just removing a disjunction that didn't do anything?
rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll
Outdated
Show resolved
Hide resolved
rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll
Outdated
Show resolved
Hide resolved
I guess my brain was already on vacation when I viewed that diff; you are absolutely right, that commit is not the culprit. I wonder why the original DCA run did not flag the slowdown then... |
Before
```
Pipeline standard for AccessAfterLifetimeExtensions::AccessAfterLifetime::mayEncloseOnStack/2#3cdefece#bf@61cb32j5 was evaluated in 30 iterations totaling 44856ms (delta sizes total: 241646328).
241404616 ~1% {2} r1 = SCAN `AccessAfterLifetimeExtensions::AccessAfterLifetime::mayEncloseOnStack/2#3cdefece#bf#prev_delta` OUTPUT In.1, In.0
7379161442 ~1080% {2} | JOIN WITH `_AstNode::AstNode.getEnclosingBlock/0#5c38e65a_AstNode::AstNode.getEnclosingCallable/0#5a548913_Bloc__#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
333897324 ~40% {2} | AND NOT `AccessAfterLifetimeExtensions::AccessAfterLifetime::mayEncloseOnStack/2#3cdefece#bf#prev`(FIRST 2)
297961888 ~24% {2} | JOIN WITH `project#AccessAfterLifetimeExtensions::AccessAfterLifetime::sourceValueScope/3#d065ba16#2` ON FIRST 1 OUTPUT Lhs.0, Lhs.1
return r1
```
…fterLifetime.ql`
2f04451 to
2543754
Compare
geoffw0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes and DCA LGTM. 👍
Manually pushes magic context into transitive closure calculation, and replaces the recursive calculation with
doubleBoundedFastTC.Before
The query started regressing with
108db75#20966, which effectively computed the transitive closure over the entire virtual dispatch graph.DCA is great; a 30 % reduction in total analysis time on
rust.