Abort instead of unwinding past FFI functions
SimonSapin opened this issue · 66 comments
More updated description (2021-03-11)
With #76570 having landed, Rust now aborts when a panic leaves an extern "C" function, so the original soundness issue is fixed. However, there is an attribute to opt-out of this behavior, #[unwind(allowed)]. Using this attribute is currently unsound, so it should be fixed, removed, or require some form of unsafe.
Updated Description
This is a tracking issue for switching the compiler to abort the program whenever a Rust program panics across an FFI boundary, notably causing any non-Rust-ABI function to panic/unwind.
This is being implemented in #55982 and some regressions were found in the wild so this is also being repurposed as a tracking issue for any breakage that comes up and a place to discuss the breakage. If you're a crate author and you find your way here, don't hesitate to ask questions!
Original Description
Doing Rust unwinding into non-Rust stack frames is undefined behavior. This was fixed in 1.24.0, and then reverted (I think by #48380 ?) in 1.24.1 because of a regression that affected rlua.
The latter blog post said:
The solution here is to generate the abort handler, but in a way that
longjmpwon’t trigger it. It’s not 100% clear if this will make it into Rust 1.25; if the landing is smooth, we may backport, otherwise, this functionality will be back in 1.26.
The link PR has landed, but my understanding is that it does not change FFI functions back to aborting on unwind (though it looks like it does fix the issue that affected rlua).
This UB is not mentioned in any later release notes.
This issue has been assigned to @BatmanAoD via this comment.
cc me
cc @diwic
I think @SimonSapin your description is accurate. The backport to stable and backport to beta were both minor patches, and the main one to nightly (at the time) was #48380 as you mentioned.
AFAIK we should be good to go again to do this.
I think I would prefer that instead of immediately landing a change to the default unwind behavior we just stabilize the unwind attribute and wait for a few cycles to let the ecosystem migrate to unwind(allowed) or unwind(abort) across extern "C" functions. It would be nice to have some docs providing guidance here as well.
Why is this attribute useful? If the issue that affected rlua is fixed, what can make the answer to "do you want to protect against some potential UB" be anything other than "yes, obviously"?
There are cases where unwinding is fine, for example if you are using a extern "C" function to talk between different Rust crates or extern "Rust" functions to do the same. There are cases where you legitimately are fine with unwinding (and IMO it shouldn't be UB). I don't think the decision whether unwinding should abort or continue should be based on the extern-ness of the function. unwind(allowed) or unwind(aborts) may be useful in normal Rust as well if you want to do -Cpanic=abort on a smaller (non-library) scale, though it's less useful to an extent in that case due to catch_unwind existing.
Good to see some movement here.
For me it's bikeshedding whether we go with #[unwind], #[unwind(allowed)] or even extern "C-unwind" as long as we default to aborting for every non-Rust ABI. But it does seem it would be helpful for the libjpeg / libpng bindings.
Alright, stabilizing some attribute (perhaps with unsafe in its name) to support this libjpeg use case sounds fine, but I don’t see a reason to wait several cycles to change the default back when the plan was to do it in 1.25.
The primary point we reverted back then was because of breakage due to changing the default. I'm not convinced that the breakage would not happen again; there's currently no way to explicitly document your requirements on stable. I see this as the same situation as we normally have for library features, where the unstable/deprecated solution isn't removed until the stable alternative lands in stable.
I also am unclear why we would/should rush this. People have waited, people can continue to wait. Stabilizing unwind seems uncontroversial; we can do so immediately.
unwind(allowed) or unwind(aborts) may be useful in normal Rust as well if you want to do -Cpanic=abort on a smaller (non-library) scale, though it's less useful to an extent in that case due to catch_unwind existing.
I'm not sure what this means, but if -Cpanic=abort is specified I don't think you should be able to override that with a #[unwind(allowed)].
I also am unclear why we would/should rush this. People have waited, people can continue to wait.
Well, why should fixing I-unsound bugs have high priority in general? I think they should have high priority, but not everyone shares that sentiment.
Stabilizing unwind seems uncontroversial; we can do so immediately.
Are there some unresolved questions here w r t:
- Which ABIs should allow
#[unwind]? Should we do this for all ABIs, or just "C"? - In a
-Cpanic=abortscenario should#[unwind]be able to override this for Rust ABI and nevertheless allow unwinding? (I prefer not.)
I'm not sure what this means, but if -Cpanic=abort is specified I don't think you should be able to override that with a #[unwind(allowed)].
I mean the reverse; if you've specified -Cpanic=unwind but for whatever reason want a specific function to not unwind, you can today do so with catch_unwind but I could see us also supporting #[unwind(abort)] annotations as an alternative. I'm not sure there's a need, though.
Well, why should fixing I-unsound bugs have high priority in general? I think they should have high priority, but not everyone shares that sentiment.
This isn't an unsoundness bug though. It is currently UB to unwind through stack frames not generated by the same Rust compiler (and will always be, at least in the short term) but I don't see how that is unsound. I've removed that tag.
Are there some unresolved questions here w r t ...
I'd be happy stabilizing #[unwind] for all extern functions regardless of ABI; it seems like a feature orthogonal to the ABI. I don't think it's possible to make #[unwind] override -Cpanic=abort anyway, or at least not trivial. That is to say, it shouldn't be supported, but perhaps should also not be an error since there's not currently a way to cfg around -Cpanic...
This isn't an unsoundness bug though
It is indeed not rustc or std that is unsound, but the current situation is that it is very difficult to write sound FFI code. You need to carefully audit every callback either to convince yourself that it cannot ever panic, or use catch_unwind (again in every callback) and then carefully audit the code that is outside of the catch_unwind closure that wraps stuff to be UnwindSafe or deals with the returned Result. (Don’t use .unwrap() !)
So I’d say this is a soundness-related issue.
I don't disagree; writing sound FFI code is hard. I'm just trying to say that changing the default without providing a way to use the old behavior seems wrong and I'm against doing it. Furthermore, I'd prefer at least one cycle that users can explicitly opt-in to unwind(allowed) for FFI functions where unwinding isn't UB (i.e., they're unwinding into Rust code). I'm okay (though not happy) with us making a change to the default behavior so long as the opt-out is stabilized along side it.
This isn't an unsoundness bug though.
Oh yes it is. Please don't remove I-unsound tags without thoroughly understanding the issue.
-
Everything that is UB is also unsound, simply because the behavior is undefined, i e, not defined as being sound.
-
Extern C functions (as well as all other non-Rust ABIs IIRC) are - unless
#[unwind]- marked asnounwindin the LLVM IR. Unwinding from a nounwind function is UB, according to LLVM docs.
I mean the reverse; if you've specified -Cpanic=unwind but for whatever reason want a specific function to not unwind, you can today do so with catch_unwind but I could see us also supporting #[unwind(abort)] annotations as an alternative. I'm not sure there's a need, though.
Okay. I don't think there is much need. Maybe it would make the crate take_mut use a fewer less characters.
I'd be happy stabilizing #[unwind] for all extern functions regardless of ABI; it seems like a feature orthogonal to the ABI.
I see unwinding as part of the ABI, but it's possible (I'm not sure) that for all practical purposes one can see it as an orthogonal feature.
the current situation is that it is very difficult to write sound FFI code. You need to carefully audit every callback either to convince yourself that it cannot ever panic, or use catch_unwind (again in every callback) and then carefully audit the code that is outside of the catch_unwind closure that wraps stuff to be UnwindSafe or deals with the returned Result. (Don’t use .unwrap() !)
Indeed. I even found such a mistake in std once. And just to clarify, this is the practical reason why were doing the "abort" solution rather than just stop marking C ABI functions as nounwind in LLVM.
@diwic I would classify a "soundness bug" as a case where "safe Rust" can generate UB. Are you saying that this is the case here? (I'm legitimately unsure)
But in any case I think that @Mark-Simulacrum's suggestion of "do not deprecate without at least offering some way to get back the old default" makes sense to me. I think I share these sentiments pretty much exactly:
I'm just trying to say that changing the default without providing a way to use the old behavior seems wrong and I'm against doing it. Furthermore, I'd prefer at least one cycle that users can explicitly opt-in to unwind(allowed) for FFI functions where unwinding isn't UB (i.e., they're unwinding into Rust code). I'm okay (though not happy) with us making a change to the default behavior so long as the opt-out is stabilized along side it.
I would classify a "soundness bug" as a case where "safe Rust" can generate UB. Are you saying that this is the case here? (I'm legitimately unsure)
Yes, this is UB as of current Rust:
extern "C" fn bad() { panic!() }
fn main() { bad() }
Here's the LLVM IR (notice the nounwind attr) :
; playground::bad
; Function Attrs: nounwind uwtable
define internal void @_ZN10playground3bad17hd96a1f8f707dc090E() unnamed_addr #3 !dbg !676 {
start:
; call std::panicking::begin_panic
call void @_ZN3std9panicking11begin_panic17h23bac1aac94eb1e7E([0 x i8]* noalias nonnull readonly bitcast (<{ [14 x i8] }>* @byte_str.7 to [0 x i8]*), i64 14, { [0 x i64], { [0 x i8]*, i64 }, [0 x i32], i32, [0 x i32], i32, [0 x i32] }* noalias readonly dereferenceable(24) bitcast (<{ i8*, [16 x i8] }>* @byte_str.6 to { [0 x i64], { [0 x i8]*, i64 }, [0 x i32], i32, [0 x i32], i32, [0 x i32] }*)), !dbg !678
unreachable, !dbg !678
}
nounwind
This function attribute indicates that the function never raises an exception. If the function does raise an exception, its runtime behavior is undefined.
I've opened a PR for this at #55982 which does the "easy thing" of basically flipping the defaults.
I personally disagree that we need to take any further action at this time. We've got a long history of a "stable default" in Rust where the opt-out is unstable (allocators are the first that come to mind). Eventually the opt-out is stabilized in its own right, but for now we have an unstable control (the #[unwind] attribute).
Additionally I don't think this can really bake without actually testing, this will remain virtually undetected unless everyone opts-in to testing it, so the only real way to get testing is to actually flip the defaults and see what happens. That's what we did last time and it's easy to always flip the defaults back if something comes up!
Ok we've executed a crater run for this issue on #55982 and it's finished but has some regressions. I'm cc'ing the authors of the crates here to make sure they're aware of this! The current plan is to land #55982 after December 6, making it part of Rust 1.33 to be released on February 28, 2019.
If those cc'd here have difficulty fixing the issues here or have any questions/concerns, please don't hesitate to comment here!
- cc @hean01 as the
mujscrate has broken, where this function now always aborts - cc @rustcc, as the
coroutinecrate's tests are broken as this function now aborts - cc @valarauca, as the
consistenttimecrate now can abort here - cc @PJB3005 as the
byondcrate can abort here
For my crate (byond) I personally don't care that much. The panic is always gonna unwind into third party C++ so abort is fine by me (better than what was probably undefined behavior, at least).
Keeping the unit test would be preferable but I'm not sure how to write a macro to keep them (I'm thinking having an extern "C" exposed symbol for the FFI and then an internal Rust one the unit test can run, but I can't think of a way to do that without the ability to concatenate macro identifiers which isn't possible..)
FYI consistenttime does not work as advertised and its README states that people should prefer alternatives so hopefully nobody will be effected. I would remove the project from crates.io but that isn't possible apparently 😭
Also I'm not using the FFI, just a different calling convention. So the title/issue may want to be updated(?). I'm just using the C calling convention as it is easier to reason about register access for assembly heavy code with a known calling convention.
@PJB3005 oh so if it's fundamentally going into C++ you've got two choices:
- Catch the panic and return an error into C++, re-raising the Rust panic at some point (optionally)
- Catch the panic and unconditionally abort the process (but manually), perhaps printing an error message
You could also leave it as-is with aborting the process via the rustc-generated code, but that may not have the best errors.
@valarauca oh true! This definitely applies to extern fn of all ABIs, not just FFI in general.
Can the #[unwind(allowed)] attribute be stabilized soon? This is forcing me to write a lot more code in C++ than I would like.
Reopening since we reverted on stable (but not beta & nightly).
Decision about next stable is tracked in #58794.
@Centril could you link to where the decision to revert was made? (Hopefully with discussion of why?) I thought we’d already landed this change and reverted it once several releases ago, and landed it again more recently.
@SimonSapin The release team decided to revert because there was no explicit decision to go ahead with the change from the language team. See #58794 for details. I am working on an FCP to confirm the change for 1.34 and beyond.
Is there an RFC or a PR specifying the semantics of this change ?
From reading the conversations, extern functions are required to abort in case of a panic, but this isn't necessarily the same as, e.g., guaranteeing that the semantics are equivalent to wrapping each extern function in a catch_unwind, and then aborting in case of a panic.
For example, can:
extern "C" fn foo(x: i32) -> i32 {
if x < 0 { panic!() }
x
}be optimized into
extern "C" fn foo(x: i32) -> i32 { x }?
AFAICT, this optimization would not result in a panic unwinding across an extern function boundary, so it would satisfy that criteria.
I wrote here that, a way to close this soundness hole on safe stable Rust, without any language changes, without any performance regressions, and without unconditionally making it impossible to unwind through FFI on stable Rust would be to just enable abort-on-panic for safe extern "C" FFI functions.
This allow us to continue emitting nounwind everywhere (no perf regression), and it allows those users that want to continue to invoke undefined behavior on stable Rust to port their code to just use unsafe extern "C" types instead. For mozjpeg, AFAICT, this would be a one line change in mozjpeg-rust.
Note that this does not give any new meaning to the unsafe keyword: the behavior of unwinding out of an unsafe extern "C" function is still undefined, and providing a way to unwind through FFI without UB would still require an RFC with new language features that allow doing that.
The only thing we would do is temporarily not abort your program if you unwind out of an unsafe extern "C" function, we would still continue to optimize it under the assumption that the code does not do this, but current users appear to be able to work with this.
Oh, so you are not suggesting to change anything about nounwind, just not add the abort-on-panic shim only for unsafe extern "C"?
Exactly, and this is enough to close this soundness bug.
This does not help at all with the issue #63909 was opened to fix. Specifically, everything I said here still applies.
Your PR does many things, what it doesn't do, is close this soundness bug. We could do some of those things as well (fixing #[unwind], maybe disabling nounwind temporarily for unsafe extern "C" definitions or for all extern "C" ABIs, etc.), but I don't think closing this soundness bug should be blocked on doing those.
Does anyone knows what is the current situation? because this seem to contradict the reference:
Functions with an ABI that differs from "Rust" do not support unwinding in the exact same way that Rust does. Therefore, unwinding past the end of functions with such ABIs causes the process to abort.
https://github.com/rust-lang/reference/blob/master/src/items/functions.md#extern-function-qualifier
As far as I have been able to tell: It was specified to abort in 1.24, reverted in 1.24.1, specified to abort again and reverted before 1.33 was released, and currently is undefined in stable, but aborts in beta/nightly by default (you can specify the behavior you want explicitly in nightly). The current documentation still says it aborts even though that's not the case in stable.
I might be wrong about that summary. I don't have context on this and had to dig through several issues and commit history to figure that out. But if I'm correct, I agree, I think the documentation here needs to be updated to just state that it's undefined behavior, or to mention that aborting is only the default in not-stable.
I'm utterly perplexed that this soundness bug has not been fixed more than two five years after this ticket was opened.
If panic = 'abort' worked as we might expect, then the most practical way to work around the safety issue would be to add this to Cargo.toml in every crate that might directly or indirectly use FFI:
[profile.release]
panic = 'abort'
[profile.dev]
panic = 'abort'
I say "the most practical way" because, although technically it is possible to wrap every extern function in a catch_unwind and abort in that case, that's a poor solution. The whole point of panics in Rust is to provide safe behaviour for unanticipated cases. If you anticipated a panic in a particular case then you'd change the code to eliminate that case. Always using catch_unwind is more verbose than just setting the default as above, and has no advantages that I can see. Also if a crate you're (maybe indirectly) depending on doesn't do that, then you need to either fork it or use the above Cargo.toml approach anyway.
However,
[profile.*]only takes effect in the top-level crate in which cargo is run (I think). As a library crate provider you have no control, apart from documenting it, over whetherpanic = 'abort'will be set at top level.panic = 'abort'is ignored fortestprofiles (unless we set the unstable panic-abort-tests flag), and also forbenchprofiles. So we can't properly test (at least on stable) the abort-on-panic behaviour in integration tests, and can't easily make sure that tests aren't accidentally invoking UB, especially for large test suites of mixed-language programs. Tests that invoke UB are very bad because it can hide failures.
This issue is an unnecessary concealed hazard for anyone (directly or indirectly) using extern functions. "Concealed" because there's no reason anyone would know about it when starting to write FFI code –which is hard enough already– and even if they did they can easily end up being misled by the incorrect documentation or the confusing history of this issue.
Just fix it already in the obvious way, and live with breaking the few things that will break. (I could live with having the safe behaviour be the default and having an --unsafe-unwind-across-ffi option, if others think that would help to get this change done, even though it would essentially always be a bad idea to use that option.)
@daira: You can effectively already do this in nightly because you have #[unwind(abort)] there, but my impression from the reading I did yesterday is that the team is reluctant to stabilize it, and wants to wait for ffi-unwind to spec out a extern "C unwind" ABI instead so that extern "C" can just abort-on-unwind in all cases.
I agree that this is a significant hole. I actually hit the issue you mentioned of not being able to guarantee that panic = 'abort' will apply to my library. (I am working inside a cargo workspace.) In the end I just created and am using a short catch_unwind wrapper. I think I've even seen a crate that turns this into an attribute you can just apply to the function and it'll wrap it for you, (Though I don't recall what it was called.) so there are at least somewhat workable workarounds in the meantime.
// Unwinding across a FFI barrier is undefined behavior. A simple way to enforce this is by turning
// all panics into aborts. In the future when/if https://github.com/rust-lang/rust/issues/58760 is
// stabilized the function annotations should probably be used instead.
fn abort_on_unwind<F: FnOnce() -> R, R>(f: F) -> R {
std::panic::catch_unwind(
// Catching a panic will always immediately abort the program, so there is never a chance
// that any non-UnwindSafe value will be observed afterwards.
std::panic::AssertUnwindSafe(f),
)
.unwrap_or_else(|_| std::process::abort())
}I emphatically agree that the fact that this is not very loudly mentioned in all documentation that mentions extern "C" functions is a huge footgun.
The C-unwind RFC was accepted a few months ago, which will allow this issue to be resolved. You can see the progress at #74990.
Just fix it already in the obvious way, and live with breaking the few things that will break.
@daira This was attempted twice, but reverted due to the breakage it caused. See #58794 for more details
It caused breakage in a small number of crates, and this led to maintaining an unsafe situation for all crates that use FFI (unless they use ugly and poorly documented workarounds), for two years nearly six years. That policy was and is wrong, IMHO.
It's worth pointing out that, as far as I read, the authors of the broken crates explicitly said that they had acceptable workarounds or didn't care, and none of them actually argued that this change should be held back for the rest of the ecosystem on account of their code (in fact some argued the opposite). Also, a previous fix was only undone for procedural reasons.
I appreciate people's efforts on #74990 etc., but I'm taking the effort to comment here because I strongly believe that similar issues should not be handled the same way in future. I use Rust because it's a safe language (compared to alternatives), not because it's possible to do obscure UB hacks like cross-language setjmp/longjmp. I obviously can't speak for the rest of the Rust community, but I don't think this is likely to be a minority position.
It's worth pointing out that, as far as I read, the authors of the broken crates explicitly said that they had acceptable workarounds or didn't care, and none of them actually argued that this change should be held back for the rest of the ecosystem on account of their code (in fact some argued the opposite). Also, a previous fix was only undone for procedural reasons.
There were other crates affected by this, including ones with no available workarounds. Also, the workaround suggested for rlua was possibly worse than this, as it completly subvert Rust's stability guarantee and it's an implementation detail only supposed to be used by the compiler itself. I would personally never suggest to a crate author to use it, and there were proposals to forbid that.
How long are we going to prioritize broken C libraries with broken error handling designs over the safety of the rest of the Rust ecosystem?
https://www.cvedetails.com/vulnerability-list/vendor_id-7294/Libpng.html
https://www.cvedetails.com/vulnerability-list/vendor_id-17990/product_id-46165/IJG-Libjpeg.html
@daira In the case of libpng, the situation is complicated. libpng is the reference implementation and, with APNG being an explicit violation of the PNG spec, people tend to create alternative implementations that, by design, violate the spec and can't supplant the reference implementation.
The first eight bytes of a PNG datastream always contain the following (decimal) values:
137 80 78 71 13 10 26 10This signature indicates that the remainder of the datastream contains a single PNG image, consisting of a series of chunks beginning with an IHDR chunk and ending with an IEND chunk.
b. The reference image, which only exists conceptually, is a rectangular array of rectangular pixels, all having the same width and height, and all containing the same number of unsigned integer samples, either three (red, green, blue) or four (red, green, blue, alpha). [...]
c. The PNG image is obtained from the reference image by a series of transformations: alpha separation, indexing, RGB merging, alpha compaction, and sample depth scaling. [...]
PNG also does not support multiple images in one file. This restriction is a reflection of the reality that many applications do not need and will not support multiple images per file. In any case, single images are a fundamentally different sort of object from sequences of images. Rather than make false promises of interchangeability, we have drawn a clear distinction between single-image and multi-image formats. PNG is a single-image format.
TL;DR: PNG was explicitly designed to avoid the "Is this GIF animated?" problem, so they insist that you use a different file header for animated content... Mozilla broke the PNG spec because the whole point of APNG is to re-create the "If we don't understand APNG, display the first frame" effect.
I just wanted to call out that I feel like the discussion here is getting somewhat unproductive. Ultimately Rust needs to meet the needs of its users. If there are people who want or need to use certain C libraries to do what they want, and the Rust project considers supporting them as being part of their scope, then breaking their usage also doesn't help advance the goals of the project, regardless of if those libraries or code can be considered "bad" or not.
Right now, the reverts already happened, and any course of action will make someone unhappy. I was happy to learn that the C-unwind RFC (#74990) had already been accepted, I hadn't noticed that and was under the impression that the ffi-unwind WG was still deliberating on how to solve this. Furthermore, there's already an implementation happening in #76570, so chances are that we'll have a solution that should work for everyone in the near future. In light of that, making breaking changes like this right now would just cause additional churn and controversy.
Like I said before, I sympathize with the concerns, but to me it seems like things are already moving in the direction of having this issue resolved soon.
Another 4 months with no fix to a problem that causes widespread unsoundness. This is due to bad policy, and people making excuses for it.
@daira Rust is an open source project, and the developers are largely volunteers who work on it in their own, personal, limited free time. People have been working on this. If you haven't been an active participant in the discussions or efforts to implement this then I don't think you can make accusations like that. And even if you have it's unnecessarily rude.
Maybe we should leave the issue open while #[unwind(allowed)] still exists, which is an unsound attribute.
Do normal extern "C" functions now abort always on unwinding, and extern "C-unwind" unwind as the name suggests? It wasn't clear to me from that PR if that is also implemented as part of that already.
Do normal extern "C" functions now abort always on unwinding
extern "C" functions defined in Rust automatically get an abort-on-unwind shim, yes. For functions defined in other languages and called from Rust, it is your responsibility to ensure that they do not unwind.
I thought the #[unwind] attribute was unsound, but that does not actually seem to be the case? Both the nounwind attribute logic and the abort-on-unwind-shim logic seem to correctly honor this attribute.
However, what is indeed unsound is -Cpanic=abort:
#![feature(c_unwind)]
extern "C-unwind" {
fn test();
}
fn main() {
unsafe { test(); }
}test should be allowed to unwind, but when building this code with -Cpanic_abort, the LLVM IR contains nounwind attributes:
; Function Attrs: nounwind nonlazybind
declare void @test() unnamed_addr #1
I opened a new issue for the panic=abort problem: #83116
Is there anything else that needs to be tracked here?
extern "C"functions defined in Rust automatically get an abort-on-unwind shim, yes. For functions defined in other languages and called from Rust, it is your responsibility to ensure that they do not unwind.
Ok, so that's also implemented as part of that PR? Then I don't think anything else is needed here.
Right now in stable it's still UB to unwind across extern "C" functions. Usually it aborts, sometimes it does very interesting things.
I didn't notice anything in the PR description or the commit messages that suggested that it adds something to ensure that it always aborts, that's why I was asking.
Ok, so that's also implemented as part of that PR?
Yes. This is controlled by this function.
Thanks for the confirmation, that's great news. I'm waiting for that to come back for years now :)
Me too. :)
Is it possible to now write somewhere "official" that unwinding across extern "C" is well defined to abort?
The abort behavior is currently only enabled with #![feature(c_unwind)].
(I'm assigning @BatmanAoD as owner of this issue, in accordance with T-compiler's shift towards a policy where all P-high/P-critical issues are either tagged with a WG-* label or have an explicit owner who can be contacted for status updates.)
@rustbot assign @BatmanAoD
Maybe we should leave the issue open while
#[unwind(allowed)]still exists, which is an unsound attribute.
This attribute is gone now, as far as I can see.
#74990 is the tracking issue for the c_unwind feature that will solve this.
I am not sure if this issue here is still tracking anything else than that?
OTOH, the behavior without any feature gates is still unsound AFAIK, so there should be some I-unsound issue open to track that.
Yes indeed :)
The description of #116088 is not super clear: does it also change extern "C" to abort if attempting to unwind across FFI?
Taking a step back: what is the guidance now for writing correct FFI code with callbacks? Where is that / should that be documented?
does it also change extern "C" to abort if attempting to unwind across FFI?
Yes.
Taking a step back: what is the guidance now for writing correct FFI code with callbacks?
If you write extern "C" functions in Rust, all outgoing panics will be turned into aborts.
If you call extern "C" functions from Rust, the compiler assumes that they cannot unwind (i.e., unwinding would be UB).
So if Rust code passes callbacks to a C library, extern "C" functions are the safe choice as they will never unwind. If the C library supports callbacks that unwind, you can define extern "C-unwind" functions instead.
Where is that / should that be documented?
Not sure, probably somewhere in the reference?
Reference PR: rust-lang/reference#1226