Rust: Add barriers for rust/access-invalid-pointer
#20941
Conversation
paldepind
force-pushed
the
rust/invalid-pointer-barriers
branch
from
b569633 to
40f629e
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds barriers and refines sink definitions to reduce false positives in the rust/access-invalid-pointer query. The changes separate concerns between the AccessInvalidPointer and AccessAfterLifetime queries by giving each its own sink definitions with appropriate restrictions.
- Adds type-based barriers (numeric, boolean, fieldless enum) to filter out non-pointer types
- Restricts
DereferenceSinkinAccessInvalidPointerto only match raw pointer dereferences in unsafe contexts - Introduces a
DefaultBarrierto handle impreciseDefault::defaultcalls that can resolve to null pointer implementations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
rust/ql/src/queries/summary/Stats.qll |
Adds import for AccessAfterLifetimeExtensions to ensure all query sink extensions are loaded |
rust/ql/lib/codeql/rust/security/AccessInvalidPointerExtensions.qll |
Adds multiple barriers (type-based, NonNull::new, Default::default) and restricts DereferenceSink to unsafe contexts with raw pointer type checks |
rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll |
Refactors Sink from type alias to abstract class and defines its own sink implementations separate from AccessInvalidPointer |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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 |
There was a problem hiding this comment.
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?
| call.getStaticTarget().getCanonicalPath() = canonicalName and | ||
| this.asExpr() = call.getArgument(pos) | ||
| | | ||
| canonicalName = "<core::ptr::non_null::NonNull>::new" and pos.asPosition() = 0 |
There was a problem hiding this comment.
Should this be a barrier or a sink???
Either way I'd like to see a test case for this.
| } | ||
| } | ||
|
|
||
| private class NumericTypeBarrier extends Barrier instanceof Barriers::NumericTypeBarrier { } |
There was a problem hiding this comment.
Could this end up blocking legitimate results where a null value is cast through an integer type and back, perhaps for integration with C-like interfaces or something? Admittedly literal 0 isn't currently a source for this query, so we don't really support this case as it is now.
| // to trait functions is more precise. | ||
| this.asExpr().(Call).getStaticTarget().getCanonicalPath() = | ||
| "<_ as core::default::Default>::default" | ||
| } |
There was a problem hiding this comment.
I'd like to see an example (test case) for this as well. I'm not quite clear what it looks like.
| // 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 | ||
| (not exists(TypeInference::inferType(p)) or TypeInference::inferType(p) instanceof PtrType) |
There was a problem hiding this comment.
I'm interested to see if this PtrType restriction effects real world [edge case] results (DCA and/or MRVA).
2 participants
This PR is intended to fix false positives for
rust/access-invalid-pointerwhich where exacerbated by #20879.There's comments in the code, so I hope things make sense.
Note that the change for
Default::defaultcould also remove true positives. But right there's just way to many false negatives coming from that.