Fix variable deallocation order in panic unwinding paths

[sladyn98]

This PR fixes a soundness bug where local variables are deallocated out of order during panic unwinding, allowing destructors to access freed memory. This violates Rust's safety guarantees and has caused real-world unsoundness in crates like generatively.

This PR removes the is_generator check and unconditionally emits StorageDead statements during unwinding for ALL functions, bringing non-generator behavior in line with generators. It ensures that during unwinding, when a local variable goes out of scope, its storage is properly marked as dead via StorageDead, allowing the borrow checker to enforce the
invariant that values must outlive their references even in panic paths.

Fixes #147875

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

[sladyn98]

r? @dianne

[dianne]

It looks like there's a bug causing an assertion failure when building the standard library. I've given it a look and offered a guess at what's causing it below. There's still work to do here beyond fixing that, though.

First, could you add a ui test to demonstrate that this fixes #147875? It looks like it might not yet, since the code for scheduling unwind drops on calls panicking looks unchanged.

Second, after verifying that this results in the correct borrow-checking behavior, we need to make sure that this change doesn't negatively affect codegen. Per the old comment on needs_cleanup, at least at the time it was written, LLVM didn't handle the unnecessary cleanup blocks and StorageDeads particularly well. If you can demonstrate with codegen tests that that's not an issue anymore, and perf isn't too bad, that might be all that's needed. But my expectation is that we'll have to get rid of or ignore the StorageDeads later in compilation (sometime after they serve their purpose in borrowck). Unless there's a reason to keep the StorageDeads around longer, my gut feeling is that this cleanup would be best as a post-borrowck MIR pass (maybe as part of CleanupPostBorrowck?), since then optimization passes can be done on cleaner MIR and we can test it works with MIR tests rather than codegen tests. Could you also add a test for this not affecting later stages of compilation? If you accomplish that by removing the unwind-path StorageDeads as part of a MIR pass, that'd be a mir-opt test.

Before you push again, you'll probably want to run the codegen and mir-opt tests to make sure the former is clean and to bless the latter. Regardless of what approach we take here, if we're changing how the MIR is built, there should be differences in the MIR building test output (part of the mir-opt suite).

View changes since this review

[dianne]

Could you please explain this change? My understanding at least is that the ordering of StorageDeads only matters relative to actual drops, since that's when it can affect borrow-checking; reordering StorageDeads amongst each other won't do anything, but a StorageDead before a drop terminator will cause a borrow-checking failure if the destructor could reference the dead memory. As such, I'd expect we'd only need a cleanup block when there's actual drops (which is what the old version of this was checking for). Is there an edge case where we'd need a cleanup block with only StorageDeads in it? Otherwise, could you reinstate the comment about avoiding creating landing pads when there's no actual destructors?

That said, from what I can tell this method is only used for determining whether cleanup blocks are required for unwinding from panics in destructors, so could you make sure there's a MIR test checking that we don't create unnecessary cleanup blocks in other cases too, particularly for calls?

Also, if the comment about LLVM is still true and we need to get rid of the StorageDeads before codegen, we should probably keep some updated version of those comments around.

[dianne]

I believe this also needs changing. This is where unwind_drops gets populated with the drops needed for calls' panic paths, so I'd expect fixing #147875 will require adjusting this.

[sladyn98]

Done

[rustbot]

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

[dianne]

Also, could you change the PR description? #147875 on its own doesn't allow destructors to access freed memory, it doesn't allow for the creation of dangling references, and I'm at least not aware of a safety guarantee that it violates. You should only get unsoundness out of it if you write unsafe code on the assumption that the borrow checker will enforce the relative drop order of locals that may have destructors and those that definitely don't. Of course, per language team decision, consistent drop order is a promise Rust would like to make. But it's not quite the same as the borrow-checker failing to ensure places outlive their references.

[sladyn98]

So what i did was write this simple rust program panic drop.rs

 //@ compile-flags: -C no-prepopulate-passes

#![crate_type = "lib"]

#[no_mangle]
pub fn function_with_drops() {

    let _a = String::from("first");
    let _b = String::from("second");
    let _c = String::from("third");
    might_panic();
}

#[inline(never)]
pub fn might_panic() {
    // This might panic
    panic!()
}

I ran the llvm to get the intermediate representaion and on looking at the IR I cannot find any llvm.lifetime.end statements suggesting to us that on master the StorageDead statements are missing, which according to my understanding means that the borrowchecker does not know when the storage becomes invalid. Let me now write the UI test to see what is up

[dianne]

