From 08574db459814ba47d57f507ecf92738b9827a70 Mon Sep 17 00:00:00 2001 From: gnzlbg Date: Mon, 9 Sep 2019 18:29:40 +0200 Subject: [PATCH 1/3] Fixes safe Rust part of soundness bug 52652 by aborting on safe extern "C" functions --- src/librustc_mir/build/mod.rs | 5 ++++- src/test/ui/abi/abort-on-c-abi.rs | 14 +++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index 7ab0bf7d66a64..5fd7cdb555c7f 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -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(); 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, } } diff --git a/src/test/ui/abi/abort-on-c-abi.rs b/src/test/ui/abi/abort-on-c-abi.rs index 2f08730ec6132..28a8bf531de13 100644 --- a/src/test/ui/abi/abort-on-c-abi.rs +++ b/src/test/ui/abi/abort-on-c-abi.rs @@ -14,14 +14,19 @@ 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"); } 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(); } @@ -37,4 +42,7 @@ fn main() { .stdin(Stdio::piped()) .arg("test").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()); } From 801e1b745c751a4ac02712ca6a7660cba575dbe9 Mon Sep 17 00:00:00 2001 From: gnzlbg Date: Mon, 9 Sep 2019 20:46:31 +0200 Subject: [PATCH 2/3] Add more tests --- src/test/ui/abi/abort-on-c-abi.rs | 66 ++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/src/test/ui/abi/abort-on-c-abi.rs b/src/test/ui/abi/abort-on-c-abi.rs index 28a8bf531de13..2b975ef40396b 100644 --- a/src/test/ui/abi/abort-on-c-abi.rs +++ b/src/test/ui/abi/abort-on-c-abi.rs @@ -22,6 +22,26 @@ 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() { // A safe extern "C" function that panics should abort the process: let _ = panic::catch_unwind(|| panic_in_ffi() ); @@ -31,10 +51,36 @@ fn test() { 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 = 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]) @@ -43,6 +89,22 @@ fn main() { .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()); } From dafc0ba120bf009dc5a3674fbd86934d817be351 Mon Sep 17 00:00:00 2001 From: gnzlbg Date: Mon, 9 Sep 2019 22:17:46 +0200 Subject: [PATCH 3/3] Fix incremental tests --- src/test/incremental/hashes/function_interfaces.rs | 2 +- src/test/incremental/hashes/inherent_impls.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/incremental/hashes/function_interfaces.rs b/src/test/incremental/hashes/function_interfaces.rs index 4515e36166eb8..84680a52ff3ce 100644 --- a/src/test/incremental/hashes/function_interfaces.rs +++ b/src/test/incremental/hashes/function_interfaces.rs @@ -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() {} diff --git a/src/test/incremental/hashes/inherent_impls.rs b/src/test/incremental/hashes/inherent_impls.rs index 24d436f5f9727..e98f9b67ca421 100644 --- a/src/test/incremental/hashes/inherent_impls.rs +++ b/src/test/incremental/hashes/inherent_impls.rs @@ -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) { } }