diff --git a/crates/macros/src/function.rs b/crates/macros/src/function.rs index bcc4865cc..ae0029b4c 100644 --- a/crates/macros/src/function.rs +++ b/crates/macros/src/function.rs @@ -207,15 +207,26 @@ impl<'a> Function<'a> { retval: &mut ::ext_php_rs::types::Zval, ) { use ::ext_php_rs::convert::IntoZval; + use ::ext_php_rs::zend::try_catch; + use ::std::panic::AssertUnwindSafe; + + // Wrap the handler body with try_catch to ensure Rust destructors + // are called if a bailout occurs (issue #537) + let catch_result = try_catch(AssertUnwindSafe(|| { + #(#arg_declarations)* + let result = { + #result + }; + + if let Err(e) = result.set_zval(retval, false) { + let e: ::ext_php_rs::exception::PhpException = e.into(); + e.throw().expect("Failed to throw PHP exception."); + } + })); - #(#arg_declarations)* - let result = { - #result - }; - - if let Err(e) = result.set_zval(retval, false) { - let e: ::ext_php_rs::exception::PhpException = e.into(); - e.throw().expect("Failed to throw PHP exception."); + // If there was a bailout, re-trigger it after Rust cleanup + if catch_result.is_err() { + unsafe { ::ext_php_rs::zend::bailout(); } } } } @@ -410,18 +421,31 @@ impl<'a> Function<'a> { ::ext_php_rs::class::ConstructorMeta { constructor: { fn inner(ex: &mut ::ext_php_rs::zend::ExecuteData) -> ::ext_php_rs::class::ConstructorResult<#class> { - #(#arg_declarations)* - let parse = ex.parser() - #(.arg(&mut #required_arg_names))* - .not_required() - #(.arg(&mut #not_required_arg_names))* - #variadic - .parse(); - if parse.is_err() { - return ::ext_php_rs::class::ConstructorResult::ArgError; + use ::ext_php_rs::zend::try_catch; + use ::std::panic::AssertUnwindSafe; + + // Wrap the constructor body with try_catch to ensure Rust destructors + // are called if a bailout occurs (issue #537) + let catch_result = try_catch(AssertUnwindSafe(|| { + #(#arg_declarations)* + let parse = ex.parser() + #(.arg(&mut #required_arg_names))* + .not_required() + #(.arg(&mut #not_required_arg_names))* + #variadic + .parse(); + if parse.is_err() { + return ::ext_php_rs::class::ConstructorResult::ArgError; + } + #(#variadic_bindings)* + #class::#ident(#({#arg_accessors}),*).into() + })); + + // If there was a bailout, re-trigger it after Rust cleanup + match catch_result { + Ok(result) => result, + Err(_) => unsafe { ::ext_php_rs::zend::bailout() }, } - #(#variadic_bindings)* - #class::#ident(#({#arg_accessors}),*).into() } inner }, diff --git a/src/builders/class.rs b/src/builders/class.rs index e14a82c6b..0d1b17ecd 100644 --- a/src/builders/class.rs +++ b/src/builders/class.rs @@ -202,31 +202,43 @@ impl ClassBuilder { zend_fastcall! { extern fn constructor(ex: &mut ExecuteData, _: &mut Zval) { - let Some(ConstructorMeta { constructor, .. }) = T::constructor() else { - PhpException::default("You cannot instantiate this class from PHP.".into()) - .throw() - .expect("Failed to throw exception when constructing class"); - return; - }; - - let this = match constructor(ex) { - ConstructorResult::Ok(this) => this, - ConstructorResult::Exception(e) => { - e.throw() + use crate::zend::try_catch; + use std::panic::AssertUnwindSafe; + + // Wrap the constructor body with try_catch to ensure Rust destructors + // are called if a bailout occurs (issue #537) + let catch_result = try_catch(AssertUnwindSafe(|| { + let Some(ConstructorMeta { constructor, .. }) = T::constructor() else { + PhpException::default("You cannot instantiate this class from PHP.".into()) + .throw() + .expect("Failed to throw exception when constructing class"); + return; + }; + + let this = match constructor(ex) { + ConstructorResult::Ok(this) => this, + ConstructorResult::Exception(e) => { + e.throw() + .expect("Failed to throw exception while constructing class"); + return; + } + ConstructorResult::ArgError => return, + }; + + let Some(this_obj) = ex.get_object::() else { + PhpException::default("Failed to retrieve reference to `this` object.".into()) + .throw() .expect("Failed to throw exception while constructing class"); return; - } - ConstructorResult::ArgError => return, - }; - - let Some(this_obj) = ex.get_object::() else { - PhpException::default("Failed to retrieve reference to `this` object.".into()) - .throw() - .expect("Failed to throw exception while constructing class"); - return; - }; - - this_obj.initialize(this); + }; + + this_obj.initialize(this); + })); + + // If there was a bailout, re-trigger it after Rust cleanup + if catch_result.is_err() { + unsafe { crate::zend::bailout(); } + } } } diff --git a/src/embed/mod.rs b/src/embed/mod.rs index 97df7e0aa..71e821f0f 100644 --- a/src/embed/mod.rs +++ b/src/embed/mod.rs @@ -17,7 +17,7 @@ use crate::types::{ZendObject, Zval}; use crate::zend::{ExecutorGlobals, panic_wrapper, try_catch}; use parking_lot::{RwLock, const_rwlock}; use std::ffi::{CString, NulError, c_char, c_void}; -use std::panic::{RefUnwindSafe, resume_unwind}; +use std::panic::{AssertUnwindSafe, UnwindSafe, resume_unwind}; use std::path::Path; use std::ptr::null_mut; @@ -105,7 +105,9 @@ impl Embed { zend_stream_init_filename(&raw mut file_handle, path.as_ptr()); } - let exec_result = try_catch(|| unsafe { php_execute_script(&raw mut file_handle) }); + let exec_result = try_catch(AssertUnwindSafe(|| unsafe { + php_execute_script(&raw mut file_handle) + })); unsafe { zend_destroy_file_handle(&raw mut file_handle) } @@ -141,7 +143,7 @@ impl Embed { /// assert_eq!(foo.unwrap().string().unwrap(), "foo"); /// }); /// ``` - pub fn run R + RefUnwindSafe>(func: F) -> R + pub fn run R + UnwindSafe>(func: F) -> R where R: Default, { @@ -161,6 +163,9 @@ impl Embed { ) }; + // Prevent the closure from being dropped here since it was consumed in panic_wrapper + std::mem::forget(func); + // This can happen if there is a bailout if panic.is_null() { return R::default(); @@ -206,13 +211,13 @@ impl Embed { let mut result = Zval::new(); - let exec_result = try_catch(|| unsafe { + let exec_result = try_catch(AssertUnwindSafe(|| unsafe { zend_eval_string( cstr.as_ptr().cast::(), &raw mut result, c"run".as_ptr().cast(), ) - }); + })); match exec_result { Err(_) => Err(EmbedError::CatchError), diff --git a/src/zend/try_catch.rs b/src/zend/try_catch.rs index ec2e95139..58aa39bc4 100644 --- a/src/zend/try_catch.rs +++ b/src/zend/try_catch.rs @@ -2,19 +2,22 @@ use crate::ffi::{ ext_php_rs_zend_bailout, ext_php_rs_zend_first_try_catch, ext_php_rs_zend_try_catch, }; use std::ffi::c_void; -use std::panic::{RefUnwindSafe, catch_unwind, resume_unwind}; +use std::panic::{UnwindSafe, catch_unwind, resume_unwind}; use std::ptr::null_mut; /// Error returned when a bailout occurs #[derive(Debug)] pub struct CatchError; -pub(crate) unsafe extern "C" fn panic_wrapper R + RefUnwindSafe>( +pub(crate) unsafe extern "C" fn panic_wrapper R + UnwindSafe>( 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(|| unsafe { (*(ctx as *mut F))() }); + // SAFETY: We read the closure from the pointer and consume it. This is safe because + // the closure is only called once. + let func = unsafe { std::ptr::read(ctx.cast::()) }; + let panic = catch_unwind(func); Box::into_raw(Box::new(panic)).cast::() } @@ -33,7 +36,7 @@ pub(crate) unsafe extern "C" fn panic_wrapper R + RefUnwindSafe /// # Errors /// /// * [`CatchError`] - A bailout occurred during the execution -pub fn try_catch R + RefUnwindSafe>(func: F) -> Result { +pub fn try_catch R + UnwindSafe>(func: F) -> Result { do_try_catch(func, false) } @@ -54,11 +57,11 @@ pub fn try_catch R + RefUnwindSafe>(func: F) -> Result R + RefUnwindSafe>(func: F) -> Result { +pub fn try_catch_first R + UnwindSafe>(func: F) -> Result { do_try_catch(func, true) } -fn do_try_catch R + RefUnwindSafe>(func: F, first: bool) -> Result { +fn do_try_catch R + UnwindSafe>(func: F, first: bool) -> Result { let mut panic_ptr = null_mut(); let has_bailout = unsafe { if first { @@ -76,6 +79,9 @@ fn do_try_catch R + RefUnwindSafe>(func: F, first: bool) -> Res } }; + // Prevent the closure from being dropped here since it was consumed in panic_wrapper + std::mem::forget(func); + let panic = panic_ptr.cast::>(); // can be null if there is a bailout @@ -190,17 +196,19 @@ mod tests { #[test] fn test_memory_leak() { + use std::panic::AssertUnwindSafe; + Embed::run(|| { let mut ptr = null_mut(); - let _ = try_catch(|| { + let _ = try_catch(AssertUnwindSafe(|| { let mut result = "foo".to_string(); ptr = &raw mut result; unsafe { bailout(); } - }); + })); // Check that the string is never released let result = unsafe { &*ptr as &str }; diff --git a/tests/sapi.rs b/tests/sapi.rs index 73136d4f6..688fd63dc 100644 --- a/tests/sapi.rs +++ b/tests/sapi.rs @@ -166,7 +166,7 @@ fn test_sapi_multithread() { Ok(zval) => { assert!(zval.is_string()); let string = zval.string().unwrap(); - let output = string.to_string(); + let output = string.clone(); assert_eq!(output, format!("Hello, thread-{i}!")); results.lock().unwrap().push((i, output)); diff --git a/tests/src/integration/bailout/bailout_control.php b/tests/src/integration/bailout/bailout_control.php new file mode 100644 index 000000000..594ae8f60 --- /dev/null +++ b/tests/src/integration/bailout/bailout_control.php @@ -0,0 +1,11 @@ + Self { + Self { _id: id } + } +} + +impl Drop for DropTracker { + fn drop(&mut self) { + // Increment the counter to prove the destructor was called + DROP_COUNTER.fetch_add(1, Ordering::SeqCst); + } +} + +/// Reset the drop counter (called from PHP before test) +#[php_function] +pub fn bailout_test_reset() { + DROP_COUNTER.store(0, Ordering::SeqCst); +} + +/// Get the current drop counter value +#[php_function] +pub fn bailout_test_get_counter() -> u32 { + DROP_COUNTER.load(Ordering::SeqCst) +} + +/// Create a `DropTracker` and then call a PHP callback that triggers `exit()`. +/// If the fix for issue #537 works, the destructor should be called +/// before the exit actually happens. +#[php_function] +pub fn bailout_test_with_callback(callback: ext_php_rs::types::ZendCallable) { + let _tracker1 = DropTracker::new(1); + let _tracker2 = DropTracker::new(2); + let _tracker3 = DropTracker::new(3); + + // Call the PHP callback which will trigger exit() + // try_call catches bailouts internally, so we need to check if it failed + // and re-trigger the bailout manually + let result = callback.try_call(vec![]); + + // If the callback triggered a bailout (exit/die/fatal error), + // re-trigger it after our destructors have a chance to run. + // The destructors will run when this function exits, before the + // bailout is re-triggered by the handler wrapper. + if result.is_err() { + // Don't re-trigger here - let the handler wrapper do it + // The handler wrapper's try_catch will see this as a normal return, + // but our destructors will still run when this function's scope ends + } +} + +/// Create a `DropTracker` without bailout (control test) +#[php_function] +pub fn bailout_test_without_exit() { + let _tracker1 = DropTracker::new(1); + let _tracker2 = DropTracker::new(2); + + // No bailout - destructors should run normally when function returns +} + +pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { + builder + .function(wrap_function!(bailout_test_reset)) + .function(wrap_function!(bailout_test_get_counter)) + .function(wrap_function!(bailout_test_with_callback)) + .function(wrap_function!(bailout_test_without_exit)) +} + +#[cfg(test)] +mod tests { + #[test] + fn bailout_destructor_called() { + // First, run the control test (no bailout) to verify basic functionality + assert!(crate::integration::test::run_php( + "bailout/bailout_control.php" + )); + + // Now run the bailout test - this verifies that destructors are called + // even when a PHP callback triggers exit() + assert!(crate::integration::test::run_php( + "bailout/bailout_exit.php" + )); + } +} diff --git a/tests/src/integration/mod.rs b/tests/src/integration/mod.rs index 60acb2958..ad2f2eb12 100644 --- a/tests/src/integration/mod.rs +++ b/tests/src/integration/mod.rs @@ -1,4 +1,5 @@ pub mod array; +pub mod bailout; pub mod binary; pub mod bool; pub mod callable; @@ -101,7 +102,7 @@ mod test { } /// Finds the location of the PHP executable. - fn find_php() -> Result { + pub fn find_php() -> Result { // If path is given via env, it takes priority. if let Some(path) = path_from_env("PHP") { if !path @@ -120,8 +121,8 @@ mod test { }) } - pub fn run_php(file: &str) -> bool { - setup(); + /// Gets the path to the compiled test extension. + pub fn get_extension_path() -> String { let mut path = env::current_dir().expect("Could not get cwd"); path.pop(); path.push("target"); @@ -137,8 +138,14 @@ mod test { "libtests" }); path.set_extension(std::env::consts::DLL_EXTENSION); + path.to_str().unwrap().to_string() + } + + pub fn run_php(file: &str) -> bool { + setup(); + let path = get_extension_path(); let output = Command::new(find_php().expect("Could not find PHP executable")) - .arg(format!("-dextension={}", path.to_str().unwrap())) + .arg(format!("-dextension={path}")) .arg("-dassert.active=1") .arg("-dassert.exception=1") .arg("-dzend.assertions=1") diff --git a/tests/src/lib.rs b/tests/src/lib.rs index 3b7b2141d..589db2ba4 100644 --- a/tests/src/lib.rs +++ b/tests/src/lib.rs @@ -12,6 +12,7 @@ mod integration; #[php_module] pub fn build_module(module: ModuleBuilder) -> ModuleBuilder { let mut module = integration::array::build_module(module); + module = integration::bailout::build_module(module); module = integration::binary::build_module(module); module = integration::bool::build_module(module); module = integration::callable::build_module(module);