Add a new lint that warns for pointers to stack memory #134218

1c3t3a · 2024-12-12T16:32:52Z

This adds a new lint with level Warn to check for code like:

fn foo() -> *const i32 {
  let x = 42;
  &x
}

and produce a warning like:

error: returning a pointer to stack memory associated with a local variable
  --> <source>:12:5
   |
 LL|  &x
   |  ^^

This fixes #134215.

rustbot · 2024-12-12T16:33:03Z

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

1c3t3a · 2024-12-12T16:34:24Z

I am aware that this may warn on existing code. I wonder what is a good strategy here? Should it be an Allow lint first and then be migrated to Warn as part of an edition? Or even stay Allow forever?

compiler-errors · 2024-12-12T16:49:32Z

Is there a reason why this was implemented as a rust lint rather than a clippy suspicious lint or something?

I guess let's find the fallout of this lint

@bors try

Add a new lint that warns for pointers to stack memory This adds a new lint with level `Warn` to check for code like: ```rust fn foo() -> *const i32 { let x = 42; &x } ``` and produce a warning like: ```text error: returning a pointer to stack memory associated with a local variable --> <source>:12:5 | LL| &x | ^^ ``` This fixes rust-lang#134215.

bors · 2024-12-12T16:50:47Z

⌛ Trying commit 40eb5d7 with merge 6db1a6a...

1c3t3a · 2024-12-12T16:52:25Z

Is there a reason why this was implemented as a rust lint rather than a clippy suspicious lint or something?

This was completely my judgment, I am happy to change this if requested. My reasoning was that this is a pretty fundamental lint, since the code it lints should probably never exist? And looking at the list of rustc lints I thought it might be a good fit.

compiler/rustc_lint/src/builtin.rs

1c3t3a · 2024-12-12T17:18:26Z

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Oops - I don't fully understand that error. The failure looks like in the .stderr file, but it still has unexpected errors that it doesn't match on... This even happens when passing --bless.

compiler-errors · 2024-12-12T19:55:46Z

@craterbot check

craterbot · 2024-12-12T19:55:49Z

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2024-12-12T19:56:50Z

@craterbot run mode=check-only start=master#8e37e151835d96d6a7415e93e6876561485a3354 end=try#6db1a6a7ebb2ad243051f3ff949f987dd70cde2b

craterbot · 2024-12-12T19:57:13Z

👌 Experiment pr-134218 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

jyn514 · 2024-12-13T02:33:31Z

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

Oops - I don't fully understand that error. The failure looks like in the .stderr file, but it still has unexpected errors that it doesn't match on... This even happens when passing --bless.

bless will generate a .stderr file for you, but it will not update the original .rs file. compiletest is complaining because it expects each warning to be annotated with //~ WARN. see https://rustc-dev-guide.rust-lang.org/tests/ui.html#error-annotations

rustbot · 2024-12-13T15:49:15Z

The Miri subtree was changed

cc @rust-lang/miri

compiler-errors · 2024-12-14T19:18:44Z

@craterbot end=try#6db1a6a7ebb2ad243051f3ff949f987dd70cde2b+rustflags=-Dreturn-local-variable-ptr

craterbot · 2024-12-14T19:18:47Z

📝 Configuration of the pr-134218 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

1c3t3a · 2025-02-18T15:25:35Z

Friendly ping on this again :) It would be great to get some feedback!

1c3t3a · 2025-02-24T16:18:45Z

Friendly ping again!

cc: @estebank, @compiler-errors

bors · 2025-03-05T18:57:08Z

☔ The latest upstream changes (presumably #138058) made this pull request unmergeable. Please resolve the merge conflicts.

1c3t3a · 2025-03-10T14:59:51Z

Pinging once again, it'd be really great to get some feedback!

workingjubilee · 2025-03-15T01:32:50Z

Can this detect the "out-param" case yet? e.g. assigning to &mut *mut Local

1c3t3a · 2025-03-16T14:02:31Z

Can this detect the "out-param" case yet? e.g. assigning to &mut *mut Local

It should, I can add a test for it.

Would be great to get feedback if this is something that is considered for Rust or better lives in Clippy.

1c3t3a · 2025-03-16T14:13:56Z

Actually it cannot, but before I invest more work, it would be great to get the fundamental bit of feedback about the lint, before I put it in the wrong place / it is not wanted in general.

1c3t3a · 2025-04-01T07:39:25Z

Friendly ping! :)

cc @compiler-errors @estebank

apiraino · 2025-04-03T11:12:00Z

@1c3t3a it is not necessary to send weekly reminders for a review. Reviewers are pretty busy and will be back when they have capacity to look at this.

Thank you for your patience.

wesleywiser · 2025-04-10T14:34:53Z

r? rust-lang/compiler

jieyouxu · 2025-04-10T14:35:33Z

r? compiler (busy on compiletest atm)

Nadrieril · 2025-04-16T18:23:04Z

tests/ui/infinite/infinite-instantiation-struct-tail-ice-114484.rs

