rustc: Tweak funclet cleanups of ffi functions #48572

alexcrichton · 2018-02-27T03:53:58Z

This commit is targeted at addressing #48251 by specifically fixing a case where
a longjmp over Rust frames on MSVC runs cleanups, accidentally running the
"abort the program" cleanup as well. Added in #46833 extern ABI functions in
Rust will abort the process if Rust panics, and currently this is modeled as a
normal cleanup like all other destructors.

Unfortunately it turns out that longjmp on MSVC is implemented with SEH, the
same mechanism used to implement panics in Rust. This means that longjmp over
Rust frames will run Rust cleanups (even though we don't necessarily want it
to). Notably this means that if you longjmp over a Rust stack frame then that
probably means you'll abort the program because one of the cleanups will abort
the process.

After some discussion on IRC it turns out that longjmp doesn't run cleanups
for caught exceptions, it only runs cleanups for cleanup pads. Using this
information this commit tweaks the codegen for an extern function to
a catch-all clause for exceptions instead of a cleanup block. This catch-all is
equivalent to the C++ code:

try {
    foo();
} catch (...) {
    bar();
}

and in fact our codegen here is designed to match exactly what clang emits for
that C++ code!

With this tweak a longjmp over Rust code will no longer abort the process. A
longjmp will continue to "accidentally" run Rust cleanups (destructors) on MSVC.
Other non-MSVC platforms will not rust destructors with a longjmp, so we'll
probably still recommend "don't have destructors on the stack", but in any case
this is a more surgical fix than #48567 and should help us stick to standard
personality functions a bit longer.

rust-highfive · 2018-02-27T03:54:01Z

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

majnemer · 2018-02-27T07:37:33Z

src/librustc_trans/mir/mod.rs

+                let cs = cs_bx.catch_switch(None, None, 1);
+                cs_bx.add_handler(cs, cp_bx.llbb());
+
+                // No idea what this null/64 are for, but it's what clang does!


The pointer usually points to an RTTI type descriptor, catch(...) doesn't have a type so this is null.
As for the 64, its a bit in a bitfield describing if the catch is by reference, const, etc. In this case, 64 indicates catch-all.

Aha, thanks!

petrochenkov · 2018-02-27T08:55:28Z

I'm not qualified to review this.
r? @nagisa @vadimcn @eddyb

nagisa · 2018-02-27T12:41:27Z

Won't be able to review till friday.

…

On Feb 27, 2018 13:08, "Alex Crichton" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/librustc_trans/mir/mod.rs <#48572 (comment)>: > + // modeled after clang's for: + // + // try { + // foo(); + // } catch (...) { + // bar(); + // } + Some(&mir::TerminatorKind::Abort) => { + let cs_bx = bx.build_sibling_block(&format!("cs_funclet{:?}", bb)); + let cp_bx = bx.build_sibling_block(&format!("cp_funclet{:?}", bb)); + ret_llbb = cs_bx.llbb(); + + let cs = cs_bx.catch_switch(None, None, 1); + cs_bx.add_handler(cs, cp_bx.llbb()); + + // No idea what this null/64 are for, but it's what clang does! Aha, thanks! — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#48572 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0kmkUkL0x5EJ-Cyj4rdTOnnb5-_Xks5tY_BLgaJpZM4SUUR0> .

eddyb · 2018-02-27T15:32:59Z

@bors r+

LGTM, although I'm wondering if putting the aborts in MIR (instead of the backend) was a mistake.

bors · 2018-02-27T15:33:20Z

📌 Commit e2af240 has been approved by eddyb

diwic · 2018-02-28T18:56:10Z

src/test/run-make/longjmp-across-rust/main.rs

+    foo();
+}
+
+fn foo() {


I'm trying to understand how this test works. According to earlier comment, longjmp will still run destructors on MSVC. As such, A's destructor will run, causing a panic, but since we're in unwind mode, this will be a double unwind, causing an abort for that reason, or...?

Ah yes that was a mistake on my part, this test was originally included in a PR where it was fixed, but I changed strategies at the last minute.

Manishearth · 2018-02-28T21:10:31Z

Fails on appveyor

#48615 (comment)

This commit is targeted at addressing rust-lang#48251 by specifically fixing a case where a longjmp over Rust frames on MSVC runs cleanups, accidentally running the "abort the program" cleanup as well. Added in rust-lang#46833 `extern` ABI functions in Rust will abort the process if Rust panics, and currently this is modeled as a normal cleanup like all other destructors. Unfortunately it turns out that `longjmp` on MSVC is implemented with SEH, the same mechanism used to implement panics in Rust. This means that `longjmp` over Rust frames will run Rust cleanups (even though we don't necessarily want it to). Notably this means that if you `longjmp` over a Rust stack frame then that probably means you'll abort the program because one of the cleanups will abort the process. After some discussion on IRC it turns out that `longjmp` doesn't run cleanups for *caught* exceptions, it only runs cleanups for cleanup pads. Using this information this commit tweaks the codegen for an `extern` function to a catch-all clause for exceptions instead of a cleanup block. This catch-all is equivalent to the C++ code: try { foo(); } catch (...) { bar(); } and in fact our codegen here is designed to match exactly what clang emits for that C++ code! With this tweak a longjmp over Rust code will no longer abort the process. A longjmp will continue to "accidentally" run Rust cleanups (destructors) on MSVC. Other non-MSVC platforms will not rust destructors with a longjmp, so we'll probably still recommend "don't have destructors on the stack", but in any case this is a more surgical fix than rust-lang#48567 and should help us stick to standard personality functions a bit longer.

alexcrichton · 2018-02-28T21:31:42Z

@bors: r=eddyb

bors · 2018-02-28T21:31:43Z

📌 Commit 804666f has been approved by eddyb

bors · 2018-02-28T21:31:44Z

🌲 The tree is currently closed for pull requests below priority 200, this pull request will be tested once the tree is reopened

Rollup of 10 pull requests - Successful merges: #48355, #48359, #48380, #48419, #48420, #48461, #48522, #48570, #48572, #48603 - Failed merges:

rustc: Tweak funclet cleanups of ffi functions This commit is targeted at addressing rust-lang#48251 by specifically fixing a case where a longjmp over Rust frames on MSVC runs cleanups, accidentally running the "abort the program" cleanup as well. Added in rust-lang#46833 `extern` ABI functions in Rust will abort the process if Rust panics, and currently this is modeled as a normal cleanup like all other destructors. Unfortunately it turns out that `longjmp` on MSVC is implemented with SEH, the same mechanism used to implement panics in Rust. This means that `longjmp` over Rust frames will run Rust cleanups (even though we don't necessarily want it to). Notably this means that if you `longjmp` over a Rust stack frame then that probably means you'll abort the program because one of the cleanups will abort the process. After some discussion on IRC it turns out that `longjmp` doesn't run cleanups for *caught* exceptions, it only runs cleanups for cleanup pads. Using this information this commit tweaks the codegen for an `extern` function to a catch-all clause for exceptions instead of a cleanup block. This catch-all is equivalent to the C++ code: try { foo(); } catch (...) { bar(); } and in fact our codegen here is designed to match exactly what clang emits for that C++ code! With this tweak a longjmp over Rust code will no longer abort the process. A longjmp will continue to "accidentally" run Rust cleanups (destructors) on MSVC. Other non-MSVC platforms will not rust destructors with a longjmp, so we'll probably still recommend "don't have destructors on the stack", but in any case this is a more surgical fix than rust-lang#48567 and should help us stick to standard personality functions a bit longer.

alexcrichton · 2018-03-02T17:37:10Z

Nominating for a backport to beta as I think this turned out to be a small enough change and this'll help re-close #48251

fweimer · 2018-03-03T15:43:28Z

Note that this:

try {
    foo();
} catch (...) {
    bar();
}

still breaks thread cancellation on GNU/Linux if bar() aborts unconditionally. The assumptions that FFI calls cannot unwind is not true on this platform.

alexcrichton · 2018-03-03T21:27:31Z

@fweimer can you expand a bit on that? I wasn't aware of this problem!

fweimer · 2018-03-04T21:53:28Z

@alexcrichton Not sure where to start. I previously tried to ask about this over there:

Thread cancellation on GNU/Linux

vadimcn · 2018-03-04T23:43:58Z

Indiscriminate unwinding through an FFI boundary is a very likely to end badly. There may be code on the other side written in a language that does not support unwinding (e.g. C). Unwinding through such code will leak all its resources (memory, open files, acquired mutexes, etc), even if the compiler has generated unwind tables.
Btw, Rust itself does not promise to execute cleanups on foreign exceptions. I know for fact that this is the case for for x86_64-pc-windows-gnu target, not sure about others. (Should we consider putting unwind guards on the caller side of FFI as well?)

There are cases when you do know that it's ok to unwind through FFI, but doing so should be opt-in. We already have the #[unwind] attribute for this purpose, though it's currently unstable.

alexcrichton · 2018-03-05T02:19:24Z

@fweimer it sounds like pthread_cancel has problems with C++ noexcept anyway? In that sense I'd at least by default not expect pthread_cancel to work whereas longjmp/setjmp seems more standard to be expected to work at least. That's not to say we couldn't get it working, but as @vadimcn mentioned it's pretty scary to unwind without Rust exceptions so it's in general best to avoid that.

fweimer · 2018-03-07T12:18:42Z

@alexcrichton pthread_cancel has problems with the current implementation of std::thread, I think. (EDIT I was wrong, this is something that was fixed back in 2012 in libstdc++.) You can still call pthread_create directly and use pthread_cancel with the thread. If I understand the current Rust changes correctly, you have just removed that possibility from the released Rust compiler, without providing a replacement.

alexcrichton · 2018-03-07T15:38:18Z

Er sorry I'm not really sure I understand, can you provide an example of the breakage?

nagisa · 2018-03-07T17:49:52Z

This specific PR only affects Windows, not POSIX platforms. Admittedly, there might be yet another case where on Windows unwinding is used and, unlike longjmp, is caught by catch(...), but we are not currently aware of any. pthread_cancel is not something supported by Windows, and such is of no concern.

On POSIX, meanwhile, code like one below, is and always was undefined Rust (ignore all the bad bindings and stuff, it is something I prototyped quickly to demonstrate the point):

use std::os::unix::thread::*;

extern "C" {
    pub fn pthread_cancel(h: RawPthread) -> i32;
    pub fn pthread_create(t: *mut RawPthread, a: *mut (), t: extern "C" fn(*const ()) -> *const (), b: *mut ()) -> i32;
}

extern "C" fn banana(x: *const ()) -> *const () {
    ::std::thread::sleep(::std::time::Duration::from_millis(500)); // might unwind due to pthread_cancel and is UB
    x
}

fn main() {
    unsafe {
        let mut t: RawPthread = ::std::mem::zeroed();
        pthread_create(&mut t, ::std::ptr::null_mut(), banana, ::std::ptr::null_mut());
        pthread_cancel(t);
    }
    ::std::thread::sleep(::std::time::Duration::from_millis(1000));
}

Currently, the safety implemented in #46833 is disabled, so this code will silently work (it does not work when the safety net is re-enabled), however, to really make sure this code is not undefined, you need to specify the #[unwind] attribute on top of banana, as such:

#[unwind]
extern "C" fn banana(x: *const ()) -> *const () {
    ::std::thread::sleep(::std::time::Duration::from_millis(500)); // might unwind due to pthread_cancel and is not UB anymore, because `banana` is specified to unwind.
    x
}

This is exactly the same rule, that applies to any extern "C" function that might unwind for any reason.

nikomatsakis · 2018-03-08T15:18:34Z

We discussed in the @rust-lang/compiler meeting. We would prefer not to backport this -- hence I'm removing the label. The main reason is that there doesn't seem to be an urgency; the aborts are currently disabled, so we can just let this ride the trains as usual.

rust-highfive assigned petrochenkov Feb 27, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2018

alexcrichton mentioned this pull request Feb 27, 2018

rustc: Use custom personality functions on MSVC #48567

Closed

majnemer reviewed Feb 27, 2018

View reviewed changes

rust-highfive assigned nagisa and unassigned petrochenkov Feb 27, 2018

kyren mentioned this pull request Feb 27, 2018

Regression in rlua crate tests between rustc 1.23.0 and 1.24.0, only on windows. #48251

Closed

alexcrichton force-pushed the noexcept-msvc2 branch from 5b2244f to e2af240 Compare February 27, 2018 12:08

eddyb approved these changes Feb 27, 2018

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2018

Manishearth mentioned this pull request Feb 28, 2018

Rollup of 10 pull requests #48615

Merged

diwic reviewed Feb 28, 2018

View reviewed changes

alexcrichton force-pushed the noexcept-msvc2 branch from e2af240 to 804666f Compare February 28, 2018 21:31

bors added a commit that referenced this pull request Feb 28, 2018

Auto merge of #48615 - Manishearth:rollup, r=Manishearth

0e3c9bb

Rollup of 10 pull requests - Successful merges: #48355, #48359, #48380, #48419, #48420, #48461, #48522, #48570, #48572, #48603 - Failed merges:

Manishearth mentioned this pull request Mar 1, 2018

Rollup of 9 pull requests #48632

Closed

Manishearth mentioned this pull request Mar 2, 2018

Rollup of 9 pull requests #48652

Merged

bors merged commit 804666f into rust-lang:master Mar 2, 2018

alexcrichton added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 2, 2018

alexcrichton deleted the noexcept-msvc2 branch March 2, 2018 17:36

nikomatsakis removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 8, 2018

kornelski mentioned this pull request Apr 12, 2018

RFC: Reserve try for try { .. } block expressions rust-lang/rfcs#2388

Merged

SimonSapin mentioned this pull request Jul 23, 2018

Abort instead of unwinding past FFI functions #52652

Closed

comex mentioned this pull request Feb 28, 2019

Default behavior of unwinding in FFI functions #58794

Closed

kyren mentioned this pull request Sep 23, 2019

Fixes soundness bug 18510 by aborting on unwind from safe extern "C" functions only #64315

Closed

BatmanAoD mentioned this pull request Oct 21, 2019

Allow foreign exceptions to unwind through Rust code and Rust panics to unwind through FFI #65646

Merged

nbdd0121 mentioned this pull request Sep 8, 2022

pthread_exit no longer works in panic=abort #101469

Closed

nbdd0121 mentioned this pull request May 7, 2023

Prevent aborting guard from aborting the process in a forced unwind #104070

Merged

RalfJung mentioned this pull request Oct 7, 2024

Make destructors on extern "C" frames to be executed #129582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: Tweak funclet cleanups of ffi functions #48572

rustc: Tweak funclet cleanups of ffi functions #48572

alexcrichton commented Feb 27, 2018

rust-highfive commented Feb 27, 2018

majnemer Feb 27, 2018

alexcrichton Feb 27, 2018

petrochenkov commented Feb 27, 2018

nagisa commented Feb 27, 2018 via email

eddyb commented Feb 27, 2018 •

edited

Loading

bors commented Feb 27, 2018

diwic Feb 28, 2018

alexcrichton Feb 28, 2018

Manishearth commented Feb 28, 2018 •

edited

Loading

alexcrichton commented Feb 28, 2018

bors commented Feb 28, 2018

bors commented Feb 28, 2018

alexcrichton commented Mar 2, 2018

fweimer commented Mar 3, 2018

alexcrichton commented Mar 3, 2018

fweimer commented Mar 4, 2018

vadimcn commented Mar 4, 2018

alexcrichton commented Mar 5, 2018

fweimer commented Mar 7, 2018 •

edited

Loading

alexcrichton commented Mar 7, 2018

nagisa commented Mar 7, 2018 •

edited

Loading

nikomatsakis commented Mar 8, 2018

rustc: Tweak funclet cleanups of ffi functions #48572

rustc: Tweak funclet cleanups of ffi functions #48572

Conversation

alexcrichton commented Feb 27, 2018

rust-highfive commented Feb 27, 2018

majnemer Feb 27, 2018

Choose a reason for hiding this comment

alexcrichton Feb 27, 2018

Choose a reason for hiding this comment

petrochenkov commented Feb 27, 2018

nagisa commented Feb 27, 2018 via email

eddyb commented Feb 27, 2018 • edited Loading

bors commented Feb 27, 2018

diwic Feb 28, 2018

Choose a reason for hiding this comment

alexcrichton Feb 28, 2018

Choose a reason for hiding this comment

Manishearth commented Feb 28, 2018 • edited Loading

alexcrichton commented Feb 28, 2018

bors commented Feb 28, 2018

bors commented Feb 28, 2018

alexcrichton commented Mar 2, 2018

fweimer commented Mar 3, 2018

alexcrichton commented Mar 3, 2018

fweimer commented Mar 4, 2018

vadimcn commented Mar 4, 2018

alexcrichton commented Mar 5, 2018

fweimer commented Mar 7, 2018 • edited Loading

alexcrichton commented Mar 7, 2018

nagisa commented Mar 7, 2018 • edited Loading

nikomatsakis commented Mar 8, 2018

eddyb commented Feb 27, 2018 •

edited

Loading

Manishearth commented Feb 28, 2018 •

edited

Loading

fweimer commented Mar 7, 2018 •

edited

Loading

nagisa commented Mar 7, 2018 •

edited

Loading