Rust: Reduce the number of sinks in DereferenceSink

[paldepind]

This PR reduces the number of sinks in DereferenceSink by not including dereference expressions that are not the source of unsafety.

The DereferenceSink is used in rust/access-invalid-pointer and rust/access-after-lifetime-ended.

The number of sinks is reduced in two ways:

1. In order for a dereference expression to dereference a pointer it must occur within an unsafe block. On rust this restriction reduces the number of sinks from 29,857 to 1,618. This filter was already applied in rust/access-after-lifetime-ended so it only makes a difference for the rust/access-invalid-pointer query.
2. If the type of the thing being dereferenced is not a raw pointer, then the dereference should be safe. So use type inference to only consider those dereferences. On rust this further reduces the number of sinks from 1,618 to 1,301.

I've looked at some of the sink that are removed on rust due to the type check, and they look like good exclusions to me. Here's a typical example:

let byte = unsafe { *src_bytes.get_unchecked(i) };

This deref is excluded because get_unchecked returns a &. Excluding this seem fine because 1/ the query can't handle this anyway and 2/ if the above unsafe block goes wrong the root cause for blame is within get_unchecked and not *.

MRVA

MRVA top 100 for "Access of invalid pointer"

• before these changes: https://gist.github.com/paldepind/7aab6cf462c86c1dc10d34b8b14ca836
• after these changes: https://gist.github.com/paldepind/09d9f132d22a1c3994fef609d343db61

Overall there's only a small change in the number of results, which seems good. The removed results that I've looked at all seemed like FPs.

[Copilot]

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 DereferenceSink in AccessInvalidPointer to only match raw pointer dereferences in unsafe contexts
• Introduces a DefaultBarrier to handle imprecise Default::default calls 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.

[paldepind]

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.

[geoffw0]

I think you could re-include the models-as-data sinks explicitly with something like:

or
node instanceof AccessAfterLifetime::ModelsAsDataSink

There ought not be too many of these.

[paldepind]

Then we might as well remove the line. I don't think it does much beside exclude MaD sinks.

My concern is that we have 318 pointer-access MaD sinks (most are generated). These are currently not used by the query, and starting to use them might cause problems that I'd like not to block other work. I'd rather we try that after getting the model generator working again.

[geoffw0]

I don't think its the right move to remove all the MaD sinks, but I appreciate that the model generator work is in progress and the TODO comment calls this out. I'd also prefer a more explicit not node instanceof AccessAfterLifetime::ModelsAsDataSink over exists(node.asExpr()), but then we need to make ModelsAsDataSink public.

I think I've come round to accepting this, for now.

[paldepind]

I don't think its the right move to remove all the MaD sinks

Just to be clear, the MaD sinks where already removed/excluded. This exists(..) is to preserve things as is.

[geoffw0]

Ah, I hadn't spotted that. Explains why it didn't affect results!

[paldepind]

Thanks for the review @geoffw0. I've now updated the PR to (almost) only include the change to DereferenceSink. I'll save the other changes for future PRs.

[geoffw0]

I've reviewed the QL and I'm happy from that point of view. I think I've figured out where I stand regarding the sink restrictions now - see the PR onto this PR - basically, losing a few mostly theoretical results is an acceptable cost to get the noise down. I've still to review your MRVA results and think about a few other details.

[geoffw0]

DCA - I'm happy that the removed results in your runs appear to be FPs, most are of the form assert_eq!(ptr, core::ptr::null()) and are removed because they're not in unsafe code. I have some ideas of how we can do better in future (other cases like this do happen to be in unsafe code), but this seems to be a step forward.

There's an unanswered question / request about exists(node.asExpr()) (answered before I posted this). And a consistency check is failing.

[paldepind]

Thanks for reviewing and for the additional test case.

And a consistency check is failing.

Fixed by rebasing on main.