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

Open
wants to merge 3 commits into
base: master
from
Open
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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();

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 9, 2019

Author Contributor

@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.

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Sep 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 28, 2019

Member

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
@@ -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,
}
}

@@ -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());
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.