Skip to content

Commit 334a909

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

File tree

17 files changed

+817
-52
lines changed

17 files changed

+817
-52
lines changed

crates/macros/src/function.rs

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,20 @@ 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, run BailoutGuard cleanups and re-trigger
304+
if catch_result.is_err() {
305+
::ext_php_rs::zend::run_bailout_cleanups();
306+
unsafe { ::ext_php_rs::zend::bailout(); }
307+
}
295308
}
296309
}
297310
handler
@@ -535,18 +548,34 @@ impl<'a> Function<'a> {
535548
::ext_php_rs::class::ConstructorMeta {
536549
constructor: {
537550
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;
551+
use ::ext_php_rs::zend::try_catch;
552+
use ::std::panic::AssertUnwindSafe;
553+
554+
// Wrap the constructor body with try_catch to ensure Rust destructors
555+
// are called if a bailout occurs (issue #537)
556+
let catch_result = try_catch(AssertUnwindSafe(|| {
557+
#(#arg_declarations)*
558+
let parse = ex.parser()
559+
#(.arg(&mut #required_arg_names))*
560+
.not_required()
561+
#(.arg(&mut #not_required_arg_names))*
562+
#variadic
563+
.parse();
564+
if parse.is_err() {
565+
return ::ext_php_rs::class::ConstructorResult::ArgError;
566+
}
567+
#(#variadic_bindings)*
568+
#class::#ident(#({#arg_accessors}),*).into()
569+
}));
570+
571+
// If there was a bailout, run BailoutGuard cleanups and re-trigger
572+
match catch_result {
573+
Ok(result) => result,
574+
Err(_) => {
575+
::ext_php_rs::zend::run_bailout_cleanups();
576+
unsafe { ::ext_php_rs::zend::bailout() }
577+
}
547578
}
548-
#(#variadic_bindings)*
549-
#class::#ident(#({#arg_accessors}),*).into()
550579
}
551580
inner
552581
},

guide/src/SUMMARY.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
# Advanced Topics
4343

4444
- [Async](./advanced/async_impl.md)
45+
- [Bailout Guard](./advanced/bailout_guard.md)
4546
- [Allowed Bindings](./advanced/allowed_bindings.md)
4647

4748
# Migration Guides
Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
# Bailout Guard
2+
3+
When PHP triggers a "bailout" (via `exit()`, `die()`, or a fatal error), it uses
4+
`longjmp` to unwind the stack. This bypasses Rust's normal drop semantics,
5+
meaning destructors for stack-allocated values won't run. This can lead to
6+
resource leaks for things like file handles, network connections, or locks.
7+
8+
## The Problem
9+
10+
Consider this code:
11+
12+
```rust,ignore
13+
#[php_function]
14+
pub fn process_file(callback: ZendCallable) {
15+
let file = File::open("data.txt").unwrap();
16+
17+
// If callback calls exit(), the file handle leaks!
18+
callback.try_call(vec![]);
19+
20+
// file.drop() never runs
21+
}
22+
```
23+
24+
If the PHP callback triggers `exit()`, the `File` handle is never closed because
25+
`longjmp` skips Rust's destructor calls.
26+
27+
## Solution 1: Using `try_call`
28+
29+
The simplest solution is to use `try_call` for PHP callbacks. It catches bailouts
30+
internally and returns normally, allowing Rust destructors to run:
31+
32+
```rust,ignore
33+
#[php_function]
34+
pub fn process_file(callback: ZendCallable) {
35+
let file = File::open("data.txt").unwrap();
36+
37+
// try_call catches bailout, function returns, file is dropped
38+
let result = callback.try_call(vec![]);
39+
40+
if result.is_err() {
41+
// Bailout occurred, but file will still be closed
42+
// when this function returns
43+
}
44+
}
45+
```
46+
47+
## Solution 2: Using `BailoutGuard`
48+
49+
For cases where you need guaranteed cleanup even if bailout occurs directly
50+
(not through `try_call`), use `BailoutGuard`:
51+
52+
```rust,ignore
53+
use ext_php_rs::prelude::*;
54+
use std::fs::File;
55+
56+
#[php_function]
57+
pub fn process_file(callback: ZendCallable) {
58+
// Wrap the file handle in BailoutGuard
59+
let file = BailoutGuard::new(File::open("data.txt").unwrap());
60+
61+
// Even if bailout occurs, the file will be closed
62+
callback.try_call(vec![]);
63+
64+
// Use the file via Deref
65+
// file.read_to_string(...);
66+
}
67+
```
68+
69+
### How `BailoutGuard` Works
70+
71+
1. **Heap allocation**: The wrapped value is heap-allocated so it survives
72+
the `longjmp` stack unwinding.
73+
74+
2. **Cleanup registration**: A cleanup callback is registered in thread-local
75+
storage when the guard is created.
76+
77+
3. **On normal drop**: The cleanup is cancelled and the value is dropped normally.
78+
79+
4. **On bailout**: Before re-triggering the bailout, all registered cleanup
80+
callbacks are executed, dropping the guarded values.
81+
82+
### API
83+
84+
```rust,ignore
85+
// Create a guard
86+
let guard = BailoutGuard::new(value);
87+
88+
// Access the value (implements Deref and DerefMut)
89+
guard.do_something();
90+
let inner: &T = &*guard;
91+
let inner_mut: &mut T = &mut *guard;
92+
93+
// Explicitly get references
94+
let inner: &T = guard.get();
95+
let inner_mut: &mut T = guard.get_mut();
96+
97+
// Extract the value, cancelling cleanup
98+
let value: T = guard.into_inner();
99+
```
100+
101+
### Performance Note
102+
103+
`BailoutGuard` incurs a heap allocation. Only use it for values that absolutely
104+
must be cleaned up, such as:
105+
106+
- File handles
107+
- Network connections
108+
- Database connections
109+
- Locks and mutexes
110+
- Other system resources
111+
112+
For simple values without cleanup requirements, the overhead isn't worth it.
113+
114+
## Nested Calls
115+
116+
`BailoutGuard` works correctly with nested function calls. Guards at all
117+
nesting levels are cleaned up when bailout occurs:
118+
119+
```rust,ignore
120+
#[php_function]
121+
pub fn outer_function(callback: ZendCallable) {
122+
let _outer_resource = BailoutGuard::new(Resource::new());
123+
124+
inner_function(&callback);
125+
}
126+
127+
fn inner_function(callback: &ZendCallable) {
128+
let _inner_resource = BailoutGuard::new(Resource::new());
129+
130+
// If bailout occurs here, both inner and outer resources are cleaned up
131+
callback.try_call(vec![]);
132+
}
133+
```
134+
135+
## Best Practices
136+
137+
1. **Prefer `try_call`**: For most cases, using `try_call` and handling the
138+
error result is simpler and doesn't require heap allocation.
139+
140+
2. **Use `BailoutGuard` for critical resources**: Only wrap values that
141+
absolutely must be cleaned up (connections, locks, etc.).
142+
143+
3. **Don't overuse**: Not every value needs to be wrapped. Simple data
144+
structures without cleanup requirements don't need `BailoutGuard`.
145+
146+
4. **Combine approaches**: Use `try_call` where possible and `BailoutGuard`
147+
for critical resources that must be cleaned up regardless.

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/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ pub mod prelude {
5656
pub use crate::php_println;
5757
pub use crate::php_write;
5858
pub use crate::types::ZendCallable;
59+
pub use crate::zend::BailoutGuard;
5960
pub use crate::{
6061
ZvalConvert, php_class, php_const, php_extern, php_function, php_impl, php_interface,
6162
php_module, wrap_constant, wrap_function, zend_fastcall,

0 commit comments

Comments
 (0)