fix(zend_bailout): Fix zend_bailout handling #537

Author: kakserpom

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
                 },

src/builders/class.rs

@@ -202,31 +202,43 @@ impl ClassBuilder {
 
         zend_fastcall! {
             extern fn constructor<T: RegisteredClass>(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::<T>() 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::<T>() 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(); }
+                }
             }
         }
 

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, F: FnMut() -> R + RefUnwindSafe>(func: F) -> R
+    pub fn run<R, F: FnOnce() -> 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::<c_char>(),
                 &raw mut result,
                 c"run".as_ptr().cast(),
             )
-        });
+        }));
 
         match exec_result {
             Err(_) => Err(EmbedError::CatchError),

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, F: FnMut() -> R + RefUnwindSafe>(
+pub(crate) unsafe extern "C" fn panic_wrapper<R, F: FnOnce() -> 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::<F>()) };
+    let panic = catch_unwind(func);
 
     Box::into_raw(Box::new(panic)).cast::<c_void>()
 }
@@ -33,7 +36,7 @@ pub(crate) unsafe extern "C" fn panic_wrapper<R, F: FnMut() -> R + RefUnwindSafe
 /// # Errors
 ///
 /// * [`CatchError`] - A bailout occurred during the execution
-pub fn try_catch<R, F: FnMut() -> R + RefUnwindSafe>(func: F) -> Result<R, CatchError> {
+pub fn try_catch<R, F: FnOnce() -> R + UnwindSafe>(func: F) -> Result<R, CatchError> {
     do_try_catch(func, false)
 }
 
@@ -54,11 +57,11 @@ pub fn try_catch<R, F: FnMut() -> R + RefUnwindSafe>(func: F) -> Result<R, Catch
 /// # Errors
 ///
 /// * [`CatchError`] - A bailout occurred during the execution
-pub fn try_catch_first<R, F: FnMut() -> R + RefUnwindSafe>(func: F) -> Result<R, CatchError> {
+pub fn try_catch_first<R, F: FnOnce() -> R + UnwindSafe>(func: F) -> Result<R, CatchError> {
     do_try_catch(func, true)
 }
 
-fn do_try_catch<R, F: FnMut() -> R + RefUnwindSafe>(func: F, first: bool) -> Result<R, CatchError> {
+fn do_try_catch<R, F: FnOnce() -> R + UnwindSafe>(func: F, first: bool) -> Result<R, CatchError> {
     let mut panic_ptr = null_mut();
     let has_bailout = unsafe {
         if first {
@@ -76,6 +79,9 @@ fn do_try_catch<R, F: FnMut() -> 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::<std::thread::Result<R>>();
 
     // 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 };

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));

tests/src/integration/bailout/bailout_control.php

@@ -0,0 +1,11 @@
+<?php
+// Control test - verify destructors work without bailout
+
+bailout_test_reset();
+
+// Call function that creates DropTrackers without bailout
+bailout_test_without_exit();
+
+// The destructors should have been called
+$counter = bailout_test_get_counter();
+assert($counter === 2, "Expected 2 destructors to be called, got $counter");

tests/src/integration/bailout/bailout_exit.php

@@ -0,0 +1,16 @@
+<?php
+// Test that Rust destructors are called when bailout occurs (issue #537)
+
+bailout_test_reset();
+
+// This function creates 3 DropTrackers and then calls a callback.
+// The callback triggers exit(), which causes a bailout.
+// Thanks to the try_catch wrapper, the Rust destructors should run
+// when the function returns, incrementing the counter to 3.
+bailout_test_with_callback(function() {
+    exit(0);
+});
+
+// After the function returns, check that all 3 destructors were called
+$counter = bailout_test_get_counter();
+assert($counter === 3, "Expected 3 destructors to be called after bailout, got $counter");