fix(zval): Heap corruption with persistent=true #424 (#622)

Author: kakserpom

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();
     }
 

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));

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;

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"
+        ));
+    }
+}

tests/src/integration/persistent_string/persistent_string.php

@@ -0,0 +1,18 @@
+<?php
+$result = test_persistent_string();
+assert($result === "persistent string test passed", "Persistent string test failed: $result");
+
+$result = test_non_persistent_string();
+assert($result === "non-persistent string test passed", "Non-persistent string test failed: $result");
+
+$result = test_persistent_string_read();
+assert($result === "read: READ_BEFORE_DROP", "Persistent string read test failed: $result");
+
+$result = test_persistent_string_loop(100);
+assert($result === "completed 100 iterations", "Persistent string loop test failed: $result");
+
+$result = test_interned_string_persistent();
+assert($result === "interned persistent test passed", "Interned persistent string test failed: $result");
+
+$result = test_interned_string_non_persistent();
+assert($result === "interned non-persistent test passed", "Interned non-persistent string test failed: $result");

tests/src/lib.rs

@@ -29,6 +29,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::persistent_string::build_module(module);
     module = integration::string::build_module(module);
     module = integration::variadic_args::build_module(module);
     module = integration::interface::build_module(module);