StorageDead is indeed not present there on the main branch, but that doesn't create a soundness hole in the borrow-checker; those locals are simply treated as being maybe live until the end of the stack frame, so it's technically sound for destructors on the unwind path to reference them. When unwinding, the stack frame will be popped anyway, so we don't need to hint to llvm that the memory's no longer in use. The issue is that we'd like to be more strict in the borrow-checker, both for consistency and so that unsafe code can rely on drop order being enforced.

edit: adjusted wording

[dianne]

This still needs CI to pass before I can review it properly. I've left a few comments on obvious things, but I don't think reviewing the code changes would be helpful at this point. Please test your changes locally. You don't have to run the whole test suite yourself, but for this change, you'll at least want make sure that the mir-opt and codegen tests all pass, that any relevant ui tests pass, and that tidy passes as well.

Could you rebase onto a more recent commit, also? I don't expect there will be conflicts in the MIR building part of this, but I'm not sure about the rest.

I don't mean to be harsh, but this is a relatively complex and nuanced change. If you're not familiar with what's being changed, why it's being changed, the consequences/needs of that, and general contribution procedure, I'd recommend gaining familiarity with easier issues instead.

View changes since this review

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[sladyn98]

I think i got a good grasp of the problem and how we want to solve it, i started by adding StorageDead to unwind paths in both diverge_cleanup_target and build_scope_drops, but only when there's a cleanup path (i.e., when there are Value or ForLint drops present). This ensures the borrow-checker treats variables as dead at the same point on all paths.

I also needed to make sure StorageDead gets removed after borrow-checking so it doesn't affect codegen, so i then went modified CleanupPostBorrowck to remove StorageDead from cleanup blocks (unwind paths) after borrow-checking. I kept StorageDead on normal paths since it's still needed for MIR validation, and it gets removed later by RemoveStorageMarkers.

I also had to figure out the right place to remove StorageDead, you suggested a post-borrowck MIR pass, so I added it to CleanupPostBorrowck, but I had to be careful to only remove it from cleanup blocks to avoid breaking MIR validation on normal paths.

Now StorageDead is emitted on unwind paths for all functions (not just coroutines), which makes the borrow-checker stricter and more consistent. The borrow-checker now treats variables as dead at the same point on all paths, which is exactly what #147875 needed. And StorageDead is properly removed from cleanup blocks after borrow-checking, so it doesn't affect codegen.

Everything else is implemented and tested. The main question is whether the comments need more precision about where StorageDead gets removed.

[rust-log-analyzer]

The job pr-check-1 failed! Check out the build log: (web) (plain enhanced) (plain)

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

REPOSITORY                                   TAG       IMAGE ID       CREATED      SIZE
ghcr.io/dependabot/dependabot-updater-core   latest    bcec0b4e062b   5 days ago   783MB
=> Removing docker images...
Deleted Images:
untagged: ghcr.io/dependabot/dependabot-updater-core:latest
untagged: ghcr.io/dependabot/dependabot-updater-core@sha256:b662be51f7b8ef7e2c8464428f14e49cb79c36aa9afb7ecb9221dfe0f507050c
deleted: sha256:bcec0b4e062b5ffe11cc1c2729558c0cd96621c0271ab5e97ff3a56e0c25045a
deleted: sha256:64e147d5e54d9be8b8aa322e511cda02296eda4b8b8d063c6a314833aca50e29
deleted: sha256:5cba409bb463f4e7fa1a19f695450170422582c1bc7c0e934d893b4e5f558bc6
deleted: sha256:cddc6ebd344b0111eaab170ead1dfda24acdfe865ed8a12599a34d338fa8e28b
deleted: sha256:2412c3f334d79134573cd45e657fb6cc0abd75bef3881458b0d498d936545c8d
---
    Checking icu_collections v2.0.0
error[E0597]: `automaton` does not live long enough
   --> src/tools/rust-analyzer/crates/hir-def/src/import_map.rs:436:44
    |
433 |             let automaton = fst::automaton::Str::new(&query.lowercased);
    |                 --------- binding `automaton` declared here
...
436 |                 op = op.add(map.fst.search(&automaton));
    |                                            ^^^^^^^^^^ borrowed value does not live long enough
...
439 |         }
    |         - `automaton` dropped here while still borrowed
...
457 | }
    | - borrow might be used here, when `op` is dropped and runs the destructor for type `fst::map::OpBuilder<'_>`
    |
    = note: values in a scope are dropped in the opposite order they are defined

[RUSTC-TIMING] icu_collections test:false 0.672
    Checking byteorder v1.5.0