Skip to content

Rust: New query rust/access-after-lifetime-ended #19702

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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jun 9, 2025

New query rust/access-after-lifetime-ended, for detecting pointer dereferences after the lifetime of the pointed-to object has ended. Makes use of some existing tests that were created for rust/access-invalid-pointer (before I realized that the idea for that query needed breaking into two separate queries). Also adds quite a lot of new test cases as well.

Note that the query is currently @precision medium due to several remaining false positive results in the tests (and in MRVA results). I made some effort to fix these, but I didn't get them all, I feel it's time to get what we have merged and plan further improvements as follow-up work.

Before merging:

  • QL review
  • docs review
  • DCA run

@Copilot Copilot AI review requested due to automatic review settings June 9, 2025 17:41
@geoffw0 geoffw0 requested a review from a team as a code owner June 9, 2025 17:41
@geoffw0 geoffw0 added the Rust Pull requests that update Rust code label Jun 9, 2025
Copy link
Contributor

github-actions bot commented Jun 9, 2025

QHelp previews:

rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.qhelp

Access of a pointer after its lifetime has ended

Dereferencing a pointer after the lifetime of its target has ended causes undefined behavior. Memory may be corrupted causing the program to crash or behave incorrectly, in some cases exposing the program to potential attacks.

Recommendation

When dereferencing a pointer in unsafe code, take care that the pointer is still valid at the time it is dereferenced. Code may need to be rearranged or changed to extend lifetimes. If possible, rewrite the code using safe Rust types to avoid this kind of problem altogether.

Example

In the following example, val is local to get_pointer so its lifetime ends when that function returns. However, a pointer to val is returned and dereferenced after that lifetime has ended, causing undefined behavior:

fn get_pointer() -> *const i64 {
	let val = 123;

	return &val;
} // lifetime of `val` ends here, the pointer becomes dangling

fn example() {
	let ptr = get_pointer();
	let val;

	// ...

	unsafe {
		val = *ptr; // BAD: dereferences `ptr` after the lifetime of `val` has ended
	}

	// ...
}

One way to fix this is to change the return type of the function from a pointer to a Box, which ensures that the value it points to remains on the heap for the lifetime of the Box itself. Notice that there is no longer a need for an unsafe block as the code no longer handles pointers directly:

fn get_box() -> Box<i64> {
	let val = 123;

	return Box::new(val); // copies `val` onto the heap, where it remains for the lifetime of the `Box`.
}

fn example() {
	let ptr = get_box();
	let val;

	// ...

	val = *ptr; // GOOD

	// ...
}

References

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a new CodeQL query to detect pointer dereferences after the lifetime of their target has ended, including examples, documentation, and scope‐tracking support.

  • Introduce AccessAfterLifetime.ql with configuration, flow definition, and filtering logic
  • Add QL test harness entries (.qlref), examples (Good.rs/Bad.rs), and documentation (.qhelp)
  • Extend the internal QL library to track enclosing blocks (getEnclosingBlock) and support the new query

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
rust/ql/test/query-tests/security/CWE-825/options.yml Add futures dependency for async tests
rust/ql/test/query-tests/security/CWE-825/main.rs Invoke newly added test cases in main()
rust/ql/test/query-tests/security/CWE-825/deallocation.rs Update existing invalid-pointer test annotations
rust/ql/test/query-tests/security/CWE-825/AccessAfterLifetime.qlref Register the new query with test harness
rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeGood.rs Add good example using Box
rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeBad.rs Add bad example that returns a dangling pointer
rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql Implement the new query logic
rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.qhelp Add documentation and examples
rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll Define sources, sinks, barriers, and scope checks
rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Add Variable.getEnclosingBlock()
rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll Add AstNode.getEnclosingBlock()
Comments suppressed due to low confidence (1)

rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql:44

  • The predicate isFromMacroExpansion() does not exist; you likely meant isInMacroExpansion() to filter out macro expansions.
not sinkNode.getNode().asExpr().getExpr().isFromMacroExpansion()

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 11, 2025

DCA:

  • 95 results for the new query; that's quite a lot...
    • confusing results that have sources in macros - fixed ✅
    • results matching self; I think this kind of thing is probably OK: match self { EnumValue(x) => &x.y }
    • but the query is @precision medium, so deferring some of these issues should be acceptable
  • otherwise LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant