From 09947d35228b6d8281411d7b5a696a8a053d6355 Mon Sep 17 00:00:00 2001 From: Vasily Zorin Date: Sun, 21 Dec 2025 20:26:57 +0700 Subject: [PATCH] fix(zval): Heap corruption with persistent=true #424 --- src/types/zval.rs | 50 ++++++++++++-- tests/sapi.rs | 2 +- tests/src/integration/mod.rs | 1 + .../src/integration/persistent_string/mod.rs | 69 +++++++++++++++++++ .../persistent_string/persistent_string.php | 18 +++++ tests/src/lib.rs | 1 + 6 files changed, 135 insertions(+), 6 deletions(-) create mode 100644 tests/src/integration/persistent_string/mod.rs create mode 100644 tests/src/integration/persistent_string/persistent_string.php diff --git a/src/types/zval.rs b/src/types/zval.rs index 1b3e9edca..e39f06ee6 100644 --- a/src/types/zval.rs +++ b/src/types/zval.rs @@ -13,8 +13,9 @@ use crate::{ convert::{FromZval, FromZvalMut, IntoZval, IntoZvalDyn}, error::{Error, Result}, ffi::{ - _zval_struct__bindgen_ty_1, _zval_struct__bindgen_ty_2, zend_is_callable, - zend_is_identical, zend_is_iterable, zend_resource, zend_value, zval, zval_ptr_dtor, + _zval_struct__bindgen_ty_1, _zval_struct__bindgen_ty_2, ext_php_rs_zend_string_release, + zend_is_callable, zend_is_identical, zend_is_iterable, zend_resource, zend_value, zval, + zval_ptr_dtor, }, flags::DataType, flags::ZvalTypeFlags, @@ -496,6 +497,21 @@ impl Zval { /// * `val` - The value to set the zval as. /// * `persistent` - Whether the string should persist between requests. /// + /// # Persistent Strings + /// + /// When `persistent` is `true`, the string is allocated from PHP's + /// persistent heap (using `malloc`) rather than the request-bound heap. + /// This is typically used for strings that need to survive across multiple + /// PHP requests, such as class names, function names, or module-level data. + /// + /// **Important:** The string will still be freed when the Zval is dropped. + /// The `persistent` flag only affects which memory allocator is used. If + /// you need a string to outlive the Zval, consider using + /// [`std::mem::forget`] on the Zval or storing the string elsewhere. + /// + /// For most use cases (return values, function arguments, temporary + /// storage), you should use `persistent: false`. + /// /// # Errors /// /// Never returns an error. @@ -507,6 +523,9 @@ impl Zval { /// Sets the value of the zval as a Zend string. /// + /// The Zval takes ownership of the string. When the Zval is dropped, + /// the string will be released. + /// /// # Parameters /// /// * `val` - String content. @@ -527,9 +546,13 @@ impl Zval { self.value.str_ = ptr; } - /// Sets the value of the zval as a interned string. Returns nothing in a + /// Sets the value of the zval as an interned string. Returns nothing in a /// result when successful. /// + /// Interned strings are stored once and are immutable. PHP stores them in + /// an internal hashtable. Unlike regular strings, interned strings are not + /// reference counted and should not be freed by `zval_ptr_dtor`. + /// /// # Parameters /// /// * `val` - The value to set the zval as. @@ -540,7 +563,10 @@ impl Zval { /// Never returns an error. // TODO: Check if we can drop the result here. pub fn set_interned_string(&mut self, val: &str, persistent: bool) -> Result<()> { - self.set_zend_string(ZendStr::new_interned(val, persistent)); + // Use InternedStringEx (without RefCounted) because interned strings + // should not have their refcount modified by zval_ptr_dtor. + self.change_type(ZvalTypeFlags::InternedStringEx); + self.value.str_ = ZendStr::new_interned(val, persistent).into_raw(); Ok(()) } @@ -676,7 +702,21 @@ impl Zval { fn change_type(&mut self, ty: ZvalTypeFlags) { // SAFETY: we have exclusive mutable access to this zval so can free the // contents. - unsafe { zval_ptr_dtor(self) }; + // + // For strings, we use zend_string_release directly instead of zval_ptr_dtor + // to correctly handle persistent strings. zend_string_release properly checks + // the IS_STR_PERSISTENT flag and uses the correct deallocator (free vs efree). + // This fixes heap corruption issues when dropping Zvals containing persistent + // strings (see issue #424). + if self.is_string() { + unsafe { + if let Some(str_ptr) = self.value.str_.as_mut() { + ext_php_rs_zend_string_release(str_ptr); + } + } + } else { + unsafe { zval_ptr_dtor(self) }; + } self.u1.type_info = ty.bits(); } diff --git a/tests/sapi.rs b/tests/sapi.rs index 73136d4f6..688fd63dc 100644 --- a/tests/sapi.rs +++ b/tests/sapi.rs @@ -166,7 +166,7 @@ fn test_sapi_multithread() { Ok(zval) => { assert!(zval.is_string()); let string = zval.string().unwrap(); - let output = string.to_string(); + let output = string.clone(); assert_eq!(output, format!("Hello, thread-{i}!")); results.lock().unwrap().push((i, output)); diff --git a/tests/src/integration/mod.rs b/tests/src/integration/mod.rs index 60acb2958..d58388e5a 100644 --- a/tests/src/integration/mod.rs +++ b/tests/src/integration/mod.rs @@ -15,6 +15,7 @@ pub mod magic_method; pub mod nullable; pub mod number; pub mod object; +pub mod persistent_string; pub mod string; pub mod types; pub mod variadic_args; diff --git a/tests/src/integration/persistent_string/mod.rs b/tests/src/integration/persistent_string/mod.rs new file mode 100644 index 000000000..9e5ff5804 --- /dev/null +++ b/tests/src/integration/persistent_string/mod.rs @@ -0,0 +1,69 @@ +use ext_php_rs::prelude::*; +use ext_php_rs::types::Zval; + +#[php_function] +pub fn test_persistent_string() -> String { + let mut z = Zval::new(); + let _ = z.set_string("PERSISTENT_STRING", true); + // z is dropped here - this was causing heap corruption before the fix + "persistent string test passed".to_string() +} + +#[php_function] +pub fn test_non_persistent_string() -> String { + let mut z = Zval::new(); + let _ = z.set_string("NON_PERSISTENT_STRING", false); + "non-persistent string test passed".to_string() +} + +#[php_function] +pub fn test_persistent_string_read() -> String { + let mut z = Zval::new(); + let _ = z.set_string("READ_BEFORE_DROP", true); + let value = z.str().unwrap_or("failed"); + format!("read: {value}") +} + +#[php_function] +pub fn test_persistent_string_loop(count: i64) -> String { + for i in 0..count { + let mut z = Zval::new(); + let s = format!("LOOP_{i}"); + let _ = z.set_string(&s, true); + } + format!("completed {count} iterations") +} + +#[php_function] +pub fn test_interned_string_persistent() -> String { + let mut z = Zval::new(); + let _ = z.set_interned_string("INTERNED_PERSISTENT", true); + "interned persistent test passed".to_string() +} + +#[php_function] +pub fn test_interned_string_non_persistent() -> String { + let mut z = Zval::new(); + let _ = z.set_interned_string("INTERNED_NON_PERSISTENT", false); + "interned non-persistent test passed".to_string() +} + +pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { + builder + .function(wrap_function!(test_persistent_string)) + .function(wrap_function!(test_non_persistent_string)) + .function(wrap_function!(test_persistent_string_read)) + .function(wrap_function!(test_persistent_string_loop)) + .function(wrap_function!(test_interned_string_persistent)) + .function(wrap_function!(test_interned_string_non_persistent)) +} + +#[cfg(test)] +mod tests { + #[test] + fn persistent_string_works() { + assert!(crate::integration::test::run_php( + "persistent_string/persistent_string.php" + )); + } +} diff --git a/tests/src/integration/persistent_string/persistent_string.php b/tests/src/integration/persistent_string/persistent_string.php new file mode 100644 index 000000000..fd850b0fe --- /dev/null +++ b/tests/src/integration/persistent_string/persistent_string.php @@ -0,0 +1,18 @@ + ModuleBuilder { module = integration::nullable::build_module(module); module = integration::number::build_module(module); module = integration::object::build_module(module); + module = integration::persistent_string::build_module(module); module = integration::string::build_module(module); module = integration::variadic_args::build_module(module); module = integration::interface::build_module(module);