Prevent unwinding when `-C panic=abort` is used regardless declared ABI #96959

nbdd0121 · 2022-05-11T22:32:05Z

Ensures that Rust code will abort with -C panic=abort regardless ABI used.

extern "C-unwind" {
    fn may_unwind();
}

// Will be nounwind with `-C panic=abort`, despite `C-unwind` ABI.
pub unsafe extern "C-unwind" fn rust_item_that_can_unwind() {
    may_unwind();
}

Current behaviour is that unwind will propagate through. While the current behaviour won't cause unsoundness it is inconsistent with the text reading of RFC2945.

I tweaked fn_can_unwind instead of tweaking AbortUnwindingCalls because this approach would allow Rust (non-direct) callers to also see that this function is nounwind, so it can prevent excessive landing pads generation.

For more discussions: https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/soundness.20in.20mixed.20panic.20mode.

cc @alexcrichton, @BatmanAoD
r? @Amanieu

@rustbot label: T-compiler T-lang F-c_unwind

Amanieu · 2022-05-12T11:43:02Z

@bors r+

bors · 2022-05-12T11:43:04Z

📌 Commit c586bc3 has been approved by Amanieu

bjorn3 · 2022-05-12T13:07:40Z

src/test/codegen/unwind-abis/c-unwind-abi-panic-abort.rs

