diff --git a/crates/macros/src/lib.rs b/crates/macros/src/lib.rs index aff60bddf..4ec435e65 100644 --- a/crates/macros/src/lib.rs +++ b/crates/macros/src/lib.rs @@ -76,6 +76,44 @@ extern crate proc_macro; /// - `flags` - Sets property visibility flags, e.g. `#[php(prop, flags = /// ext_php_rs::flags::PropertyFlags::Private)]` /// +/// ### Known limitation: `#[php(prop)]` on owned refcounted types accessed via +/// `Exception::getMessage`-style C methods leaks one `zend_string` per call. +/// +/// The generated read-property handler writes a fresh `zend_string` with +/// refcount=1 into the `rv` slot via `set_zval`. PHP's `Exception::getMessage` +/// (and siblings such as `getFile`) read this with `zval_get_string + RETURN_STR`, +/// which addrefs to 2 and transfers the pointer to `return_value` without +/// changing the refcount. The stack `rv` then goes out of scope without +/// `zval_ptr_dtor`, orphaning one refcount per call. +/// +/// This affects any `#[php_class]` extending `\Exception` with a `#[php(prop)]` +/// field whose type allocates a `zend_string` (e.g. `String`, `Vec` when +/// converted to a binary string, or any `IntoZval` impl producing `IS_STRING_EX`) +/// — most commonly when the field shadows the parent `\Exception::$message`. +/// Direct property access (`$obj->field`) via the `FETCH_OBJ_R` opcode is +/// **not** affected because the bytecode handler properly consumes the rv ref. +/// +/// **Workarounds, in order of preference:** +/// +/// 1. **Do not shadow the inherited property name.** Rename the field +/// (e.g. `payload` instead of `message`) and expose it through a +/// `#[php_method]` getter. The method-return path is not affected by +/// this leak. +/// Note: `\Exception::getMessage` is `final` in PHP, so overriding it +/// directly via `#[php_method] fn get_message(...)` is rejected at +/// class registration. +/// +/// 2. **Write the value into the parent's real property slot via +/// `zend_update_property_stringl`** (raw FFI). PHP's `getMessage` +/// then reads from real storage through `zend_std_read_property`, +/// bypassing the leaky `rv` path entirely. This requires dropping +/// `#[php(prop)]` from the shadow field and populating the parent +/// slot at construction time. See `biscuit-php` (`src/errors.rs`) +/// for a worked example. +/// +/// Tracked by the +/// `prop_string_field_does_not_leak_on_repeated_get_message` regression test. +/// /// ## Restrictions /// /// ### No lifetime parameters diff --git a/tests/src/integration/class/mod.rs b/tests/src/integration/class/mod.rs index 5e2e4fe4e..d03f78295 100644 --- a/tests/src/integration/class/mod.rs +++ b/tests/src/integration/class/mod.rs @@ -160,6 +160,50 @@ pub fn throw_class_object_exception_with_prop() -> PhpResult { Err(PhpException::from_class::("ignored".into()).with_object(zval)) } +/// Regression coverage for a refcount leak in the `#[php(prop)]` field getter +/// when the field is an owned refcounted type (here `String`) AND the property +/// is read via a C-level method that uses `zval_get_string` + `RETURN_STR` +/// (e.g. `Exception::getMessage`). +/// +/// The generated getter writes a fresh `zend_string` with refcount=1 into the +/// `rv` slot. PHP's `getMessage` then calls `zval_get_string(prop)` which +/// addrefs (→2) and `RETURN_STR` transfers the pointer to `return_value` +/// without changing the refcount. When the method returns, the stack `rv` +/// goes out of scope without being dtor'd, orphaning one refcount per call. +/// Each `$e->getMessage()` therefore leaks a `zend_string`. +/// +/// Surfaced first in production by biscuit-php's `DatalogException` subclasses, +/// which declare `#[php(prop, flags = Protected)] message: String` and shadow +/// the parent `\Exception::$message`. +#[php_class] +#[php(extends(ce = ce::exception, stub = "\\Exception"))] +#[derive(Default)] +pub struct TestExceptionMessageLeak { + /// Public to keep the test focused on the refcount leak rather than the + /// visibility-check path. The leak reproduces on any `#[php(prop)]` field + /// whose type allocates a `zend_string` via `IntoZval`; biscuit-php's + /// real-world trigger happens to use Protected, but the codegen bug is the + /// same shape regardless of visibility. + #[php(prop)] + pub message: String, +} + +#[php_impl] +impl TestExceptionMessageLeak { + pub fn __construct() -> Self { + Self::default() + } +} + +#[php_function] +pub fn throw_exception_with_message_prop() -> PhpResult { + let payload = TestExceptionMessageLeak { + message: "leak-bait message contents".to_string(), + }; + let zval = ZendClassObject::new(payload).into_zval(false)?; + Err(PhpException::from_class::("ignored".into()).with_object(zval)) +} + #[php_class] #[php(implements(ce = ce::arrayaccess, stub = "ArrayAccess"))] #[php(extends(ce = ce::exception, stub = "\\Exception"))] @@ -652,9 +696,11 @@ pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { .class::() .class::() .class::() + .class::() .function(wrap_function!(test_class)) .function(wrap_function!(throw_exception)) - .function(wrap_function!(throw_class_object_exception_with_prop)); + .function(wrap_function!(throw_class_object_exception_with_prop)) + .function(wrap_function!(throw_exception_with_message_prop)); #[cfg(php84)] let builder = builder @@ -671,6 +717,26 @@ pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { #[cfg(test)] mod tests { + /// Documents an outstanding bug in the `#[php(prop)]` field-property + /// getter codegen: when an owned refcounted field (e.g. `String`) is read + /// via the `Exception::getMessage` pattern (`zval_get_string` + + /// `RETURN_STR`), the getter orphans one refcount on the `rv` stack zval + /// per call. See the `#[php(prop)]` docs in `crates/macros/src/lib.rs` + /// for the full mechanic and the recommended `#[php_method]` workaround. + /// + /// Ignored because a proper fix requires either an upstream PHP patch + /// (`zval_ptr_dtor(&rv)` in `Exception::getMessage` when `retval == &rv`) + /// or mirroring `#[php(prop)]` shadow fields to the parent's real + /// property slot via `zend_update_property_stringl`. Run with + /// `cargo test -- --ignored` to reproduce. + #[test] + #[ignore = "documents the #[php(prop)] String getter leak on the Exception::getMessage path; see crate-level docs"] + fn prop_string_field_does_not_leak_on_repeated_get_message() { + assert!(crate::integration::test::run_php( + "class/prop_string_leak.php" + )); + } + #[test] fn class_works() { assert!(crate::integration::test::run_php("class/class.php")); diff --git a/tests/src/integration/class/prop_string_leak.php b/tests/src/integration/class/prop_string_leak.php new file mode 100644 index 000000000..fd26e17ca --- /dev/null +++ b/tests/src/integration/class/prop_string_leak.php @@ -0,0 +1,53 @@ +getMessage(); + // Warm up so any one-shot allocations (cache slots, opcode cache, + // hashtable resizes) settle before measurement. + for ($i = 0; $i < 16; $i++) { + $e->getMessage(); + } + gc_collect_cycles(); + $mem_before = memory_get_usage(); + for ($i = 0; $i < 500; $i++) { + $e->getMessage(); + } + gc_collect_cycles(); + $mem_after = memory_get_usage(); + $leak_observed = $mem_after - $mem_before; +} +assert( + $first_call_returned === 'leak-bait message contents', + "Sanity check: one getMessage() call should return the stored message " . + "verbatim. Got: " . var_export($first_call_returned, true) +); +assert( + $leak_observed !== null && $leak_observed < 8192, + "getMessage() on a #[php(prop)] String field leaks zend_strings: 500 " . + "calls grew the emalloc heap by " . var_export($leak_observed, true) . + " bytes. Each call orphans one zend_string refcount (~150 bytes); " . + "with the fix this delta should be near zero." +);