Repository navigation
catch_unwind incorrectly tries to stop ForceUnwinds #65441
Description
MWE:
use std::os::unix::thread::JoinHandleExt;
use libc::pthread_cancel;
struct DropGuard;
impl Drop for DropGuard {
fn drop(&mut self) {
println!("unwinding foo");
}
}
fn foo() {
let _x = DropGuard;
println!("thread started");
std::thread::sleep(std::time::Duration::new(1, 0));
println!("thread finished");
}
fn main() {
let handle0 = std::thread::spawn(foo);
std::thread::sleep(std::time::Duration::new(0, 10_000));
unsafe {
let x = pthread_cancel(handle0.as_pthread_t());
assert_eq!(x, 0);
}
handle0.join();
}Output:
thread started
unwinding foo
Errors:
Compiling playground v0.0.1 (/playground)
warning: unused `std::result::Result` that must be used
--> src/main.rs:25:5
|
25 | handle0.join();
| ^^^^^^^^^^^^^^^
|
= note: `#[warn(unused_must_use)]` on by default
= note: this `Result` may be an `Err` variant, which should be handled
Finished release [optimized] target(s) in 0.86s
Running `target/release/playground`
FATAL: exception not rethrown
timeout: the monitored command dumped core
/root/entrypoint.sh: line 8: 7 Aborted timeout --signal=KILL ${timeout} "$@"
According to the Itanium ABI:
- "forced" unwinding (such as caused by longjmp or thread termination).
[...]
During "forced unwinding", on the other hand, an external agent is driving the unwinding. For instance, this can be the longjmp routine. This external agent, not each personality routine, knows when to stop unwinding. The fact that a personality routine is not given a choice about whether unwinding will proceed is indicated by the _UA_FORCE_UNWIND flag.
So catch_unwind definitely needs to always catch Rust exceptions, and at the thread boundary it probably also want to catch foreign exceptions and abort since whatever that should do probably won't work. When not on a thread boundary, aborting on foreign exceptions might be a conservative step until a better solution is discussed (an alternative solution would be not to catch them and just let them escape, or to catch them and have the Any be some ForeignPanic type defined somewhere in std::panic that users can use to detect that a foreign exception was raised). What it probably shouldn't do is catch "ForceUnwind" otherwise longjmp might not work across it and threads will not be canceled, but... maybe the conservative thing to do would be for this to abort for the time being?
Activity
gnzlbg commentedon Oct 15, 2019
cc @Amanieu @alexcrichton
Amanieu commentedon Oct 15, 2019
I feel that we should simply let all foreign exceptions unwind through
catch_unwindwithout catching them. This roughly matches the behavior of a C++catchclause which only catches therust_unwindexception type. This should of course be documented.gnzlbg commentedon Oct 15, 2019
Note that C++ with Itanium ABI also catches foreign exceptions if a
catch(...)clause is used. That is, in C++, if your implementations supports it (which AFAIK none does right now), you can usecatch(__rust_exception const& e)to catch a Rust panic (you can't modify it though). According to the Itanium ABI, the__rust_exceptiontype should be defined in the<exception>header.Amanieu commentedon Oct 16, 2019
Actually, looking at the implementation of
catch_unwind, there is no check that the exception object that was caught is actually a panic generated by Rust. The code just blindly casts the exception pointer to a pointer tostruct Exception. This will fail horribly if the exception was not generated by Rust.gnzlbg commentedon Oct 17, 2019
Yes, I think that's correct at the language spec level, because right now, letting a foreign exception (or any kind of exception actually) unwind into Rust is UB, so we do not provide any kind of guarantees about what
catch_unwinddoes.As you mention, what we currently do for foreign exceptions is horrible. Since there are some platforms where we can easily do better (e.g. just aborting), I think we should do so as a "quality-of-implementation" issue at least, even though it is still UB.
Auto merge of #65646 - Amanieu:foreign-exceptions, r=nikomatsakis