Skip to content

Lint for returning raw pointers to temporary stack locations that don't outlive the statement that creates them #10959

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
sdroege opened this issue Jun 15, 2023 · 4 comments
Assignees
Labels
A-lint Area: New lints

Comments

@sdroege
Copy link

sdroege commented Jun 15, 2023

What it does

The general pattern of the code this should complain about is something like

let x = &(some expression that is stored in a temporary variable on the stack) as *const T;

Similar with using addr_of! instead of & and casts, etc.

Concrete examples this would catch are

let x = &(1 + 2) as *const i32;
let x = &(x as *const i32) as *const *const i32;

In both cases the part in the parenthesis is stored in a temporary stack location that is no longer valid after the whole statement.

It should however not catch

let x = &(*ptr).x as *const T;
let x = &(some_variable) as *const T;

Advantage

Whatever pointer is created there is pointing to no longer valid stack memory, so any usage afterwards will be unsound

Drawbacks

Theoretically this could cause false positives but the only case I can see where the resulting code is not unsound is if you cast the pointer to an usize and do some calculations with it. I don't see how that could lead to any useful results in such a context though.

Example

See examples above

@Centri3
Copy link
Member

Centri3 commented Jun 15, 2023

This would be very nice. I've written code like

EnumProcessModules(
	/*
	etc...
	*/
	&mut 0 as _,
);

way too many times, since it both compiles and almost always causes nothing out of the ordinary due to it still pointing at allocated memory (though is of course UB).

@rustbot claim

@y21
Copy link
Member

y21 commented Jun 15, 2023

Hmm, is this actually UB? It seems to me like let x = &(temporary) as *const _ specifically is ok because of Temporary Lifetime Extension (the temporary gets promoted to its own binding). Much like how you can also write something like let x = &mut String::new(); and use that x then.
Regardless though, it seems like a safety hazard and a lint for this seems nice.

For example:

struct NoisyDrop;
impl Drop for NoisyDrop {
    fn drop(&mut self) {
        println!("drop!");
    }
}
fn temporary() -> NoisyDrop {
    NoisyDrop
}
fn main() {
    let x = &(temporary()) as *const NoisyDrop;
    println!("after declaration of x");
}

This prints "after declaration of x!", "drop!" in that order, implying that it is in fact not dropped immediately and has its lifetime extended

@sdroege
Copy link
Author

sdroege commented Jun 15, 2023

That's interesting. The case I had that prompted me to create this issue was something like &((*ptr).x as *const _) as *const _ (the part inside the parenthesis becomes a temporary on the stack), but that then was converted into a reference and actually returned from the function (the lifetime of ptr and the returned reference are bound correctly, but the returned reference is actually pointing to the temporary!). And that then did anything from crashes to asan/valgrind warnings. (The code in question should've been &(*ptr).x as *const _)

So maybe the problem is actually a bit more narrow here: the lifetime of the temporary is extended until the end up the scope, but it's not obvious that a temporary is involved here and the lifetime of the pointer is actually bound to the temporary.

@Centri3
Copy link
Member

Centri3 commented Jun 15, 2023

Yeah, looks like that in particular (let x = &(temporary) as *const _) is fine. Though something like

#[derive(Debug)]
#[repr(transparent)]
struct A(usize);

impl Drop for A {
    fn drop(&mut self) {
        println!("dropped! {self:?}");
    }
}

fn main() {
    const PS: [A; 3] = [A(2usize), A(3usize), A(11usize)];
    let mut ps = vec![];

    for p in PS {
        ps.push(&p as *const A);
    }

    for p in ps {
        println!("read! {:?}", unsafe { p.read() });
    }
}

is UB.

playground link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants