From 334a90963a6d3b903b813a8449df03b54c1a4a47 Mon Sep 17 00:00:00 2001 From: Vasily Zorin Date: Tue, 23 Dec 2025 20:00:33 +0700 Subject: [PATCH] fix(zend_bailout): Fix zend_bailout handling #537 --- crates/macros/src/function.rs | 53 +++- guide/src/SUMMARY.md | 1 + guide/src/advanced/bailout_guard.md | 147 +++++++++ src/builders/class.rs | 58 ++-- src/embed/mod.rs | 15 +- src/lib.rs | 1 + src/zend/bailout_guard.rs | 282 ++++++++++++++++++ src/zend/mod.rs | 3 + src/zend/try_catch.rs | 24 +- .../integration/bailout/bailout_control.php | 11 + .../bailout/bailout_deep_nested.php | 18 ++ .../src/integration/bailout/bailout_exit.php | 16 + .../src/integration/bailout/bailout_guard.php | 15 + .../integration/bailout/bailout_nested.php | 16 + tests/src/integration/bailout/mod.rs | 193 ++++++++++++ tests/src/integration/mod.rs | 15 +- tests/src/lib.rs | 1 + 17 files changed, 817 insertions(+), 52 deletions(-) create mode 100644 guide/src/advanced/bailout_guard.md create mode 100644 src/zend/bailout_guard.rs create mode 100644 tests/src/integration/bailout/bailout_control.php create mode 100644 tests/src/integration/bailout/bailout_deep_nested.php create mode 100644 tests/src/integration/bailout/bailout_exit.php create mode 100644 tests/src/integration/bailout/bailout_guard.php create mode 100644 tests/src/integration/bailout/bailout_nested.php create mode 100644 tests/src/integration/bailout/mod.rs diff --git a/crates/macros/src/function.rs b/crates/macros/src/function.rs index 7935b9ab1..6c8ba077e 100644 --- a/crates/macros/src/function.rs +++ b/crates/macros/src/function.rs @@ -291,7 +291,20 @@ impl<'a> Function<'a> { ex: &mut ::ext_php_rs::zend::ExecuteData, retval: &mut ::ext_php_rs::types::Zval, ) { - #handler_body + 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(|| { + #handler_body + })); + + // If there was a bailout, run BailoutGuard cleanups and re-trigger + if catch_result.is_err() { + ::ext_php_rs::zend::run_bailout_cleanups(); + unsafe { ::ext_php_rs::zend::bailout(); } + } } } handler @@ -535,18 +548,34 @@ 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, run BailoutGuard cleanups and re-trigger + match catch_result { + Ok(result) => result, + Err(_) => { + ::ext_php_rs::zend::run_bailout_cleanups(); + unsafe { ::ext_php_rs::zend::bailout() } + } } - #(#variadic_bindings)* - #class::#ident(#({#arg_accessors}),*).into() } inner }, diff --git a/guide/src/SUMMARY.md b/guide/src/SUMMARY.md index 914918618..0a5a2d490 100644 --- a/guide/src/SUMMARY.md +++ b/guide/src/SUMMARY.md @@ -42,6 +42,7 @@ # Advanced Topics - [Async](./advanced/async_impl.md) +- [Bailout Guard](./advanced/bailout_guard.md) - [Allowed Bindings](./advanced/allowed_bindings.md) # Migration Guides diff --git a/guide/src/advanced/bailout_guard.md b/guide/src/advanced/bailout_guard.md new file mode 100644 index 000000000..a3ce53e0f --- /dev/null +++ b/guide/src/advanced/bailout_guard.md @@ -0,0 +1,147 @@ +# Bailout Guard + +When PHP triggers a "bailout" (via `exit()`, `die()`, or a fatal error), it uses +`longjmp` to unwind the stack. This bypasses Rust's normal drop semantics, +meaning destructors for stack-allocated values won't run. This can lead to +resource leaks for things like file handles, network connections, or locks. + +## The Problem + +Consider this code: + +```rust,ignore +#[php_function] +pub fn process_file(callback: ZendCallable) { + let file = File::open("data.txt").unwrap(); + + // If callback calls exit(), the file handle leaks! + callback.try_call(vec![]); + + // file.drop() never runs +} +``` + +If the PHP callback triggers `exit()`, the `File` handle is never closed because +`longjmp` skips Rust's destructor calls. + +## Solution 1: Using `try_call` + +The simplest solution is to use `try_call` for PHP callbacks. It catches bailouts +internally and returns normally, allowing Rust destructors to run: + +```rust,ignore +#[php_function] +pub fn process_file(callback: ZendCallable) { + let file = File::open("data.txt").unwrap(); + + // try_call catches bailout, function returns, file is dropped + let result = callback.try_call(vec![]); + + if result.is_err() { + // Bailout occurred, but file will still be closed + // when this function returns + } +} +``` + +## Solution 2: Using `BailoutGuard` + +For cases where you need guaranteed cleanup even if bailout occurs directly +(not through `try_call`), use `BailoutGuard`: + +```rust,ignore +use ext_php_rs::prelude::*; +use std::fs::File; + +#[php_function] +pub fn process_file(callback: ZendCallable) { + // Wrap the file handle in BailoutGuard + let file = BailoutGuard::new(File::open("data.txt").unwrap()); + + // Even if bailout occurs, the file will be closed + callback.try_call(vec![]); + + // Use the file via Deref + // file.read_to_string(...); +} +``` + +### How `BailoutGuard` Works + +1. **Heap allocation**: The wrapped value is heap-allocated so it survives + the `longjmp` stack unwinding. + +2. **Cleanup registration**: A cleanup callback is registered in thread-local + storage when the guard is created. + +3. **On normal drop**: The cleanup is cancelled and the value is dropped normally. + +4. **On bailout**: Before re-triggering the bailout, all registered cleanup + callbacks are executed, dropping the guarded values. + +### API + +```rust,ignore +// Create a guard +let guard = BailoutGuard::new(value); + +// Access the value (implements Deref and DerefMut) +guard.do_something(); +let inner: &T = &*guard; +let inner_mut: &mut T = &mut *guard; + +// Explicitly get references +let inner: &T = guard.get(); +let inner_mut: &mut T = guard.get_mut(); + +// Extract the value, cancelling cleanup +let value: T = guard.into_inner(); +``` + +### Performance Note + +`BailoutGuard` incurs a heap allocation. Only use it for values that absolutely +must be cleaned up, such as: + +- File handles +- Network connections +- Database connections +- Locks and mutexes +- Other system resources + +For simple values without cleanup requirements, the overhead isn't worth it. + +## Nested Calls + +`BailoutGuard` works correctly with nested function calls. Guards at all +nesting levels are cleaned up when bailout occurs: + +```rust,ignore +#[php_function] +pub fn outer_function(callback: ZendCallable) { + let _outer_resource = BailoutGuard::new(Resource::new()); + + inner_function(&callback); +} + +fn inner_function(callback: &ZendCallable) { + let _inner_resource = BailoutGuard::new(Resource::new()); + + // If bailout occurs here, both inner and outer resources are cleaned up + callback.try_call(vec![]); +} +``` + +## Best Practices + +1. **Prefer `try_call`**: For most cases, using `try_call` and handling the + error result is simpler and doesn't require heap allocation. + +2. **Use `BailoutGuard` for critical resources**: Only wrap values that + absolutely must be cleaned up (connections, locks, etc.). + +3. **Don't overuse**: Not every value needs to be wrapped. Simple data + structures without cleanup requirements don't need `BailoutGuard`. + +4. **Combine approaches**: Use `try_call` where possible and `BailoutGuard` + for critical resources that must be cleaned up regardless. diff --git a/src/builders/class.rs b/src/builders/class.rs index cb4d111d1..8b61f9eab 100644 --- a/src/builders/class.rs +++ b/src/builders/class.rs @@ -219,31 +219,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 6dadb37d1..d175c3f69 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/lib.rs b/src/lib.rs index 1f9c53a53..b33ef56e0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -56,6 +56,7 @@ pub mod prelude { pub use crate::php_println; pub use crate::php_write; pub use crate::types::ZendCallable; + pub use crate::zend::BailoutGuard; pub use crate::{ ZvalConvert, php_class, php_const, php_extern, php_function, php_impl, php_interface, php_module, wrap_constant, wrap_function, zend_fastcall, diff --git a/src/zend/bailout_guard.rs b/src/zend/bailout_guard.rs new file mode 100644 index 000000000..699e85cff --- /dev/null +++ b/src/zend/bailout_guard.rs @@ -0,0 +1,282 @@ +//! Provides cleanup guarantees for values that need to be dropped even when PHP bailout occurs. +//! +//! When PHP triggers a bailout (via `exit()`, fatal error, etc.), it uses `longjmp` which +//! bypasses Rust's normal stack unwinding. This means destructors for stack-allocated values +//! won't run. `BailoutGuard` solves this by heap-allocating values and registering cleanup +//! callbacks that run when a bailout is caught. +//! +//! # Example +//! +//! ```ignore +//! use ext_php_rs::zend::BailoutGuard; +//! +//! #[php_function] +//! pub fn my_function(callback: ZendCallable) { +//! // Wrap resources that MUST be cleaned up in BailoutGuard +//! let resource = BailoutGuard::new(ExpensiveResource::new()); +//! +//! // Use the resource (BailoutGuard implements Deref/DerefMut) +//! resource.do_something(); +//! +//! // If the callback triggers exit(), the resource will still be cleaned up +//! let _ = callback.try_call(vec![]); +//! } +//! ``` + +use std::cell::RefCell; +use std::ops::{Deref, DerefMut}; + +/// A cleanup entry: (callback, active). The active flag is set to false when +/// the guard is dropped normally, so we don't double-drop. +type CleanupEntry = (Box, bool); + +thread_local! { + /// Stack of cleanup callbacks to run when bailout is caught. + static CLEANUP_STACK: RefCell> = const { RefCell::new(Vec::new()) }; +} + +/// A guard that ensures a value is dropped even if PHP bailout occurs. +/// +/// `BailoutGuard` heap-allocates the wrapped value and registers a cleanup callback. +/// If a bailout occurs, the cleanup runs before the bailout is re-triggered. +/// If the guard is dropped normally, the cleanup is cancelled and the value is dropped. +/// +/// # Performance Note +/// +/// This incurs a heap allocation. Only use for values that absolutely must be +/// cleaned up (file handles, network connections, locks, etc.). For simple values, +/// the overhead isn't worth it. +pub struct BailoutGuard { + /// Pointer to the heap-allocated value. Using raw pointer because we need + /// to pass it to the cleanup callback. + value: *mut T, + /// Index in the cleanup stack. Used to deactivate cleanup on normal drop. + index: usize, +} + +// SAFETY: BailoutGuard can be sent between threads if T can. +// The cleanup stack is thread-local, so each thread has its own. +unsafe impl Send for BailoutGuard {} + +impl BailoutGuard { + /// Creates a new `BailoutGuard` wrapping the given value. + /// + /// The value is heap-allocated and a cleanup callback is registered. + /// If a bailout occurs, the value will be dropped. If this guard is + /// dropped normally, the value is dropped and the cleanup is cancelled. + pub fn new(value: T) -> Self { + let boxed = Box::new(value); + let ptr = Box::into_raw(boxed); + + let index = CLEANUP_STACK.with(|stack| { + let mut stack = stack.borrow_mut(); + let idx = stack.len(); + let ptr_copy = ptr; + // Register cleanup that drops the heap-allocated value + stack.push(( + Box::new(move || { + // SAFETY: This only runs if bailout occurred and normal drop didn't. + // The pointer is valid because we heap-allocated it. + unsafe { + drop(Box::from_raw(ptr_copy)); + } + }), + true, // active + )); + idx + }); + + Self { value: ptr, index } + } + + /// Returns a reference to the wrapped value. + #[inline] + #[must_use] + pub fn get(&self) -> &T { + // SAFETY: The pointer is valid as long as self exists. + unsafe { &*self.value } + } + + /// Returns a mutable reference to the wrapped value. + #[inline] + pub fn get_mut(&mut self) -> &mut T { + // SAFETY: The pointer is valid as long as self exists, and we have &mut self. + unsafe { &mut *self.value } + } + + /// Consumes the guard and returns the wrapped value. + /// + /// The cleanup callback is cancelled. + #[must_use] + pub fn into_inner(self) -> T { + // Deactivate cleanup + CLEANUP_STACK.with(|stack| { + let mut stack = stack.borrow_mut(); + if self.index < stack.len() { + stack[self.index].1 = false; + } + }); + + // Take ownership of the value + // SAFETY: We're consuming self, so no one else can access the pointer. + let value = unsafe { *Box::from_raw(self.value) }; + + // Prevent Drop from running (we've already handled cleanup) + std::mem::forget(self); + + value + } +} + +impl Deref for BailoutGuard { + type Target = T; + + #[inline] + fn deref(&self) -> &T { + // SAFETY: The pointer is valid as long as self exists. + unsafe { &*self.value } + } +} + +impl DerefMut for BailoutGuard { + #[inline] + fn deref_mut(&mut self) -> &mut T { + // SAFETY: The pointer is valid as long as self exists, and we have &mut self. + unsafe { &mut *self.value } + } +} + +impl Drop for BailoutGuard { + fn drop(&mut self) { + // Deactivate cleanup callback (we're dropping normally) + CLEANUP_STACK.with(|stack| { + let mut stack = stack.borrow_mut(); + if self.index < stack.len() { + stack[self.index].1 = false; + } + }); + + // Drop the heap-allocated value + // SAFETY: We're in Drop, so no one else can access the pointer. + unsafe { + drop(Box::from_raw(self.value)); + } + } +} + +/// Runs all registered bailout cleanup callbacks. +/// +/// This should be called after catching a bailout and before re-triggering it. +/// Only active cleanups (those whose guards haven't been dropped) are run. +/// +/// # Note +/// +/// This function is automatically called by the generated handler code when a +/// bailout is caught. You typically don't need to call this directly. +#[doc(hidden)] +pub fn run_bailout_cleanups() { + CLEANUP_STACK.with(|stack| { + // Drain and run all active cleanups in reverse order (LIFO) + for (cleanup, active) in stack.borrow_mut().drain(..).rev() { + if active { + cleanup(); + } + } + }); +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + + /// Creates a drop counter that increments the given `AtomicUsize` on drop. + fn make_drop_counter(counter: Arc) -> impl Drop + 'static { + struct DropCounter(Arc); + impl Drop for DropCounter { + fn drop(&mut self) { + self.0.fetch_add(1, Ordering::SeqCst); + } + } + DropCounter(counter) + } + + #[test] + fn test_normal_drop() { + let drop_count = Arc::new(AtomicUsize::new(0)); + // Clear any leftover cleanup entries from previous tests + CLEANUP_STACK.with(|stack| stack.borrow_mut().clear()); + + { + let _guard = BailoutGuard::new(make_drop_counter(Arc::clone(&drop_count))); + assert_eq!(drop_count.load(Ordering::SeqCst), 0); + } + + // Value should be dropped when guard goes out of scope + assert_eq!(drop_count.load(Ordering::SeqCst), 1); + + // Cleanup stack should be empty (cleanup was deactivated) + CLEANUP_STACK.with(|stack| { + assert!(stack.borrow().is_empty() || !stack.borrow()[0].1); + }); + } + + #[test] + fn test_bailout_cleanup() { + let drop_count = Arc::new(AtomicUsize::new(0)); + // Clear any leftover cleanup entries from previous tests + CLEANUP_STACK.with(|stack| stack.borrow_mut().clear()); + + // Simulate what happens during bailout: + // 1. Guard is created + // 2. Bailout occurs (longjmp) - guard's Drop doesn't run + // 3. run_bailout_cleanups() is called + + let guard = BailoutGuard::new(make_drop_counter(Arc::clone(&drop_count))); + + // Simulate bailout - don't drop the guard normally + std::mem::forget(guard); + + // Value hasn't been dropped yet + assert_eq!(drop_count.load(Ordering::SeqCst), 0); + + // Run bailout cleanups (simulating what try_catch does) + run_bailout_cleanups(); + + // Value should now be dropped + assert_eq!(drop_count.load(Ordering::SeqCst), 1); + } + + #[test] + fn test_into_inner() { + let drop_count = Arc::new(AtomicUsize::new(0)); + // Clear any leftover cleanup entries from previous tests + CLEANUP_STACK.with(|stack| stack.borrow_mut().clear()); + + let guard = BailoutGuard::new(make_drop_counter(Arc::clone(&drop_count))); + let value = guard.into_inner(); + + // Value hasn't been dropped yet (we own it now) + assert_eq!(drop_count.load(Ordering::SeqCst), 0); + + drop(value); + + // Now it's dropped + assert_eq!(drop_count.load(Ordering::SeqCst), 1); + } + + #[test] + fn test_deref() { + let guard = BailoutGuard::new(String::from("hello")); + assert_eq!(&*guard, "hello"); + assert_eq!(guard.len(), 5); + } + + #[test] + fn test_deref_mut() { + let mut guard = BailoutGuard::new(String::from("hello")); + guard.push_str(" world"); + assert_eq!(&*guard, "hello world"); + } +} diff --git a/src/zend/mod.rs b/src/zend/mod.rs index 330f221a5..a87251013 100644 --- a/src/zend/mod.rs +++ b/src/zend/mod.rs @@ -1,6 +1,7 @@ //! Types used to interact with the Zend engine. mod _type; +mod bailout_guard; pub mod ce; mod class; mod ex; @@ -21,6 +22,8 @@ use std::ffi::CString; use std::os::raw::c_char; pub use _type::ZendType; +pub use bailout_guard::BailoutGuard; +pub use bailout_guard::run_bailout_cleanups; pub use class::ClassEntry; pub use ex::ExecuteData; pub use function::Function; 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/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 +} + +/// Test `BailoutGuard` - wrap resources that MUST be cleaned up in `BailoutGuard`. +/// This demonstrates using `BailoutGuard` for guaranteed cleanup even on direct bailout. +#[php_function] +pub fn bailout_test_with_guard(callback: ext_php_rs::types::ZendCallable) { + // Wrap trackers in BailoutGuard - these will be cleaned up even if bailout + // occurs directly (not caught by try_call) + let _guarded1 = BailoutGuard::new(DropTracker::new(1)); + let _guarded2 = BailoutGuard::new(DropTracker::new(2)); + + // This unguarded tracker demonstrates the difference - without BailoutGuard, + // and without try_call catching the bailout, this would NOT be cleaned up. + // But since try_call catches it, all destructors run normally. + let _unguarded = DropTracker::new(3); + + // Call the PHP callback which will trigger exit() + let _ = callback.try_call(vec![]); +} + +/// Inner function for nested bailout test - creates guarded resources +fn nested_inner(callback: &ext_php_rs::types::ZendCallable) { + let _inner_guard1 = BailoutGuard::new(DropTracker::new(10)); + let _inner_guard2 = BailoutGuard::new(DropTracker::new(11)); + + // Call the PHP callback which will trigger exit() + let _ = callback.try_call(vec![]); +} + +/// Test nested calls with `BailoutGuard` - verifies cleanup happens at all nesting levels. +/// This creates guards at multiple call stack levels, then triggers bailout from the innermost. +#[php_function] +pub fn bailout_test_nested(callback: ext_php_rs::types::ZendCallable) { + // Outer level guards + let _outer_guard1 = BailoutGuard::new(DropTracker::new(1)); + let _outer_guard2 = BailoutGuard::new(DropTracker::new(2)); + + // Call inner function which creates more guards and triggers bailout + nested_inner(&callback); + + // This code won't be reached due to bailout, but the guards should still be cleaned up +} + +/// Test deeply nested calls (3 levels) with `BailoutGuard` +#[php_function] +pub fn bailout_test_deep_nested(callback: ext_php_rs::types::ZendCallable) { + // Level 1: outer guards + let _level1_guard = BailoutGuard::new(DropTracker::new(1)); + + // Level 2: call a closure that creates more guards + let level2 = || { + let _level2_guard = BailoutGuard::new(DropTracker::new(2)); + + // Level 3: another closure + let level3 = || { + let _level3_guard = BailoutGuard::new(DropTracker::new(3)); + + // Trigger bailout at the deepest level + let _ = callback.try_call(vec![]); + }; + + level3(); + }; + + level2(); +} + +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)) + .function(wrap_function!(bailout_test_with_guard)) + .function(wrap_function!(bailout_test_nested)) + .function(wrap_function!(bailout_test_deep_nested)) +} + +#[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" + )); + + // Test BailoutGuard cleanup mechanism + assert!(crate::integration::test::run_php( + "bailout/bailout_guard.php" + )); + + // Test nested calls with BailoutGuard (2 levels) + assert!(crate::integration::test::run_php( + "bailout/bailout_nested.php" + )); + + // Test deeply nested calls with BailoutGuard (3 levels via closures) + assert!(crate::integration::test::run_php( + "bailout/bailout_deep_nested.php" + )); + } +} diff --git a/tests/src/integration/mod.rs b/tests/src/integration/mod.rs index d58388e5a..94313f756 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; @@ -102,7 +103,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 @@ -121,8 +122,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"); @@ -138,8 +139,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 34347bedf..2d261c6d1 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);