From 51782485b0a5c08ba22c8b09e5cb1d67edbc2872 Mon Sep 17 00:00:00 2001 From: Shayne Fletcher Date: Wed, 24 Feb 2021 16:37:58 -0800 Subject: [PATCH] Don't unwind Rust into foreign code Summary: Catch unwinding panics. Differential Revision: D26640551 fbshipit-source-id: f19fa08b65f4e772b732c9b3399b69d3949fcef3 --- hphp/hack/src/facts/rust_facts_ffi.rs | 62 ++++--- hphp/hack/src/hhbc/compile_ffi.rs | 195 +++++++++++---------- .../parser/positioned_full_trivia_parser_ffi.rs | 81 +++++---- 3 files changed, 183 insertions(+), 155 deletions(-) diff --git a/hphp/hack/src/facts/rust_facts_ffi.rs b/hphp/hack/src/facts/rust_facts_ffi.rs index fd624ff628e..ad743e7fe52 100644 --- a/hphp/hack/src/facts/rust_facts_ffi.rs +++ b/hphp/hack/src/facts/rust_facts_ffi.rs @@ -21,34 +21,42 @@ unsafe extern "C" fn extract_as_json_cpp_ffi( text_ptr: *const c_char, mangle_xhp: bool, ) -> *const c_char { - use std::os::unix::ffi::OsStrExt; - // Safety : We rely on the C caller that `text_ptr` be a valid - // nul-terminated C string. - let text = std::ffi::CStr::from_ptr(text_ptr).to_bytes(); - // Safety: We rely on the C caller that `filename` be a valid - // nul-terminated C string. - let filename = RelativePath::make( - oxidized::relative_path::Prefix::Dummy, - std::path::PathBuf::from(std::ffi::OsStr::from_bytes( - std::ffi::CStr::from_ptr(filename).to_bytes(), - )), - ); - match extract_as_json_ffi0( - ((1 << 0) & flags) != 0, // php5_compat_mode - ((1 << 1) & flags) != 0, // hhvm_compat_mode - ((1 << 2) & flags) != 0, // allow_new_attribute_syntax - ((1 << 3) & flags) != 0, // enable_xhp_class_modifier - ((1 << 4) & flags) != 0, // disable_xhp_element_mangling - filename, - text, - mangle_xhp, - ) { - Some(s) => { - let cs = std::ffi::CString::new(s) - .expect("rust_facts_ffi: extract_as_json_cpp_ffi: String::new failed"); - cs.into_raw() as *const c_char + match std::panic::catch_unwind(|| { + use std::os::unix::ffi::OsStrExt; + // Safety : We rely on the C caller that `text_ptr` be a valid + // nul-terminated C string. + let text = std::ffi::CStr::from_ptr(text_ptr).to_bytes(); + // Safety: We rely on the C caller that `filename` be a valid + // nul-terminated C string. + let filename = RelativePath::make( + oxidized::relative_path::Prefix::Dummy, + std::path::PathBuf::from(std::ffi::OsStr::from_bytes( + std::ffi::CStr::from_ptr(filename).to_bytes(), + )), + ); + match extract_as_json_ffi0( + ((1 << 0) & flags) != 0, // php5_compat_mode + ((1 << 1) & flags) != 0, // hhvm_compat_mode + ((1 << 2) & flags) != 0, // allow_new_attribute_syntax + ((1 << 3) & flags) != 0, // enable_xhp_class_modifier + ((1 << 4) & flags) != 0, // disable_xhp_element_mangling + filename, + text, + mangle_xhp, + ) { + Some(s) => { + let cs = std::ffi::CString::new(s) + .expect("rust_facts_ffi: extract_as_json_cpp_ffi: String::new failed"); + cs.into_raw() as *const c_char + } + None => std::ptr::null(), + } + }) { + Ok(ptr) => ptr, + Err(_) => { + eprintln!("Error: panic in ffi function extract_as_json_cpp_ffi"); + std::ptr::null() } - None => std::ptr::null(), } } diff --git a/hphp/hack/src/hhbc/compile_ffi.rs b/hphp/hack/src/hhbc/compile_ffi.rs index 263bd7d6b30..7b5e69d4af1 100644 --- a/hphp/hack/src/hhbc/compile_ffi.rs +++ b/hphp/hack/src/hhbc/compile_ffi.rs @@ -144,105 +144,114 @@ unsafe extern "C" fn compile_from_text_cpp_ffi( output_cfg: usize, err_buf: usize, ) -> *const c_char { - // Safety: We rely on the C caller that `env` can be legitmately - // reinterpreted as a `*const CErrBuf` and that on doing so, it is - // non-null is well aligned and points to a valid properly - // initialized value. - let err_buf: &CErrBuf = (err_buf as *const CErrBuf).as_ref().unwrap(); - let buf_len: c_int = err_buf.buf_len; - // Safety : We rely on the C caller that `err_buf.buf` be valid for - // reads and write for `buf_len * mem::sizeof::()` bytes. - let buf: &mut [u8] = std::slice::from_raw_parts_mut(err_buf.buf as *mut u8, buf_len as usize); + match std::panic::catch_unwind(|| { + // Safety: We rely on the C caller that `env` can be legitmately + // reinterpreted as a `*const CErrBuf` and that on doing so, it is + // non-null is well aligned and points to a valid properly + // initialized value. + let err_buf: &CErrBuf = (err_buf as *const CErrBuf).as_ref().unwrap(); + let buf_len: c_int = err_buf.buf_len; + // Safety : We rely on the C caller that `err_buf.buf` be valid for + // reads and write for `buf_len * mem::sizeof::()` bytes. + let buf: &mut [u8] = + std::slice::from_raw_parts_mut(err_buf.buf as *mut u8, buf_len as usize); - // Safety: We rely on the C caller that `output_cfg` can be - // legitmately reinterpreted as a `*const COutputConfig` and that - // on doing so, it points to a valid properly initialized value. - let _output_config: Option = - RustOutputConfig::from_c_output_config(output_cfg as *const COutputConfig); - // Safety: We rely on the C caller that `source_text` be a - // properly iniitalized null-terminated C string. - let text: &[u8] = std::ffi::CStr::from_ptr(source_text).to_bytes(); + // Safety: We rely on the C caller that `output_cfg` can be + // legitmately reinterpreted as a `*const COutputConfig` and that + // on doing so, it points to a valid properly initialized value. + let _output_config: Option = + RustOutputConfig::from_c_output_config(output_cfg as *const COutputConfig); + // Safety: We rely on the C caller that `source_text` be a + // properly iniitalized null-terminated C string. + let text: &[u8] = std::ffi::CStr::from_ptr(source_text).to_bytes(); - let job_builder = move || { - move |stack_limit: &StackLimit, _nomain_stack_size: Option| { - // Safety: We rely on the C caller that `env` can be - // legitmately reinterpreted as a `*const CEnv` and that - // on doing so, it points to a valid properly initialized - // value. - let env: compile::Env<&str> = CEnv::to_compile_env(env as *const CEnv).unwrap(); - let source_text = SourceText::make(RcOc::new(env.filepath.clone()), text); - let mut w = String::new(); - match compile::from_text_(&env, stack_limit, &mut w, source_text) { - Ok(_) => { - //print_output(w, output_config, &env.filepath, profile)?; - Ok(w) + let job_builder = move || { + move |stack_limit: &StackLimit, _nomain_stack_size: Option| { + // Safety: We rely on the C caller that `env` can be + // legitmately reinterpreted as a `*const CEnv` and that + // on doing so, it points to a valid properly initialized + // value. + let env: compile::Env<&str> = CEnv::to_compile_env(env as *const CEnv).unwrap(); + let source_text = SourceText::make(RcOc::new(env.filepath.clone()), text); + let mut w = String::new(); + match compile::from_text_(&env, stack_limit, &mut w, source_text) { + Ok(_) => { + //print_output(w, output_config, &env.filepath, profile)?; + Ok(w) + } + Err(e) => Err(anyhow!("{}", e)), } - Err(e) => Err(anyhow!("{}", e)), } - } - }; - // Assume peak is 2.5x of stack. This is initial estimation, need - // to be improved later. - let stack_slack = |stack_size| stack_size * 6 / 10; - let on_retry = &mut |stack_size_tried: usize| { - // Not always printing warning here because this would fail - // some HHVM tests. - if atty::is(atty::Stream::Stderr) || std::env::var_os("HH_TEST_MODE").is_some() { - // Safety : We rely on the C caller that `env` can be - // legitmately reinterpreted as a `*const CEnv` and that - // on doing so, it points to a valid properly initialized - // value. - let env = CEnv::to_compile_env(env as *const CEnv).unwrap(); - eprintln!( - "[hrust] warning: compile_from_text_ffi exceeded stack of {} KiB on: {}", - (stack_size_tried - stack_slack(stack_size_tried)) / KI, - env.filepath.path_str(), - ); - } - }; - let job = stack_limit::retry::Job { - nonmain_stack_min: 13 * MI, - // TODO(hrust) aast_parser_ffi only requies 1 * GI, it's like - // rust compiler produce inconsistent binary. - nonmain_stack_max: Some(7 * GI), - ..Default::default() - }; - - let r: Result = job - .with_elastic_stack(job_builder, on_retry, stack_slack) - .map_err(|e| format!("{}", e)) - .expect("compile_ffi: compile_from_text_cpp_ffi: retry failed") - .map_err(|e| e.to_string()); - match r { - Ok(out) => { - let cs = std::ffi::CString::new(out) - .expect("compile_ffi: compile_from_text_cpp_ffi: String::new failed"); - cs.into_raw() as *const c_char - } - Err(e) => { - if e.len() >= buf.len() { - warn!("Provided error buffer too is too small."); - warn!( - "Expected at least {} bytes but got {}", - e.len() + 1, - buf.len() + }; + // Assume peak is 2.5x of stack. This is initial estimation, need + // to be improved later. + let stack_slack = |stack_size| stack_size * 6 / 10; + let on_retry = &mut |stack_size_tried: usize| { + // Not always printing warning here because this would fail + // some HHVM tests. + if atty::is(atty::Stream::Stderr) || std::env::var_os("HH_TEST_MODE").is_some() { + // Safety : We rely on the C caller that `env` can be + // legitmately reinterpreted as a `*const CEnv` and that + // on doing so, it points to a valid properly initialized + // value. + let env = CEnv::to_compile_env(env as *const CEnv).unwrap(); + eprintln!( + "[hrust] warning: compile_from_text_ffi exceeded stack of {} KiB on: {}", + (stack_size_tried - stack_slack(stack_size_tried)) / KI, + env.filepath.path_str(), ); - } else { - // Safety: - // - `e` must be valid for reads of `e.len() * - // size_of::()` bytes; - // - `buf` must be valid for writes of of `e.len() * - // size_of::()` bytes; - // - The region of memory beginning at `e` with a - // size of of `e.len() * size_of::()` bytes must - // not overlap with the region of memory beginning - // at `buf` with the same size; - // - Even if the of `e.len() * size_of::()` is - // `0`, the pointers must be non-null and properly - // aligned. - std::ptr::copy_nonoverlapping(e.as_ptr(), buf.as_mut_ptr(), e.len()); - buf[e.len()] = 0; } + }; + let job = stack_limit::retry::Job { + nonmain_stack_min: 13 * MI, + // TODO(hrust) aast_parser_ffi only requies 1 * GI, it's like + // rust compiler produce inconsistent binary. + nonmain_stack_max: Some(7 * GI), + ..Default::default() + }; + + match job + .with_elastic_stack(job_builder, on_retry, stack_slack) + .map_err(|e| format!("{}", e)) + .expect("compile_ffi: compile_from_text_cpp_ffi: retry failed") + .map_err(|e| e.to_string()) + { + Ok(out) => { + let cs = std::ffi::CString::new(out) + .expect("compile_ffi: compile_from_text_cpp_ffi: String::new failed"); + cs.into_raw() as *const c_char + } + Err(e) => { + if e.len() >= buf.len() { + warn!("Provided error buffer too small."); + warn!( + "Expected at least {} bytes but got {}.", + e.len() + 1, + buf.len() + ); + } else { + // Safety: + // - `e` must be valid for reads of `e.len() * + // size_of::()` bytes; + // - `buf` must be valid for writes of of `e.len() * + // size_of::()` bytes; + // - The region of memory beginning at `e` with a + // size of of `e.len() * size_of::()` bytes must + // not overlap with the region of memory beginning + // at `buf` with the same size; + // - Even if the of `e.len() * size_of::()` is + // `0`, the pointers must be non-null and properly + // aligned. + std::ptr::copy_nonoverlapping(e.as_ptr(), buf.as_mut_ptr(), e.len()); + buf[e.len()] = 0; + } + std::ptr::null() + } + } + }) { + Ok(ptr) => ptr, + Err(_) => { + eprintln!("Error: panic in ffi function compile_from_text_cpp_ffi"); std::ptr::null() } } diff --git a/hphp/hack/src/parser/positioned_full_trivia_parser_ffi.rs b/hphp/hack/src/parser/positioned_full_trivia_parser_ffi.rs index 19f053b537f..13a3dde0c2d 100644 --- a/hphp/hack/src/parser/positioned_full_trivia_parser_ffi.rs +++ b/hphp/hack/src/parser/positioned_full_trivia_parser_ffi.rs @@ -72,41 +72,52 @@ unsafe extern "C" fn parse_positioned_full_trivia_cpp_ffi( source_text: *const libc::c_char, env: usize, ) -> *const libc::c_char { - use std::os::unix::ffi::OsStrExt; - // Safety: We rely on the C caller that `filename` be a properly - // initialized nul-terminated C string. - let filepath = oxidized::relative_path::RelativePath::make( - oxidized::relative_path::Prefix::Dummy, - std::path::PathBuf::from(std::ffi::OsStr::from_bytes( - std::ffi::CStr::from_ptr(filename).to_bytes(), - )), - ); - // Safety : We rely on the C caller that `text` be a properly - // iniitalized nul-terminated C string. - let text: &[u8] = std::ffi::CStr::from_ptr(source_text).to_bytes(); - // Safety : We rely on the C caller that `env` can be legitmately - // reinterpreted as a `*const CParserEnv` and that on doing so, it - // points to a valid properly initialized value. - let env: parser_core_types::parser_env::ParserEnv = - CParserEnv::to_parser_env(env as *const CParserEnv).unwrap(); - let indexed_source = parser_core_types::indexed_source_text::IndexedSourceText::new( - parser_core_types::source_text::SourceText::make(ocamlrep::rc::RcOc::new(filepath), text), - ); - let alloc = bumpalo::Bump::new(); - let mut serializer = serde_json::Serializer::new(std::vec![]); - let stack_limit: std::option::Option<&stack_limit::StackLimit> = None; - match positioned_full_trivia_parser::parse_script_to_json( - &alloc, - &mut serializer, - &indexed_source, - env, - stack_limit, - ) { - Ok(()) => { - // Safety : No runtime assertion is made that `v` contains no - // 0 bytes. - std::ffi::CString::from_vec_unchecked(serializer.into_inner()).into_raw() + match std::panic::catch_unwind(|| { + use std::os::unix::ffi::OsStrExt; + // Safety: We rely on the C caller that `filename` be a properly + // initialized nul-terminated C string. + let filepath = oxidized::relative_path::RelativePath::make( + oxidized::relative_path::Prefix::Dummy, + std::path::PathBuf::from(std::ffi::OsStr::from_bytes( + std::ffi::CStr::from_ptr(filename).to_bytes(), + )), + ); + // Safety : We rely on the C caller that `text` be a properly + // iniitalized nul-terminated C string. + let text: &[u8] = std::ffi::CStr::from_ptr(source_text).to_bytes(); + // Safety : We rely on the C caller that `env` can be legitmately + // reinterpreted as a `*const CParserEnv` and that on doing so, it + // points to a valid properly initialized value. + let env: parser_core_types::parser_env::ParserEnv = + CParserEnv::to_parser_env(env as *const CParserEnv).unwrap(); + let indexed_source = parser_core_types::indexed_source_text::IndexedSourceText::new( + parser_core_types::source_text::SourceText::make( + ocamlrep::rc::RcOc::new(filepath), + text, + ), + ); + let alloc = bumpalo::Bump::new(); + let mut serializer = serde_json::Serializer::new(std::vec![]); + let stack_limit: std::option::Option<&stack_limit::StackLimit> = None; + match positioned_full_trivia_parser::parse_script_to_json( + &alloc, + &mut serializer, + &indexed_source, + env, + stack_limit, + ) { + Ok(()) => { + // Safety : No runtime assertion is made that `v` contains no + // 0 bytes. + std::ffi::CString::from_vec_unchecked(serializer.into_inner()).into_raw() + } + _ => std::ptr::null(), + } + }) { + Ok(ptr) => ptr, + Err(_) => { + eprintln!("Error: panic in ffi function parse_positioned_full_trivia_cpp_ffi"); + std::ptr::null() } - _ => std::ptr::null(), } } -- 2.11.4.GIT