Swift: Diff-informed queries: phase 3 (non-trivial locations)#20082
Swift: Diff-informed queries: phase 3 (non-trivial locations)#20082d10c merged 4 commits intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables diff-informed mode on Swift security queries that select non-trivial locations (other than dataflow source or sink). The changes add location override predicates to handle cases where queries select locations that need special handling for diff-informed analysis.
- Adds
observeDiffInformedIncrementalMode()predicate to four security query modules - Implements
getASelectedSinkLocation()predicate for queries that can support diff-informed mode - Disables diff-informed mode for queries with location selection complexities
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| UnsafeWebViewFetchQuery.qll | Disables diff-informed mode due to secondary location use in select |
| InsecureTLSQuery.qll | Disables diff-informed mode due to Swift nodes with problematic locations |
| CleartextStoragePreferencesQuery.qll | Enables diff-informed mode with custom sink location handling |
| CleartextStorageDatabaseQuery.qll | Enables diff-informed mode with custom sink location handling |
|
|
||
| predicate observeDiffInformedIncrementalMode() { any() } | ||
|
|
||
| Location getASelectedSinkLocation(DataFlow::Node sink) { |
There was a problem hiding this comment.
The logic in getASelectedSinkLocation is duplicated across CleartextStoragePreferencesQuery.qll and CleartextStorageDatabaseQuery.qll. Consider extracting this common pattern into a shared utility predicate to improve maintainability.
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Thanks for the links in the commit descriptions, they were a helpful shortcut.
|
|
||
| predicate observeDiffInformedIncrementalMode() { | ||
| none() // query selects some Swift nodes (e.g. "[post] self") that have location file://:0:0:0:0, which always fall outside the diff range. | ||
| } |
There was a problem hiding this comment.
Do you recall if there's any reason we don't give post-update nodes a Location (equal to that of the pre-update node)?
I may do a quick experiment with this (probably after your PR, to avoid complicating things).
There was a problem hiding this comment.
I think it comes down to the use of UnknownLocation when an AstNode is synthesized.
There was a problem hiding this comment.
OK, I'll have a very quick go at making a workaround, but as I say, lets not let it get in the way of this PR.
There was a problem hiding this comment.
Yeah, it looks like there's no easy way to provide a location for such a node in Swift.
|
|
||
| predicate observeDiffInformedIncrementalMode() { | ||
| none() // can't override location accurately because of secondary use in select. | ||
| } |
There was a problem hiding this comment.
What's the issue here? The only unusual thing I can see in that query is selecting from three possible alert messages.
There was a problem hiding this comment.
This issue is the second flow call (UnsafeWebViewFetchFlow::flowToExpr) in the third disjunct: we can't include it in the location override because of monotonic recursion, but if we exclude it then the --check-diff-informed test fails, as it usually does with these cases of secondary flows. So we can't make these queries diff-informed, but their incrementality story will be handled separately using overlay-informed dataflow.
There was a problem hiding this comment.
Thanks for the explanation.
2 participants
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.