From 4d689cdf84615f17cd0049a110b17b571323acc9 Mon Sep 17 00:00:00 2001 From: Pierre Tondereau Date: Sun, 6 Sep 2026 20:44:33 +0200 Subject: [PATCH 1/4] fix!: close the v0.16 soundness backlog --- benches/benches/binary_bench.rs | 5 + benches/benches/binary_slice.php | 9 ++ benches/ext/src/lib.rs | 7 + guide/src/advanced/custom_sapi.md | 4 +- guide/src/migration-guides/v0.16.md | 139 ++++++++++++++++++ guide/src/types/binary_slice.md | 6 + guide/src/types/iterable.md | 3 + guide/src/types/iterator.md | 4 + src/alloc.rs | 13 +- src/binary.rs | 28 ++-- src/binary_slice.rs | 27 +++- src/boxed.rs | 16 +- src/builders/class.rs | 11 +- src/embed/sapi.rs | 9 -- src/enum_.rs | 5 +- src/flags.rs | 5 +- src/types/class_object.rs | 4 +- src/types/iterable.rs | 13 +- src/types/iterator.rs | 22 ++- src/types/object.rs | 15 +- src/types/zval.rs | 86 +++++++++-- src/zend/function.rs | 7 - src/zend/handlers.rs | 66 +++++++-- src/zend/ini_entry_def.rs | 7 - src/zend/module.rs | 10 -- tests/sapi.rs | 32 ++-- .../integration/binary_slice/binary_slice.php | 14 ++ tests/src/integration/binary_slice/mod.rs | 27 ++++ tests/src/integration/class/class.php | 29 ++++ tests/src/integration/mod.rs | 1 + tests/src/lib.rs | 1 + 31 files changed, 491 insertions(+), 134 deletions(-) create mode 100644 benches/benches/binary_slice.php create mode 100644 tests/src/integration/binary_slice/binary_slice.php create mode 100644 tests/src/integration/binary_slice/mod.rs diff --git a/benches/benches/binary_bench.rs b/benches/benches/binary_bench.rs index 36d67e24a3..a99346756c 100644 --- a/benches/benches/binary_bench.rs +++ b/benches/benches/binary_bench.rs @@ -122,6 +122,11 @@ fn array_interned_keys(cnt: usize) -> ExitStatus { run_php("array_interned_keys.php", cnt) } +#[divan::bench(args = [1, 10, 100_000])] +fn binary_slice_reads(cnt: usize) -> ExitStatus { + run_php("binary_slice.php", cnt) +} + fn main() { setup(); divan::main(); diff --git a/benches/benches/binary_slice.php b/benches/benches/binary_slice.php new file mode 100644 index 0000000000..900b9ab99b --- /dev/null +++ b/benches/benches/binary_slice.php @@ -0,0 +1,9 @@ +) -> u64 { + values.iter().sum() +} + #[php_function] pub fn bench_function(n: u64) -> u64 { n @@ -137,6 +143,7 @@ pub fn build_module(module: ModuleBuilder) -> ModuleBuilder { .function(wrap_function!(bench_callback_function)) .function(wrap_function!(bench_array_with_str_ref_keys)) .function(wrap_function!(bench_array_with_interned_keys)) + .function(wrap_function!(bench_binary_slice_sum)) .class::() .class::() .globals(&INTERNED_KEYS) diff --git a/guide/src/advanced/custom_sapi.md b/guide/src/advanced/custom_sapi.md index 9ba9a1c0bf..23844da039 100644 --- a/guide/src/advanced/custom_sapi.md +++ b/guide/src/advanced/custom_sapi.md @@ -118,7 +118,9 @@ let module = MySapi::build_module().expect("failed to build SAPI module"); ``` The returned `SapiModule` can then be passed to `sapi_startup()` and -`php_module_startup()` just like a manually-built one. +`php_module_startup()` just like a manually-built one. Both copy the struct by +value (`sapi_module = *sf`), so a local variable is enough: keep it alive until +`sapi_shutdown()` has returned and pass `&raw mut module`. The builder places the SAPI's `name`, `pretty_name`, `executable_location` and `php_ini_path_override` strings on the heap. PHP never frees them, so after diff --git a/guide/src/migration-guides/v0.16.md b/guide/src/migration-guides/v0.16.md index 6a1b95f06a..da0f0428ae 100644 --- a/guide/src/migration-guides/v0.16.md +++ b/guide/src/migration-guides/v0.16.md @@ -421,3 +421,142 @@ the one the CLI was built with. `Module::enums` is always present and `Class::closure()` is always available; the `enum` and `closure` features only decide whether `ext-php-rs` fills them. + +## `PackSlice::unpack_into` Requires an Unsafe Block + +`PackSlice::unpack_into` builds a `&[T]` over the bytes of a `zend_string`, so +its length and alignment preconditions are now part of the signature. +`Zval::binary_slice` checks both and stays safe. `Pack::unpack_into` stays safe +as well: it now reads unaligned and never relied on alignment. + +### Before (v0.15) + +```rust,ignore +let values: &[u64] = u64::unpack_into(zend_str); +``` + +### After (v0.16) + +```rust,ignore +// Preferred: the checked entry point. +let values: &[u64] = zval.binary_slice::()?; + +// Or, when the checks are done by hand: +// SAFETY: `zend_str.len` is a multiple of 8 and `val` is 8-byte aligned. +let values: &[u64] = unsafe { u64::unpack_into(zend_str) }; +``` + +`Zval::binary_slice` also returns `None` when the string bytes are not aligned +for `T`. Zend allocates every string on an 8-byte boundary, so this only fires +for types wider than 8 bytes or strings from a foreign allocator. + +## `Zval::set_ptr` Requires an Unsafe Block + +`Zval::set_ptr` stores a raw pointer in an `IS_PTR` zval. The engine reads +`IS_PTR` entries with a fixed type in several of its tables (property tables as +`zend_property_info *`, constant tables as `zend_class_constant *`, INI +directives as `zend_ini_entry *`), so storing an arbitrary pointer and letting +the zval reach one of those tables is undefined behaviour. + +### Before (v0.15) + +```rust,ignore +zval.set_ptr(ptr); +``` + +### After (v0.16) + +```rust,ignore +// SAFETY: `ptr` outlives the zval and the zval never reaches an engine table. +unsafe { zval.set_ptr(ptr) }; +``` + +`set_ptr` now also releases the previous value of the zval instead of +overwriting it in place. + +## Iterators Yield Owned Values + +`ZendIterator::get_current_data` returned `&'a Zval` with an unconstrained +`'a`, so callers could keep the reference past the next `move_forward`. Zend +releases that zval on every step (`zend_user_it_invalidate_current` runs +`zval_ptr_dtor` on it), so the reference dangled. The reference is now tied to +the `&mut self` borrow, and both `ZendIterator`'s and `Iterable`'s `Iter` yield +`(Zval, Zval)`: the value is a shallow clone, the same copy `foreach` makes in +`ZEND_FE_FETCH_R`. + +### Before (v0.15) + +```rust,ignore +let values: Vec<&Zval> = iterator.iter()?.map(|(_, v)| v).collect(); +``` + +### After (v0.16) + +```rust,ignore +let values: Vec = iterator.iter()?.map(|(_, v)| v).collect(); +``` + +Destructuring `for (k, v) in iterator.iter()?` keeps working; `v` is a `Zval` +instead of a `&Zval`, so drop the explicit `&` where you spelled the type. + +## `ZBox::into_raw` Returns `*mut T` + +`ZBox::into_raw` fabricated a `&'static mut T`. It now mirrors `Box::into_raw` +and returns `*mut T`, which `ZBox::from_raw` accepts back unchanged. + +### Before (v0.15) + +```rust,ignore +let obj: &'static mut ZendObject = zbox.into_raw(); +zval.set_object(obj); +``` + +### After (v0.16) + +```rust,ignore +let obj: *mut ZendObject = zbox.into_raw(); +// SAFETY: `into_raw` yields a valid, exclusively owned object. +zval.set_object(unsafe { &mut *obj }); +``` + +## Heap-Leaking `into_raw` Constructors Removed + +`FunctionEntry::into_raw`, `IniEntryDef::into_raw`, `SapiModule::into_raw` and +the deprecated `ModuleEntry::into_raw` boxed a value and returned the pointer +with no owner. None of them was needed: `ModuleBuilder` and `ClassBuilder` hand +function tables to the engine themselves, `IniEntryDef::register` takes a +`Vec`, and `sapi_startup`/`php_module_startup` copy the `sapi_module_struct` +by value (`sapi_module = *sf`), so a local is enough. + +### Before (v0.15) + +```rust,ignore +let sapi = builder.build()?.into_raw(); +unsafe { sapi_startup(sapi) }; +// ... +unsafe { cleanup_sapi_allocations(sapi); drop(Box::from_raw(sapi)) }; +``` + +### After (v0.16) + +```rust,ignore +let mut sapi = builder.build()?; +unsafe { sapi_startup(&raw mut sapi) }; +// ... +unsafe { cleanup_sapi_allocations(&raw mut sapi) }; +``` + +Keep the `SapiModule` alive until `sapi_shutdown()` has returned: the engine +copies the struct but keeps reading the strings it points to. + +## `GlobalConstantFlags::CaseSensitive` Removed + +The flag was deprecated and had no effect since PHP 8.0, where constants are +always case-sensitive. Remove it from any flag set you build. + +## `alloc::emalloc` Honours `Layout::align` + +`emalloc` ignored the alignment of the `Layout` it was given. The Zend allocator +guarantees `ZEND_MM_ALIGNMENT` (8 bytes) and has no aligned-allocation entry +point, so `emalloc` now returns a null pointer for any `Layout` whose alignment +exceeds 8 instead of handing back under-aligned memory. diff --git a/guide/src/types/binary_slice.md b/guide/src/types/binary_slice.md index 1ea193f7da..d4ce9b8474 100644 --- a/guide/src/types/binary_slice.md +++ b/guide/src/types/binary_slice.md @@ -16,6 +16,12 @@ string pointer, with the length of the array being the length of the string. implemented on most primitive numbers (i8, i16, i32, i64, u8, u16, u32, u64, isize, usize, f32, f64). +The conversion fails (the argument is rejected) when the string length is not a +whole multiple of `size_of::()` or when the string bytes are not aligned for +`T`. Zend allocates strings on 8-byte boundaries, so the alignment check only +fails for exotic allocators. `PackSlice::unpack_into` itself is `unsafe`; use +`Zval::binary_slice`, which performs both checks. + [`pack`]: https://www.php.net/manual/en/function.pack.php [`unpack`]: https://www.php.net/manual/en/function.unpack.php diff --git a/guide/src/types/iterable.md b/guide/src/types/iterable.md index 9fa1bdf68c..2094292090 100644 --- a/guide/src/types/iterable.md +++ b/guide/src/types/iterable.md @@ -10,6 +10,9 @@ Converting from a zval to a `Iterable` is valid when the value is either an arra that implements the `Traversable` interface. This means that any value that can be used in a `foreach` loop can be converted into a `Iterable`. +Both variants yield owned `(Zval, Zval)` pairs; see [`ZendIterator`](./iterator.md) +for why the `Traversable` side cannot lend references. + ## Rust example ```rust,no_run diff --git a/guide/src/types/iterator.md b/guide/src/types/iterator.md index d6d694e587..8866855b1b 100644 --- a/guide/src/types/iterator.md +++ b/guide/src/types/iterator.md @@ -13,6 +13,10 @@ used but also a the result of a `query` call with `PDO`. If you want a more universal `iterable` type that also supports arrays, see [Iterable](./iterable.md). +Each iteration yields an owned `(Zval, Zval)` pair. The value is a shallow clone +of the zval Zend hands out, because the engine releases its own copy on the +next `move_forward`; this is the same copy `foreach` performs. + ## Rust example ```rust,no_run diff --git a/src/alloc.rs b/src/alloc.rs index c52f789cbc..63fd00bdb4 100644 --- a/src/alloc.rs +++ b/src/alloc.rs @@ -3,7 +3,7 @@ use cfg_if::cfg_if; -use crate::ffi::{_efree, _emalloc, _estrdup}; +use crate::ffi::{_efree, _emalloc, _estrdup, ZEND_MM_ALIGNMENT}; use std::{ alloc::Layout, ffi::{CString, c_char, c_void}, @@ -11,16 +11,23 @@ use std::{ /// Uses the PHP memory allocator to allocate request-bound memory. /// +/// The Zend allocator only guarantees `ZEND_MM_ALIGNMENT` (8 bytes) and has +/// no aligned-allocation entry point, so a `layout` requesting a stricter +/// alignment cannot be honoured and yields a null pointer. +/// /// # Parameters /// /// * `layout` - The layout of the requested memory. /// /// # Returns /// -/// A pointer to the memory allocated. +/// A pointer to the memory allocated, or null if `layout.align()` exceeds +/// `ZEND_MM_ALIGNMENT`. #[must_use] pub fn emalloc(layout: Layout) -> *mut u8 { - // TODO account for alignment + if layout.align() > ZEND_MM_ALIGNMENT.unsigned_abs() { + return std::ptr::null_mut(); + } let size = layout.size(); (unsafe { diff --git a/src/binary.rs b/src/binary.rs index fde07d444d..2a27a6611e 100644 --- a/src/binary.rs +++ b/src/binary.rs @@ -123,7 +123,8 @@ pub unsafe trait Pack: Clone { /// format. Note that the data *must* be all one type, as this /// implementation only unpacks one type. /// - /// # Safety + /// Trailing bytes that do not fill a whole `Self` are ignored. The bytes + /// are read unaligned, so any valid `zend_string` is accepted. /// /// There is no way to tell if the data stored in the string is actually of /// the given type. The results of this function can also differ from @@ -154,25 +155,18 @@ macro_rules! pack_impl { } fn unpack_into(s: &zend_string) -> Vec { - #[allow(clippy::cast_lossless)] - let bytes = ($d / 8) as u64; - let len = (s.len as u64) / bytes; - let mut result = - Vec::with_capacity(len.try_into().expect("Capacity integer overflow")); - // TODO: Check alignment + let len = s.len / ($d as usize / 8); + // Only `read_unaligned` dereferences this pointer, so the alignment + // the cast would normally promise is never relied on. #[allow(clippy::cast_ptr_alignment)] let ptr = s.val.as_ptr().cast::<$t>(); - // SAFETY: We calculate the length of memory that we can legally read based on - // the side of the type, therefore we never read outside the memory we - // should. - for i in 0..len { - result.push(unsafe { - *ptr.offset(i.try_into().expect("Offset integer overflow")) - }); - } - - result + // SAFETY: a valid `zend_string` has `len` readable bytes at `val`, and + // `len * size_of::() <= s.len`. `read_unaligned` has no alignment + // requirement, so a 1-byte-aligned `val` is fine. + (0..len) + .map(|i| unsafe { ptr.add(i).read_unaligned() }) + .collect() } } }; diff --git a/src/binary_slice.rs b/src/binary_slice.rs index 9c62f1b6d5..8be2eb7889 100644 --- a/src/binary_slice.rs +++ b/src/binary_slice.rs @@ -88,20 +88,32 @@ pub unsafe trait PackSlice: Clone { /// format. Note that the data *must* be all one type, as this /// implementation only unpacks one type. /// - /// # Safety - /// /// There is no way to tell if the data stored in the string is actually of /// the given type. The results of this function can also differ from /// platform-to-platform due to the different representation of some /// types on different platforms. Consult the [`pack`] function /// documentation for more details. /// + /// # Safety + /// + /// The returned slice aliases the string's bytes, so the caller must + /// guarantee: + /// + /// * `s.len` is a whole multiple of `size_of::()`. + /// * `s.val` is aligned to `align_of::()`. Zend allocates strings on + /// `ZEND_MM_ALIGNMENT` (8 bytes) boundaries and `val` sits at an 8-byte + /// offset, but the `zend_string` type itself only promises 1-byte + /// alignment for `val`. + /// + /// [`Zval::binary_slice`](crate::types::Zval::binary_slice) performs both + /// checks and is the safe entry point. + /// /// # Parameters /// /// * `s` - The Zend string containing the binary data. /// /// [`pack`]: https://www.php.net/manual/en/function.pack.php - fn unpack_into(s: &zend_string) -> &[Self]; + unsafe fn unpack_into(s: &zend_string) -> &[Self]; } /// Implements the [`PackSlice`] trait for a given type. @@ -112,12 +124,13 @@ macro_rules! pack_slice_impl { ($t: ty, $d: expr) => { unsafe impl PackSlice for $t { - fn unpack_into(s: &zend_string) -> &[Self] { - let bytes = ($d / 8) as usize; - let len = (s.len as usize) / bytes; - // TODO: alignment needs fixing? + unsafe fn unpack_into(s: &zend_string) -> &[Self] { + let len = s.len / ($d as usize / 8); + // The caller guarantees `val` is aligned for `Self`, see `PackSlice`. #[allow(clippy::cast_ptr_alignment)] let ptr = s.val.as_ptr().cast::<$t>(); + // SAFETY: the caller guarantees alignment and that `len` whole elements + // fit in the string's `s.len` readable bytes. unsafe { from_raw_parts(ptr, len) } } } diff --git a/src/boxed.rs b/src/boxed.rs index e710122e25..d57c2b97ed 100644 --- a/src/boxed.rs +++ b/src/boxed.rs @@ -19,7 +19,7 @@ //! //! [memory arenas]: https://en.wikipedia.org/wiki/Region-based_memory_management //! [`ZendStr`]: crate::types::ZendStr -//! [`emalloc`]: super::alloc::efree +//! [`emalloc`]: super::alloc::emalloc use std::{ borrow::Borrow, @@ -55,16 +55,12 @@ impl ZBox { /// process. The data pointed to by the returned pointer is not /// released. /// - /// # Safety - /// - /// The caller is responsible for managing the memory pointed to by the - /// returned pointer, including freeing the memory. + /// Mirrors [`Box::into_raw`]: the pointer is non-null and well-aligned, and + /// the caller becomes responsible for releasing it, typically by passing it + /// back to [`ZBox::from_raw`] or to the engine. #[must_use] - pub fn into_raw(self) -> &'static mut T { - let mut this = ManuallyDrop::new(self); - // SAFETY: All constructors ensure the contained pointer is well-aligned and - // dereferenceable. - unsafe { this.0.as_mut() } + pub fn into_raw(self) -> *mut T { + ManuallyDrop::new(self).0.as_ptr() } } diff --git a/src/builders/class.rs b/src/builders/class.rs index a49e5ae3ec..f9ea964917 100644 --- a/src/builders/class.rs +++ b/src/builders/class.rs @@ -229,14 +229,17 @@ impl ClassBuilder { // Without default initialization, accessing properties on uninitialized // objects would panic. if let Some(instance) = T::default_init() { - let obj = ZendClassObject::::new(instance); - return obj.into_raw().get_mut_zend_obj(); + let obj = ZendClassObject::::new(instance).into_raw(); + // SAFETY: `into_raw` yields a valid, exclusively owned object that + // the engine takes over. + return unsafe { (*obj).get_mut_zend_obj() }; } // SAFETY: After calling this function, PHP will always call the constructor // defined below, which assumes that the object is uninitialized. - let obj = unsafe { ZendClassObject::::new_uninit(ce.as_ref()) }; - obj.into_raw().get_mut_zend_obj() + let obj = unsafe { ZendClassObject::::new_uninit(ce.as_ref()) }.into_raw(); + // SAFETY: same ownership transfer as above. + unsafe { (*obj).get_mut_zend_obj() } } zend_fastcall! { diff --git a/src/embed/sapi.rs b/src/embed/sapi.rs index 53e31b2a05..aa702f5166 100644 --- a/src/embed/sapi.rs +++ b/src/embed/sapi.rs @@ -12,15 +12,6 @@ pub type SapiModule = sapi_module_struct; unsafe impl Send for SapiModule {} unsafe impl Sync for SapiModule {} -impl SapiModule { - /// Allocates the module entry on the heap, returning a pointer to the - /// memory location. The caller is responsible for the memory pointed to. - #[must_use] - pub fn into_raw(self) -> *mut Self { - Box::into_raw(Box::new(self)) - } -} - /// Frees the string allocations that /// [`SapiBuilder`](crate::builders::SapiBuilder) placed inside a /// [`SapiModule`]: `name`, `pretty_name`, `executable_location` and diff --git a/src/enum_.rs b/src/enum_.rs index 3ab85c513c..e5ebb3e18e 100644 --- a/src/enum_.rs +++ b/src/enum_.rs @@ -95,7 +95,10 @@ where // `ZBox::set_zval` in `object.rs`. let mut obj = self.into_zend_object()?; obj.dec_count(); - zv.set_object(obj.into_raw()); + let obj = obj.into_raw(); + // SAFETY: `into_raw` yields a valid, exclusively owned object whose + // reference is transferred to the zval. + zv.set_object(unsafe { &mut *obj }); Ok(()) } } diff --git a/src/flags.rs b/src/flags.rs index 211bda6f80..3f349b50cf 100644 --- a/src/flags.rs +++ b/src/flags.rs @@ -7,7 +7,7 @@ use crate::ffi::ZEND_ACC_ENUM; #[cfg(not(php82))] use crate::ffi::ZEND_ACC_REUSE_GET_ITERATOR; use crate::ffi::{ - _IS_BOOL, CONST_CS, CONST_DEPRECATED, CONST_NO_FILE_CACHE, CONST_PERSISTENT, E_COMPILE_ERROR, + _IS_BOOL, CONST_DEPRECATED, CONST_NO_FILE_CACHE, CONST_PERSISTENT, E_COMPILE_ERROR, E_COMPILE_WARNING, E_CORE_ERROR, E_CORE_WARNING, E_DEPRECATED, E_ERROR, E_NOTICE, E_PARSE, E_RECOVERABLE_ERROR, E_STRICT, E_USER_DEPRECATED, E_USER_ERROR, E_USER_NOTICE, E_USER_WARNING, E_WARNING, GC_IMMUTABLE, IS_ARRAY, IS_CALLABLE, IS_CONSTANT_AST, IS_DOUBLE, IS_FALSE, @@ -264,9 +264,6 @@ bitflags! { /// Flags for building module global constants. #[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Clone, Copy)] pub struct GlobalConstantFlags: u32 { - /// No longer used -- always case-sensitive - #[deprecated(note = "No longer used -- always case-sensitive")] - const CaseSensitive = CONST_CS; /// Persistent const Persistent = CONST_PERSISTENT; /// Can't be saved in file cache diff --git a/src/types/class_object.rs b/src/types/class_object.rs index 9dace43648..bf3a64648d 100644 --- a/src/types/class_object.rs +++ b/src/types/class_object.rs @@ -360,7 +360,9 @@ impl IntoZval for ZBox> { // net refcount at 1. Matches `ZBox::set_zval` in object.rs. self.std.dec_count(); let obj = self.into_raw(); - zv.set_object(&mut obj.std); + // SAFETY: `into_raw` yields a valid, exclusively owned object whose + // reference is transferred to the zval. + zv.set_object(unsafe { &mut (*obj).std }); Ok(()) } } diff --git a/src/types/iterable.rs b/src/types/iterable.rs index 7228cff7e8..20707ac89c 100644 --- a/src/types/iterable.rs +++ b/src/types/iterable.rs @@ -43,7 +43,7 @@ impl Iterable<'_> { // TODO: Implement `iter_mut` #[allow(clippy::into_iter_without_iter)] impl<'a> IntoIterator for &'a mut Iterable<'a> { - type Item = (Zval, &'a Zval); + type Item = (Zval, Zval); type IntoIter = Iter<'a>; fn into_iter(self) -> Self::IntoIter { @@ -64,17 +64,22 @@ impl<'a> FromZvalMut<'a> for Iterable<'a> { } /// Rust iterator over a PHP iterable. +/// +/// Values are owned shallow clones so that both variants share one item type; +/// see [`super::iterator::Iter`] for why the `Traversable` side cannot lend. pub enum Iter<'a> { Array(ZendHashTableIter<'a>), Traversable(ZendIteratorIter<'a>), } -impl<'a> Iterator for Iter<'a> { - type Item = (Zval, &'a Zval); +impl Iterator for Iter<'_> { + type Item = (Zval, Zval); fn next(&mut self) -> Option { match self { - Iter::Array(array) => array.next_zval(), + Iter::Array(array) => array + .next_zval() + .map(|(key, value)| (key, value.shallow_clone())), Iter::Traversable(traversable) => traversable.next(), } } diff --git a/src/types/iterator.rs b/src/types/iterator.rs index 3d1a5687e9..d42601cadf 100644 --- a/src/types/iterator.rs +++ b/src/types/iterator.rs @@ -91,11 +91,17 @@ impl ZendIterator { /// Get the current data of the iterator. /// + /// The reference borrows the iterator: Zend owns the returned zval and + /// releases it on the next [`move_forward`](Self::move_forward), + /// [`rewind`](Self::rewind) or when the iterator is destroyed + /// (`zend_user_it_invalidate_current` calls `zval_ptr_dtor` on it). Use + /// [`Zval::shallow_clone`] to keep the value across iterations. + /// /// # Returns /// /// Returns a reference to the current data of the iterator if available /// , [`None`] otherwise. - pub fn get_current_data<'a>(&mut self) -> Option<&'a Zval> { + pub fn get_current_data(&mut self) -> Option<&Zval> { let get_current_data = unsafe { (*self.funcs).get_current_data }?; let value = unsafe { &*get_current_data(&raw mut *self) }; @@ -131,7 +137,7 @@ impl ZendIterator { // TODO: Implement `iter_mut` #[allow(clippy::into_iter_without_iter)] impl<'a> IntoIterator for &'a mut ZendIterator { - type Item = (Zval, &'a Zval); + type Item = (Zval, Zval); type IntoIter = Iter<'a>; fn into_iter(self) -> Self::IntoIter { @@ -146,12 +152,16 @@ impl Debug for ZendIterator { } /// Immutable iterator upon a reference to a PHP iterator. +/// +/// Values are shallow clones of the zval Zend hands out, the same copy +/// `foreach` performs (`ZVAL_COPY` in `ZEND_FE_FETCH_R`), because the engine +/// releases its own copy on the next `move_forward`. pub struct Iter<'a> { zi: &'a mut ZendIterator, } -impl<'a> Iterator for Iter<'a> { - type Item = (Zval, &'a Zval); +impl Iterator for Iter<'_> { + type Item = (Zval, Zval); fn next(&mut self) -> Option { // Call next when index > 0, so next is really called at the start of each @@ -177,7 +187,9 @@ impl<'a> Iterator for Iter<'a> { Some(key) => key, }; - self.zi.get_current_data().map(|value| (key, value)) + self.zi + .get_current_data() + .map(|value| (key, value.shallow_clone())) } } diff --git a/src/types/object.rs b/src/types/object.rs index 483a767f13..aabdd5f683 100644 --- a/src/types/object.rs +++ b/src/types/object.rs @@ -147,8 +147,9 @@ impl ZendObject { #[must_use] pub fn from_class_object(obj: ZBox>) -> ZBox { let this = obj.into_raw(); - // SAFETY: Consumed box must produce a well-aligned non-null pointer. - unsafe { ZBox::from_raw(this.get_mut_zend_obj()) } + // SAFETY: Consumed box yields a well-aligned non-null pointer to a live + // class object, whose `std` header is the `ZendObject` we re-box. + unsafe { ZBox::from_raw((*this).get_mut_zend_obj()) } } /// Returns the [`ClassEntry`] associated with this object. @@ -752,11 +753,13 @@ impl IntoZval for ZBox { #[inline] fn set_zval(mut self, zv: &mut Zval, _: bool) -> Result<()> { - // We must decrement the refcounter on the object before inserting into the - // zval, as the reference counter will be incremented on add. - // NOTE(david): again is this needed, we increment in `set_object`. + // `set_object` is `ZVAL_OBJ_COPY` and increments the refcount; the box + // already owns one reference, so drop it first to keep the net count at 1. self.dec_count(); - zv.set_object(self.into_raw()); + let obj = self.into_raw(); + // SAFETY: `into_raw` yields a valid, exclusively owned object whose + // reference is transferred to the zval. + zv.set_object(unsafe { &mut *obj }); Ok(()) } } diff --git a/src/types/zval.rs b/src/types/zval.rs index 5e71d623d5..b11726c4f0 100644 --- a/src/types/zval.rs +++ b/src/types/zval.rs @@ -199,8 +199,6 @@ impl Zval { /// behavior (silent truncation of trailing bytes) hid off-by-one input /// errors and was a correctness hazard. /// - /// # Safety - /// /// There is no way to tell if the data stored in the string is actually of /// the given type. The results of this function can also differ from /// platform-to-platform due to the different representation of some @@ -226,10 +224,11 @@ impl Zval { /// returned instead of a vector, meaning the contents of the string is /// not copied. /// - /// Returns `None` if the zval is not a string, or if the byte length of - /// the string is not a whole multiple of `size_of::()`. - /// - /// # Safety + /// Returns `None` if the zval is not a string, if the byte length of the + /// string is not a whole multiple of `size_of::()`, or if the string + /// bytes are not aligned for `T`. Zend allocates strings on 8-byte + /// boundaries, so the alignment check only fails for types wider than + /// `ZEND_MM_ALIGNMENT` or for strings from a foreign allocator. /// /// There is no way to tell if the data stored in the string is actually of /// the given type. The results of this function can also differ from @@ -242,10 +241,14 @@ impl Zval { #[must_use] pub fn binary_slice(&self) -> Option<&[T]> { let s = self.zend_str()?; - if !s.len.is_multiple_of(std::mem::size_of::()) { + if !s.len.is_multiple_of(std::mem::size_of::()) + || s.val.as_ptr().align_offset(std::mem::align_of::()) != 0 + { return None; } - Some(T::unpack_into(s)) + // SAFETY: length and alignment were checked above, which is the whole + // `PackSlice::unpack_into` contract. + Some(unsafe { T::unpack_into(s) }) } /// Returns the value of the zval if it is a resource. @@ -1047,12 +1050,17 @@ impl Zval { /// Sets the value of the zval as a reference to an object. /// + /// This is `ZVAL_OBJ_COPY`: the object refcount is incremented because the + /// zval now holds its own reference, which Zend releases when the zval is + /// destroyed. Callers that already own a reference (e.g. a + /// `ZBox`) must `dec_count()` before handing the object over. + /// /// # Parameters /// /// * `val` - The value to set the zval as. pub fn set_object(&mut self, val: &mut ZendObject) { self.change_type(ZvalTypeFlags::ObjectEx); - val.inc_count(); // TODO(david): not sure if this is needed :/ + val.inc_count(); self.value.obj = ptr::from_ref(val).cast_mut(); } @@ -1092,13 +1100,47 @@ impl Zval { self.value.arr = val.into_raw(); } - /// Sets the value of the zval as a pointer. + /// Sets the value of the zval as a raw pointer (`IS_PTR`). + /// + /// The previous value of the zval is released first. /// /// # Parameters /// /// * `ptr` - The pointer to set the zval as. - pub fn set_ptr(&mut self, ptr: *mut T) { - self.u1.type_info = ZvalTypeFlags::Ptr.bits(); + /// + /// # Safety + /// + /// `IS_PTR` zvals are opaque to refcounting, so Zend never frees `ptr`, but + /// several engine tables dereference `IS_PTR` entries with a fixed type: + /// property tables read them as `zend_property_info *`, constant tables as + /// `zend_class_constant *`, `EG(ini_directives)` as `zend_ini_entry *`. The + /// caller must not insert this zval into any table the engine reads unless + /// `ptr` points to the type that table expects, and `ptr` must stay valid + /// for as long as the zval (or any copy of it) is alive. + /// + /// Calling this method requires an unsafe block: + /// + /// ```compile_fail,E0133 + /// use ext_php_rs::types::Zval; + /// + /// fn assign_ptr(zval: &mut Zval, ptr: *mut u32) { + /// zval.set_ptr(ptr); + /// } + /// ``` + /// + /// # Examples + /// + /// ```no_run + /// # use ext_php_rs::types::Zval; + /// let mut slot = 42u32; + /// let mut zval = Zval::new(); + /// + /// // SAFETY: `slot` outlives `zval` and the zval never reaches an engine + /// // table. + /// unsafe { zval.set_ptr(&raw mut slot) }; + /// ``` + pub unsafe fn set_ptr(&mut self, ptr: *mut T) { + self.change_type(ZvalTypeFlags::Ptr); self.value.ptr = ptr.cast::(); } @@ -1457,6 +1499,26 @@ mod tests { }); } + #[test] + fn binary_slice_should_reject_partial_element() { + Embed::run(|| { + let mut zval = Zval::new(); + zval.set_string("1234567", false).unwrap(); + assert!(zval.binary_slice::().is_none()); + assert_eq!(zval.binary_slice::().map(<[u8]>::len), Some(7)); + }); + } + + #[test] + fn binary_slice_should_view_packed_u64() { + Embed::run(|| { + let mut zval = Zval::new(); + zval.set_binary(vec![1u64, 2, 3]); + assert_eq!(zval.binary_slice::(), Some(&[1u64, 2, 3][..])); + assert_eq!(zval.binary::(), Some(vec![1, 2, 3])); + }); + } + #[test] fn test_is_scalar() { Embed::run(|| { diff --git a/src/zend/function.rs b/src/zend/function.rs index 6fb583e272..f331809a93 100644 --- a/src/zend/function.rs +++ b/src/zend/function.rs @@ -45,13 +45,6 @@ impl FunctionEntry { frameless_function_infos: ptr::null(), } } - - /// Converts the function entry into a raw and pointer, releasing it to the - /// C world. - #[must_use] - pub fn into_raw(self) -> *mut Self { - Box::into_raw(Box::new(self)) - } } /// PHP function. diff --git a/src/zend/handlers.rs b/src/zend/handlers.rs index f4ade380f4..27db03b5bc 100644 --- a/src/zend/handlers.rs +++ b/src/zend/handlers.rs @@ -4,7 +4,7 @@ use crate::{ class::RegisteredClass, exception::PhpResult, ffi::{ - ext_php_rs_executor_globals, instanceof_function_slow, std_object_handlers, + ext_php_rs_executor_globals, instanceof_function_slow, std_object_handlers, zend_array_dup, zend_class_entry, zend_is_true, zend_object_handlers, zend_object_std_dtor, zend_objects_clone_members, zend_std_get_properties, zend_std_has_property, zend_std_read_property, zend_std_write_property, zend_throw_error, @@ -56,9 +56,40 @@ impl ZendObjectHandlers { unsafe { (*ptr).read_property = Some(Self::read_property::) }; unsafe { (*ptr).write_property = Some(Self::write_property::) }; unsafe { (*ptr).get_properties = Some(Self::get_properties::) }; + unsafe { (*ptr).get_gc = Some(Self::get_gc) }; unsafe { (*ptr).has_property = Some(Self::has_property::) }; } + /// `zend_std_get_gc` routes objects with a custom `get_properties` through + /// that handler, and the collector calls it repeatedly while holding an + /// extra reference on the returned table (`GC_ADDREF(ht)` in + /// `gc_mark_grey`, restored in `gc_scan`). Merging Rust properties into a + /// table in that state trips `HT_ASSERT_RC1` and, once `gc_collect_white` + /// has dropped the children's counts, destroys zvals the collector still + /// walks. Rust-backed properties are produced by getters and own no zvals + /// of their own, so the collector only needs the engine-owned storage: + /// this is the standard-object branch of `zend_std_get_gc`. + unsafe extern "C" fn get_gc( + object: *mut ZendObject, + table: *mut *mut Zval, + n: *mut c_int, + ) -> *mut ZendHashTable { + let obj = unsafe { &mut *object }; + if obj.properties.is_null() { + unsafe { + *table = obj.properties_table.as_mut_ptr(); + *n = (*obj.ce).default_properties_count; + } + ptr::null_mut() + } else { + unsafe { + *table = ptr::null_mut(); + *n = 0; + } + obj.properties + } + } + unsafe extern "C" fn free_obj(object: *mut ZendObject) { // Try to get the ZendClassObject. This may return None for: // - PHP subclasses/mocks that didn't call the parent constructor @@ -92,7 +123,8 @@ impl ZendObjectHandlers { let mut new = ZendClassObject::::new(val); unsafe { zend_objects_clone_members(&raw mut new.std, object) }; let raw = new.into_raw(); - &raw mut raw.std + // 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 {}", @@ -104,7 +136,8 @@ impl ZendObjectHandlers { // free_obj handles uninitialized (None) objects gracefully. let empty = unsafe { ZendClassObject::::new_uninit(None) }; let raw = empty.into_raw(); - &raw mut raw.std + // SAFETY: `into_raw` yields a valid object the engine takes over. + unsafe { &raw mut (*raw).std } } } @@ -254,13 +287,26 @@ impl ZendObjectHandlers { unsafe extern "C" fn get_properties( object: *mut ZendObject, ) -> *mut ZendHashTable { - // Get the standard properties first (this works for all objects) - let props = unsafe { - zend_std_get_properties(object) - .as_mut() - .or_else(|| Some(ZendHashTable::new().into_raw())) - .expect("Failed to get property hashtable") - }; + // SAFETY: `zend_std_get_properties` builds `obj->properties` on demand + // and never returns null for a live object. Rust properties are merged + // into that table below, so it is separated first exactly like + // `zend_std_write_property` does: the engine hands the table out with an + // extra reference (`GC_TRY_ADDREF` in `zend_std_get_properties_for`) and + // writing through a shared table is a copy-on-write violation + // (`HT_ASSERT_RC1`). + let mut props = unsafe { zend_std_get_properties(object) }; + unsafe { + let ht = &*props; + if ht.is_immutable() { + props = zend_array_dup(props); + (*object).properties = props; + } else if ht.gc.refcount > 1 { + (*props).gc.refcount -= 1; + props = zend_array_dup(props); + (*object).properties = props; + } + } + let props = unsafe { &mut *props }; // If the object doesn't have a valid Rust backing (e.g., a mock or subclass // that didn't call the parent constructor), just return standard properties diff --git a/src/zend/ini_entry_def.rs b/src/zend/ini_entry_def.rs index 049f37fabd..e6fdf18a98 100644 --- a/src/zend/ini_entry_def.rs +++ b/src/zend/ini_entry_def.rs @@ -66,13 +66,6 @@ impl IniEntryDef { } } - /// Converts the ini entry into a raw and pointer, releasing it to the - /// C world. - #[must_use] - pub fn into_raw(self) -> *mut Self { - Box::into_raw(Box::new(self)) - } - /// Registers a list of ini entries. pub fn register(mut entries: Vec, module_number: i32) { entries.push(Self::end()); diff --git a/src/zend/module.rs b/src/zend/module.rs index 8eff7b3391..5d8293690e 100644 --- a/src/zend/module.rs +++ b/src/zend/module.rs @@ -23,16 +23,6 @@ fn zend_type_has_name(type_mask: u32) -> bool { /// A Zend module entry, also known as an extension. pub type ModuleEntry = zend_module_entry; -impl ModuleEntry { - /// Allocates the module entry on the heap, returning a pointer to the - /// memory location. The caller is responsible for the memory pointed to. - #[deprecated(note = "use StaticModuleEntry to avoid leaking the allocation")] - #[must_use] - pub fn into_raw(self) -> *mut Self { - Box::into_raw(Box::new(self)) - } -} - /// Static storage for a [`ModuleEntry`] that avoids heap allocation. /// /// Mimics how C extensions declare a `static zend_module_entry`. The entry diff --git a/tests/sapi.rs b/tests/sapi.rs index fe59f6b571..f9954609b4 100644 --- a/tests/sapi.rs +++ b/tests/sapi.rs @@ -62,7 +62,8 @@ fn test_sapi() { let mut builder = SapiBuilder::new("test", "Test"); builder = builder.ub_write_function(output_tester); - let sapi = builder.build().unwrap().into_raw(); + let mut sapi = builder.build().unwrap(); + let sapi = &raw mut sapi; let module = get_module(); unsafe { @@ -114,7 +115,6 @@ fn test_sapi() { unsafe { ext_php_rs_sapi_shutdown(); cleanup_sapi_allocations(sapi); - drop(Box::from_raw(sapi)); } } @@ -141,7 +141,8 @@ fn test_sapi_multithread() { let mut builder = SapiBuilder::new("test-mt", "Test Multi-threaded"); builder = builder.ub_write_function(output_tester); - let sapi = builder.build().unwrap().into_raw(); + let mut sapi = builder.build().unwrap(); + let sapi = &raw mut sapi; let module = get_module(); unsafe { @@ -224,7 +225,6 @@ fn test_sapi_multithread() { unsafe { ext_php_rs_sapi_shutdown(); cleanup_sapi_allocations(sapi); - drop(Box::from_raw(sapi)); } } @@ -327,7 +327,8 @@ fn test_php_thread_guard_drop() { let mut builder = SapiBuilder::new("test-guard", "Test Guard"); builder = builder.ub_write_function(output_tester); - let sapi = builder.build().unwrap().into_raw(); + let mut sapi = builder.build().unwrap(); + let sapi = &raw mut sapi; let module = get_module(); unsafe { @@ -370,7 +371,6 @@ fn test_php_thread_guard_drop() { unsafe { ext_php_rs_sapi_shutdown(); cleanup_sapi_allocations(sapi); - drop(Box::from_raw(sapi)); } } @@ -382,7 +382,8 @@ fn test_server_var_registrar() { .ub_write_function(output_tester) .register_server_variables_function(register_vars); - let sapi = builder.build().unwrap().into_raw(); + let mut sapi = builder.build().unwrap(); + let sapi = &raw mut sapi; let module = get_module(); unsafe { @@ -424,7 +425,6 @@ fn test_server_var_registrar() { unsafe { ext_php_rs_sapi_shutdown(); cleanup_sapi_allocations(sapi); - drop(Box::from_raw(sapi)); } } @@ -432,7 +432,8 @@ fn test_server_var_registrar() { fn test_sapi_trait_lifecycle() { let _guard = SAPI_TEST_MUTEX.lock().unwrap(); - let sapi = TestSapi::build_module().unwrap().into_raw(); + let mut sapi = TestSapi::build_module().unwrap(); + let sapi = &raw mut sapi; let module = get_module(); unsafe { @@ -468,7 +469,6 @@ fn test_sapi_trait_lifecycle() { unsafe { ext_php_rs_sapi_shutdown(); cleanup_sapi_allocations(sapi); - drop(Box::from_raw(sapi)); } } @@ -478,7 +478,8 @@ fn test_worker_request_cycle() { let mut builder = SapiBuilder::new("test-worker", "Test Worker"); builder = builder.ub_write_function(output_tester); - let sapi = builder.build().unwrap().into_raw(); + let mut sapi = builder.build().unwrap(); + let sapi = &raw mut sapi; let module = get_module(); unsafe { @@ -520,7 +521,6 @@ fn test_worker_request_cycle() { unsafe { ext_php_rs_sapi_shutdown(); cleanup_sapi_allocations(sapi); - drop(Box::from_raw(sapi)); } } @@ -529,7 +529,8 @@ fn test_worker_request_cycle() { fn test_full_sapi_worker_flow() { let _guard = SAPI_TEST_MUTEX.lock().unwrap(); - let sapi = TestSapi::build_module().unwrap().into_raw(); + let mut sapi = TestSapi::build_module().unwrap(); + let sapi = &raw mut sapi; let module = get_module(); unsafe { @@ -593,7 +594,6 @@ fn test_full_sapi_worker_flow() { unsafe { ext_php_rs_sapi_shutdown(); cleanup_sapi_allocations(sapi); - drop(Box::from_raw(sapi)); } } @@ -602,7 +602,8 @@ fn test_full_sapi_worker_flow() { fn test_sapi_trait_captures_headers() { let _guard = SAPI_TEST_MUTEX.lock().unwrap(); - let sapi = TestSapi::build_module().unwrap().into_raw(); + let mut sapi = TestSapi::build_module().unwrap(); + let sapi = &raw mut sapi; let module = get_module(); unsafe { @@ -661,6 +662,5 @@ fn test_sapi_trait_captures_headers() { unsafe { ext_php_rs_sapi_shutdown(); cleanup_sapi_allocations(sapi); - drop(Box::from_raw(sapi)); } } diff --git a/tests/src/integration/binary_slice/binary_slice.php b/tests/src/integration/binary_slice/binary_slice.php new file mode 100644 index 0000000000..f4746ab98a --- /dev/null +++ b/tests/src/integration/binary_slice/binary_slice.php @@ -0,0 +1,14 @@ +) -> u64 { + values.iter().sum() +} + +#[php_function] +pub fn test_binary_slice_len(values: BinarySlice) -> usize { + values.len() +} + +pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { + builder + .function(wrap_function!(test_binary_slice_sum)) + .function(wrap_function!(test_binary_slice_len)) +} + +#[cfg(test)] +mod tests { + #[test] + fn binary_slice_works() { + assert!(crate::integration::test::run_php( + "binary_slice/binary_slice.php" + )); + } +} diff --git a/tests/src/integration/class/class.php b/tests/src/integration/class/class.php index 8a9c3c775d..b4e17c9f2b 100644 --- a/tests/src/integration/class/class.php +++ b/tests/src/integration/class/class.php @@ -15,6 +15,9 @@ $class->selfMultiRef('bar'); assert($class->string === 'Changed to bar'); +gc_collect_cycles(); +assert($class->string === 'Changed to bar', 'GC scan of an object returned through &mut Self must not trip HT_ASSERT_RC1'); + // Test method returning Self (new instance) $newClass = $class->withString('new string'); assert($newClass instanceof TestClass, 'withString should return TestClass instance'); @@ -169,6 +172,32 @@ $selfRef = $builder2->getSelf(); assert($selfRef === $builder2, 'getSelf should return $this'); +$builder3 = new FluentBuilder(); +$builder3->setValue(7)->setName('seven'); +ob_start(); +debug_zval_dump($builder3); +$dump = ob_get_clean(); +assert(preg_match('/refcount\((\d+)\)\{/', $dump, $m) === 1); +assert( + (int) $m[1] === 2, + "\$builder3 refcount should be 2 (\$builder3 + debug_zval_dump's copy); a leak in " + . '&mut ZendClassObject::set_zval pushes it higher. Got: ' . $m[1] +); + +class SelfLinked extends TestClassExtendsWithProp +{ + public $self; +} +$linked = new SelfLinked(); +$linked->self = $linked; +ob_start(); +var_dump($linked); +$dump = ob_get_clean(); +assert(str_contains($dump, '*RECURSION*')); +assert(substr_count($dump, '["payload"]') === 1); +unset($linked); +assert(gc_collect_cycles() >= 1, 'a cycle through a Rust-backed object must be collected via get_gc'); + // Test readonly class (PHP 8.2+) if (PHP_VERSION_ID >= 80_200) { $readonlyObj = new TestReadonlyClass('hello', 42); diff --git a/tests/src/integration/mod.rs b/tests/src/integration/mod.rs index ee9533ed4c..685d86d87e 100644 --- a/tests/src/integration/mod.rs +++ b/tests/src/integration/mod.rs @@ -1,6 +1,7 @@ pub mod array; pub mod bailout; pub mod binary; +pub mod binary_slice; pub mod bool; pub mod callable; pub mod class; diff --git a/tests/src/lib.rs b/tests/src/lib.rs index 7b7f09f3c7..dc1519de67 100644 --- a/tests/src/lib.rs +++ b/tests/src/lib.rs @@ -14,6 +14,7 @@ pub fn build_module(module: ModuleBuilder) -> ModuleBuilder { let mut module = integration::array::build_module(module); module = integration::bailout::build_module(module); module = integration::binary::build_module(module); + module = integration::binary_slice::build_module(module); module = integration::bool::build_module(module); module = integration::callable::build_module(module); module = integration::class::build_module(module); From 83ca17a948b574dea17b913e77ae0263620e5caf Mon Sep 17 00:00:00 2001 From: Pierre Tondereau Date: Sun, 6 Sep 2026 20:52:16 +0200 Subject: [PATCH 2/4] style(tests): format class.php with mago --- tests/src/integration/class/class.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/src/integration/class/class.php b/tests/src/integration/class/class.php index b4e17c9f2b..17b78a0ba8 100644 --- a/tests/src/integration/class/class.php +++ b/tests/src/integration/class/class.php @@ -16,7 +16,10 @@ assert($class->string === 'Changed to bar'); gc_collect_cycles(); -assert($class->string === 'Changed to bar', 'GC scan of an object returned through &mut Self must not trip HT_ASSERT_RC1'); +assert( + $class->string === 'Changed to bar', + 'GC scan of an object returned through &mut Self must not trip HT_ASSERT_RC1' +); // Test method returning Self (new instance) $newClass = $class->withString('new string'); @@ -181,13 +184,15 @@ assert( (int) $m[1] === 2, "\$builder3 refcount should be 2 (\$builder3 + debug_zval_dump's copy); a leak in " - . '&mut ZendClassObject::set_zval pushes it higher. Got: ' . $m[1] + . '&mut ZendClassObject::set_zval pushes it higher. Got: ' + . $m[1] ); class SelfLinked extends TestClassExtendsWithProp { public $self; } + $linked = new SelfLinked(); $linked->self = $linked; ob_start(); From 5b089bc3f2c8f7fe3af809e6becb9330a72a41b5 Mon Sep 17 00:00:00 2001 From: Pierre Tondereau Date: Sun, 6 Sep 2026 20:53:23 +0200 Subject: [PATCH 3/4] build: bump mago to 1.47.6 --- .github/workflows/build.yml | 2 +- flake.nix | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 742e2b8e24..65620e8173 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -32,7 +32,7 @@ jobs: - name: Setup Mago uses: nhedger/setup-mago@v2 with: - version: 1.45.0 + version: 1.47.6 - name: Check PHP formatting run: mago format --check - name: Lint PHP diff --git a/flake.nix b/flake.nix index 0c2c51f680..96998b2e67 100644 --- a/flake.nix +++ b/flake.nix @@ -56,10 +56,10 @@ # local dev and CI (nhedger/setup-mago) run the exact same version. mago = pkgs.stdenvNoCC.mkDerivation rec { pname = "mago"; - version = "1.45.0"; + version = "1.47.6"; src = pkgs.fetchurl { url = "https://github.com/carthage-software/mago/releases/download/${version}/mago-${version}-x86_64-unknown-linux-musl.tar.gz"; - hash = "sha256-aNsEDrmx3uGPvf9iTBN1TPdM0z58W2CZHh/jX9mxkNE="; + hash = "sha256-1A7DxEHspUvhsbpU/hdTD41CTS3CP4x8puptppq25Zg="; }; installPhase = '' runHook preInstall From 2d4718543c00100b30548a0228f54a5debe437de Mon Sep 17 00:00:00 2001 From: Pierre Tondereau Date: Sun, 6 Sep 2026 21:18:27 +0200 Subject: [PATCH 4/4] fix(handlers): drop property-table separation, guard null table --- src/zend/handlers.rs | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/src/zend/handlers.rs b/src/zend/handlers.rs index 27db03b5bc..af6240085b 100644 --- a/src/zend/handlers.rs +++ b/src/zend/handlers.rs @@ -4,7 +4,7 @@ use crate::{ class::RegisteredClass, exception::PhpResult, ffi::{ - ext_php_rs_executor_globals, instanceof_function_slow, std_object_handlers, zend_array_dup, + ext_php_rs_executor_globals, instanceof_function_slow, std_object_handlers, zend_class_entry, zend_is_true, zend_object_handlers, zend_object_std_dtor, zend_objects_clone_members, zend_std_get_properties, zend_std_has_property, zend_std_read_property, zend_std_write_property, zend_throw_error, @@ -69,6 +69,11 @@ impl ZendObjectHandlers { /// walks. Rust-backed properties are produced by getters and own no zvals /// of their own, so the collector only needs the engine-owned storage: /// this is the standard-object branch of `zend_std_get_gc`. + /// + /// Not reported, as before this handler existed: zvals held inside the Rust + /// struct itself, and the initializer or proxy instance of a lazy object + /// (`zend_lazy_object_get_gc` is not exported). Cycles through those leak + /// instead of being collected. unsafe extern "C" fn get_gc( object: *mut ZendObject, table: *mut *mut Zval, @@ -287,25 +292,17 @@ impl ZendObjectHandlers { unsafe extern "C" fn get_properties( object: *mut ZendObject, ) -> *mut ZendHashTable { - // SAFETY: `zend_std_get_properties` builds `obj->properties` on demand - // and never returns null for a live object. Rust properties are merged - // into that table below, so it is separated first exactly like - // `zend_std_write_property` does: the engine hands the table out with an - // extra reference (`GC_TRY_ADDREF` in `zend_std_get_properties_for`) and - // writing through a shared table is a copy-on-write violation - // (`HT_ASSERT_RC1`). - let mut props = unsafe { zend_std_get_properties(object) }; - unsafe { - let ht = &*props; - if ht.is_immutable() { - props = zend_array_dup(props); - (*object).properties = props; - } else if ht.gc.refcount > 1 { - (*props).gc.refcount -= 1; - props = zend_array_dup(props); - (*object).properties = props; - } + let props = unsafe { zend_std_get_properties(object) }; + if props.is_null() { + return props; } + // SAFETY: non-null, and owned by the object with a single reference: the + // engine only shares a property table for objects without declared + // properties on standard handlers (`zend_proptable_to_symtable` fast + // paths), and the collector no longer reaches this handler since `get_gc` + // is installed. Separating here with `zend_array_dup` would be wrong: it + // resolves the `IS_INDIRECT` slots of declared properties into a detached + // snapshot. let props = unsafe { &mut *props }; // If the object doesn't have a valid Rust backing (e.g., a mock or subclass