Skip to content
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

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

Merged
merged 3 commits into from
May 17, 2022

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented May 11, 2022

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

@rustbot 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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2022
@rustbot 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
@Amanieu
Copy link
Member

Amanieu commented May 12, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 12, 2022

📌 Commit c586bc3 has been approved by Amanieu

@bors 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
@@ -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{{.*}} }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@nbdd0121 nbdd0121 May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

bjorn3 commented May 12, 2022

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-

@bors 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
@nbdd0121
Copy link
Contributor Author

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

Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Amanieu
Copy link
Member

Amanieu commented May 12, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 12, 2022

📌 Commit a315bb4 has been approved by Amanieu

@bors 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 added a commit to GuillaumeGomez/rust that referenced this pull request May 12, 2022
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
Copy link
Contributor

bors commented May 13, 2022

⌛ Testing commit a315bb4 with merge ba34d4dfc9f16a4ece928ffb324cc298dcff5f0e...

@bors
Copy link
Contributor

bors commented May 13, 2022

💔 Test failed - checks-actions

@bors 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
@rust-log-analyzer

This comment has been minimized.

@nbdd0121
Copy link
Contributor Author

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

@Amanieu
Copy link
Member

Amanieu commented May 17, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 17, 2022

📌 Commit f86e409 has been approved by Amanieu

@bors 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
Copy link
Contributor

bors commented May 17, 2022

⌛ Testing commit f86e409 with merge 00755e4...

@bors
Copy link
Contributor

bors commented May 17, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 17, 2022
@bors bors merged commit 00755e4 into rust-lang:master May 17, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 17, 2022
@rust-timer
Copy link
Collaborator

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
Copy link
Member

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

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 10, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-c_unwind `#![feature(c_unwind)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants