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

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/librustc_mir/build/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,7 @@ fn should_abort_on_panic(tcx: TyCtxt<'_>, fn_def_id: DefId, abi: Abi) -> bool {

// Validate `#[unwind]` syntax regardless of platform-specific panic strategy
let attrs = &tcx.get_attrs(fn_def_id);
let unsafety = &tcx.fn_sig(fn_def_id).skip_binder().unsafety.clone();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eddyb I don't know if this is the best way to do this. I'm not sure what the binder is for and just ended up playing type tetris here.

Copy link
Member

Choose a reason for hiding this comment

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

If i am correct, the binder is the for<'a> part of for<'a> fn(&'a ()). This seems correct, as the safety of a function doesnt depend on lifetimes.

Copy link
Member

Choose a reason for hiding this comment

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

Just... call .unsafety()? There should be a method for every component of ty::FnSig, on the binder-wrapped version.

let unwind_attr = attr::find_unwind_attr(Some(tcx.sess.diagnostic()), attrs);

// We never unwind, so it's not relevant to stop an unwind
Expand All @@ -502,9 +503,11 @@ fn should_abort_on_panic(tcx: TyCtxt<'_>, fn_def_id: DefId, abi: Abi) -> bool {
// This is a special case: some functions have a C abi but are meant to
// unwind anyway. Don't stop them.
match unwind_attr {
None => false, // FIXME(#58794)
Some(UnwindAttr::Allowed) => false,
Some(UnwindAttr::Aborts) => true,
// If no `#[unwind]` attribute present unsafe function definitions
// are temporarily allowed to unwind:
None => unsafety == &rustc::hir::Unsafety::Normal,
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/incremental/hashes/function_interfaces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ pub unsafe fn make_unsafe() {}
pub fn make_extern() {}

#[cfg(not(cfail1))]
#[rustc_clean(cfg = "cfail2", except = "Hir, HirBody, typeck_tables_of, fn_sig")]
#[rustc_clean(cfg = "cfail2", except = "Hir, HirBody, mir_built, typeck_tables_of, fn_sig")]
#[rustc_clean(cfg = "cfail3")]
pub extern "C" fn make_extern() {}

Expand Down
2 changes: 1 addition & 1 deletion src/test/incremental/hashes/inherent_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ impl Foo {
#[rustc_clean(cfg="cfail2")]
#[rustc_clean(cfg="cfail3")]
impl Foo {
#[rustc_clean(cfg="cfail2", except="Hir,HirBody,fn_sig,typeck_tables_of")]
#[rustc_clean(cfg="cfail2", except="Hir,HirBody,mir_built,fn_sig,typeck_tables_of")]
#[rustc_clean(cfg="cfail3")]
pub extern fn make_method_extern(&self) { }
}
Expand Down
80 changes: 75 additions & 5 deletions src/test/ui/abi/abort-on-c-abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,27 +14,97 @@ use std::io::prelude::*;
use std::io;
use std::process::{Command, Stdio};

#[unwind(aborts)] // FIXME(#58794)
unsafe extern "C" fn unsafe_panic_in_ffi() {
panic!("Test");
}

extern "C" fn panic_in_ffi() {
panic!("Test");
}

#[unwind(allowed)]
unsafe extern "C" fn unsafe_panic_allow_in_ffi() {
panic!("Test");
}

#[unwind(aborts)]
unsafe extern "C" fn unsafe_abort_in_ffi() {
panic!("Test");
}

#[unwind(allowed)]
extern "C" fn nopanic_in_ffi() {
panic!("Test");
}

#[unwind(aborts)]
extern "C" fn abort_in_ffi() {
panic!("Test");
}

fn test() {
let _ = panic::catch_unwind(|| { panic_in_ffi(); });
// The process should have aborted by now.
// A safe extern "C" function that panics should abort the process:
let _ = panic::catch_unwind(|| panic_in_ffi() );

// If the process did not abort, the panic escaped FFI:
io::stdout().write(b"This should never be printed.\n");
let _ = io::stdout().flush();
}

fn test2() {
// A safe extern "C" function that panics should abort the process:
let _ = panic::catch_unwind(|| abort_in_ffi() );

// If the process did not abort, the panic escaped FFI:
io::stdout().write(b"This should never be printed.\n");
let _ = io::stdout().flush();
}

fn test3() {
// An unsafe #[unwind(abort)] extern "C" function that panics should abort the process:
let _ = panic::catch_unwind(|| unsafe { unsafe_abort_in_ffi() });

// If the process did not abort, the panic escaped FFI:
io::stdout().write(b"This should never be printed.\n");
let _ = io::stdout().flush();
}

fn main() {
let args: Vec<String> = env::args().collect();
if args.len() > 1 && args[1] == "test" {
return test();
if args.len() > 1 {
if args[1] == "test" {
return test();
}
if args[1] == "test2" {
return test2();
}
if args[1] == "test3" {
return test3();
}
}

let mut p = Command::new(&args[0])
.stdout(Stdio::piped())
.stdin(Stdio::piped())
.arg("test").spawn().unwrap();
assert!(!p.wait().unwrap().success());

let mut p = Command::new(&args[0])
.stdout(Stdio::piped())
.stdin(Stdio::piped())
.arg("test2").spawn().unwrap();
assert!(!p.wait().unwrap().success());

let mut p = Command::new(&args[0])
.stdout(Stdio::piped())
.stdin(Stdio::piped())
.arg("test3").spawn().unwrap();
assert!(!p.wait().unwrap().success());

// An unsafe extern "C" function that panics should let the panic escape:
assert!(panic::catch_unwind(|| unsafe { unsafe_panic_in_ffi() }).is_err());
assert!(panic::catch_unwind(|| unsafe { unsafe_panic_allow_in_ffi() }).is_err());

// A safe extern "C" unwind(allows) that panics should let the panic escape:
assert!(panic::catch_unwind(|| nopanic_in_ffi()).is_err());
}