Rust: Exclude self parameter accesses from rust/access-after-lifetime-ended

[geoffw0]

Exclude self parameter accesses from being sources in rust/access-after-lifetime-ended. The analysis in this query doesn't make any attempt to track object lifetimes, the (occasional) inclusion of self parameters was accidental, and the results we got from this path weren't great.

A smarter solution might figure out the actual variable(s) self can refer to (in the test, obj) and use the existing query logic from there.

TODO:

• I'm expecting some changes on the DCA run.
  • there were none; though the result I was expecting to fix has turned out to be a "wobbly" result, hopefully we have fixed that.
• change note.
• create a follow-up issue.

[Copilot]

Pull request overview

This PR refines the rust/access-after-lifetime-ended query by excluding self parameter accesses from being treated as sources. The query doesn't attempt to track object lifetimes, so including self parameters was unintended and produced suboptimal results.

Changes:

• Modified the query logic to filter out self parameters when identifying potential lifetime-ending variable accesses
• Added a comprehensive test case demonstrating methods with both &self and self parameters that return pointers to internal fields
• Updated the test runner to execute the new test case

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll	Added not var.getParameter() instanceof SelfParam condition to exclude self parameters from source identification
rust/ql/test/query-tests/security/CWE-825/lifetime.rs	Added test_get_self() function and MyObjectWithGetters struct to test the exclusion of self parameter accesses
rust/ql/test/query-tests/security/CWE-825/main.rs	Added call to test_get_self() in the main test execution function

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[paldepind]

I don't understand why we want to treat self as special here? For the purpose of this query I'd think self is just a parameter as any other.

Let's say I change the self parameter to foo parameter like in the diff below. I haven't really changed the behavior of the program in any way, so I'd expect the query results to not change either?

For &raw foo.bar we already have implemented the fact that this tracks the lifetime of foo. And per my other comments it seems that the query results prior to the self might in fact be just fine?

@@ -871,13 +871,13 @@ impl MyObjectWithGetters {
                }
        }
 
-       pub unsafe fn get_ptr1(&self) -> *const i64 {
-               &raw const self.value // $ MISSING: Source[rust/access-after-lifetime-ended]=self_value
+       pub unsafe fn get_ptr1(foo: &Self) -> *const i64 {
+               &raw const foo.value // $ MISSING: Source[rust/access-after-lifetime-ended]=self_value
                // (the returned pointer is valid as long as the containing object is)
     }
 
-       pub unsafe fn get_ptr2(self) -> *const i64 {
-               &raw const self.value // $ MISSING: Source[rust/access-after-lifetime-ended]=self_value
+       pub unsafe fn get_ptr2(foo: Self) -> *const i64 {
+               &raw const foo.value // $ MISSING: Source[rust/access-after-lifetime-ended]=foo_value
                // (the returned pointer is valid as long as the containing object is)
     }
 }
@@ -888,8 +888,8 @@ pub fn test_get_self() {
 
        unsafe {
                let obj = MyObjectWithGetters::new(1111);
-               ptr1 = obj.get_ptr1();
-               ptr2 = obj.get_ptr2();
+               ptr1 = MyObjectWithGetters::get_ptr1(&obj);
+               ptr2 = MyObjectWithGetters::get_ptr2(obj);
 
                let v1 = *ptr1; // GOOD
                let v2 = *ptr2; // GOOD

[paldepind]

This one is in fact bad.

I tried inserting use_the_stack(); above these two lines and can then observe this pointer access resulting in gibberish.

get_ptr2 receives a copy of MyObjectWithGetters which will sit on its stack. The &raw const foo.value then takes the address of the value field on the stack, and that becomes invalid after return.

[geoffw0]

That's interesting ... then I think the first step to fixing this PR is going to be correcting and expanding the test cases.

[geoffw0]

I don't understand why we want to treat self as special here?

Yeah, and I have since found cases that don't mention self where we have a similar problem.

I need to rethink what we want to do here.