Abort when foreign exceptions are caught by catch_unwind #70212
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
Amanieu
force-pushed
the
catch_foreign
branch
2 times, most recently
from
3270761
to
5a677d8
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
For some context -- are we intending to land this or is this for experimentation? I think at this point I would personally expect that catch_unwind and similar in Rust does not act at all (i.e., is essentially invisible) to exceptions from other languages, though we may have special handlers at FFI boundaries to abort on foreign exceptions (depending on outcome of unwind WG RFCs, I guess). What is the behavior on the side of C++? Does I've not reviewed the implementation in great detail (I can do another pass later with more context). |
|
(Also, please try to limit additional builders on PRs to at most 5, to limit strain on our limited CI resources). |
|
In C++, The reason for this change is that there is code that relies on (I've removed the windows builders, it was just a one-off thing to check that I didn't break windows) |
|
Hm, the soundness argument is interesting. I note that catch_unwind currently gives somewhat ambiguous wording in whether it's correct to use for e.g. take_mut:
To me, this implies that not only are aborts not caught, but also that not even all unwinding in Rust is caught by catch_unwind. But it sounds like (especially after this PR), we would want to update the documentation for catch_unwind to say something like: "catch_unwind guarantees that program execution either halts (e.g., via an abort) or that the catch closure executes, if unwinding occurs within the try closure." -- I believe this or similar is the wording we would need for take_mut to be correct. There's also a middle ground which -- AFAICT, based on a brief skim -- this PR does not take: we can abort on non-Rust exceptions caught by catch_unwind instead of returning them in an opaque manner (as this PR does). Am I correct that this PR intends |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Is the inability to rethrow foreign exceptions with resume_unwind an inherent problem, or just something to be left for a follow up? |
|
It's a bit complicated. Basically libunwind-level exceptions ( C++ exceptions ( Now, we could do this and call into the C++ runtime when we catch a foreign exception, but then we add the C++ runtime as a dependency for all rust programs, which is probably unacceptable. |
|
All tests pass now, rebased & squashed. |
Mark-Simulacrum
approved these changes
Mar 25, 2020
|
I think before we land this, we should make sure that some team has signed off on this (perhaps lang?) -- I agree that we must catch the exception, but one could come up with a number of different ways of handling that, e.g., we could abort instead. I would also like to see the documentation for catch_unwind updated -- perhaps with my suggestion here: #70212 (comment), but please make sure that my suggestions there are indeed accurate :) Other than those two concerns, r=me |
Nominating -- if you both can attend to provide context and answer questions that'd be great. :) |
Centril
added
I-nominated
T-lang
|
I've added a brief blurb to the prior state to the PR description. I think I got it right :) Note that the only reason I suggested lang is due to overlap with the FFI unwind group (it is unclear to me what the right team here is -- I mainly don't want to be the one signing off on the new behavior :). |
|
@Mark-Simulacrum Lang is the correct group; project groups (such as FFI-unwind) have no formal decision-making power. |
|
I am hesitant to make any changes to the documentation that might imply that throwing foreign exceptions over Rust frames is not UB since the lang team has not yet made an official decision regarding FFI unwinding. This will probably require an RFC from the FFI-unwind group to resolve. |
|
The changes proposed in this PR reflect the current consensus of the FFI-unwind group as to how |
|
Hm, okay, then I'm less worried about getting signoff, I wasn't clear if you were intending this as a guarantee or not. However, I do think moving carefully here is worthwhile, and lang will meet tomorrow anyway. |
|
☔ The latest upstream changes (presumably #70536) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm not quite sure where we left this in our discussion at the meeting. It seemed like there was some concern about people coming to rely on this behavior, even if it's undocumented, and a slight preference towards something like a "hard abort"..? I tend to think that we ultimately want to offer some way to capture and rethrow a C++ exception as transparently as possible, and having it be converted into a rust panic doesn't seem very satisfying. (I appreciate this is hard to achieve technically.) |
|
I think it's possible to transparently handle C++ exceptions, I just haven't had time to work on this PR lately. However the resulting code is likely to be quite hacky since we need to check at runtime whether certain symbols exist using |
|
From what I remember, I suggested that if we were to land this before we had an RFC (under the "technically not documented, so just changing undefined behavior's actual behavior") we should do so with a hard abort rather than the return of a opaque I agree that in the ideal world we would probably want the capture of the foreign exception to permit re-throwing. I'm not sure if we'd want |
Mark-Simulacrum
approved these changes
Aug 27, 2020
|
This was discussed in a meeting a while back and the conclusion was that foreign exceptions unwinding into Rust is UB until further notice. This PR basically gives a nicer error message when this happens instead of silently allowing foreign unwinding through |
|
@Mark-Simulacrum Per the |
|
I do recall that meeting, but was unsure if there was any movement since then. I don't personally feel comfortable r+ing this without some form of lang/project group 👍 (though, IMO, @Amanieu or @BatmanAoD as involved members of the project group, you could give that approval -- I am uncertain if you meant to in the last two comments). cc @rust-lang/lang, too, I guess, though I don't expect that anyone has objections. |
|
Sorry, yes, my previous comment was meant to indicate approval. |
|
@bors r+ rollup=never We're early in the cycle, so this should get plenty of testing. Also marking as rollup=never because I want easy bisection to this PR if people note problems in their (large) codebases. |
|
📌 Commit 239f833 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
yvt
added a commit
to r3-os/r3
that referenced
this pull request
Sep 8, 2020
…ngjmp` `pthread_exit` can't unwind through `catch_unwind` anymore because of [1]. Because the actual `longjmp` isn't supported by Rust at this time and [3] is tricky to set up, this commit implements something similar using inline assembler. [1]: rust-lang/rust#70212 [2]: rust-lang/rfcs#2625 [3]: https://github.com/jeff-davis/setjmp.rs
yvt
added a commit
to r3-os/r3
that referenced
this pull request
Sep 8, 2020
…ngjmp` `pthread_exit` can't unwind through `catch_unwind` anymore because of [1]. Because the actual `longjmp` [2] isn't supported by Rust at this time and [3] is tricky to set up, this commit implements something similar using inline assembler. [1]: rust-lang/rust#70212 [2]: rust-lang/rfcs#2625 [3]: https://github.com/jeff-davis/setjmp.rs
|
How is this supposed to interact with thread cancellation on GNU/Linux, i.e. |
|
Thread cancellation is not safe. If the canceled thread used scoped threads from for example crossbeam or rayon, then the stack may be deallocated on thread cancellation when there is still another thread borrowing values from the stack of the canceled thread. |
|
Shouldn't crossbeam or rayon detect this, and not the core runtime? Note that glibc also pdeallocates thread stacks on |
|
Crossbeam and rayon have no way to detect this. Many libc functions have the possibility to trigger a canceled thread perform the cancelation. There is no cross-platform way for crossbeam and rayon to run any code before this happens. While |
|
How do crossbeam and rayon handle panics? They also unwind the stack, and the memory donated to these libraries must be deregistered as well before the stack frame goes away. |
|
Panics are handled by waiting for the child threads and then resuming unwinding. |
This is different from a forced unwind. As explained in this RFC:
|
wip-sync
pushed a commit
to NetBSD/pkgsrc-wip
that referenced
this pull request
Nov 24, 2020
Clean up some of the pkgsrc Makefile, there's still lots in here that
should just be deleted though. Switch SunOS to the illumos bootstrap
by default.
Version 1.48.0 (2020-11-19)
==========================
Language
--------
- [The `unsafe` keyword is now syntactically permitted on modules.][75857] This
is still rejected *semantically*, but can now be parsed by procedural macros.
Compiler
--------
- [Stabilised the `-C link-self-contained=<yes|no>` compiler flag.][76158] This tells
`rustc` whether to link its own C runtime and libraries or to rely on a external
linker to find them. (Supported only on `windows-gnu`, `linux-musl`, and `wasi` platforms.)
- [You can now use `-C target-feature=+crt-static` on `linux-gnu` targets.][77386]
Note: If you're using cargo you must explicitly pass the `--target` flag.
- [Added tier 2\* support for `aarch64-unknown-linux-musl`.][76420]
\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.
Libraries
---------
- [`io::Write` is now implemented for `&ChildStdin` `&Sink`, `&Stdout`,
and `&Stderr`.][76275]
- [All arrays of any length now implement `TryFrom<Vec<T>>`.][76310]
- [The `matches!` macro now supports having a trailing comma.][74880]
- [`Vec<A>` now implements `PartialEq<[B]>` where `A: PartialEq<B>`.][74194]
- [The `RefCell::{replace, replace_with, clone}` methods now all use `#[track_caller]`.][77055]
Stabilized APIs
---------------
- [`slice::as_ptr_range`]
- [`slice::as_mut_ptr_range`]
- [`VecDeque::make_contiguous`]
- [`future::pending`]
- [`future::ready`]
The following previously stable methods are now `const fn`'s:
- [`Option::is_some`]
- [`Option::is_none`]
- [`Option::as_ref`]
- [`Result::is_ok`]
- [`Result::is_err`]
- [`Result::as_ref`]
- [`Ordering::reverse`]
- [`Ordering::then`]
Cargo
-----
Rustdoc
-------
- [You can now link to items in `rustdoc` using the intra-doc link
syntax.][74430] E.g. ``/// Uses [`std::future`]`` will automatically generate
a link to `std::future`'s documentation. See ["Linking to items by
name"][intradoc-links] for more information.
- [You can now specify `#[doc(alias = "<alias>")]` on items to add search aliases
when searching through `rustdoc`'s UI.][75740]
Compatibility Notes
-------------------
- [Promotion of references to `'static` lifetime inside `const fn` now follows the
same rules as inside a `fn` body.][75502] In particular, `&foo()` will not be
promoted to `'static` lifetime any more inside `const fn`s.
- [Associated type bindings on trait objects are now verified to meet the bounds
declared on the trait when checking that they implement the trait.][27675]
- [When trait bounds on associated types or opaque types are ambiguous, the
compiler no longer makes an arbitrary choice on which bound to use.][54121]
- [Fixed recursive nonterminals not being expanded in macros during
pretty-print/reparse check.][77153] This may cause errors if your macro wasn't
correctly handling recursive nonterminal tokens.
- [`&mut` references to non zero-sized types are no longer promoted.][75585]
- [`rustc` will now warn if you use attributes like `#[link_name]` or `#[cold]`
in places where they have no effect.][73461]
- [Updated `_mm256_extract_epi8` and `_mm256_extract_epi16` signatures in
`arch::{x86, x86_64}` to return `i32` to match the vendor signatures.][73166]
- [`mem::uninitialized` will now panic if any inner types inside a struct or enum
disallow zero-initialization.][71274]
- [`#[target_feature]` will now error if used in a place where it has no effect.][78143]
- [Foreign exceptions are now caught by `catch_unwind` and will cause an abort.][70212]
Note: This behaviour is not guaranteed and is still considered undefined behaviour,
see the [`catch_unwind`] documentation for further information.
Internal Only
-------------
These changes provide no direct user facing benefits, but represent significant
improvements to the internals and overall performance of rustc and
related tools.
- [Building `rustc` from source now uses `ninja` by default over `make`.][74922]
You can continue building with `make` by setting `ninja=false` in
your `config.toml`.
- [cg_llvm: `fewer_names` in `uncached_llvm_type`][76030]
- [Made `ensure_sufficient_stack()` non-generic][76680]
[78143]: rust-lang/rust#78143
[76680]: rust-lang/rust#76680
[76030]: rust-lang/rust#76030
[70212]: rust-lang/rust#70212
[27675]: rust-lang/rust#27675
[54121]: rust-lang/rust#54121
[71274]: rust-lang/rust#71274
[77386]: rust-lang/rust#77386
[77153]: rust-lang/rust#77153
[77055]: rust-lang/rust#77055
[76275]: rust-lang/rust#76275
[76310]: rust-lang/rust#76310
[76420]: rust-lang/rust#76420
[76158]: rust-lang/rust#76158
[75857]: rust-lang/rust#75857
[75585]: rust-lang/rust#75585
[75740]: rust-lang/rust#75740
[75502]: rust-lang/rust#75502
[74880]: rust-lang/rust#74880
[74922]: rust-lang/rust#74922
[74430]: rust-lang/rust#74430
[74194]: rust-lang/rust#74194
[73461]: rust-lang/rust#73461
[73166]: rust-lang/rust#73166
[intradoc-links]: https://doc.rust-lang.org/rustdoc/linking-to-items-by-name.html
[`catch_unwind`]: https://doc.rust-lang.org/std/panic/fn.catch_unwind.html
[`Option::is_some`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.is_some
[`Option::is_none`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.is_none
[`Option::as_ref`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.as_ref
[`Result::is_ok`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.is_ok
[`Result::is_err`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.is_err
[`Result::as_ref`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.as_ref
[`Ordering::reverse`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.reverse
[`Ordering::then`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.then
[`slice::as_ptr_range`]: https://doc.rust-lang.org/std/primitive.slice.html#method.as_ptr_range
[`slice::as_mut_ptr_range`]: https://doc.rust-lang.org/std/primitive.slice.html#method.as_mut_ptr_range
[`VecDeque::make_contiguous`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.make_contiguous
[`future::pending`]: https://doc.rust-lang.org/std/future/fn.pending.html
[`future::ready`]: https://doc.rust-lang.org/std/future/fn.ready.html
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Jan 1, 2021
Pkgsrc changes:
* Compensate for files being moved around upstream.
* Introduce optional, on-by-default semi-static building of cargo,
using the internal curl and openssl sources. This reduces the dynamic
dependencies of cargo and therefore the rust package itself.
Ref. options.mk.
* The 1.47.0 bootstrap kits have been re-built with the above option
turned on, so no longer depends on curl or openssl from pkgsrc and/or
from earlier OS or pkgsrc versions. This should hopefully fix
installation of rust with non-default PREFIX, ref. PR#54453.
Upstream changes:
Version 1.48.0 (2020-11-19)
==========================
Language
--------
- [The `unsafe` keyword is now syntactically permitted on modules.][75857] This
is still rejected *semantically*, but can now be parsed by procedural macros.
Compiler
--------
- [Stabilised the `-C link-self-contained=<yes|no>` compiler flag.][76158]
This tells `rustc` whether to link its own C runtime and libraries
or to rely on a external linker to find them. (Supported only on
`windows-gnu`, `linux-musl`, and `wasi` platforms.)
- [You can now use `-C target-feature=+crt-static` on `linux-gnu` targets.]
[77386]
Note: If you're using cargo you must explicitly pass the `--target` flag.
- [Added tier 2\* support for `aarch64-unknown-linux-musl`.][76420]
\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.
Libraries
---------
- [`io::Write` is now implemented for `&ChildStdin` `&Sink`, `&Stdout`,
and `&Stderr`.][76275]
- [All arrays of any length now implement `TryFrom<Vec<T>>`.][76310]
- [The `matches!` macro now supports having a trailing comma.][74880]
- [`Vec<A>` now implements `PartialEq<[B]>` where `A: PartialEq<B>`.][74194]
- [The `RefCell::{replace, replace_with, clone}` methods now all use
`#[track_caller]`.][77055]
Stabilized APIs
---------------
- [`slice::as_ptr_range`]
- [`slice::as_mut_ptr_range`]
- [`VecDeque::make_contiguous`]
- [`future::pending`]
- [`future::ready`]
The following previously stable methods are now `const fn`'s:
- [`Option::is_some`]
- [`Option::is_none`]
- [`Option::as_ref`]
- [`Result::is_ok`]
- [`Result::is_err`]
- [`Result::as_ref`]
- [`Ordering::reverse`]
- [`Ordering::then`]
Cargo
-----
Rustdoc
-------
- [You can now link to items in `rustdoc` using the intra-doc link
syntax.][74430] E.g. ``/// Uses [`std::future`]`` will automatically generate
a link to `std::future`'s documentation. See ["Linking to items by
name"][intradoc-links] for more information.
- [You can now specify `#[doc(alias = "<alias>")]` on items to add
search aliases when searching through `rustdoc`'s UI.][75740]
Compatibility Notes
-------------------
- [Promotion of references to `'static` lifetime inside `const fn`
now follows the same rules as inside a `fn` body.][75502] In
particular, `&foo()` will not be promoted to `'static` lifetime
any more inside `const fn`s.
- [Associated type bindings on trait objects are now verified to meet the bounds
declared on the trait when checking that they implement the trait.][27675]
- [When trait bounds on associated types or opaque types are ambiguous, the
compiler no longer makes an arbitrary choice on which bound to use.][54121]
- [Fixed recursive nonterminals not being expanded in macros during
pretty-print/reparse check.][77153] This may cause errors if your macro wasn't
correctly handling recursive nonterminal tokens.
- [`&mut` references to non zero-sized types are no longer promoted.][75585]
- [`rustc` will now warn if you use attributes like `#[link_name]` or `#[cold]`
in places where they have no effect.][73461]
- [Updated `_mm256_extract_epi8` and `_mm256_extract_epi16` signatures in
`arch::{x86, x86_64}` to return `i32` to match the vendor signatures.][73166]
- [`mem::uninitialized` will now panic if any inner types inside
a struct or enum disallow zero-initialization.][71274]
- [`#[target_feature]` will now error if used in a place where it
has no effect.][78143]
- [Foreign exceptions are now caught by `catch_unwind` and will
cause an abort.][70212] Note: This behaviour is not guaranteed
and is still considered undefined behaviour, see the [`catch_unwind`]
documentation for further information.
Internal Only
-------------
These changes provide no direct user facing benefits, but represent significant
improvements to the internals and overall performance of rustc and
related tools.
- [Building `rustc` from source now uses `ninja` by default over `make`.][74922]
You can continue building with `make` by setting `ninja=false` in
your `config.toml`.
- [cg_llvm: `fewer_names` in `uncached_llvm_type`][76030]
- [Made `ensure_sufficient_stack()` non-generic][76680]
[78143]: rust-lang/rust#78143
[76680]: rust-lang/rust#76680
[76030]: rust-lang/rust#76030
[70212]: rust-lang/rust#70212
[27675]: rust-lang/rust#27675
[54121]: rust-lang/rust#54121
[71274]: rust-lang/rust#71274
[77386]: rust-lang/rust#77386
[77153]: rust-lang/rust#77153
[77055]: rust-lang/rust#77055
[76275]: rust-lang/rust#76275
[76310]: rust-lang/rust#76310
[76420]: rust-lang/rust#76420
[76158]: rust-lang/rust#76158
[75857]: rust-lang/rust#75857
[75585]: rust-lang/rust#75585
[75740]: rust-lang/rust#75740
[75502]: rust-lang/rust#75502
[74880]: rust-lang/rust#74880
[74922]: rust-lang/rust#74922
[74430]: rust-lang/rust#74430
[74194]: rust-lang/rust#74194
[73461]: rust-lang/rust#73461
[73166]: rust-lang/rust#73166
[intradoc-links]: https://doc.rust-lang.org/rustdoc/linking-to-items-by-name.html
[`catch_unwind`]: https://doc.rust-lang.org/std/panic/fn.catch_unwind.html
[`Option::is_some`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.is_some
[`Option::is_none`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.is_none
[`Option::as_ref`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.as_ref
[`Result::is_ok`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.is_ok
[`Result::is_err`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.is_err
[`Result::as_ref`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.as_ref
[`Ordering::reverse`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.reverse
[`Ordering::then`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.then
[`slice::as_ptr_range`]: https://doc.rust-lang.org/std/primitive.slice.html#method.as_ptr_range
[`slice::as_mut_ptr_range`]: https://doc.rust-lang.org/std/primitive.slice.html#method.as_mut_ptr_range
[`VecDeque::make_contiguous`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.make_contiguous
[`future::pending`]: https://doc.rust-lang.org/std/future/fn.pending.html
[`future::ready`]: https://doc.rust-lang.org/std/future/fn.ready.html
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this PR, foreign exceptions were not caught by catch_unwind, and instead passed through invisibly. This represented a painful soundness hole in some libraries (take_mut), which relied on
catch_unwindto handle all possible exit paths from a closure.With this PR, foreign exceptions are now caught by
catch_unwindand will trigger an abort since catching foreign exceptions is currently UB according to the latest proposals by the FFI unwind project group.cc @rust-lang/wg-ffi-unwind