@@ -9,6 +9,7 @@
 // Regression test for #114484: This used to ICE during monomorphization, because we treated
 // `<VirtualWrapper<...> as Pointee>::Metadata` as a rigid projection after reaching the recursion
 // limit when finding the struct tail.
+#![allow(returning_pointers_to_local_variables)]


Which part of the test was triggering the lint? All the returned poiunters in this test look legit to me.

You are right! The problem was the gen_vtable_ptr which returns *const (), which should be fine. I'll add an exclude for the unit type :)

I don't think excluding the unit type is correct: it's not particularly special. Where in that test did your lint think there was a pointer to stack memory? The vtable in gen_vtable_ptr was transmuted from pointer metadata which is not pointing to stack memory, and the ptr in virtualize_my_trait is obtained by transmuting an existing pointer.

Shouldn't your lint be operating on MIR actually? Because right now operating on HIR you have to think about implicit coercions and reborrows and things.

```rust fn foo() -> *const i32 { let x = 42; &x } ``` and produce a warning like: ```text error: returning a pointer to stack memory associated with a local variable --> <source>:12:5 | LL| &x | ^^ ``` Disable the lint for another test

Nadrieril · 2025-04-27T15:40:13Z

compiler/rustc_lint/src/builtin.rs

+            hir::Expr {
+                kind:
+                    hir::ExprKind::Path(
+                        hir::QPath::Resolved(_, hir::Path { res: hir::def::Res::Local(_), .. }),
+                        ..,
+                    ),
+                ..
+            } => cx.emit_span_lint(
+                RETURNING_POINTERS_TO_LOCAL_VARIABLES,
+                return_expr.span,
+                BuiltinReturningPointersToLocalVariables,
+            ),


hum, doesn't this also catch

fn foo() -> *const Something { let x: *const Something = ...; ... x }

?

Nadrieril · 2025-04-27T15:40:46Z

compiler/rustc_lint/src/builtin.rs

+        if matches!(ptr_ty.ty.kind, hir::TyKind::Tup([])) {
+            return;
+        }


There's no good reason to exclude the unit type from this analysis.

Nadrieril · 2025-04-27T15:49:24Z

I'm overall mixed about this lint: it feels extremely targeted; do we have such targeted lints in rustc usually?

I'd also prefer a MIR-based lint honestly, it could then catch the difference between fn foo() -> *const u32 { &42 } (ok because of const promotion) and fn foo() -> *const u32 { &bar() } (not ok), which seems more likely to be where ppl get confused.

@rustbot author

rustbot · 2025-04-27T15:49:29Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

1c3t3a · 2025-04-28T12:49:52Z

I'm overall mixed about this lint: it feels extremely targeted; do we have such targeted lints in rustc usually?

Could you explain with what you mean by extremely targeted? This lint generally just catches a pattern of code that is never going to be correct, similar to let foo: T = *std::ptr::null<T>() and should always be avoided. This doesn't feel to targeted to me? But I am also happy to take this to clippy if that is the conclusion.

I'd also prefer a MIR-based lint honestly

That's a very good point. Especially the const promotion was sth I didn't think about. I will try to reimplement the lint on MIR.

Nadrieril · 2025-04-29T12:15:57Z

Could you explain with what you mean by extremely targeted?

I say targeted because it seems to be exclusively about a syntactic &local expression at the return of a function and I guess I don't expect people to actually write code like that? I also cannot imagine someone writing let foo = *ptr::null(); so this may just be a failure of imagination on my part x).

1c3t3a · 2025-04-29T13:30:00Z

Could you explain with what you mean by extremely targeted?

I say targeted because it seems to be exclusively about a syntactic &local expression at the return of a function and I guess I don't expect people to actually write code like that? I also cannot imagine someone writing let foo = *ptr::null(); so this may just be a failure of imagination on my part x).

Mhm I think with enough control flow that adds indirection this can get complicated enough. This PR was motivated by clang having this warning and two weeks after I opened the PR my team found a piece of code that survived for half a year in our codebase and was the perfect motivation for this lint:

// Import actual list from source and use this temporarily for now
const GLOBAL: &[&str] = &[
    "abcd",
    ...
];

#[no_mangle]
pub extern "C" fn foo() -> *mut *const c_char {
    let c_strings: Vec<_> = GLOBAL.iter().map(|&val| CString::new(val).unwrap()).collect();

    let mut ptrs: Vec<_> = c_strings.iter().map(|cs| cs.as_ptr()).collect();
    ptrs.push(std::ptr::null()); // Now safe to push

    let boxed_ptrs = ptrs.into_boxed_slice();
    let raw_ptr = Box::into_raw(boxed_ptrs);

    return raw_ptr as *mut *const c_char;
}

I think this is reasonably indirect to not make it entirely clear what is going on. Especially if you are new to Rust or write FFI code that frequently makes use of raw pointers.

But maybe the fact that we are discussing this is already a sign that it should be a clippy lint? Idk.

rustbot assigned compiler-errors Dec 12, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 12, 2024