@@ -19,4 +19,4 @@ pub unsafe extern "C-unwind" fn rust_item_that_can_unwind() {
 // Now, make sure that the LLVM attributes for this functions are correct.  First, make
 // sure that the first item is correctly marked with the `nounwind` attribute:
 //
-// CHECK-NOT: attributes #0 = { {{.*}}nounwind{{.*}} }
+// CHECK: attributes #0 = { {{.*}}nounwind{{.*}} }


may_unwind must not be marked with nounwind as otherwise you would get UB when it unwinds instead of an abort. Can you add a check that may_unwind gets a different attributes list without the nounwind attribute?

Basically C-unwind foreign declarations must not get nounwind. C-unwind function definitions can get nounwind.

That's what this is doing though: The #0 attributes with nounwind apply to rust_item_that_can_unwind and the #1 attributes without nounwind apply to may_unwind.

I just added the #1 check after @bjorn3's comments -- the original test doesn't check the attribute of may_unwind. It's not wrong, but the test could be expanded.

bjorn3 · 2022-05-12T13:10:51Z

Going to r- as I am pretty sure this implementation is incorrect as indicated in #96959 (comment). Feel free to approve again if a test shows that it is actually correct.

@bors r-

nbdd0121 · 2022-05-12T13:12:37Z

The implementation is correct. may_unwind does not get attribute #0.

bjorn3

Right, missed the tcx.is_foreign_item condition. My bad.

Amanieu · 2022-05-12T13:42:26Z

@bors r+

bors · 2022-05-12T13:42:28Z

📌 Commit a315bb4 has been approved by Amanieu

Prevent unwinding when `-C panic=abort` is used regardless declared ABI Ensures that Rust code will abort with `-C panic=abort` regardless ABI used. ```rust extern "C-unwind" { fn may_unwind(); } // Will be nounwind with `-C panic=abort`, despite `C-unwind` ABI. pub unsafe extern "C-unwind" fn rust_item_that_can_unwind() { may_unwind(); } ``` Current behaviour is that unwind will propagate through. While the current behaviour won't cause unsoundness it is inconsistent with the text reading of [RFC2945](https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html). I tweaked `fn_can_unwind` instead of tweaking `AbortUnwindingCalls` because this approach would allow Rust (non-direct) callers to also see that this function is nounwind, so it can prevent excessive landing pads generation. For more discussions: https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/soundness.20in.20mixed.20panic.20mode. cc `@alexcrichton,` `@BatmanAoD` r? `@Amanieu` `@rustbot` label: T-compiler T-lang F-c_unwind

bors · 2022-05-13T05:56:10Z

⌛ Testing commit a315bb4 with merge ba34d4dfc9f16a4ece928ffb324cc298dcff5f0e...

bors · 2022-05-13T06:20:51Z

💔 Test failed - checks-actions

nbdd0121 · 2022-05-17T14:57:44Z

@Amanieu there's an update to the test after the latest r+, PTAL, thanks!

Amanieu · 2022-05-17T14:58:40Z

@bors r+

bors · 2022-05-17T14:58:42Z

📌 Commit f86e409 has been approved by Amanieu

bors · 2022-05-17T15:04:52Z

⌛ Testing commit f86e409 with merge 00755e4...

bors · 2022-05-17T17:20:21Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 00755e4 to master...

rust-timer · 2022-05-17T20:46:00Z

Finished benchmarking commit (00755e4): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

programmerjake · 2022-09-06T05:12:24Z

I think i found a bug in this PR: #101469

Pkgsrc changes: * Adjust patches as needed & checksum updates. Upstream changes: Version 1.63.0 (2022-08-11) ========================== Language -------- - [Remove migrate borrowck mode for pre-NLL errors.][95565] - [Modify MIR building to drop repeat expressions with length zero.][95953] - [Remove label/lifetime shadowing warnings.][96296] - [Allow explicit generic arguments in the presence of `impl Trait` args.] [96868] - [Make `cenum_impl_drop_cast` warnings deny-by-default.][97652] - [Prevent unwinding when `-C panic=abort` is used regardless of declared ABI.][96959] - [lub: don't bail out due to empty binders.][97867] Compiler -------- - [Stabilize the `bundle` native library modifier,][95818] also removing the deprecated `static-nobundle` linking kind. - [Add Apple WatchOS compile targets\*.][95243] - [Add a Windows application manifest to rustc-main.][96737] \* Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Implement `Copy`, `Clone`, `PartialEq` and `Eq` for `core::fmt::Alignment`.][94530] - [Extend `ptr::null` and `null_mut` to all thin (including extern) types.][94954] - [`impl Read and Write for VecDeque<u8>`.][95632] - [STD support for the Nintendo 3DS.][95897] - [Make write/print macros eagerly drop temporaries.][96455] - [Implement internal traits that enable `[OsStr]::join`.][96881] - [Implement `Hash` for `core::alloc::Layout`.][97034] - [Add capacity documentation for `OsString`.][97202] - [Put a bound on collection misbehavior.][97316] - [Make `std::mem::needs_drop` accept `?Sized`.][97675] - [`impl Termination for Infallible` and then make the `Result` impls of `Termination` more generic.][97803] - [Document Rust's stance on `/proc/self/mem`.][97837] Stabilized APIs --------------- - [`array::from_fn`] - [`Box::into_pin`] - [`BinaryHeap::try_reserve`] - [`BinaryHeap::try_reserve_exact`] - [`OsString::try_reserve`] - [`OsString::try_reserve_exact`] - [`PathBuf::try_reserve`] - [`PathBuf::try_reserve_exact`] - [`Path::try_exists`] - [`Ref::filter_map`] - [`RefMut::filter_map`] - [`NonNull::<[T]>::len`][`NonNull::<slice>::len`] - [`ToOwned::clone_into`] - [`Ipv6Addr::to_ipv4_mapped`] - [`unix::io::AsFd`] - [`unix::io::BorrowedFd<'fd>`] - [`unix::io::OwnedFd`] - [`windows::io::AsHandle`] - [`windows::io::BorrowedHandle<'handle>`] - [`windows::io::OwnedHandle`] - [`windows::io::HandleOrInvalid`] - [`windows::io::HandleOrNull`] - [`windows::io::InvalidHandleError`] - [`windows::io::NullHandleError`] - [`windows::io::AsSocket`] - [`windows::io::BorrowedSocket<'handle>`] - [`windows::io::OwnedSocket`] - [`thread::scope`] - [`thread::Scope`] - [`thread::ScopedJoinHandle`] These APIs are now usable in const contexts: - [`array::from_ref`] - [`slice::from_ref`] - [`intrinsics::copy`] - [`intrinsics::copy_nonoverlapping`] - [`<*const T>::copy_to`] - [`<*const T>::copy_to_nonoverlapping`] - [`<*mut T>::copy_to`] - [`<*mut T>::copy_to_nonoverlapping`] - [`<*mut T>::copy_from`] - [`<*mut T>::copy_from_nonoverlapping`] - [`str::from_utf8`] - [`Utf8Error::error_len`] - [`Utf8Error::valid_up_to`] - [`Condvar::new`] - [`Mutex::new`] - [`RwLock::new`] Cargo ----- - [Stabilize the `--config path` command-line argument.][cargo/10755] - [Expose rust-version in the environment as `CARGO_PKG_RUST_VERSION`.][cargo/10713] Compatibility Notes ------------------- - [`#[link]` attributes are now checked more strictly,][96885] which may introduce errors for invalid attribute arguments that were previously ignored. Internal Changes ---------------- These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of rustc and related tools. - [Prepare Rust for LLVM opaque pointers.][94214] [94214]: rust-lang/rust#94214 [94530]: rust-lang/rust#94530 [94954]: rust-lang/rust#94954 [95243]: rust-lang/rust#95243 [95565]: rust-lang/rust#95565 [95632]: rust-lang/rust#95632 [95818]: rust-lang/rust#95818 [95897]: rust-lang/rust#95897 [95953]: rust-lang/rust#95953 [96296]: rust-lang/rust#96296 [96455]: rust-lang/rust#96455 [96737]: rust-lang/rust#96737 [96868]: rust-lang/rust#96868 [96881]: rust-lang/rust#96881 [96885]: rust-lang/rust#96885 [96959]: rust-lang/rust#96959 [97034]: rust-lang/rust#97034 [97202]: rust-lang/rust#97202 [97316]: rust-lang/rust#97316 [97652]: rust-lang/rust#97652 [97675]: rust-lang/rust#97675 [97803]: rust-lang/rust#97803 [97837]: rust-lang/rust#97837 [97867]: rust-lang/rust#97867 [cargo/10713]: rust-lang/cargo#10713 [cargo/10755]: rust-lang/cargo#10755 [`array::from_fn`]: https://doc.rust-lang.org/stable/std/array/fn.from_fn.html [`Box::into_pin`]: https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.into_pin [`BinaryHeap::try_reserve_exact`]: https://doc.rust-lang.org/stable/alloc/collections/binary_heap/struct.BinaryHeap.html#method.try_reserve_exact [`BinaryHeap::try_reserve`]: https://doc.rust-lang.org/stable/std/collections/struct.BinaryHeap.html#method.try_reserve [`OsString::try_reserve`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve [`OsString::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve_exact [`PathBuf::try_reserve`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve [`PathBuf::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve_exact [`Path::try_exists`]: https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.try_exists [`Ref::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.Ref.html#method.filter_map [`RefMut::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.RefMut.html#method.filter_map [`NonNull::<slice>::len`]: https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.len [`ToOwned::clone_into`]: https://doc.rust-lang.org/stable/std/borrow/trait.ToOwned.html#method.clone_into [`Ipv6Addr::to_ipv4_mapped`]: https://doc.rust-lang.org/stable/std/net/struct.Ipv6Addr.html#method.to_ipv4_mapped [`unix::io::AsFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/trait.AsFd.html [`unix::io::BorrowedFd<'fd>`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.BorrowedFd.html [`unix::io::OwnedFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.OwnedFd.html [`windows::io::AsHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsHandle.html [`windows::io::BorrowedHandle<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedHandle.html [`windows::io::OwnedHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedHandle.html [`windows::io::HandleOrInvalid`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrInvalid.html [`windows::io::HandleOrNull`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrNull.html [`windows::io::InvalidHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.InvalidHandleError.html [`windows::io::NullHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.NullHandleError.html [`windows::io::AsSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsSocket.html [`windows::io::BorrowedSocket<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedSocket.html [`windows::io::OwnedSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedSocket.html [`thread::scope`]: https://doc.rust-lang.org/stable/std/thread/fn.scope.html [`thread::Scope`]: https://doc.rust-lang.org/stable/std/thread/struct.Scope.html [`thread::ScopedJoinHandle`]: https://doc.rust-lang.org/stable/std/thread/struct.ScopedJoinHandle.html [`array::from_ref`]: https://doc.rust-lang.org/stable/std/array/fn.from_ref.html [`slice::from_ref`]: https://doc.rust-lang.org/stable/std/slice/fn.from_ref.html [`intrinsics::copy`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy.html [`intrinsics::copy_nonoverlapping`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy_nonoverlapping.html [`<*const T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to [`<*const T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping [`<*mut T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to-1 [`<*mut T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping-1 [`<*mut T>::copy_from`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from [`<*mut T>::copy_from_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from_nonoverlapping [`str::from_utf8`]: https://doc.rust-lang.org/stable/std/str/fn.from_utf8.html [`Utf8Error::error_len`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.error_len [`Utf8Error::valid_up_to`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.valid_up_to [`Condvar::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html#method.new [`Mutex::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.new [`RwLock::new`]: https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.new

rust-highfive assigned Amanieu May 11, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 11, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2022

rustbot added F-c_unwind `#![feature(c_unwind)]` T-lang Relevant to the language team, which will review and decide on the PR/issue. labels May 11, 2022

Prevent unwinding when -C panic=abort is used regardless declared ABI

c586bc3

nbdd0121 force-pushed the unwind branch from adbc464 to c586bc3 Compare May 11, 2022 23:04

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2022

bjorn3 reviewed May 12, 2022

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 12, 2022

Expand c-unwind-abi-panic-abort test

a315bb4

bjorn3 approved these changes May 12, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 12, 2022

GuillaumeGomez mentioned this pull request May 12, 2022

Rollup of 6 pull requests #96994

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 13, 2022

This comment has been minimized.

Sign in to view

Don't hardcode attribute id in codegen test

f86e409

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 17, 2022

bors merged commit 00755e4 into rust-lang:master May 17, 2022

rustbot added this to the 1.63.0 milestone May 17, 2022

nbdd0121 deleted the unwind branch May 17, 2022 22:19

eggyal mentioned this pull request Jun 15, 2022

Compiling std for UEFI with panic=abort fails #98133

Closed

programmerjake mentioned this pull request Sep 6, 2022

pthread_exit no longer works in panic=abort #101469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent unwinding when `-C panic=abort` is used regardless declared ABI #96959

Prevent unwinding when `-C panic=abort` is used regardless declared ABI #96959

nbdd0121 commented May 11, 2022 •

edited

Loading

Amanieu commented May 12, 2022

bors commented May 12, 2022

bjorn3 May 12, 2022

bjorn3 May 12, 2022

Amanieu May 12, 2022

nbdd0121 May 12, 2022 •

edited

Loading

bjorn3 commented May 12, 2022

nbdd0121 commented May 12, 2022

bjorn3 left a comment

Amanieu commented May 12, 2022

bors commented May 12, 2022

bors commented May 13, 2022

bors commented May 13, 2022

This comment has been minimized.

nbdd0121 commented May 17, 2022

Amanieu commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

rust-timer commented May 17, 2022

programmerjake commented Sep 6, 2022

Prevent unwinding when -C panic=abort is used regardless declared ABI #96959

Prevent unwinding when -C panic=abort is used regardless declared ABI #96959

Conversation

nbdd0121 commented May 11, 2022 • edited Loading

Amanieu commented May 12, 2022

bors commented May 12, 2022

bjorn3 May 12, 2022

Choose a reason for hiding this comment

bjorn3 May 12, 2022

Choose a reason for hiding this comment

Amanieu May 12, 2022

Choose a reason for hiding this comment

nbdd0121 May 12, 2022 • edited Loading

Choose a reason for hiding this comment

bjorn3 commented May 12, 2022

nbdd0121 commented May 12, 2022

bjorn3 left a comment

Choose a reason for hiding this comment

Amanieu commented May 12, 2022

bors commented May 12, 2022

bors commented May 13, 2022

bors commented May 13, 2022

This comment has been minimized.

nbdd0121 commented May 17, 2022

Amanieu commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

bors commented May 17, 2022

rust-timer commented May 17, 2022

programmerjake commented Sep 6, 2022

Prevent unwinding when `-C panic=abort` is used regardless declared ABI #96959

Prevent unwinding when `-C panic=abort` is used regardless declared ABI #96959

nbdd0121 commented May 11, 2022 •

edited

Loading

nbdd0121 May 12, 2022 •

edited

Loading