-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rust: Reduce the number of sinks in DereferenceSink
#20941
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * Fixed false positives from the `rust/access-invalid-pointer` query, by only considering dereferences of raw pointers as sinks. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,18 +26,18 @@ module AccessAfterLifetimeConfig implements DataFlow::ConfigSig { | |
| predicate isSource(DataFlow::Node node) { | ||
| node instanceof AccessAfterLifetime::Source and | ||
| // exclude cases with sources in macros, since these results are difficult to interpret | ||
| not node.asExpr().isFromMacroExpansion() | ||
| not node.asExpr().isFromMacroExpansion() and | ||
| AccessAfterLifetime::sourceValueScope(node, _, _) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sources needs to satisfy this in the end anyway, so we might check for that here to prune some sources earlier in the process. |
||
| } | ||
|
|
||
| predicate isSink(DataFlow::Node node) { | ||
| node instanceof AccessAfterLifetime::Sink and | ||
| // exclude cases with sinks in macros, since these results are difficult to interpret | ||
| // Exclude cases with sinks in macros, since these results are difficult to interpret | ||
| not node.asExpr().isFromMacroExpansion() and | ||
| // include only results inside `unsafe` blocks, as other results tend to be false positives | ||
| ( | ||
| node.asExpr().getEnclosingBlock*().isUnsafe() or | ||
| node.asExpr().getEnclosingCallable().(Function).isUnsafe() | ||
| ) | ||
| // TODO: Remove this condition if it can be done without negatively | ||
| // impacting performance. This condition only include nodes with | ||
| // corresponding to an expression. This excludes sinks from models-as-data. | ||
| exists(node.asExpr()) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is weird, but note that the check before the changes had the same effect of excluding all nodes without an expression. So this is only preserving what was already there. Since my present goal is to improve performance I don't really have appetite for introducing additional sinks, hence I left this condition in place. As the todo says I think we should try to remove this in the future and see what it does for performance and results.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could re-include the models-as-data sinks explicitly with something like: There ought not be too many of these. |
||
| } | ||
|
|
||
| predicate isBarrier(DataFlow::Node barrier) { barrier instanceof AccessAfterLifetime::Barrier } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.