Rust: Diff-informed queries: phase 3 (non-trivial locations) #20081
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables diff-informed mode on Rust security queries that select non-trivial locations (locations other than dataflow source or sink). The change adds diff-informed query capabilities to improve incremental analysis performance by only analyzing code changes that are relevant to the query results.
- Adds
observeDiffInformedIncrementalMode()predicate to enable diff-informed mode on 8 security queries - Implements custom location override in
AccessAfterLifetime.qlto return the actual selected locations - Maintains existing query logic while adding incremental analysis optimization
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| AccessInvalidPointer.ql | Adds diff-informed mode enablement |
| AccessAfterLifetime.ql | Adds diff-informed mode with custom location override for target variables |
| UncontrolledAllocationSize.ql | Adds diff-informed mode enablement |
| CleartextLogging.ql | Adds diff-informed mode enablement |
| CleartextTransmission.ql | Adds diff-informed mode enablement |
| SqlInjection.ql | Adds diff-informed mode enablement |
| TaintedPath.ql | Adds diff-informed mode enablement |
| RegexInjection.ql | Adds diff-informed mode enablement |
There was a problem hiding this comment.
Other than rust/access-after-lifetime-ended these are very straightforward.
I have a couple of new Rust queries in progress, should I make similar changes to those before they are merged? And is there anything I should do to test if I do that (beyond letting CI run as usual)?
| Location getASelectedSourceLocation(DataFlow::Node source) { | ||
| exists(Variable target, DataFlow::Node sink | result = target.getLocation() | | ||
| isSink(sink) and | ||
| AccessAfterLifetime::dereferenceAfterLifetime(source, sink, target) |
There was a problem hiding this comment.
Should this not check the flowPath exists as well?
I guess it may not be required for correctness actually ... but dereferenceAfterLifetime is bindingset[source, sink] for performance reasons, I'm a little bit concerned that not checking the flowPath here may greatly increase the amount we compute for it.
This PR enables diff-informed mode on queries that select a location other than dataflow source or sink. This entails adding a non-trivial location override that returns the locations that are actually selected.
Prior work includes PRs like #19663, #19759, and #19817. This PR uses the same patch script as those PRs to find candidate queries to convert to diff-enabled. This is the final step in mass-enabling diff-informed queries on all the languages.
Commit-by-commit reviewing is recommended.