From 20316fb7512ba09236613d7e919af158feb69ea9 Mon Sep 17 00:00:00 2001 From: Pierre Tondereau Date: Sun, 20 Sep 2026 12:09:14 +0200 Subject: [PATCH 1/5] fix(zend)!: report panics from try_catch instead of resuming them --- allowed_bindings.rs | 1 + docsrs_bindings.rs | 3 + src/php_eval.rs | 6 +- src/zend/ce.rs | 11 +++- src/zend/mod.rs | 3 +- src/zend/try_catch.rs | 124 ++++++++++++++++++++++++++++++------------ 6 files changed, 109 insertions(+), 39 deletions(-) diff --git a/allowed_bindings.rs b/allowed_bindings.rs index 71b61bede2..44d4de9e94 100644 --- a/allowed_bindings.rs +++ b/allowed_bindings.rs @@ -69,6 +69,7 @@ bind! { zend_ce_arithmetic_error, zend_ce_compile_error, zend_ce_division_by_zero_error, + zend_ce_error, zend_ce_error_exception, zend_ce_exception, zend_ce_parse_error, diff --git a/docsrs_bindings.rs b/docsrs_bindings.rs index 3cd33f50ec..5f10f56e80 100644 --- a/docsrs_bindings.rs +++ b/docsrs_bindings.rs @@ -3597,6 +3597,9 @@ unsafe extern "C" { unsafe extern "C" { pub static mut zend_ce_error_exception: *mut zend_class_entry; } +unsafe extern "C" { + pub static mut zend_ce_error: *mut zend_class_entry; +} unsafe extern "C" { pub static mut zend_ce_compile_error: *mut zend_class_entry; } diff --git a/src/php_eval.rs b/src/php_eval.rs index a3a6adfbee..ae4c67d465 100644 --- a/src/php_eval.rs +++ b/src/php_eval.rs @@ -22,7 +22,7 @@ use crate::ffi; use crate::types::ZendStr; -use crate::zend::try_catch; +use crate::zend::{CatchError, try_catch}; use std::mem; use std::panic::AssertUnwindSafe; @@ -42,6 +42,9 @@ pub enum PhpEvalError { /// A PHP fatal error (bailout) occurred during execution. #[error("PHP fatal error (bailout) during execution")] Bailout, + /// Rust code reached from the PHP code panicked. + #[error("Rust panic during execution: {0}")] + Panic(String), } /// Execute embedded PHP code within the running PHP engine. @@ -103,6 +106,7 @@ pub fn execute(code: impl AsRef<[u8]>) -> Result<(), PhpEvalError> { unsafe { (*eg).error_reporting = prev_error_reporting }; match result { + Err(CatchError::Panic(message)) => Err(PhpEvalError::Panic(message)), Err(_) => Err(PhpEvalError::Bailout), Ok(inner) => inner, } diff --git a/src/zend/ce.rs b/src/zend/ce.rs index 34ea133631..b952d9fad1 100644 --- a/src/zend/ce.rs +++ b/src/zend/ce.rs @@ -4,7 +4,7 @@ use crate::ffi::{ zend_ce_aggregate, zend_ce_argument_count_error, zend_ce_arithmetic_error, zend_ce_arrayaccess, - zend_ce_compile_error, zend_ce_countable, zend_ce_division_by_zero_error, + zend_ce_compile_error, zend_ce_countable, zend_ce_division_by_zero_error, zend_ce_error, zend_ce_error_exception, zend_ce_exception, zend_ce_iterator, zend_ce_parse_error, zend_ce_serializable, zend_ce_stringable, zend_ce_throwable, zend_ce_traversable, zend_ce_type_error, zend_ce_unhandled_match_error, zend_ce_value_error, @@ -40,6 +40,15 @@ pub fn exception() -> &'static ClassEntry { unsafe { zend_ce_exception.as_ref() }.unwrap() } +/// Returns the base [`Error`](https://www.php.net/manual/en/class.error.php) class. +/// +/// # Panics +/// +/// If error [`ClassEntry`] is not available +pub fn error() -> &'static ClassEntry { + unsafe { zend_ce_error.as_ref() }.unwrap() +} + /// Returns the base [`ErrorException`](https://www.php.net/manual/en/class.errorexception.php) class. /// /// # Panics diff --git a/src/zend/mod.rs b/src/zend/mod.rs index 51a2e778bb..47041225c8 100644 --- a/src/zend/mod.rs +++ b/src/zend/mod.rs @@ -55,9 +55,10 @@ pub use module_globals::{ModuleGlobal, ModuleGlobals}; #[cfg(feature = "observer")] pub use observer::{FcallInfo, FcallObserver}; pub use streams::*; +pub(crate) use try_catch::catch_panic; #[cfg(feature = "embed")] pub(crate) use try_catch::panic_wrapper; -pub use try_catch::{CatchError, bailout, try_catch, try_catch_first}; +pub use try_catch::{CatchError, bailout, run_handler, try_catch, try_catch_first}; #[cfg(feature = "observer")] pub use zend_extension::{ZendExtensionBuilder, ZendExtensionHandler}; diff --git a/src/zend/try_catch.rs b/src/zend/try_catch.rs index e271616af9..1412f05300 100644 --- a/src/zend/try_catch.rs +++ b/src/zend/try_catch.rs @@ -1,9 +1,12 @@ use super::bailout_guard::{cleanup_depth, run_cleanups_above}; +use crate::exception::{PhpException, PhpResult}; use crate::ffi::{ ext_php_rs_zend_bailout, ext_php_rs_zend_first_try_catch, ext_php_rs_zend_try_catch, }; +use crate::zend::ce; +use std::any::Any; use std::ffi::c_void; -use std::panic::{UnwindSafe, catch_unwind, resume_unwind}; +use std::panic::{AssertUnwindSafe, UnwindSafe, catch_unwind}; use std::ptr::null_mut; /// Error returned when the engine did not run the closure to completion. @@ -16,6 +19,57 @@ pub enum CatchError { /// The closure result was never written back. #[error("the closure result pointer was null")] NullPanicPtr, + /// The closure panicked. Holds the panic message; the frames inside the + /// closure have already been unwound and their destructors run. + #[error("the closure panicked: {0}")] + Panic(String), +} + +fn panic_message(payload: &(dyn Any + Send)) -> String { + if let Some(s) = payload.downcast_ref::<&str>() { + (*s).to_owned() + } else if let Some(s) = payload.downcast_ref::() { + s.clone() + } else { + "non-string panic payload".to_owned() + } +} + +fn panic_exception(message: &str) -> PhpException { + PhpException::new(format!("Rust panic: {message}"), 0, ce::error()) +} + +/// Runs the body of an `extern "C"` handler and keeps every Rust failure on the +/// Rust side of the boundary. +/// +/// A bailout inside `func` drops the [`BailoutGuard`](crate::zend::BailoutGuard)s +/// of this frame and is re-raised to the enclosing `zend_try`. A panic inside +/// `func` has already unwound and run the destructors of every frame inside the +/// handler; it is reported to PHP as an `Error` exception whose message starts +/// with `Rust panic: `, and the handler returns normally. An exception that was +/// already pending is kept, the panic message then only reaches the panic hook. +/// +/// The handlers generated by the macros use this. Hand-written handlers passed +/// to [`FunctionBuilder::new`](crate::builders::FunctionBuilder::new) must call +/// it themselves, since a panic escaping an `extern "C"` function aborts the +/// process. +pub fn run_handler(func: F) { + match try_catch(func) { + Ok(()) => {} + Err(CatchError::Bailout) => unsafe { bailout() }, + Err(CatchError::Panic(message)) => panic_exception(&message).throw(), + Err(err @ CatchError::NullPanicPtr) => panic_exception(&err.to_string()).throw(), + } +} + +/// Runs `func` under `catch_unwind` only, with no `zend_try`, and turns a panic +/// into the same `Error` exception [`run_handler`] throws. Used by the object +/// handlers, where a `setjmp` per property access is not acceptable. +pub(crate) fn catch_panic(func: impl FnOnce() -> PhpResult) -> PhpResult { + match catch_unwind(AssertUnwindSafe(func)) { + Ok(result) => result, + Err(payload) => Err(panic_exception(&panic_message(payload.as_ref()))), + } } pub(crate) unsafe extern "C" fn panic_wrapper R + UnwindSafe>( @@ -42,13 +96,18 @@ pub(crate) unsafe extern "C" fn panic_wrapper R + UnwindSafe>( /// that was created inside `func`, newest first, before it returns. The guards that /// were created before the call are not changed. /// +/// A panic inside `func` is caught and reported as [`CatchError::Panic`]; it is +/// not resumed. Callers that want the panic to propagate call +/// [`std::panic::resume_unwind`] with their own payload. +/// /// # Returns /// /// * The result of the function /// /// # Errors /// -/// * [`CatchError`] - A bailout occurred during the execution +/// * [`CatchError::Bailout`] - A bailout occurred during the execution +/// * [`CatchError::Panic`] - The closure panicked pub fn try_catch R + UnwindSafe>(func: F) -> Result { do_try_catch(func, false) } @@ -69,7 +128,8 @@ pub fn try_catch R + UnwindSafe>(func: F) -> Result R + UnwindSafe>(func: F) -> Result { do_try_catch(func, true) } @@ -109,10 +169,7 @@ fn do_try_catch R + UnwindSafe>(func: F, first: bool) -> Resul match unsafe { *Box::from_raw(panic.cast::>()) } { Ok(r) => Ok(r), - Err(err) => { - // we resume the panic here so it can be caught correctly by the test framework - resume_unwind(err); - } + Err(payload) => Err(CatchError::Panic(panic_message(payload.as_ref()))), } } @@ -136,6 +193,7 @@ pub unsafe fn bailout() -> ! { #[cfg(feature = "embed")] #[cfg(test)] mod tests { + use super::CatchError; use crate::embed::Embed; use crate::zend::{BailoutGuard, bailout, try_catch}; use std::sync::Arc; @@ -187,35 +245,19 @@ mod tests { #[test] fn test_catch() { - Embed::run(|| { - let catch = try_catch(|| { - unsafe { - bailout(); - } - - #[allow(unreachable_code)] - #[allow(clippy::assertions_on_constants)] - { - assert!(false); - } - }); - - assert!(catch.is_err()); + let caught = Embed::run(|| { + let catch = try_catch(|| unsafe { bailout() }); + matches!(catch, Err(CatchError::Bailout)) }); + + assert!(caught); } #[test] fn test_no_catch() { - Embed::run(|| { - let catch = try_catch(|| { - #[allow(clippy::assertions_on_constants)] - { - assert!(true); - } - }); + let ok = Embed::run(|| try_catch(|| 1).is_ok()); - assert!(catch.is_ok()); - }); + assert!(ok); } #[test] @@ -234,13 +276,23 @@ mod tests { } #[test] - #[should_panic(expected = "should panic")] - fn test_panic() { - Embed::run(|| { - let _ = try_catch(|| { - panic!("should panic"); - }); + fn test_panic_is_reported_not_resumed() { + let message = Embed::run(|| match try_catch(|| panic!("should panic")) { + Err(CatchError::Panic(message)) => message, + other => format!("unexpected: {other:?}"), + }); + + assert_eq!(message, "should panic"); + } + + #[test] + fn test_panic_payload_fallback() { + let message = Embed::run(|| match try_catch(|| std::panic::panic_any(42)) { + Err(CatchError::Panic(message)) => message, + other => format!("unexpected: {other:?}"), }); + + assert_eq!(message, "non-string panic payload"); } #[test] From a108c95c023c6918b166ab4803a96bdc8bfd46c9 Mon Sep 17 00:00:00 2001 From: Pierre Tondereau Date: Sun, 20 Sep 2026 12:09:14 +0200 Subject: [PATCH 2/5] fix(exception)!: make PhpException::throw infallible --- src/convert.rs | 5 +- src/exception.rs | 46 +++++++------------ tests/src/integration/exception/exception.php | 17 +++++++ tests/src/integration/exception/mod.rs | 18 ++++++-- 4 files changed, 51 insertions(+), 35 deletions(-) diff --git a/src/convert.rs b/src/convert.rs index 3126ae3141..c38b6a570f 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -2,7 +2,7 @@ use crate::{ boxed::ZBox, - error::Result, + error::{Error, Result}, exception::PhpException, flags::DataType, types::{ZendObject, Zval}, @@ -244,7 +244,8 @@ where Ok(val) => val.set_zval(zv, persistent), Err(e) => { let ex: PhpException = e.into(); - ex.throw() + ex.throw(); + Err(Error::pending_exception().unwrap_or(Error::Callable)) } } } diff --git a/src/exception.rs b/src/exception.rs index 481d1b04d5..295e3b7ac2 100644 --- a/src/exception.rs +++ b/src/exception.rs @@ -96,25 +96,32 @@ impl PhpException { self } - /// Throws the exception, returning nothing inside a result if successful - /// and an error otherwise. + /// Throws the exception. Always leaves an exception pending in the engine. /// /// Does nothing if an exception is already pending: the engine propagates /// that one, and throwing over it would discard its class and stack trace. /// - /// # Errors - /// - /// * [`Error::InvalidException`] - If the exception type is an interface or - /// abstract class. - /// * If the message contains NUL bytes. - pub fn throw(self) -> Result<()> { + /// When the exception itself cannot be thrown (an interface or abstract + /// class, a message containing a NUL byte, or an attached value that is not + /// an object) a plain `Error` is thrown instead. Its message names the + /// cause and carries the original message, so the failure is reported to + /// PHP rather than to the Rust caller, which sits inside an `extern "C"` + /// frame with no way to propagate it. + pub fn throw(self) { if ExecutorGlobals::has_exception() { - return Ok(()); + return; } - match self.object { + let class = self.ex.name().unwrap_or_default(); + let message = self.message.replace('\0', "\\0"); + let result = match self.object { Some(object) => throw_object(object), None => throw_with_code(self.ex, self.code, &self.message), + }; + + if let Err(err) = result { + let fallback = format!("cannot throw {class}: {err}; original message: {message}"); + let _ = throw_with_code(ce::error(), 0, &fallback); } } } @@ -315,25 +322,6 @@ mod tests { }); } - #[test] - fn test_throw_code() { - Embed::run(|| { - let ex = PhpException::from_message("Test".into()); - assert!(ex.throw().is_ok()); - - assert!(false, "Should not reach here"); - }); - } - - #[test] - fn test_throw_object_rejects_a_non_object() { - Embed::run(|| { - let ex = PhpException::from_message("Test".into()).with_object(Zval::new()); - - assert!(matches!(ex.throw(), Err(Error::Object))); - }); - } - #[test] fn test_from_string() { Embed::run(|| { diff --git a/tests/src/integration/exception/exception.php b/tests/src/integration/exception/exception.php index 0c35eba115..d6b295b987 100644 --- a/tests/src/integration/exception/exception.php +++ b/tests/src/integration/exception/exception.php @@ -43,3 +43,20 @@ } catch (\Throwable $e) { assert('an object was expected' === $e->getMessage(), $e->getMessage()); } + +try { + throw_interface_class(); + assert(false, 'an Error should have been thrown instead of the interface'); +} catch (\Throwable $e) { + assert(get_class($e) === \Error::class, get_class($e)); + assert(str_contains($e->getMessage(), 'cannot throw Throwable'), $e->getMessage()); + assert(str_contains($e->getMessage(), 'original message: cannot instantiate'), $e->getMessage()); +} + +try { + throw_nul_message(); + assert(false, 'an Error should have been thrown instead of the NUL message'); +} catch (\Throwable $e) { + assert(get_class($e) === \Error::class, get_class($e)); + assert(str_contains($e->getMessage(), 'original message: before\\0after'), $e->getMessage()); +} diff --git a/tests/src/integration/exception/mod.rs b/tests/src/integration/exception/mod.rs index 1fa62219d6..fcc99c7ed1 100644 --- a/tests/src/integration/exception/mod.rs +++ b/tests/src/integration/exception/mod.rs @@ -30,11 +30,9 @@ pub fn call_throwing_callable(call: ZendCallable) -> PhpResult<()> { } #[php_function] -pub fn throw_over_pending_exception(call: ZendCallable) -> PhpResult<()> { +pub fn throw_over_pending_exception(call: ZendCallable) { let _ = call.try_call(vec![]); - PhpException::from_message("second".into()).throw()?; - - Ok(()) + PhpException::from_message("second".into()).throw(); } #[php_function] @@ -44,6 +42,16 @@ pub fn throw_non_object() -> PhpResult<()> { Ok(()) } +#[php_function] +pub fn throw_interface_class() { + PhpException::new("cannot instantiate".into(), 0, ce::throwable()).throw(); +} + +#[php_function] +pub fn throw_nul_message() { + PhpException::from_message("before\0after".into()).throw(); +} + pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { builder .class::() @@ -52,6 +60,8 @@ pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { .function(wrap_function!(call_throwing_callable)) .function(wrap_function!(throw_over_pending_exception)) .function(wrap_function!(throw_non_object)) + .function(wrap_function!(throw_interface_class)) + .function(wrap_function!(throw_nul_message)) } #[cfg(test)] From 256feb0df47add83bf0ed948e0552b8c0af33fb7 Mon Sep 17 00:00:00 2001 From: Pierre Tondereau Date: Sun, 20 Sep 2026 12:09:14 +0200 Subject: [PATCH 3/5] fix(macros)!: turn handler panics into PHP Error exceptions --- crates/macros/src/function.rs | 68 ++--- crates/macros/src/impl_interface.rs | 21 +- src/builders/class.rs | 18 +- src/closure.rs | 33 ++- src/zend/handlers.rs | 113 ++++--- tests/src/integration/mod.rs | 27 +- tests/src/integration/panic/_panic_utils.php | 16 + tests/src/integration/panic/mod.rs | 275 ++++++++++++++++++ .../integration/panic/panic_after_throw.php | 10 + tests/src/integration/panic/panic_clone.php | 8 + tests/src/integration/panic/panic_closure.php | 7 + .../integration/panic/panic_constructor.php | 10 + tests/src/integration/panic/panic_drop.php | 16 + tests/src/integration/panic/panic_extern.php | 6 + .../src/integration/panic/panic_function.php | 7 + tests/src/integration/panic/panic_guard.php | 7 + tests/src/integration/panic/panic_method.php | 10 + tests/src/integration/panic/panic_nested.php | 7 + tests/src/integration/panic/panic_payload.php | 6 + .../src/integration/panic/panic_property.php | 13 + tests/src/lib.rs | 1 + 21 files changed, 541 insertions(+), 138 deletions(-) create mode 100644 tests/src/integration/panic/_panic_utils.php create mode 100644 tests/src/integration/panic/mod.rs create mode 100644 tests/src/integration/panic/panic_after_throw.php create mode 100644 tests/src/integration/panic/panic_clone.php create mode 100644 tests/src/integration/panic/panic_closure.php create mode 100644 tests/src/integration/panic/panic_constructor.php create mode 100644 tests/src/integration/panic/panic_drop.php create mode 100644 tests/src/integration/panic/panic_extern.php create mode 100644 tests/src/integration/panic/panic_function.php create mode 100644 tests/src/integration/panic/panic_guard.php create mode 100644 tests/src/integration/panic/panic_method.php create mode 100644 tests/src/integration/panic/panic_nested.php create mode 100644 tests/src/integration/panic/panic_payload.php create mode 100644 tests/src/integration/panic/panic_property.php diff --git a/crates/macros/src/function.rs b/crates/macros/src/function.rs index ba11f55c83..aeaf2c252f 100644 --- a/crates/macros/src/function.rs +++ b/crates/macros/src/function.rs @@ -267,7 +267,7 @@ impl<'a> Function<'a> { // The method returns &Self or &mut Self, use `this` directly if let Err(e) = this.set_zval(retval, false) { let e: ::ext_php_rs::exception::PhpException = e.into(); - e.throw().expect("Failed to throw PHP exception."); + e.throw(); } } } else { @@ -281,7 +281,7 @@ impl<'a> Function<'a> { 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."); + e.throw(); } } }; @@ -294,19 +294,9 @@ impl<'a> Function<'a> { ex: &mut ::ext_php_rs::zend::ExecuteData, retval: &mut ::ext_php_rs::types::Zval, ) { - 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(|| { + ::ext_php_rs::zend::run_handler(::std::panic::AssertUnwindSafe(|| { #handler_body })); - - // try_catch already dropped the BailoutGuards of this frame; re-trigger the bailout - if catch_result.is_err() { - unsafe { ::ext_php_rs::zend::bailout(); } - } } } handler @@ -400,7 +390,7 @@ impl<'a> Function<'a> { let arg_accessors = self.args.typed.iter().map(|arg| { arg.accessor(|e| { quote! { - #e.throw().expect("Failed to throw PHP exception."); + #e.throw(); return; } }) @@ -434,8 +424,7 @@ impl<'a> Function<'a> { Some(this) => this, None => { ::ext_php_rs::exception::PhpException::from_message("Failed to retrieve reference to `$this`".into()) - .throw() - .unwrap(); + .throw(); return; } }; @@ -561,7 +550,7 @@ impl<'a> Function<'a> { None => { ::ext_php_rs::exception::PhpException::from_message( concat!("Invalid value given for argument `", stringify!(#name), "`.").into() - ).throw().expect("Failed to throw PHP exception."); + ).throw(); return; } } @@ -571,7 +560,7 @@ impl<'a> Function<'a> { let throw_invalid = quote! { ::ext_php_rs::exception::PhpException::from_message( concat!("Invalid value given for argument `", stringify!(#name), "`.").into() - ).throw().expect("Failed to throw PHP exception."); + ).throw(); return; }; @@ -580,7 +569,7 @@ impl<'a> Function<'a> { concat!("Argument `$", stringify!(#name), "` must not be null").into(), 0, ::ext_php_rs::zend::ce::type_error(), - ).throw().expect("Failed to throw PHP exception."); + ).throw(); return; }; @@ -671,7 +660,7 @@ impl<'a> Function<'a> { let this_error = quote! { ::ext_php_rs::exception::PhpException::from_message( "Failed to retrieve reference to `$this`".into() - ).throw().unwrap(); + ).throw(); return; }; @@ -731,7 +720,7 @@ impl<'a> Function<'a> { if let Err(e) = __this.set_zval(retval, false) { let e: ::ext_php_rs::exception::PhpException = e.into(); - e.throw().expect("Failed to throw PHP exception."); + e.throw(); } } } else { @@ -745,7 +734,7 @@ impl<'a> Function<'a> { 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."); + e.throw(); } } } @@ -823,32 +812,17 @@ 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> { - 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))* - .parse(); - if parse.is_err() { - return ::ext_php_rs::class::ConstructorResult::ArgError; - } - #(#variadic_bindings)* - #class::#ident(#({#arg_accessors}),*).into() - })); - - // try_catch already dropped the BailoutGuards of this frame; re-trigger the bailout - match catch_result { - Ok(result) => result, - Err(_) => { - unsafe { ::ext_php_rs::zend::bailout() } - } + #(#arg_declarations)* + let parse = ex.parser() + #(.arg(&mut #required_arg_names))* + .not_required() + #(.arg(&mut #not_required_arg_names))* + .parse(); + if parse.is_err() { + return ::ext_php_rs::class::ConstructorResult::ArgError; } + #(#variadic_bindings)* + #class::#ident(#({#arg_accessors}),*).into() } inner }, diff --git a/crates/macros/src/impl_interface.rs b/crates/macros/src/impl_interface.rs index 68f4e8ebca..3301407cdb 100644 --- a/crates/macros/src/impl_interface.rs +++ b/crates/macros/src/impl_interface.rs @@ -232,8 +232,7 @@ fn generate_method_builder( None => { let msg = format!("Invalid value for argument `{}`", #php_name); ::ext_php_rs::exception::PhpException::from_message(msg.into()) - .throw() - .expect("Failed to throw PHP exception."); + .throw(); return; } }; @@ -307,7 +306,7 @@ fn generate_method_builder( quote! { 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."); + e.throw(); } } }; @@ -354,8 +353,7 @@ fn generate_method_builder( Some(this) => this, None => { ::ext_php_rs::exception::PhpException::from_message("Failed to get $this".into()) - .throw() - .expect("Failed to throw PHP exception."); + .throw(); return; } }; @@ -380,8 +378,7 @@ fn generate_method_builder( Some(this) => this, None => { ::ext_php_rs::exception::PhpException::from_message("Failed to get $this".into()) - .throw() - .expect("Failed to throw PHP exception."); + .throw(); return; } }; @@ -411,18 +408,10 @@ fn generate_method_builder( retval: &mut ::ext_php_rs::types::Zval, ) { use ::ext_php_rs::convert::IntoZval; - use ::ext_php_rs::zend::try_catch; - use ::std::panic::AssertUnwindSafe; - let catch_result = try_catch(AssertUnwindSafe(|| { + ::ext_php_rs::zend::run_handler(::std::panic::AssertUnwindSafe(|| { #handler_body })); - - if catch_result.is_err() { - unsafe { - ::ext_php_rs::zend::bailout(); - } - } } } handler diff --git a/src/builders/class.rs b/src/builders/class.rs index 4439f7ac16..bf49408a86 100644 --- a/src/builders/class.rs +++ b/src/builders/class.rs @@ -250,14 +250,9 @@ impl ClassBuilder { zend_fastcall! { extern fn constructor(ex: &mut ExecuteData, _: &mut Zval) { - 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(|| { + crate::zend::run_handler(std::panic::AssertUnwindSafe(|| { let Some(ConstructorMeta { constructor, .. }) = T::constructor() else { - let _ = PhpException::from_message("You cannot instantiate this class from PHP.".into()) + PhpException::from_message("You cannot instantiate this class from PHP.".into()) .throw(); return; }; @@ -265,7 +260,7 @@ impl ClassBuilder { let this = match constructor(ex) { ConstructorResult::Ok(this) => this, ConstructorResult::Exception(e) => { - let _ = e.throw(); + e.throw(); return; } ConstructorResult::ArgError => return, @@ -274,18 +269,13 @@ impl ClassBuilder { // Use get_object_uninit because the Rust backing is not yet initialized. // We need access to the ZendClassObject to call initialize() on it. let Some(this_obj) = ex.get_object_uninit::() else { - let _ = PhpException::from_message("Failed to retrieve reference to `this` object.".into()) + PhpException::from_message("Failed to retrieve reference to `this` object.".into()) .throw(); return; }; 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/closure.rs b/src/closure.rs index 63a42f37ca..6e4e3f1108 100644 --- a/src/closure.rs +++ b/src/closure.rs @@ -147,12 +147,19 @@ impl Closure { zend_fastcall! { /// External function used by the Zend interpreter to call the closure. - #[expect(clippy::expect_used, reason = "the engine only dispatches this handler on RustClosure instances")] extern "C" fn invoke(ex: &mut ExecuteData, ret: &mut Zval) { - let (parser, this) = ex.parser_method::(); - let this = this.expect("Internal closure function called on non-closure class"); + crate::zend::run_handler(std::panic::AssertUnwindSafe(|| { + let (parser, this) = ex.parser_method::(); + let Some(this) = this else { + PhpException::from_message( + "Rust closure invoked on an object that is not a RustClosure".into(), + ) + .throw(); + return; + }; - this.0.invoke(parser, ret); + this.0.invoke(parser, ret); + })); } } } @@ -218,9 +225,8 @@ where { fn invoke(&mut self, _: ArgParser, ret: &mut Zval) { if let Err(e) = self().set_zval(ret, false) { - let _ = - PhpException::from_message(format!("Failed to return closure result to PHP: {e}")) - .throw(); + PhpException::from_message(format!("Failed to return closure result to PHP: {e}")) + .throw(); } } } @@ -231,9 +237,8 @@ where { fn invoke(&mut self, _: ArgParser, ret: &mut Zval) { if let Err(e) = self().set_zval(ret, false) { - let _ = - PhpException::from_message(format!("Failed to return closure result to PHP: {e}")) - .throw(); + PhpException::from_message(format!("Failed to return closure result to PHP: {e}")) + .throw(); } } } @@ -247,7 +252,7 @@ where Closure::wrap(Box::new(move || { let Some(this) = this.take() else { - let _ = PhpException::from_message( + PhpException::from_message( "Attempted to call `FnOnce` closure more than once.".into(), ) .throw(); @@ -274,7 +279,7 @@ macro_rules! php_closure_impl { Closure::wrap(Box::new(move |$($gen),*| { let Some(this) = this.take() else { - let _ = PhpException::from_message( + PhpException::from_message( "Attempted to call `FnOnce` closure more than once.".into(), ) .throw(); @@ -311,7 +316,7 @@ macro_rules! php_closure_impl { match $gen.consume() { Ok(val) => val, _ => { - let _ = PhpException::from_message(concat!("Invalid parameter type for `", stringify!($gen), "`.").into()).throw(); + PhpException::from_message(concat!("Invalid parameter type for `", stringify!($gen), "`.").into()).throw(); return; } } @@ -319,7 +324,7 @@ macro_rules! php_closure_impl { ); if let Err(e) = result.set_zval(ret, false) { - let _ = PhpException::from_message(format!("Failed to return closure result to PHP: {}", e)).throw(); + PhpException::from_message(format!("Failed to return closure result to PHP: {}", e)).throw(); } } } diff --git a/src/zend/handlers.rs b/src/zend/handlers.rs index 463857eb9d..3dba2ba09b 100644 --- a/src/zend/handlers.rs +++ b/src/zend/handlers.rs @@ -1,7 +1,11 @@ -use std::{ffi::CString, ffi::c_void, mem::MaybeUninit, os::raw::c_int, ptr}; +use std::{ + ffi::CString, ffi::c_void, mem::MaybeUninit, os::raw::c_int, panic::AssertUnwindSafe, + panic::catch_unwind, ptr, +}; use crate::{ class::RegisteredClass, + error::php_error, exception::PhpResult, ffi::{ ext_php_rs_executor_globals, instanceof_function_slow, std_object_handlers, @@ -9,9 +13,11 @@ use crate::{ zend_objects_clone_members, zend_std_get_properties, zend_std_has_property, zend_std_read_property, zend_std_write_property, zend_throw_error, }, + flags::ErrorType, flags::{PropertyFlags, ZvalTypeFlags}, internal::property::PropertyDescriptor, types::{ZendClassObject, ZendHashTable, ZendObject, ZendStr, Zval}, + zend::catch_panic, }; /// A set of functions associated with a PHP class. @@ -109,7 +115,23 @@ impl ZendObjectHandlers { .and_then(|obj| ZendClassObject::::from_zend_obj_mut(obj)) } { // Manually drop the object as we don't want to free the underlying memory. - unsafe { ptr::drop_in_place(&raw mut obj.obj) }; + // A panic in the user's `Drop` must not escape this `extern "C"` frame, and + // no exception can be thrown here: this also runs from the collector and at + // shutdown. Report it as a warning and keep freeing the object. + let dropped = catch_unwind(AssertUnwindSafe(|| unsafe { + ptr::drop_in_place(&raw mut obj.obj); + })); + if let Err(payload) = dropped { + let message = payload + .downcast_ref::<&str>() + .map(|s| (*s).to_owned()) + .or_else(|| payload.downcast_ref::().cloned()) + .unwrap_or_else(|| "non-string panic payload".to_owned()); + php_error( + &ErrorType::Warning, + &format!("Rust panic in Drop for {}: {message}", T::CLASS_NAME), + ); + } } // Always call the standard destructor to clean up the PHP object @@ -124,36 +146,51 @@ impl ZendObjectHandlers { // PHP will call OBJ_RELEASE on the returned pointer if an exception // is thrown, so we must NEVER return the original object. Always // allocate a new (possibly uninitialized) object for error paths. - let cloned_val = unsafe { - object - .as_ref() - .and_then(|obj| ZendClassObject::::from_zend_obj(obj)) - .and_then(|old| old.obj.as_ref()) - .and_then(RegisteredClass::clone_obj) - }; - - if let Some(val) = cloned_val { - let mut new = ZendClassObject::::new(val); - unsafe { zend_objects_clone_members(&raw mut new.std, object) }; - let raw = new.into_raw(); - // SAFETY: `into_raw` yields a valid object the engine takes over. - unsafe { &raw mut (*raw).std } - } else { - let msg = CString::new(format!( - "Trying to clone an uncloneable object of class {}", - T::CLASS_NAME - )) - .expect("Failed to create error message"); - unsafe { zend_throw_error(ptr::null_mut(), msg.as_ptr()) }; - // Return a new uninitialized object that PHP can safely release. - // free_obj handles uninitialized (None) objects gracefully. - let empty = unsafe { ZendClassObject::::new_uninit(None) }; - let raw = empty.into_raw(); - // SAFETY: `into_raw` yields a valid object the engine takes over. - unsafe { &raw mut (*raw).std } + let cloned_val = catch_panic(|| { + Ok(unsafe { + object + .as_ref() + .and_then(|obj| ZendClassObject::::from_zend_obj(obj)) + .and_then(|old| old.obj.as_ref()) + .and_then(RegisteredClass::clone_obj) + }) + }); + + match cloned_val { + Ok(Some(val)) => { + let mut new = ZendClassObject::::new(val); + unsafe { zend_objects_clone_members(&raw mut new.std, object) }; + let raw = new.into_raw(); + // SAFETY: `into_raw` yields a valid object the engine takes over. + unsafe { &raw mut (*raw).std } + } + Ok(None) => { + let msg = CString::new(format!( + "Trying to clone an uncloneable object of class {}", + T::CLASS_NAME + )) + .expect("Failed to create error message"); + unsafe { zend_throw_error(ptr::null_mut(), msg.as_ptr()) }; + Self::released_placeholder::() + } + Err(panic) => { + panic.throw(); + Self::released_placeholder::() + } } } + /// A fresh uninitialised object for the error paths of `clone_obj`: PHP calls + /// `OBJ_RELEASE` on whatever `clone_obj` returns once an exception is pending, + /// so the original must never be handed back. `free_obj` accepts the `None` + /// backing. + fn released_placeholder() -> *mut ZendObject { + let empty = unsafe { ZendClassObject::::new_uninit(None) }; + let raw = empty.into_raw(); + // SAFETY: `into_raw` yields a valid object the engine takes over. + unsafe { &raw mut (*raw).std } + } + #[allow(clippy::items_after_statements)] unsafe extern "C" fn read_property( object: *mut ZendObject, @@ -218,10 +255,10 @@ impl ZendObjectHandlers { }) } - match unsafe { internal::(object, obj, member, type_, cache_slot, rv) } { + match catch_panic(|| unsafe { internal::(object, obj, member, type_, cache_slot, rv) }) { Ok(rv) => rv, Err(e) => { - let _ = e.throw(); + e.throw(); unsafe { (*rv).set_null() }; rv } @@ -287,10 +324,10 @@ impl ZendObjectHandlers { }) } - match unsafe { internal::(object, obj, member, value, cache_slot) } { + match catch_panic(|| unsafe { internal::(object, obj, member, value, cache_slot) }) { Ok(rv) => rv, Err(e) => { - let _ = e.throw(); + e.throw(); value } } @@ -358,8 +395,8 @@ impl ZendObjectHandlers { Ok(()) } - if let Err(e) = unsafe { internal::(obj, props) } { - let _ = e.throw(); + if let Err(e) = catch_panic(|| unsafe { internal::(obj, props) }) { + e.throw(); } props @@ -446,10 +483,12 @@ impl ZendObjectHandlers { Ok(unsafe { zend_std_has_property(object, member, has_set_exists, cache_slot) }) } - match unsafe { internal::(object, obj, member, has_set_exists, cache_slot) } { + match catch_panic(|| unsafe { + internal::(object, obj, member, has_set_exists, cache_slot) + }) { Ok(rv) => rv, Err(e) => { - let _ = e.throw(); + e.throw(); 0 } } diff --git a/tests/src/integration/mod.rs b/tests/src/integration/mod.rs index 685d86d87e..07c08ab10d 100644 --- a/tests/src/integration/mod.rs +++ b/tests/src/integration/mod.rs @@ -21,6 +21,7 @@ pub mod number; pub mod object; #[cfg(feature = "observer")] pub mod observer; +pub mod panic; pub mod persistent_string; pub mod reference; pub mod separated; @@ -187,6 +188,13 @@ mod test { } pub fn run_php(file: &str) -> bool { + run_php_capturing_stderr(file); + true + } + + /// Runs the script in a real `php` subprocess, panics unless it exits + /// successfully, and returns what it wrote to stderr. + pub fn run_php_capturing_stderr(file: &str) -> String { setup(); let path = get_extension_path(); let output = Command::new(find_php().expect("Could not find PHP executable")) @@ -197,19 +205,18 @@ mod test { .arg(format!("src/integration/{file}")) .output() .expect("failed to run php file"); - if output.status.success() { - true - } else { - panic!( - " + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!( + output.status.success(), + " status: {} stdout: {} stderr: {} ", - output.status, - String::from_utf8(output.stdout).unwrap(), - String::from_utf8(output.stderr).unwrap() - ); - } + output.status, + String::from_utf8_lossy(&output.stdout), + stderr + ); + stderr } } diff --git a/tests/src/integration/panic/_panic_utils.php b/tests/src/integration/panic/_panic_utils.php new file mode 100644 index 0000000000..d4be247e51 --- /dev/null +++ b/tests/src/integration/panic/_panic_utils.php @@ -0,0 +1,16 @@ +getMessage(), 'Rust panic: '), $e->getMessage()); + assert(str_contains($e->getMessage(), $needle), $e->getMessage()); + return; + } + assert(false, 'no Error was thrown'); +} diff --git a/tests/src/integration/panic/mod.rs b/tests/src/integration/panic/mod.rs new file mode 100644 index 0000000000..40e6644658 --- /dev/null +++ b/tests/src/integration/panic/mod.rs @@ -0,0 +1,275 @@ +//! A Rust panic reached from PHP must never cross the `extern "C"` boundary. +//! Every fixture here panics on purpose; the PHP side asserts that it sees a +//! PHP `Error` whose message starts with `Rust panic: ` and that the engine +//! keeps running afterwards. +use std::sync::Mutex; +use std::sync::atomic::{AtomicU32, Ordering}; + +use ext_php_rs::{prelude::*, types::ZendCallable, zend::BailoutGuard, zend::ce}; + +#[php_function] +pub fn panic_test_function() { + panic!("function panicked"); +} + +#[php_function] +pub fn panic_test_healthy() -> i32 { + 42 +} + +#[php_function] +pub fn panic_test_payload() { + std::panic::panic_any(42_u8); +} + +#[php_function] +pub fn panic_test_after_throw() { + PhpException::new("thrown first".into(), 0, ce::type_error()).throw(); + panic!("then panicked"); +} + +static NESTED_RESULT: Mutex = Mutex::new(String::new()); + +#[php_function] +pub fn panic_test_nested(callback: ZendCallable) { + let outcome = match callback.try_call(vec![]) { + Ok(_) => "no exception".to_owned(), + Err(ext_php_rs::error::Error::ExceptionPending { class }) => format!("pending: {class}"), + Err(other) => format!("other: {other}"), + }; + *NESTED_RESULT + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) = outcome; +} + +#[php_function] +pub fn panic_test_nested_result() -> String { + NESTED_RESULT + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clone() +} + +static GUARD_DROPS: AtomicU32 = AtomicU32::new(0); + +struct DropCounter; + +impl Drop for DropCounter { + fn drop(&mut self) { + GUARD_DROPS.fetch_add(1, Ordering::SeqCst); + } +} + +#[php_function] +pub fn panic_test_guard() { + let _guarded = BailoutGuard::new(DropCounter); + let _plain = DropCounter; + panic!("guarded panic"); +} + +#[php_function] +pub fn panic_test_guard_drops() -> u32 { + GUARD_DROPS.load(Ordering::SeqCst) +} + +#[php_extern] +extern "C" { + fn strpos(haystack: &str, needle: &str) -> i64; +} + +#[php_function] +pub fn panic_test_extern() -> i64 { + unsafe { strpos("abc", "z") } +} + +#[php_interface] +#[php(name = "PanicIface")] +#[allow(dead_code)] +pub trait PanicIface { + fn iface_method(&self) -> i32; +} + +#[php_class] +#[php(name = "PanicClass")] +pub struct PanicClass { + #[php(prop)] + pub plain: i32, +} + +#[php_impl] +impl PanicClass { + pub fn __construct(explode: bool) -> Self { + assert!(!explode, "constructor panicked"); + Self { plain: 7 } + } + + pub fn method(&self) { + panic!("method panicked"); + } + + pub fn static_method() { + panic!("static method panicked"); + } + + #[php(getter)] + pub fn get_boom(&self) -> i32 { + panic!("getter panicked"); + } + + #[php(setter)] + pub fn set_boom(&mut self, _value: i32) { + panic!("setter panicked"); + } + + pub fn healthy(&self) -> i32 { + self.plain + } +} + +#[php_impl_interface] +impl PanicIface for PanicClass { + fn iface_method(&self) -> i32 { + panic!("interface method panicked"); + } +} + +struct ExplodingOnClone; + +impl Clone for ExplodingOnClone { + fn clone(&self) -> Self { + panic!("clone panicked"); + } +} + +#[php_class] +#[php(name = "PanicClone")] +#[derive(Clone)] +pub struct PanicClone { + _inner: ExplodingOnClone, +} + +#[php_impl] +impl PanicClone { + pub fn __construct() -> Self { + Self { + _inner: ExplodingOnClone, + } + } +} + +#[php_class] +#[php(name = "PanicDrop")] +pub struct PanicDrop; + +impl Drop for PanicDrop { + fn drop(&mut self) { + panic!("drop panicked"); + } +} + +#[php_impl] +impl PanicDrop { + pub fn __construct() -> Self { + Self + } +} + +#[cfg(feature = "closure")] +#[php_function] +pub fn panic_test_closure() -> Closure { + Closure::wrap(Box::new(|| -> i32 { panic!("closure panicked") }) as Box i32>) +} + +pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { + let builder = builder + .function(wrap_function!(panic_test_function)) + .function(wrap_function!(panic_test_healthy)) + .function(wrap_function!(panic_test_payload)) + .function(wrap_function!(panic_test_after_throw)) + .function(wrap_function!(panic_test_nested)) + .function(wrap_function!(panic_test_nested_result)) + .function(wrap_function!(panic_test_guard)) + .function(wrap_function!(panic_test_guard_drops)) + .function(wrap_function!(panic_test_extern)) + .interface::() + .class::() + .class::() + .class::(); + #[cfg(feature = "closure")] + let builder = builder.function(wrap_function!(panic_test_closure)); + builder +} + +#[cfg(test)] +mod tests { + use crate::integration::test::run_php_capturing_stderr; + + fn panic_script_survives(file: &str) { + let stderr = run_php_capturing_stderr(&format!("panic/{file}")); + assert!( + stderr.contains("panicked at"), + "the panic hook never fired for {file}:\n{stderr}" + ); + } + + #[test] + fn function_panic_becomes_error() { + panic_script_survives("panic_function.php"); + } + + #[test] + fn method_panics_become_error() { + panic_script_survives("panic_method.php"); + } + + #[test] + fn constructor_panic_becomes_error() { + panic_script_survives("panic_constructor.php"); + } + + #[test] + fn property_handler_panics_become_error() { + panic_script_survives("panic_property.php"); + } + + #[test] + fn clone_panic_becomes_error() { + panic_script_survives("panic_clone.php"); + } + + #[test] + fn drop_panic_becomes_warning() { + panic_script_survives("panic_drop.php"); + } + + #[cfg(feature = "closure")] + #[test] + fn closure_panic_becomes_error() { + panic_script_survives("panic_closure.php"); + } + + #[test] + fn pending_exception_wins_over_panic() { + panic_script_survives("panic_after_throw.php"); + } + + #[test] + fn non_string_payload_has_fallback_text() { + panic_script_survives("panic_payload.php"); + } + + #[test] + fn extern_failure_becomes_error() { + panic_script_survives("panic_extern.php"); + } + + #[test] + fn nested_call_chain_survives() { + panic_script_survives("panic_nested.php"); + } + + #[test] + fn bailout_guard_drops_once_on_panic() { + panic_script_survives("panic_guard.php"); + } +} diff --git a/tests/src/integration/panic/panic_after_throw.php b/tests/src/integration/panic/panic_after_throw.php new file mode 100644 index 0000000000..1aac17fb7b --- /dev/null +++ b/tests/src/integration/panic/panic_after_throw.php @@ -0,0 +1,10 @@ +getMessage() === 'thrown first', $e->getMessage()); + assert($e->getPrevious() === null); +} +assert(panic_test_healthy() === 42); diff --git a/tests/src/integration/panic/panic_clone.php b/tests/src/integration/panic/panic_clone.php new file mode 100644 index 0000000000..d7d552b692 --- /dev/null +++ b/tests/src/integration/panic/panic_clone.php @@ -0,0 +1,8 @@ + clone $obj, 'clone panicked'); +assert($obj instanceof PanicClone); +assert(panic_test_healthy() === 42); diff --git a/tests/src/integration/panic/panic_closure.php b/tests/src/integration/panic/panic_closure.php new file mode 100644 index 0000000000..a76b8b3fe8 --- /dev/null +++ b/tests/src/integration/panic/panic_closure.php @@ -0,0 +1,7 @@ + $closure(), 'closure panicked'); +assert(panic_test_healthy() === 42); diff --git a/tests/src/integration/panic/panic_constructor.php b/tests/src/integration/panic/panic_constructor.php new file mode 100644 index 0000000000..8832f2a036 --- /dev/null +++ b/tests/src/integration/panic/panic_constructor.php @@ -0,0 +1,10 @@ +healthy() === 7); diff --git a/tests/src/integration/panic/panic_drop.php b/tests/src/integration/panic/panic_drop.php new file mode 100644 index 0000000000..a922f8f4a6 --- /dev/null +++ b/tests/src/integration/panic/panic_drop.php @@ -0,0 +1,16 @@ + panic_test_extern(), 'strpos'); +assert(panic_test_healthy() === 42); diff --git a/tests/src/integration/panic/panic_function.php b/tests/src/integration/panic/panic_function.php new file mode 100644 index 0000000000..ae2a1e0490 --- /dev/null +++ b/tests/src/integration/panic/panic_function.php @@ -0,0 +1,7 @@ + panic_test_function(), 'function panicked'); +expect_rust_panic(fn() => panic_test_function(), 'function panicked'); +assert(panic_test_healthy() === 42); diff --git a/tests/src/integration/panic/panic_guard.php b/tests/src/integration/panic/panic_guard.php new file mode 100644 index 0000000000..0cc6a8cd60 --- /dev/null +++ b/tests/src/integration/panic/panic_guard.php @@ -0,0 +1,7 @@ + panic_test_guard(), 'guarded panic'); +assert(panic_test_guard_drops() === 2, (string) panic_test_guard_drops()); +assert(panic_test_healthy() === 42); diff --git a/tests/src/integration/panic/panic_method.php b/tests/src/integration/panic/panic_method.php new file mode 100644 index 0000000000..a2a778e368 --- /dev/null +++ b/tests/src/integration/panic/panic_method.php @@ -0,0 +1,10 @@ +method(...), 'method panicked'); +expect_rust_panic(PanicClass::staticMethod(...), 'static method panicked'); +expect_rust_panic($obj->ifaceMethod(...), 'interface method panicked'); +assert($obj instanceof PanicIface); +assert($obj->healthy() === 7); diff --git a/tests/src/integration/panic/panic_nested.php b/tests/src/integration/panic/panic_nested.php new file mode 100644 index 0000000000..d0b56e68bd --- /dev/null +++ b/tests/src/integration/panic/panic_nested.php @@ -0,0 +1,7 @@ + panic_test_nested(fn() => panic_test_function()), 'function panicked'); +assert(panic_test_nested_result() === 'pending: Error', panic_test_nested_result()); +assert(panic_test_healthy() === 42); diff --git a/tests/src/integration/panic/panic_payload.php b/tests/src/integration/panic/panic_payload.php new file mode 100644 index 0000000000..d37e0327a0 --- /dev/null +++ b/tests/src/integration/panic/panic_payload.php @@ -0,0 +1,6 @@ + panic_test_payload(), 'non-string panic payload'); +assert(panic_test_healthy() === 42); diff --git a/tests/src/integration/panic/panic_property.php b/tests/src/integration/panic/panic_property.php new file mode 100644 index 0000000000..2992d610cf --- /dev/null +++ b/tests/src/integration/panic/panic_property.php @@ -0,0 +1,13 @@ + $obj->boom, 'getter panicked'); +expect_rust_panic(function () use ($obj) { + $obj->boom = 1; +}, 'setter panicked'); +expect_rust_panic(fn() => isset($obj->boom), 'getter panicked'); +expect_rust_panic(fn() => (array) $obj, 'getter panicked'); +assert($obj->plain === 7); +assert($obj->healthy() === 7); diff --git a/tests/src/lib.rs b/tests/src/lib.rs index dc1519de67..0815bd16a3 100644 --- a/tests/src/lib.rs +++ b/tests/src/lib.rs @@ -33,6 +33,7 @@ pub fn build_module(module: ModuleBuilder) -> ModuleBuilder { module = integration::nullable::build_module(module); module = integration::number::build_module(module); module = integration::object::build_module(module); + module = integration::panic::build_module(module); #[cfg(feature = "observer")] { module = integration::observer::build_module(module); From 7034271eb529d63e16419eea5ff7674454bf0030 Mon Sep 17 00:00:00 2001 From: Pierre Tondereau Date: Sun, 20 Sep 2026 12:09:14 +0200 Subject: [PATCH 4/5] fix(module)!: fail MINIT instead of panicking on registration errors --- Cargo.toml | 2 + crates/macros/src/module.rs | 62 +++++++++++++---- src/builders/module.rs | 27 +++----- src/internal/mod.rs | 64 ++++++++++++++++++ tests/broken-minit/Cargo.toml | 24 +++++++ tests/broken-minit/src/lib.rs | 12 ++++ tests/broken-module/Cargo.toml | 24 +++++++ tests/broken-module/src/lib.rs | 18 +++++ tests/src/integration/mod.rs | 104 ++++++++++++++++++++--------- tests/src/integration/panic/mod.rs | 28 ++++++++ 10 files changed, 302 insertions(+), 63 deletions(-) create mode 100644 tests/broken-minit/Cargo.toml create mode 100644 tests/broken-minit/src/lib.rs create mode 100644 tests/broken-module/Cargo.toml create mode 100644 tests/broken-module/src/lib.rs diff --git a/Cargo.toml b/Cargo.toml index 9c1f05f1ed..a9bf8f3d7f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,6 +71,8 @@ members = [ "crates/php-build", "crates/introspection", "tests", + "tests/broken-module", + "tests/broken-minit", ] [package.metadata.docs.rs] diff --git a/crates/macros/src/module.rs b/crates/macros/src/module.rs index da345a09a4..5907f66dd2 100644 --- a/crates/macros/src/module.rs +++ b/crates/macros/src/module.rs @@ -74,22 +74,27 @@ fn parser_impl(input: ItemFn, crate_name: Option<&str>, static_ext: bool) -> Res extern "C" fn ext_php_rs_startup(ty: i32, mod_num: i32) -> i32 { let a = unsafe { #startup }; - let b = __EXT_PHP_RS_MODULE_STARTUP - .lock() - .take() - .map(|startup| { - ::ext_php_rs::internal::ext_php_rs_startup(); - startup.startup(ty, mod_num).map(|_| 0).unwrap_or(1) - }) - .unwrap_or_else(|| { - // Module already started, call ext_php_rs_startup for idempotent - // initialization (e.g., Closure::build early-returns if already built) - ::ext_php_rs::internal::ext_php_rs_startup(); - 0 - }); + let b = ::ext_php_rs::internal::startup_guard(|| { + // ext_php_rs_startup is idempotent (Closure::build early-returns once + // built), so it runs whether or not this is the first startup. + ::ext_php_rs::internal::ext_php_rs_startup(); + match __EXT_PHP_RS_MODULE_STARTUP.lock().take() { + Some(startup) => startup.startup(ty, mod_num), + None => Ok(()), + } + }); a | b } + static __EXT_PHP_RS_BUILD_ERROR: ::std::sync::OnceLock<::std::string::String> = + ::std::sync::OnceLock::new(); + + extern "C" fn ext_php_rs_failed_startup(_ty: i32, _mod_num: i32) -> i32 { + ::ext_php_rs::internal::failed_module_startup( + __EXT_PHP_RS_BUILD_ERROR.get().map_or("unknown error", ::std::string::String::as_str), + ) + } + __EXT_PHP_RS_MODULE_ENTRY.get_or_init(|| { #[inline] fn internal(#inputs) #output { @@ -107,7 +112,20 @@ fn parser_impl(input: ItemFn, crate_name: Option<&str>, static_ext: bool) -> Res __EXT_PHP_RS_MODULE_STARTUP.lock().replace(startup); (entry, owned) }, - Err(e) => panic!("Failed to build PHP module: {:?}", e), + Err(e) => { + // get_module cannot report failure to the engine (dl() dereferences the + // returned entry unchecked), so hand back a placeholder entry whose + // MINIT logs the build error and fails. + let _ = __EXT_PHP_RS_BUILD_ERROR.set(e.to_string()); + let (entry, _, owned) = ::ext_php_rs::builders::ModuleBuilder::new( + env!("CARGO_PKG_NAME"), + env!("CARGO_PKG_VERSION"), + ) + .startup_function(ext_php_rs_failed_startup) + .try_into() + .unwrap_or_else(|_| ::std::unreachable!("a module with only env! strings always builds")); + (entry, owned) + } } }) } @@ -163,6 +181,22 @@ mod tests { assert!(out.contains(r#"#[unsafe(no_mangle)]extern"C"fnmy_ext_get_module("#)); } + #[test] + fn build_failure_yields_a_placeholder_entry_whose_minit_fails() { + let out = expand(Some("my_ext"), false); + assert!(!out.contains("panic!")); + assert!(out.contains(r#"extern"C"fnext_php_rs_failed_startup("#)); + assert!(out.contains("::ext_php_rs::internal::failed_module_startup(")); + assert!(out.contains(".startup_function(ext_php_rs_failed_startup)")); + } + + #[test] + fn minit_runs_registration_under_startup_guard() { + let out = expand(Some("my_ext"), false); + assert!(out.contains("::ext_php_rs::internal::startup_guard(||")); + assert!(!out.contains("unwrap_or(1)")); + } + #[test] fn missing_crate_name_skips_delegate_in_dynamic_build() { assert!(!expand(None, false).contains("_get_module")); diff --git a/src/builders/module.rs b/src/builders/module.rs index 543688199f..a65b0f63f1 100644 --- a/src/builders/module.rs +++ b/src/builders/module.rs @@ -643,15 +643,9 @@ impl ModuleStartup { /// /// # Errors /// - /// * Returns an error if a constant could not be registered. - /// - /// # Panics - /// - /// * Panics if a class could not be registered. - #[expect( - clippy::expect_used, - reason = "registration runs in MINIT, where a failure is an extension bug the engine cannot recover from" - )] + /// * Returns an error if a constant, interface, class or enum could not be + /// registered. The generated MINIT then returns `FAILURE` and PHP refuses + /// to start the module. pub fn startup(self, _ty: i32, mod_num: i32) -> Result<()> { for (name, val) in self.constants { val.register_constant(&name, mod_num)?; @@ -659,21 +653,16 @@ impl ModuleStartup { // Interfaces must be registered before classes so that classes can implement // them - self.interfaces.into_iter().map(|c| c()).for_each(|c| { - c.register().expect("Failed to build interface"); - }); + self.interfaces + .into_iter() + .try_for_each(|c| c().register())?; - self.classes.into_iter().map(|c| c()).for_each(|c| { - c.register().expect("Failed to build class"); - }); + self.classes.into_iter().try_for_each(|c| c().register())?; #[cfg(feature = "enum")] self.enums .into_iter() - .map(|builder| builder()) - .for_each(|e| { - e.register().expect("Failed to build enum"); - }); + .try_for_each(|builder| builder().register())?; // Initialize observer systems if registered #[cfg(feature = "observer")] diff --git a/src/internal/mod.rs b/src/internal/mod.rs index 24c25ef7db..18ec2cf572 100644 --- a/src/internal/mod.rs +++ b/src/internal/mod.rs @@ -26,3 +26,67 @@ pub fn ext_php_rs_startup() { #[cfg(feature = "closure")] crate::closure::Closure::build(); } + +/// Runs the registration part of the generated MINIT and maps every Rust +/// failure to the engine's `FAILURE`. +/// +/// A registration error or a panic is logged as an `E_CORE_WARNING` naming the +/// cause before `FAILURE` is returned; the engine then reports +/// `Unable to start module` and stops startup (or fails the `dl()` +/// request) instead of the process aborting inside an `extern "C"` frame. +#[must_use] +pub fn startup_guard(func: impl FnOnce() -> crate::error::Result<()>) -> i32 { + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(func)); + let failure = match outcome { + Ok(Ok(())) => return crate::ffi::ZEND_RESULT_CODE_SUCCESS, + Ok(Err(err)) => format!("module startup failed: {err}"), + Err(payload) => { + let message = payload + .downcast_ref::<&str>() + .map(|s| (*s).to_owned()) + .or_else(|| payload.downcast_ref::().cloned()) + .unwrap_or_else(|| "non-string panic payload".to_owned()); + format!("module startup panicked: {message}") + } + }; + crate::error::php_error(&crate::flags::ErrorType::CoreWarning, &failure); + crate::ffi::ZEND_RESULT_CODE_FAILURE +} + +#[cfg(all(test, feature = "embed"))] +mod tests { + use super::startup_guard; + use crate::embed::Embed; + use crate::error::Error; + use crate::ffi::{ZEND_RESULT_CODE_FAILURE, ZEND_RESULT_CODE_SUCCESS}; + + #[test] + fn startup_guard_maps_success_to_zero() { + let code = Embed::run(|| startup_guard(|| Ok(()))); + assert_eq!(code, ZEND_RESULT_CODE_SUCCESS); + } + + #[test] + fn startup_guard_maps_an_error_to_failure() { + let code = Embed::run(|| startup_guard(|| Err(Error::InvalidScope))); + assert_eq!(code, ZEND_RESULT_CODE_FAILURE); + } + + #[test] + fn startup_guard_maps_a_panic_to_failure() { + let code = Embed::run(|| startup_guard(|| panic!("registration exploded"))); + assert_eq!(code, ZEND_RESULT_CODE_FAILURE); + } +} + +/// Startup function of the placeholder module entry that `#[php_module]` +/// returns when the real module could not be built. Logs the build error and +/// fails MINIT. +#[must_use] +pub fn failed_module_startup(reason: &str) -> i32 { + crate::error::php_error( + &crate::flags::ErrorType::CoreWarning, + &format!("module could not be built: {reason}"), + ); + crate::ffi::ZEND_RESULT_CODE_FAILURE +} diff --git a/tests/broken-minit/Cargo.toml b/tests/broken-minit/Cargo.toml new file mode 100644 index 0000000000..ff2276f1d6 --- /dev/null +++ b/tests/broken-minit/Cargo.toml @@ -0,0 +1,24 @@ +[package] +name = "broken-minit" +version = "0.0.0" +edition = "2024" +publish = false +license = "MIT OR Apache-2.0" + +[dependencies] +ext-php-rs = { path = "../../", default-features = false } + +[features] +default = ["enum", "runtime", "closure"] +enum = ["ext-php-rs/enum"] +anyhow = ["ext-php-rs/anyhow"] +runtime = ["ext-php-rs/runtime"] +closure = ["ext-php-rs/closure"] +static = ["ext-php-rs/_static"] +observer = ["ext-php-rs/observer"] +embed = ["ext-php-rs/embed"] +smartstring = ["ext-php-rs/smartstring"] +indexmap = ["ext-php-rs/indexmap"] + +[lib] +crate-type = ["cdylib"] diff --git a/tests/broken-minit/src/lib.rs b/tests/broken-minit/src/lib.rs new file mode 100644 index 0000000000..bd951372b9 --- /dev/null +++ b/tests/broken-minit/src/lib.rs @@ -0,0 +1,12 @@ +//! An extension whose module entry builds fine but whose MINIT fails: a constant +//! name carries a NUL byte, so registration returns an error. Loading it must +//! make PHP refuse the module, not abort the process. +#![cfg_attr(windows, feature(abi_vectorcall))] +#![allow(missing_docs)] + +use ext_php_rs::prelude::*; + +#[php_module] +pub fn get_module(module: ModuleBuilder) -> ModuleBuilder { + module.constant(("BAD\0CONST", 1i64, &[])) +} diff --git a/tests/broken-module/Cargo.toml b/tests/broken-module/Cargo.toml new file mode 100644 index 0000000000..aa336a4e46 --- /dev/null +++ b/tests/broken-module/Cargo.toml @@ -0,0 +1,24 @@ +[package] +name = "broken-module" +version = "0.0.0" +edition = "2024" +publish = false +license = "MIT OR Apache-2.0" + +[dependencies] +ext-php-rs = { path = "../../", default-features = false } + +[features] +default = ["enum", "runtime", "closure"] +enum = ["ext-php-rs/enum"] +anyhow = ["ext-php-rs/anyhow"] +runtime = ["ext-php-rs/runtime"] +closure = ["ext-php-rs/closure"] +static = ["ext-php-rs/_static"] +observer = ["ext-php-rs/observer"] +embed = ["ext-php-rs/embed"] +smartstring = ["ext-php-rs/smartstring"] +indexmap = ["ext-php-rs/indexmap"] + +[lib] +crate-type = ["cdylib"] diff --git a/tests/broken-module/src/lib.rs b/tests/broken-module/src/lib.rs new file mode 100644 index 0000000000..32459fbf46 --- /dev/null +++ b/tests/broken-module/src/lib.rs @@ -0,0 +1,18 @@ +//! An extension whose `ModuleBuilder` cannot become a module entry: one function +//! name carries a NUL byte. Loading it must fail MINIT, not abort the process. +#![cfg_attr(windows, feature(abi_vectorcall))] +#![allow(missing_docs)] + +use ext_php_rs::builders::FunctionBuilder; +use ext_php_rs::prelude::*; +use ext_php_rs::types::Zval; +use ext_php_rs::zend::ExecuteData; + +ext_php_rs::zend_fastcall! { + extern fn noop(_ex: &mut ExecuteData, _retval: &mut Zval) {} +} + +#[php_module] +pub fn get_module(module: ModuleBuilder) -> ModuleBuilder { + module.function(FunctionBuilder::new("bad\0name", noop)) +} diff --git a/tests/src/integration/mod.rs b/tests/src/integration/mod.rs index 07c08ab10d..e7d5d2a254 100644 --- a/tests/src/integration/mod.rs +++ b/tests/src/integration/mod.rs @@ -39,40 +39,52 @@ mod test { static BUILD: Once = Once::new(); - fn setup() { - BUILD.call_once(|| { - let mut command = Command::new("cargo"); - command.arg("build"); + /// A `cargo build` for a workspace extension crate, with the feature set this + /// test binary was compiled with. Every extension crate in `tests/` declares the + /// same features, so `ext-php-rs` resolves to the artifact that is already built + /// and its build script does not run again inside the test process. + fn cargo_build(package: Option<&str>) -> Command { + let mut command = Command::new("cargo"); + command.arg("build"); + if let Some(package) = package { + command.args(["-p", package]); + } - #[cfg(not(debug_assertions))] - command.arg("--release"); + #[cfg(not(debug_assertions))] + command.arg("--release"); - // Build features list dynamically based on compiled features - // Note: Using vec_init_then_push pattern here is intentional due to conditional - // compilation - #[allow(clippy::vec_init_then_push)] - { - let mut features = vec![]; - #[cfg(feature = "enum")] - features.push("enum"); - #[cfg(feature = "closure")] - features.push("closure"); - #[cfg(feature = "anyhow")] - features.push("anyhow"); - #[cfg(feature = "runtime")] - features.push("runtime"); - #[cfg(feature = "static")] - features.push("static"); - #[cfg(feature = "observer")] - features.push("observer"); - - if !features.is_empty() { - command.arg("--no-default-features"); - command.arg("--features").arg(features.join(",")); - } + // Build features list dynamically based on compiled features + // Note: Using vec_init_then_push pattern here is intentional due to conditional + // compilation + #[allow(clippy::vec_init_then_push)] + { + let mut features = vec![]; + #[cfg(feature = "enum")] + features.push("enum"); + #[cfg(feature = "closure")] + features.push("closure"); + #[cfg(feature = "anyhow")] + features.push("anyhow"); + #[cfg(feature = "runtime")] + features.push("runtime"); + #[cfg(feature = "static")] + features.push("static"); + #[cfg(feature = "observer")] + features.push("observer"); + + if !features.is_empty() { + command.arg("--no-default-features"); + command.arg("--features").arg(features.join(",")); } + } + command + } - let result = command.output().expect("failed to execute cargo build"); + fn setup() { + BUILD.call_once(|| { + let result = cargo_build(None) + .output() + .expect("failed to execute cargo build"); assert!( result.status.success(), @@ -187,6 +199,38 @@ mod test { }); } + /// Builds the named broken extension crate and loads it in a `php` + /// subprocess. Returns the exit status and the combined stdout and stderr. + pub fn load_broken_module(crate_name: &str) -> (std::process::ExitStatus, String) { + let built = cargo_build(Some(crate_name)) + .output() + .expect("failed to execute cargo build"); + assert!( + built.status.success(), + "{crate_name} build failed:\n{}", + String::from_utf8_lossy(&built.stderr) + ); + + let lib_name = crate_name.replace('-', "_"); + let mut path = PathBuf::from(get_extension_path()); + path.set_file_name(if std::env::consts::DLL_EXTENSION == "dll" { + lib_name + } else { + format!("lib{lib_name}") + }); + path.set_extension(std::env::consts::DLL_EXTENSION); + + let output = Command::new(find_php().expect("Could not find PHP executable")) + .arg(format!("-dextension={}", path.display())) + .arg("-ddisplay_startup_errors=1") + .args(["-r", "echo 'alive';"]) + .output() + .expect("failed to run php"); + let mut combined = String::from_utf8_lossy(&output.stdout).into_owned(); + combined.push_str(&String::from_utf8_lossy(&output.stderr)); + (output.status, combined) + } + pub fn run_php(file: &str) -> bool { run_php_capturing_stderr(file); true diff --git a/tests/src/integration/panic/mod.rs b/tests/src/integration/panic/mod.rs index 40e6644658..c88162e9f5 100644 --- a/tests/src/integration/panic/mod.rs +++ b/tests/src/integration/panic/mod.rs @@ -2,6 +2,7 @@ //! Every fixture here panics on purpose; the PHP side asserts that it sees a //! PHP `Error` whose message starts with `Rust panic: ` and that the engine //! keeps running afterwards. +#![allow(clippy::unused_self)] use std::sync::Mutex; use std::sync::atomic::{AtomicU32, Ordering}; @@ -272,4 +273,31 @@ mod tests { fn bailout_guard_drops_once_on_panic() { panic_script_survives("panic_guard.php"); } + + fn module_refused_at_startup(crate_name: &str, logged: &str) { + let (status, output) = crate::integration::test::load_broken_module(crate_name); + + assert!(!status.success(), "{output}"); + assert!( + status.code().is_some(), + "php was killed by a signal: {status}\n{output}" + ); + assert!(output.contains(logged), "{output}"); + assert!( + output.contains(&format!("Unable to start {crate_name} module")), + "{output}" + ); + assert!(!output.contains("panicked"), "{output}"); + assert!(!output.contains("alive"), "{output}"); + } + + #[test] + fn unbuildable_module_fails_minit_instead_of_aborting() { + module_refused_at_startup("broken-module", "module could not be built"); + } + + #[test] + fn failing_registration_fails_minit_instead_of_aborting() { + module_refused_at_startup("broken-minit", "module startup failed"); + } } From 7543b9541379800d68641cfa034be1d2a8de95c4 Mon Sep 17 00:00:00 2001 From: Pierre Tondereau Date: Sun, 20 Sep 2026 12:09:14 +0200 Subject: [PATCH 5/5] docs(guide): document the panic policy at the FFI boundary --- crates/macros/src/lib.rs | 22 +++++++ guide/src/advanced/bailout_guard.md | 4 ++ guide/src/exceptions.md | 62 ++++++++++++++++++++ guide/src/macros/extern.md | 3 + guide/src/macros/module.md | 18 ++++++ guide/src/migration-guides/v0.16.md | 91 +++++++++++++++++++++++++++++ 6 files changed, 200 insertions(+) diff --git a/crates/macros/src/lib.rs b/crates/macros/src/lib.rs index 9042d24993..f12c5cad1e 100644 --- a/crates/macros/src/lib.rs +++ b/crates/macros/src/lib.rs @@ -1705,6 +1705,25 @@ fn php_const_internal(args: TokenStream2, input: TokenStream2) -> TokenStream2 { /// Classes and constants are not registered with PHP in the `get_module` /// function. These are registered inside the extension startup function. /// +/// ## Startup failures +/// +/// If a constant, an interface, a class or an enum cannot be registered, the +/// generated startup function logs the cause as an `E_CORE_WARNING` and returns +/// `FAILURE`. PHP then reports `Unable to start module`. At engine +/// startup PHP exits. From `dl()` the request fails. A panic during +/// registration gets the same treatment. The process does not abort in either +/// case. +/// +/// `get_module` cannot report a failure, because PHP reads the returned entry +/// without a check. If the `ModuleBuilder` cannot become a module entry, for +/// example because a function name, an argument name or the module name +/// contains a NUL byte, the macro returns a placeholder entry with the name of +/// the crate. The startup function of that entry logs the build error and fails +/// as described above. +/// +/// The `startup` function that you name in `#[php_module(startup = ...)]` is +/// your own `extern "C"` function. A panic inside it is not caught. +/// /// ## Usage /// /// ```rust,no_run,ignore @@ -2235,6 +2254,9 @@ fn php_impl_interface_internal(args: TokenStream2, input: TokenStream2) -> Token /// * The actual function call failed internally. /// * The output [`Zval`] could not be parsed into the output type. /// +/// Inside a function that PHP calls, these panics become a PHP `Error`. The +/// process does not abort. See [Exceptions](../exceptions.md#panics). +/// /// The last point can be important when interacting with functions that return /// unions, such as [`strpos`] which can return an integer or a boolean. In this /// case, a [`Zval`] should be returned as parsing a boolean to an integer is diff --git a/guide/src/advanced/bailout_guard.md b/guide/src/advanced/bailout_guard.md index 567e6c635b..55db938d13 100644 --- a/guide/src/advanced/bailout_guard.md +++ b/guide/src/advanced/bailout_guard.md @@ -141,3 +141,7 @@ let result = try_catch(|| { // On Err, try_catch dropped _tmp. connection is still valid here. connection.query("..."); ``` + +`try_catch` also catches a panic inside the closure and returns +`Err(CatchError::Panic(message))`. It does not resume the panic. If you want the +panic to continue, call `std::panic::resume_unwind` with your own payload. diff --git a/guide/src/exceptions.md b/guide/src/exceptions.md index bccb1fea7c..22070d1ef4 100644 --- a/guide/src/exceptions.md +++ b/guide/src/exceptions.md @@ -67,4 +67,66 @@ pub fn module(module: ModuleBuilder) -> ModuleBuilder { # fn main() {} ``` +## Panics + +A Rust panic never crosses the `extern "C"` boundary into PHP. The crate catches +it at every entry point that it generates or installs: + +- Functions, methods, static methods, constructors and `#[php_impl_interface]` + methods, through `zend::run_handler`. +- `Closure` calls. +- Property getters and setters, `isset()`, the `(array)` cast, `var_export` on a + Rust-backed object, and `clone`. + +When the panic reaches the boundary, the unwind already ran the destructors of +every Rust frame inside the handler. The crate then throws a PHP `Error` +exception and the handler returns normally. The message starts with +`Rust panic: ` and continues with the panic message. When the payload is not a +string, for example after `panic_any(42)`, the message continues with +`non-string panic payload`. A `catch (\Exception $e)` block does not catch this +`Error`. A `catch (\Error $e)` or `catch (\Throwable $e)` block does. The +process does not stop, and the default panic hook still writes the location and +the message to stderr. + +If an exception is already pending when the panic happens, that exception +continues to propagate. The panic then only reaches the panic hook. This is the +rule that `throw()` also follows. + +If `Drop` of a Rust-backed object panics, the crate reports an `E_WARNING` with +the message `Rust panic in Drop for : ...` and frees the object. No +exception is possible there, because the destructor also runs from the garbage +collector and at request shutdown. + +The following cases still stop the process: + +- A second panic while the first one unwinds, for example a `Drop` that panics. + Rust turns this into an abort. +- A build profile with `panic = "abort"`. Nothing is caught. +- The hooks that the crate installs outside of a PHP call: the observer, the + zend extension, the error and exception observers, the module globals + constructor and destructor, and the embed `Sapi` trampolines. The engine is in + the middle of an operation there, so an exception or a bailout is not safe. +- An `extern "C"` function that you give to the engine yourself: `info_function`, + the request startup and shutdown functions, `post_deactivate_function`, the + `startup` function of `#[php_module]`, stream wrapper operations, and a + handler that you write by hand for `FunctionBuilder::new`. + +If you write a request-time handler by hand, run its body through +`zend::run_handler`. The handler then behaves like generated code: + +```rust,ignore +use ext_php_rs::zend::{ExecuteData, run_handler}; +use ext_php_rs::types::Zval; +use std::panic::AssertUnwindSafe; + +extern "C" fn my_handler(ex: &mut ExecuteData, retval: &mut Zval) { + run_handler(AssertUnwindSafe(|| { + // This code can panic, bail out or throw. + })); +} +``` + +A `Mutex` in your own statics stays poisoned after a caught panic, as in any +Rust program. + [`PhpException`]: https://docs.rs/ext-php-rs/0.5.0/ext_php_rs/php/exceptions/struct.PhpException.html diff --git a/guide/src/macros/extern.md b/guide/src/macros/extern.md index 2d2bd2d2f4..152a141be6 100644 --- a/guide/src/macros/extern.md +++ b/guide/src/macros/extern.md @@ -21,6 +21,9 @@ The function can panic when called under a few circumstances: * The actual function call failed internally. * The output [`Zval`] could not be parsed into the output type. +Inside a function that PHP calls, these panics become a PHP `Error`. The +process does not abort. See [Exceptions](../exceptions.md#panics). + The last point can be important when interacting with functions that return unions, such as [`strpos`] which can return an integer or a boolean. In this case, a [`Zval`] should be returned as parsing a boolean to an integer is diff --git a/guide/src/macros/module.md b/guide/src/macros/module.md index 63d8cb8d8c..19bd0081f7 100644 --- a/guide/src/macros/module.md +++ b/guide/src/macros/module.md @@ -27,6 +27,24 @@ register the following (if required): Classes and constants are not registered with PHP in the `get_module` function. These are registered inside the extension startup function. +## Startup failures + +If a constant, an interface, a class or an enum cannot be registered, the +generated startup function logs the cause as an `E_CORE_WARNING` and returns +`FAILURE`. PHP then reports `Unable to start module`. At engine +startup PHP exits. From `dl()` the request fails. A panic during registration +gets the same treatment. The process does not abort in either case. + +`get_module` cannot report a failure, because PHP reads the returned entry +without a check. If the `ModuleBuilder` cannot become a module entry, for +example because a function name, an argument name or the module name contains a +NUL byte, the macro returns a placeholder entry with the name of the crate. The +startup function of that entry logs the build error and fails as described +above. + +The `startup` function that you name in `#[php_module(startup = ...)]` is your +own `extern "C"` function. A panic inside it is not caught. + ## Usage ```rust,no_run diff --git a/guide/src/migration-guides/v0.16.md b/guide/src/migration-guides/v0.16.md index 47ed1f66fd..8b3225aa33 100644 --- a/guide/src/migration-guides/v0.16.md +++ b/guide/src/migration-guides/v0.16.md @@ -870,3 +870,94 @@ does not compile anymore. Keep the guard on the thread that created it, or call `into_inner` before you send the value. `zend::run_bailout_cleanups` is removed. `try_catch` runs the cleanup itself. + +## Panics Stay on the Rust Side of the FFI Boundary + +A panic that unwinds out of an `extern "C"` function aborts the process. In +v0.15 every generated handler let a panic do exactly that, because `try_catch` +caught the panic only to resume it. v0.16 keeps every panic on the Rust side. +See [Panics](../exceptions.md#panics) for the full policy. + +### `try_catch` reports a panic instead of resuming it + +`try_catch` and `try_catch_first` return `Err(CatchError::Panic(message))` when +the closure panics. `CatchError::Panic` is a new variant, and `Error::Catch` +prints its message. + +```rust,ignore +// v0.15: the panic continued past this line +let result = try_catch(|| risky()); + +// v0.16 +match try_catch(|| risky()) { + Err(CatchError::Panic(message)) => std::panic::resume_unwind(Box::new(message)), + Err(CatchError::Bailout) => { /* ... */ } + Err(_) => { /* ... */ } + Ok(value) => { /* ... */ } +} +``` + +A test that asserts inside a `try_catch` closure and ignores the result now +passes even when the assertion fails. Assert on the result of `try_catch`. + +`Embed::eval` and `Embed::run_script` return +`EmbedError::CatchError(CatchError::Panic(..))` in that case. `PhpEvalError` +gained a `Panic(String)` variant. `Embed::run` still resumes a panic, so a test +that panics inside `Embed::run` still fails. + +### A panic in a handler throws a PHP `Error` + +A panic in a generated function, method, constructor, `#[php_impl_interface]` +method, `Closure`, property getter or setter, or `clone` throws a PHP `Error` +with the message `Rust panic: ` and returns normally. A panic in +`Drop` of a Rust-backed object reports an `E_WARNING` and frees the object. +Before v0.16 each of these aborted the PHP process. + +A handler that you write by hand for `FunctionBuilder::new` does not get this +behaviour on its own. Run its body through the new `zend::run_handler`. + +### `PhpException::throw` returns `()` + +`throw()` no longer returns a `Result`. If the exception cannot be thrown, for +example because the class is an interface or the message contains a NUL byte, +`throw()` throws a plain `Error` whose message names the cause and contains the +original message. + +```rust,ignore +// v0.15 +exception.throw()?; +exception.throw().expect("Failed to throw PHP exception."); + +// v0.16 +exception.throw(); +``` + +`IntoZval for Result` throws the error as before and now returns +`Err(Error::ExceptionPending { class })` instead of the conversion error. + +### MINIT fails instead of panicking + +`ModuleStartup::startup` returns an `Err` when a class, an interface or an enum +cannot be registered. The generated startup function logs the cause as an +`E_CORE_WARNING` and returns `FAILURE`, and PHP reports +`Unable to start module`. A panic during registration gets the same +treatment. In v0.15 a failed constant registration made the startup function +return `1`, which is not `FAILURE` (`-1`), so PHP started the module anyway. + +`get_module` never panics. If the module cannot be built, the macro returns a +placeholder entry whose startup function logs the build error and fails as +above. + +### New items + +`zend::run_handler` runs the body of a hand-written handler with the panic and +bailout behaviour of generated code. `zend::ce::error` returns the `Error` class +entry. + +### What still aborts + +The observer, zend extension, error observer and exception observer hooks, the +module globals constructor and destructor, the embed `Sapi` trampolines, and +every `extern "C"` function that you give to the engine yourself still abort on +a panic. This includes the `startup` function of `#[php_module]`. +`BailoutGuard` semantics do not change.