-
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 |
|---|---|---|
|
|
@@ -10,6 +10,8 @@ private import codeql.rust.dataflow.FlowSink | |
| private import codeql.rust.Concepts | ||
| private import codeql.rust.dataflow.internal.Node | ||
| private import codeql.rust.security.Barriers as Barriers | ||
| private import codeql.rust.internal.TypeInference as TypeInference | ||
| private import codeql.rust.internal.Type | ||
|
|
||
| /** | ||
| * Provides default sources, sinks and barriers for detecting accesses to | ||
|
|
@@ -47,16 +49,22 @@ module AccessInvalidPointer { | |
| ModelsAsDataSource() { sourceNode(this, "pointer-invalidate") } | ||
| } | ||
|
|
||
| /** | ||
| * A pointer access using the unary `*` operator. | ||
| */ | ||
| /** A raw pointer access using the unary `*` operator. */ | ||
| private class DereferenceSink extends Sink { | ||
| DereferenceSink() { any(DerefExpr p).getExpr() = this.asExpr() } | ||
| DereferenceSink() { | ||
| exists(Expr p, DerefExpr d | p = d.getExpr() and p = this.asExpr() | | ||
| // Dereferencing a raw pointer is an unsafe operation. Hence relevant | ||
| // dereferences must occur inside code marked as unsafe. | ||
| // See: https://doc.rust-lang.org/reference/types/pointer.html#r-type.pointer.raw.safety | ||
| (p.getEnclosingBlock*().isUnsafe() or p.getEnclosingCallable().(Function).isUnsafe()) and | ||
| // We are only interested in dereferences of raw pointers, as other uses | ||
| // of `*` are safe. | ||
| (not exists(TypeInference::inferType(p)) or TypeInference::inferType(p) instanceof PtrType) | ||
|
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'm interested to see if this |
||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A pointer access from model data. | ||
| */ | ||
| /** A pointer access from model data. */ | ||
| private class ModelsAsDataSink extends Sink { | ||
| ModelsAsDataSink() { sinkNode(this, "pointer-access") } | ||
| } | ||
|
|
||
| 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. |
||
| } | ||
|
|
||
| predicate isBarrier(DataFlow::Node barrier) { barrier instanceof AccessAfterLifetime::Barrier } | ||
|
|
||
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.
Do you think this check is better here, in the sink definition, or in the place where the query uses the sink (as it is for
rust/access-after-lifetime-ended)? Should we be consistent between the two?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.
I think it makes sense to have for the sink, if it's intended to represent pointer dereferences. We should remove the check in
rust/access-after-lifetime-ended. I've done that now.