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.

[geoffw0]

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?

[paldepind]

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.

[geoffw0]

Should this be a barrier or a sink???

Either way I'd like to see a test case for this.

[geoffw0]

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.

[geoffw0]

I'd like to see an example (test case) for this as well. I'm not quite clear what it looks like.

[geoffw0]

I'm interested to see if this PtrType restriction effects real world [edge case] results (DCA and/or MRVA).

[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.

[paldepind]

Sources needs to satisfy this in the end anyway, so we might check for that here to prune some sources earlier in the process.

[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.