Skip to content

Commit aee30ac

Browse files
committed
fix(zend_bailout): Fix zend_bailout handling #537
1 parent 36a1811 commit aee30ac

File tree

9 files changed

+238
-52
lines changed

9 files changed

+238
-52
lines changed

crates/macros/src/function.rs

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,19 @@ impl<'a> Function<'a> {
291291
ex: &mut ::ext_php_rs::zend::ExecuteData,
292292
retval: &mut ::ext_php_rs::types::Zval,
293293
) {
294-
#handler_body
294+
use ::ext_php_rs::zend::try_catch;
295+
use ::std::panic::AssertUnwindSafe;
296+
297+
// Wrap the handler body with try_catch to ensure Rust destructors
298+
// are called if a bailout occurs (issue #537)
299+
let catch_result = try_catch(AssertUnwindSafe(|| {
300+
#handler_body
301+
}));
302+
303+
// If there was a bailout, re-trigger it after Rust cleanup
304+
if catch_result.is_err() {
305+
unsafe { ::ext_php_rs::zend::bailout(); }
306+
}
295307
}
296308
}
297309
handler
@@ -535,18 +547,31 @@ impl<'a> Function<'a> {
535547
::ext_php_rs::class::ConstructorMeta {
536548
constructor: {
537549
fn inner(ex: &mut ::ext_php_rs::zend::ExecuteData) -> ::ext_php_rs::class::ConstructorResult<#class> {
538-
#(#arg_declarations)*
539-
let parse = ex.parser()
540-
#(.arg(&mut #required_arg_names))*
541-
.not_required()
542-
#(.arg(&mut #not_required_arg_names))*
543-
#variadic
544-
.parse();
545-
if parse.is_err() {
546-
return ::ext_php_rs::class::ConstructorResult::ArgError;
550+
use ::ext_php_rs::zend::try_catch;
551+
use ::std::panic::AssertUnwindSafe;
552+
553+
// Wrap the constructor body with try_catch to ensure Rust destructors
554+
// are called if a bailout occurs (issue #537)
555+
let catch_result = try_catch(AssertUnwindSafe(|| {
556+
#(#arg_declarations)*
557+
let parse = ex.parser()
558+
#(.arg(&mut #required_arg_names))*
559+
.not_required()
560+
#(.arg(&mut #not_required_arg_names))*
561+
#variadic
562+
.parse();
563+
if parse.is_err() {
564+
return ::ext_php_rs::class::ConstructorResult::ArgError;
565+
}
566+
#(#variadic_bindings)*
567+
#class::#ident(#({#arg_accessors}),*).into()
568+
}));
569+
570+
// If there was a bailout, re-trigger it after Rust cleanup
571+
match catch_result {
572+
Ok(result) => result,
573+
Err(_) => unsafe { ::ext_php_rs::zend::bailout() },
547574
}
548-
#(#variadic_bindings)*
549-
#class::#ident(#({#arg_accessors}),*).into()
550575
}
551576
inner
552577
},

src/builders/class.rs

Lines changed: 35 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -219,31 +219,43 @@ impl ClassBuilder {
219219

220220
zend_fastcall! {
221221
extern fn constructor<T: RegisteredClass>(ex: &mut ExecuteData, _: &mut Zval) {
222-
let Some(ConstructorMeta { constructor, .. }) = T::constructor() else {
223-
PhpException::default("You cannot instantiate this class from PHP.".into())
224-
.throw()
225-
.expect("Failed to throw exception when constructing class");
226-
return;
227-
};
228-
229-
let this = match constructor(ex) {
230-
ConstructorResult::Ok(this) => this,
231-
ConstructorResult::Exception(e) => {
232-
e.throw()
222+
use crate::zend::try_catch;
223+
use std::panic::AssertUnwindSafe;
224+
225+
// Wrap the constructor body with try_catch to ensure Rust destructors
226+
// are called if a bailout occurs (issue #537)
227+
let catch_result = try_catch(AssertUnwindSafe(|| {
228+
let Some(ConstructorMeta { constructor, .. }) = T::constructor() else {
229+
PhpException::default("You cannot instantiate this class from PHP.".into())
230+
.throw()
231+
.expect("Failed to throw exception when constructing class");
232+
return;
233+
};
234+
235+
let this = match constructor(ex) {
236+
ConstructorResult::Ok(this) => this,
237+
ConstructorResult::Exception(e) => {
238+
e.throw()
239+
.expect("Failed to throw exception while constructing class");
240+
return;
241+
}
242+
ConstructorResult::ArgError => return,
243+
};
244+
245+
let Some(this_obj) = ex.get_object::<T>() else {
246+
PhpException::default("Failed to retrieve reference to `this` object.".into())
247+
.throw()
233248
.expect("Failed to throw exception while constructing class");
234249
return;
235-
}
236-
ConstructorResult::ArgError => return,
237-
};
238-
239-
let Some(this_obj) = ex.get_object::<T>() else {
240-
PhpException::default("Failed to retrieve reference to `this` object.".into())
241-
.throw()
242-
.expect("Failed to throw exception while constructing class");
243-
return;
244-
};
245-
246-
this_obj.initialize(this);
250+
};
251+
252+
this_obj.initialize(this);
253+
}));
254+
255+
// If there was a bailout, re-trigger it after Rust cleanup
256+
if catch_result.is_err() {
257+
unsafe { crate::zend::bailout(); }
258+
}
247259
}
248260
}
249261

src/embed/mod.rs

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use crate::types::{ZendObject, Zval};
1717
use crate::zend::{ExecutorGlobals, panic_wrapper, try_catch};
1818
use parking_lot::{RwLock, const_rwlock};
1919
use std::ffi::{CString, NulError, c_char, c_void};
20-
use std::panic::{RefUnwindSafe, resume_unwind};
20+
use std::panic::{AssertUnwindSafe, UnwindSafe, resume_unwind};
2121
use std::path::Path;
2222
use std::ptr::null_mut;
2323

@@ -105,7 +105,9 @@ impl Embed {
105105
zend_stream_init_filename(&raw mut file_handle, path.as_ptr());
106106
}
107107

108-
let exec_result = try_catch(|| unsafe { php_execute_script(&raw mut file_handle) });
108+
let exec_result = try_catch(AssertUnwindSafe(|| unsafe {
109+
php_execute_script(&raw mut file_handle)
110+
}));
109111

110112
unsafe { zend_destroy_file_handle(&raw mut file_handle) }
111113

@@ -141,7 +143,7 @@ impl Embed {
141143
/// assert_eq!(foo.unwrap().string().unwrap(), "foo");
142144
/// });
143145
/// ```
144-
pub fn run<R, F: FnMut() -> R + RefUnwindSafe>(func: F) -> R
146+
pub fn run<R, F: FnOnce() -> R + UnwindSafe>(func: F) -> R
145147
where
146148
R: Default,
147149
{
@@ -161,6 +163,9 @@ impl Embed {
161163
)
162164
};
163165

166+
// Prevent the closure from being dropped here since it was consumed in panic_wrapper
167+
std::mem::forget(func);
168+
164169
// This can happen if there is a bailout
165170
if panic.is_null() {
166171
return R::default();
@@ -206,13 +211,13 @@ impl Embed {
206211

207212
let mut result = Zval::new();
208213

209-
let exec_result = try_catch(|| unsafe {
214+
let exec_result = try_catch(AssertUnwindSafe(|| unsafe {
210215
zend_eval_string(
211216
cstr.as_ptr().cast::<c_char>(),
212217
&raw mut result,
213218
c"run".as_ptr().cast(),
214219
)
215-
});
220+
}));
216221

217222
match exec_result {
218223
Err(_) => Err(EmbedError::CatchError),

src/zend/try_catch.rs

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,22 @@ use crate::ffi::{
22
ext_php_rs_zend_bailout, ext_php_rs_zend_first_try_catch, ext_php_rs_zend_try_catch,
33
};
44
use std::ffi::c_void;
5-
use std::panic::{RefUnwindSafe, catch_unwind, resume_unwind};
5+
use std::panic::{UnwindSafe, catch_unwind, resume_unwind};
66
use std::ptr::null_mut;
77

88
/// Error returned when a bailout occurs
99
#[derive(Debug)]
1010
pub struct CatchError;
1111

12-
pub(crate) unsafe extern "C" fn panic_wrapper<R, F: FnMut() -> R + RefUnwindSafe>(
12+
pub(crate) unsafe extern "C" fn panic_wrapper<R, F: FnOnce() -> R + UnwindSafe>(
1313
ctx: *const c_void,
1414
) -> *const c_void {
1515
// we try to catch panic here so we correctly shutdown php if it happens
1616
// mandatory when we do assert on test as other test would not run correctly
17-
let panic = catch_unwind(|| unsafe { (*(ctx as *mut F))() });
17+
// SAFETY: We read the closure from the pointer and consume it. This is safe because
18+
// the closure is only called once.
19+
let func = unsafe { std::ptr::read(ctx.cast::<F>()) };
20+
let panic = catch_unwind(func);
1821

1922
Box::into_raw(Box::new(panic)).cast::<c_void>()
2023
}
@@ -33,7 +36,7 @@ pub(crate) unsafe extern "C" fn panic_wrapper<R, F: FnMut() -> R + RefUnwindSafe
3336
/// # Errors
3437
///
3538
/// * [`CatchError`] - A bailout occurred during the execution
36-
pub fn try_catch<R, F: FnMut() -> R + RefUnwindSafe>(func: F) -> Result<R, CatchError> {
39+
pub fn try_catch<R, F: FnOnce() -> R + UnwindSafe>(func: F) -> Result<R, CatchError> {
3740
do_try_catch(func, false)
3841
}
3942

@@ -54,11 +57,11 @@ pub fn try_catch<R, F: FnMut() -> R + RefUnwindSafe>(func: F) -> Result<R, Catch
5457
/// # Errors
5558
///
5659
/// * [`CatchError`] - A bailout occurred during the execution
57-
pub fn try_catch_first<R, F: FnMut() -> R + RefUnwindSafe>(func: F) -> Result<R, CatchError> {
60+
pub fn try_catch_first<R, F: FnOnce() -> R + UnwindSafe>(func: F) -> Result<R, CatchError> {
5861
do_try_catch(func, true)
5962
}
6063

61-
fn do_try_catch<R, F: FnMut() -> R + RefUnwindSafe>(func: F, first: bool) -> Result<R, CatchError> {
64+
fn do_try_catch<R, F: FnOnce() -> R + UnwindSafe>(func: F, first: bool) -> Result<R, CatchError> {
6265
let mut panic_ptr = null_mut();
6366
let has_bailout = unsafe {
6467
if first {
@@ -76,6 +79,9 @@ fn do_try_catch<R, F: FnMut() -> R + RefUnwindSafe>(func: F, first: bool) -> Res
7679
}
7780
};
7881

82+
// Prevent the closure from being dropped here since it was consumed in panic_wrapper
83+
std::mem::forget(func);
84+
7985
let panic = panic_ptr.cast::<std::thread::Result<R>>();
8086

8187
// can be null if there is a bailout
@@ -190,17 +196,19 @@ mod tests {
190196

191197
#[test]
192198
fn test_memory_leak() {
199+
use std::panic::AssertUnwindSafe;
200+
193201
Embed::run(|| {
194202
let mut ptr = null_mut();
195203

196-
let _ = try_catch(|| {
204+
let _ = try_catch(AssertUnwindSafe(|| {
197205
let mut result = "foo".to_string();
198206
ptr = &raw mut result;
199207

200208
unsafe {
201209
bailout();
202210
}
203-
});
211+
}));
204212

205213
// Check that the string is never released
206214
let result = unsafe { &*ptr as &str };
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<?php
2+
// Control test - verify destructors work without bailout
3+
4+
bailout_test_reset();
5+
6+
// Call function that creates DropTrackers without bailout
7+
bailout_test_without_exit();
8+
9+
// The destructors should have been called
10+
$counter = bailout_test_get_counter();
11+
assert($counter === 2, "Expected 2 destructors to be called, got $counter");
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<?php
2+
// Test that Rust destructors are called when bailout occurs (issue #537)
3+
4+
bailout_test_reset();
5+
6+
// This function creates 3 DropTrackers and then calls a callback.
7+
// The callback triggers exit(), which causes a bailout.
8+
// Thanks to the try_catch wrapper, the Rust destructors should run
9+
// when the function returns, incrementing the counter to 3.
10+
bailout_test_with_callback(function() {
11+
exit(0);
12+
});
13+
14+
// After the function returns, check that all 3 destructors were called
15+
$counter = bailout_test_get_counter();
16+
assert($counter === 3, "Expected 3 destructors to be called after bailout, got $counter");
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
//! Test for issue #537 - Rust destructors should be called when PHP bailout occurs.
2+
//!
3+
//! This test verifies that when PHP triggers a bailout (e.g., via `exit()`), Rust
4+
//! destructors are properly called before the bailout is re-triggered.
5+
6+
use ext_php_rs::prelude::*;
7+
use std::sync::atomic::{AtomicU32, Ordering};
8+
9+
/// Static counter to track how many times the destructor was called.
10+
/// This is used to verify destructors run even when bailout occurs.
11+
static DROP_COUNTER: AtomicU32 = AtomicU32::new(0);
12+
13+
/// A struct that increments a counter when dropped.
14+
/// Used to verify destructors are called during bailout.
15+
struct DropTracker {
16+
_id: u32,
17+
}
18+
19+
impl DropTracker {
20+
fn new(id: u32) -> Self {
21+
Self { _id: id }
22+
}
23+
}
24+
25+
impl Drop for DropTracker {
26+
fn drop(&mut self) {
27+
// Increment the counter to prove the destructor was called
28+
DROP_COUNTER.fetch_add(1, Ordering::SeqCst);
29+
}
30+
}
31+
32+
/// Reset the drop counter (called from PHP before test)
33+
#[php_function]
34+
pub fn bailout_test_reset() {
35+
DROP_COUNTER.store(0, Ordering::SeqCst);
36+
}
37+
38+
/// Get the current drop counter value
39+
#[php_function]
40+
pub fn bailout_test_get_counter() -> u32 {
41+
DROP_COUNTER.load(Ordering::SeqCst)
42+
}
43+
44+
/// Create a `DropTracker` and then call a PHP callback that triggers `exit()`.
45+
/// If the fix for issue #537 works, the destructor should be called
46+
/// before the exit actually happens.
47+
#[php_function]
48+
pub fn bailout_test_with_callback(callback: ext_php_rs::types::ZendCallable) {
49+
let _tracker1 = DropTracker::new(1);
50+
let _tracker2 = DropTracker::new(2);
51+
let _tracker3 = DropTracker::new(3);
52+
53+
// Call the PHP callback which will trigger exit()
54+
// try_call catches bailouts internally, so we need to check if it failed
55+
// and re-trigger the bailout manually
56+
let result = callback.try_call(vec![]);
57+
58+
// If the callback triggered a bailout (exit/die/fatal error),
59+
// re-trigger it after our destructors have a chance to run.
60+
// The destructors will run when this function exits, before the
61+
// bailout is re-triggered by the handler wrapper.
62+
if result.is_err() {
63+
// Don't re-trigger here - let the handler wrapper do it
64+
// The handler wrapper's try_catch will see this as a normal return,
65+
// but our destructors will still run when this function's scope ends
66+
}
67+
}
68+
69+
/// Create a `DropTracker` without bailout (control test)
70+
#[php_function]
71+
pub fn bailout_test_without_exit() {
72+
let _tracker1 = DropTracker::new(1);
73+
let _tracker2 = DropTracker::new(2);
74+
75+
// No bailout - destructors should run normally when function returns
76+
}
77+
78+
pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder {
79+
builder
80+
.function(wrap_function!(bailout_test_reset))
81+
.function(wrap_function!(bailout_test_get_counter))
82+
.function(wrap_function!(bailout_test_with_callback))
83+
.function(wrap_function!(bailout_test_without_exit))
84+
}
85+
86+
#[cfg(test)]
87+
mod tests {
88+
#[test]
89+
fn bailout_destructor_called() {
90+
// First, run the control test (no bailout) to verify basic functionality
91+
assert!(crate::integration::test::run_php(
92+
"bailout/bailout_control.php"
93+
));
94+
95+
// Now run the bailout test - this verifies that destructors are called
96+
// even when a PHP callback triggers exit()
97+
assert!(crate::integration::test::run_php(
98+
"bailout/bailout_exit.php"
99+
));
100+
}
101+
}

0 commit comments

Comments
 (0)