From dd0b864ee48559af7af996e6665a377e945c798c Mon Sep 17 00:00:00 2001 From: Joel Wurtz Date: Sat, 21 Oct 2023 21:56:05 +0200 Subject: [PATCH 1/7] feat(zend): add helper for try catch and bailout in PHP --- build.rs | 2 + src/embed/embed.c | 2 +- src/embed/ffi.rs | 2 +- src/embed/mod.rs | 50 ++++++++++------- src/ffi.rs | 6 ++ src/wrapper.c | 14 +++++ src/wrapper.h | 4 +- src/zend/mod.rs | 3 + src/zend/try_catch.rs | 126 ++++++++++++++++++++++++++++++++++++++++++ 9 files changed, 186 insertions(+), 23 deletions(-) create mode 100644 src/zend/try_catch.rs diff --git a/build.rs b/build.rs index 4ee804171..78c9cf322 100644 --- a/build.rs +++ b/build.rs @@ -248,6 +248,8 @@ fn main() -> Result<()> { for path in [ manifest.join("src").join("wrapper.h"), manifest.join("src").join("wrapper.c"), + manifest.join("src").join("embed").join("embed.h"), + manifest.join("src").join("embed").join("embed.c"), manifest.join("allowed_bindings.rs"), manifest.join("windows_build.rs"), manifest.join("unix_build.rs"), diff --git a/src/embed/embed.c b/src/embed/embed.c index d8b3a7831..ae7d8bc62 100644 --- a/src/embed/embed.c +++ b/src/embed/embed.c @@ -3,7 +3,7 @@ // We actually use the PHP embed API to run PHP code in test // At some point we might want to use our own SAPI to do that void* ext_php_rs_embed_callback(int argc, char** argv, void* (*callback)(void *), void *ctx) { - void *result; + void *result = NULL; PHP_EMBED_START_BLOCK(argc, argv) diff --git a/src/embed/ffi.rs b/src/embed/ffi.rs index 3a1f6d741..b52ce6a92 100644 --- a/src/embed/ffi.rs +++ b/src/embed/ffi.rs @@ -10,7 +10,7 @@ extern "C" { pub fn ext_php_rs_embed_callback( argc: c_int, argv: *mut *mut c_char, - func: unsafe extern "C" fn(*const c_void) -> *mut c_void, + func: unsafe extern "C" fn(*const c_void) -> *const c_void, ctx: *const c_void, ) -> *mut c_void; } diff --git a/src/embed/mod.rs b/src/embed/mod.rs index 581e38537..6f8083846 100644 --- a/src/embed/mod.rs +++ b/src/embed/mod.rs @@ -13,10 +13,10 @@ use crate::ffi::{ zend_stream_init_filename, ZEND_RESULT_CODE_SUCCESS, }; use crate::types::{ZendObject, Zval}; -use crate::zend::ExecutorGlobals; +use crate::zend::{panic_wrapper, ExecutorGlobals}; use parking_lot::{const_rwlock, RwLock}; use std::ffi::{c_char, c_void, CString, NulError}; -use std::panic::{catch_unwind, resume_unwind, RefUnwindSafe}; +use std::panic::{resume_unwind, RefUnwindSafe}; use std::path::Path; use std::ptr::null_mut; @@ -93,6 +93,12 @@ impl Embed { /// Which means subsequent calls to `Embed::eval` or `Embed::run_script` will be able to access /// variables defined in previous calls /// + /// # Returns + /// + /// * R - The result of the function passed to this method + /// + /// R must implement [`Default`] so it can be returned in case of a bailout + /// /// # Example /// /// ``` @@ -105,41 +111,36 @@ impl Embed { /// assert_eq!(foo.unwrap().string().unwrap(), "foo"); /// }); /// ``` - pub fn run(func: F) { + pub fn run R + RefUnwindSafe>(func: F) -> R + where + R: Default, + { // @TODO handle php thread safe // // This is to prevent multiple threads from running php at the same time // At some point we should detect if php is compiled with thread safety and avoid doing that in this case let _guard = RUN_FN_LOCK.write(); - unsafe extern "C" fn wrapper(ctx: *const c_void) -> *mut c_void { - // we try to catch panic here so we correctly shutdown php if it happens - // mandatory when we do assert on test as other test would not run correctly - let panic = catch_unwind(|| { - (*(ctx as *const F))(); - }); - - let panic_ptr = Box::into_raw(Box::new(panic)); - - panic_ptr as *mut c_void - } - let panic = unsafe { ext_php_rs_embed_callback( 0, null_mut(), - wrapper::, + panic_wrapper::, &func as *const F as *const c_void, ) }; + // This can happen if there is a bailout if panic.is_null() { - return; + return R::default(); } - if let Err(err) = unsafe { *Box::from_raw(panic as *mut std::thread::Result<()>) } { - // we resume the panic here so it can be catched correctly by the test framework - resume_unwind(err); + match unsafe { *Box::from_raw(panic as *mut std::thread::Result) } { + Ok(r) => r, + Err(err) => { + // we resume the panic here so it can be catched correctly by the test framework + resume_unwind(err); + } } } @@ -244,4 +245,13 @@ mod tests { panic!("test panic"); }); } + + #[test] + fn test_return() { + let foo = Embed::run(|| { + return "foo"; + }); + + assert_eq!(foo, "foo"); + } } diff --git a/src/ffi.rs b/src/ffi.rs index 92614c475..37b79ed26 100644 --- a/src/ffi.rs +++ b/src/ffi.rs @@ -26,6 +26,12 @@ extern "C" { pub fn ext_php_rs_zend_object_alloc(obj_size: usize, ce: *mut zend_class_entry) -> *mut c_void; pub fn ext_php_rs_zend_object_release(obj: *mut zend_object); pub fn ext_php_rs_executor_globals() -> *mut zend_executor_globals; + pub fn ext_php_rs_zend_try_catch( + func: unsafe extern "C" fn(*const c_void) -> *const c_void, + ctx: *const c_void, + result: *mut *mut c_void, + ) -> bool; + pub fn ext_php_rs_zend_bailout(); } include!(concat!(env!("OUT_DIR"), "/bindings.rs")); diff --git a/src/wrapper.c b/src/wrapper.c index faf585e41..a1fd900ff 100644 --- a/src/wrapper.c +++ b/src/wrapper.c @@ -39,3 +39,17 @@ zend_executor_globals *ext_php_rs_executor_globals() { return &executor_globals; #endif } + +bool ext_php_rs_zend_try_catch(void* (*callback)(void *), void *ctx, void **result) { + zend_try { + *result = callback(ctx); + } zend_catch { + return true; + } zend_end_try(); + + return false; +} + +void ext_php_rs_zend_bailout() { + zend_bailout(); +} diff --git a/src/wrapper.h b/src/wrapper.h index ed9dea629..e4e55517c 100644 --- a/src/wrapper.h +++ b/src/wrapper.h @@ -30,4 +30,6 @@ void ext_php_rs_set_known_valid_utf8(zend_string *zs); const char *ext_php_rs_php_build_id(); void *ext_php_rs_zend_object_alloc(size_t obj_size, zend_class_entry *ce); void ext_php_rs_zend_object_release(zend_object *obj); -zend_executor_globals *ext_php_rs_executor_globals(); \ No newline at end of file +zend_executor_globals *ext_php_rs_executor_globals(); +bool ext_php_rs_zend_try_catch(void* (*callback)(void *), void *ctx, void **result); +void ext_php_rs_zend_bailout(); diff --git a/src/zend/mod.rs b/src/zend/mod.rs index b3b1cfbdb..bd7d8f93e 100644 --- a/src/zend/mod.rs +++ b/src/zend/mod.rs @@ -9,6 +9,7 @@ mod globals; mod handlers; mod ini_entry_def; mod module; +mod try_catch; use crate::{error::Result, ffi::php_printf}; use std::ffi::CString; @@ -22,6 +23,8 @@ pub use globals::ExecutorGlobals; pub use handlers::ZendObjectHandlers; pub use ini_entry_def::IniEntryDef; pub use module::ModuleEntry; +pub(crate) use try_catch::panic_wrapper; +pub use try_catch::{bailout, try_catch}; // Used as the format string for `php_printf`. const FORMAT_STR: &[u8] = b"%s\0"; diff --git a/src/zend/try_catch.rs b/src/zend/try_catch.rs new file mode 100644 index 000000000..a80be1b63 --- /dev/null +++ b/src/zend/try_catch.rs @@ -0,0 +1,126 @@ +use crate::ffi::{ext_php_rs_zend_bailout, ext_php_rs_zend_try_catch}; +use std::ffi::c_void; +use std::panic::{catch_unwind, resume_unwind, RefUnwindSafe}; +use std::ptr::null_mut; + +#[derive(Debug)] +pub struct CatchError; + +pub(crate) unsafe extern "C" fn panic_wrapper R + RefUnwindSafe>( + ctx: *const c_void, +) -> *const c_void { + // we try to catch panic here so we correctly shutdown php if it happens + // mandatory when we do assert on test as other test would not run correctly + let panic = catch_unwind(|| (*(ctx as *const F))()); + + Box::into_raw(Box::new(panic)) as *mut c_void +} + +/// PHP propose a try catch mechanism in C using setjmp and longjmp (bailout) +/// It store the arg of setjmp into the bailout field of the global executor +/// If a bailout is triggered, the executor will jump to the setjmp and restore the previous setjmp +/// +/// try_catch allow to use this mechanism +/// +/// # Returns +/// +/// * `Ok(R)` - The result of the function +/// * `Err(CatchError)` - A bailout occurred during the execution +pub fn try_catch R + RefUnwindSafe>(func: F) -> Result { + let mut panic_ptr = null_mut(); + let has_bailout = unsafe { + ext_php_rs_zend_try_catch( + panic_wrapper::, + &func as *const F as *const c_void, + (&mut panic_ptr) as *mut *mut c_void, + ) + }; + + let panic = panic_ptr as *mut std::thread::Result; + + // can be null if there is a bailout + if panic.is_null() || has_bailout { + return Err(CatchError); + } + + match unsafe { *Box::from_raw(panic as *mut std::thread::Result) } { + Ok(r) => Ok(r), + Err(err) => { + // we resume the panic here so it can be catched correctly by the test framework + resume_unwind(err); + } + } +} + +/// Trigger a bailout +/// +/// This function will stop the execution of the current script +/// and jump to the last try catch block +pub fn bailout() { + unsafe { + ext_php_rs_zend_bailout(); + } +} + +#[cfg(test)] +mod tests { + use crate::embed::Embed; + use crate::zend::{bailout, try_catch}; + + #[test] + fn test_catch() { + Embed::run(|| { + let catch = try_catch(|| { + bailout(); + assert!(false); + }); + + assert!(catch.is_err()); + }); + } + + #[test] + fn test_no_catch() { + Embed::run(|| { + let catch = try_catch(|| { + assert!(true); + }); + + assert!(catch.is_ok()); + }); + } + + #[test] + fn test_bailout() { + Embed::run(|| { + bailout(); + + assert!(false); + }); + } + + #[test] + #[should_panic] + fn test_panic() { + Embed::run(|| { + let _ = try_catch(|| { + panic!("should panic"); + }); + }); + } + + #[test] + fn test_return() { + let foo = Embed::run(|| { + let result = try_catch(|| { + return "foo"; + }); + + assert!(result.is_ok()); + + result.unwrap() + }); + + assert_eq!(foo, "foo"); + } +} From 26b6067fff378c22a57d751229561f16ef9148f3 Mon Sep 17 00:00:00 2001 From: Joel Wurtz Date: Sat, 21 Oct 2023 22:34:01 +0200 Subject: [PATCH 2/7] feat(try): add bindings for bailout --- allowed_bindings.rs | 3 ++- docsrs_bindings.rs | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/allowed_bindings.rs b/allowed_bindings.rs index 4a5dbfc4d..ca2195bde 100644 --- a/allowed_bindings.rs +++ b/allowed_bindings.rs @@ -261,5 +261,6 @@ bind! { zend_file_handle, zend_stream_init_filename, php_execute_script, - zend_register_module_ex + zend_register_module_ex, + _zend_bailout } diff --git a/docsrs_bindings.rs b/docsrs_bindings.rs index 2d224908c..f7e54f1e2 100644 --- a/docsrs_bindings.rs +++ b/docsrs_bindings.rs @@ -789,6 +789,9 @@ pub struct _zend_class_entry__bindgen_ty_4__bindgen_ty_2 { pub builtin_functions: *const _zend_function_entry, pub module: *mut _zend_module_entry, } +extern "C" { + pub fn _zend_bailout(filename: *const ::std::os::raw::c_char, lineno: u32) -> !; +} extern "C" { pub static mut zend_interrupt_function: ::std::option::Option; From 2a3e3bc6b5f35ea502e0ee1d08ca310e093f223f Mon Sep 17 00:00:00 2001 From: Joel Wurtz Date: Sat, 21 Oct 2023 22:37:02 +0200 Subject: [PATCH 3/7] fix(try): add missing feature flag for test --- src/zend/mod.rs | 1 + src/zend/try_catch.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/zend/mod.rs b/src/zend/mod.rs index bd7d8f93e..af8a5c2d8 100644 --- a/src/zend/mod.rs +++ b/src/zend/mod.rs @@ -23,6 +23,7 @@ pub use globals::ExecutorGlobals; pub use handlers::ZendObjectHandlers; pub use ini_entry_def::IniEntryDef; pub use module::ModuleEntry; +#[cfg(feature = "embed")] pub(crate) use try_catch::panic_wrapper; pub use try_catch::{bailout, try_catch}; diff --git a/src/zend/try_catch.rs b/src/zend/try_catch.rs index a80be1b63..00ef4144b 100644 --- a/src/zend/try_catch.rs +++ b/src/zend/try_catch.rs @@ -62,6 +62,7 @@ pub fn bailout() { } } +#[cfg(feature = "embed")] #[cfg(test)] mod tests { use crate::embed::Embed; From 0ab1aea2094daa9d8ca96a8bd5670d4df1911b35 Mon Sep 17 00:00:00 2001 From: Joel Wurtz Date: Sun, 22 Oct 2023 00:07:25 +0200 Subject: [PATCH 4/7] feat(try): add a test that expose memory leak problem --- src/embed/mod.rs | 2 +- src/zend/try_catch.rs | 24 +++++++++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/embed/mod.rs b/src/embed/mod.rs index 6f8083846..9d8addb23 100644 --- a/src/embed/mod.rs +++ b/src/embed/mod.rs @@ -111,7 +111,7 @@ impl Embed { /// assert_eq!(foo.unwrap().string().unwrap(), "foo"); /// }); /// ``` - pub fn run R + RefUnwindSafe>(func: F) -> R + pub fn run R + RefUnwindSafe>(func: F) -> R where R: Default, { diff --git a/src/zend/try_catch.rs b/src/zend/try_catch.rs index 00ef4144b..b48c38aab 100644 --- a/src/zend/try_catch.rs +++ b/src/zend/try_catch.rs @@ -6,12 +6,12 @@ use std::ptr::null_mut; #[derive(Debug)] pub struct CatchError; -pub(crate) unsafe extern "C" fn panic_wrapper R + RefUnwindSafe>( +pub(crate) unsafe extern "C" fn panic_wrapper R + RefUnwindSafe>( ctx: *const c_void, ) -> *const c_void { // we try to catch panic here so we correctly shutdown php if it happens // mandatory when we do assert on test as other test would not run correctly - let panic = catch_unwind(|| (*(ctx as *const F))()); + let panic = catch_unwind(|| (*(ctx as *mut F))()); Box::into_raw(Box::new(panic)) as *mut c_void } @@ -26,7 +26,7 @@ pub(crate) unsafe extern "C" fn panic_wrapper R + RefUnwindSafe>( /// /// * `Ok(R)` - The result of the function /// * `Err(CatchError)` - A bailout occurred during the execution -pub fn try_catch R + RefUnwindSafe>(func: F) -> Result { +pub fn try_catch R + RefUnwindSafe>(func: F) -> Result { let mut panic_ptr = null_mut(); let has_bailout = unsafe { ext_php_rs_zend_try_catch( @@ -67,6 +67,7 @@ pub fn bailout() { mod tests { use crate::embed::Embed; use crate::zend::{bailout, try_catch}; + use std::ptr::null_mut; #[test] fn test_catch() { @@ -124,4 +125,21 @@ mod tests { assert_eq!(foo, "foo"); } + + #[test] + fn test_memory_leak() { + let mut ptr = null_mut(); + + let _ = try_catch(|| { + let mut result = "foo".to_string(); + ptr = &mut result; + + bailout(); + }); + + // Check that the string is never released + let result = unsafe { &*ptr as &str }; + + assert_eq!(result, "foo"); + } } From eecacb4ff0441dd5e4b37aa61da3b606e7488f1c Mon Sep 17 00:00:00 2001 From: Joel Wurtz Date: Sun, 22 Oct 2023 00:13:03 +0200 Subject: [PATCH 5/7] feat(try): make bailout unsafe and explain why --- src/zend/try_catch.rs | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/zend/try_catch.rs b/src/zend/try_catch.rs index b48c38aab..99c0aaf69 100644 --- a/src/zend/try_catch.rs +++ b/src/zend/try_catch.rs @@ -56,10 +56,16 @@ pub fn try_catch R + RefUnwindSafe>(func: F) -> Result Date: Sun, 22 Oct 2023 00:50:00 +0200 Subject: [PATCH 6/7] feat(bailout): flag bailout as a panic function --- src/ffi.rs | 2 +- src/zend/try_catch.rs | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/ffi.rs b/src/ffi.rs index 37b79ed26..a6c3d9486 100644 --- a/src/ffi.rs +++ b/src/ffi.rs @@ -31,7 +31,7 @@ extern "C" { ctx: *const c_void, result: *mut *mut c_void, ) -> bool; - pub fn ext_php_rs_zend_bailout(); + pub fn ext_php_rs_zend_bailout() -> !; } include!(concat!(env!("OUT_DIR"), "/bindings.rs")); diff --git a/src/zend/try_catch.rs b/src/zend/try_catch.rs index 99c0aaf69..f74b427a5 100644 --- a/src/zend/try_catch.rs +++ b/src/zend/try_catch.rs @@ -64,7 +64,7 @@ pub fn try_catch R + RefUnwindSafe>(func: F) -> Result ! { ext_php_rs_zend_bailout(); } @@ -83,7 +83,10 @@ mod tests { bailout(); } - assert!(false); + #[allow(unreachable_code)] + { + assert!(false); + } }); assert!(catch.is_err()); @@ -108,7 +111,10 @@ mod tests { bailout(); } - assert!(false); + #[allow(unreachable_code)] + { + assert!(false); + } }); } From e28c57e34cc227ee504cfcea5cd177a662cdbd10 Mon Sep 17 00:00:00 2001 From: Joel Wurtz Date: Sun, 22 Oct 2023 01:45:05 +0200 Subject: [PATCH 7/7] feat(embed): add try catch on script / eval --- src/embed/mod.rs | 44 ++++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/src/embed/mod.rs b/src/embed/mod.rs index 9d8addb23..0ad64a615 100644 --- a/src/embed/mod.rs +++ b/src/embed/mod.rs @@ -13,7 +13,7 @@ use crate::ffi::{ zend_stream_init_filename, ZEND_RESULT_CODE_SUCCESS, }; use crate::types::{ZendObject, Zval}; -use crate::zend::{panic_wrapper, ExecutorGlobals}; +use crate::zend::{panic_wrapper, try_catch, ExecutorGlobals}; use parking_lot::{const_rwlock, RwLock}; use std::ffi::{c_char, c_void, CString, NulError}; use std::panic::{resume_unwind, RefUnwindSafe}; @@ -29,6 +29,13 @@ pub enum EmbedError { ExecuteScriptError, InvalidEvalString(NulError), InvalidPath, + CatchError, +} + +impl EmbedError { + pub fn is_bailout(&self) -> bool { + matches!(self, EmbedError::CatchError) + } } static RUN_FN_LOCK: RwLock<()> = const_rwlock(()); @@ -79,10 +86,12 @@ impl Embed { zend_stream_init_filename(&mut file_handle, path.as_ptr()); } - if unsafe { php_execute_script(&mut file_handle) } { - Ok(()) - } else { - Err(EmbedError::ExecuteScriptError) + let exec_result = try_catch(|| unsafe { php_execute_script(&mut file_handle) }); + + match exec_result { + Err(_) => Err(EmbedError::CatchError), + Ok(true) => Ok(()), + Ok(false) => Err(EmbedError::ExecuteScriptError), } } @@ -171,21 +180,18 @@ impl Embed { let mut result = Zval::new(); - // this eval is very limited as it only allow simple code, it's the same eval used by php -r - let exec_result = unsafe { + let exec_result = try_catch(|| unsafe { zend_eval_string( cstr.as_ptr() as *const c_char, &mut result, b"run\0".as_ptr() as *const _, ) - }; - - let exception = ExecutorGlobals::take_exception(); + }); - if exec_result != ZEND_RESULT_CODE_SUCCESS { - Err(EmbedError::ExecuteError(exception)) - } else { - Ok(result) + match exec_result { + Err(_) => Err(EmbedError::CatchError), + Ok(ZEND_RESULT_CODE_SUCCESS) => Ok(result), + Ok(_) => Err(EmbedError::ExecuteError(ExecutorGlobals::take_exception())), } } } @@ -254,4 +260,14 @@ mod tests { assert_eq!(foo, "foo"); } + + #[test] + fn test_eval_bailout() { + Embed::run(|| { + let result = Embed::eval("str_repeat('a', 100_000_000_000_000);"); + + assert!(result.is_err()); + assert!(result.unwrap_err().is_bailout()); + }); + } }