From 0f8a7f4a6d8c9424e50559dd93ad7236e3a1964e Mon Sep 17 00:00:00 2001 From: David Cole Date: Sun, 3 Oct 2021 20:13:38 +1300 Subject: [PATCH 1/6] Added `ZBox` and `ZBoxable` --- Cargo.toml | 1 - example/skel/Cargo.toml | 2 +- src/php/boxed.rs | 106 ++++++++++++++++++++++++++++++++++++++++ src/php/mod.rs | 4 +- 4 files changed, 108 insertions(+), 5 deletions(-) create mode 100644 src/php/boxed.rs diff --git a/Cargo.toml b/Cargo.toml index 14bbd2f6f6..2b63197971 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,6 @@ regex = "1" cc = "1.0" [features] -alloc = [] closure = [] [workspace] diff --git a/example/skel/Cargo.toml b/example/skel/Cargo.toml index 42b0d6afa0..426e778e52 100644 --- a/example/skel/Cargo.toml +++ b/example/skel/Cargo.toml @@ -7,7 +7,7 @@ edition = "2018" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -ext-php-rs = { path = "../../", features = ["alloc", "closure"] } +ext-php-rs = { path = "../../", features = ["closure"] } [lib] name = "skel" diff --git a/src/php/boxed.rs b/src/php/boxed.rs new file mode 100644 index 0000000000..2433ca924a --- /dev/null +++ b/src/php/boxed.rs @@ -0,0 +1,106 @@ +//! A pointer type for heap allocation using the Zend memory manager. +//! +//! Heap memory in PHP is usually request-bound and allocated inside [memory arenas], which are cleared +//! at the start and end of a PHP request. Allocating and freeing memory that is allocated on the Zend +//! heap is done through two separate functions [`efree`] and [`emalloc`]. +//! +//! As such, most heap-allocated PHP types **cannot** be allocated on the stack, such as [`ZendStr`], which +//! is a dynamically-sized type, and therefore must be allocated on the heap. A regular [`Box`] would not +//! work in this case, as the memory needs to be freed from a separate function `zend_string_release`. The +//! [`ZBox`] type provides a wrapper which calls the relevant release functions based on the type and what is +//! inside the implementation of [`ZBoxable`]. +//! +//! This type is not created directly, but rather through a function implemented on the downstream type. For +//! example, [`ZendStr`] has a function `new` which returns a [`ZBox`]. +//! +//! [memory arenas]: https://en.wikipedia.org/wiki/Region-based_memory_management +//! [`ZendStr`]: super::types::string::ZendStr +//! [`emalloc`]: super::alloc::efree + +use std::{ + fmt::Debug, + mem::ManuallyDrop, + ops::{Deref, DerefMut}, + ptr::NonNull, +}; + +use super::alloc::efree; + +/// A pointer type for heap allocation using the Zend memory manager. +/// +/// See the [module level documentation](../index.html) for more. +pub struct ZBox(NonNull); + +impl ZBox { + /// Creates a new box from a given pointer. + /// + /// # Parameters + /// + /// * `ptr` - A non-null, well-aligned pointer to a `T`. + /// + /// # Safety + /// + /// Caller must ensure that `ptr` is non-null, well-aligned and pointing to a `T`. + pub unsafe fn from_raw(ptr: *mut T) -> Self { + Self(NonNull::new_unchecked(ptr)) + } + + /// Returns the pointer contained by the box, dropping the box in the 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. + pub fn into_raw(self) -> *mut T { + let this = ManuallyDrop::new(self); + this.0.as_ptr() + } +} + +impl Drop for ZBox { + fn drop(&mut self) { + self.deref_mut().free() + } +} + +impl Deref for ZBox { + type Target = T; + + fn deref(&self) -> &Self::Target { + // SAFETY: All constructors ensure the contained pointer is well-aligned and dereferencable. + unsafe { self.0.as_ref() } + } +} + +impl DerefMut for ZBox { + fn deref_mut(&mut self) -> &mut Self::Target { + // SAFETY: All constructors ensure the contained pointer is well-aligned and dereferencable. + unsafe { self.0.as_mut() } + } +} + +impl Debug for ZBox { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.deref().fmt(f) + } +} + +/// Implemented on types that can be heap allocated using the Zend memory manager. These types are stored +/// inside a [`ZBox`] when heap-allocated, and the [`free`] method is called when the box is dropped. +/// +/// # Safety +/// +/// The default implementation of the [`free`] function uses the [`efree`] function to free the memory without +/// calling any destructors. +/// +/// The implementor must ensure that any time a pointer to the implementor is passed into a [`ZBox`] that the +/// memory pointed to was allocated by the Zend memory manager. +/// +/// [`free`]: #method.free +pub unsafe trait ZBoxable { + /// Frees the memory pointed to by `self`, calling any destructors required in the process. + fn free(&mut self) { + unsafe { efree(self as *mut _ as *mut u8) }; + } +} diff --git a/src/php/mod.rs b/src/php/mod.rs index da5c82faa9..0c8f8b7f75 100644 --- a/src/php/mod.rs +++ b/src/php/mod.rs @@ -1,10 +1,8 @@ //! Objects relating to PHP and the Zend engine. -#[cfg(any(docs, feature = "alloc"))] -#[cfg_attr(docs, doc(cfg(feature = "alloc")))] pub mod alloc; - pub mod args; +pub mod boxed; pub mod class; pub mod constants; pub mod enums; From 54c3f797e56744f5aed9afac20621bf6e42010cc Mon Sep 17 00:00:00 2001 From: David Cole Date: Sun, 3 Oct 2021 20:30:57 +1300 Subject: [PATCH 2/6] Implement `ZBoxable` for `ZendStr` Build for all f eatures on CI --- .github/workflows/build.yml | 2 +- src/php/boxed.rs | 19 +++ src/php/class.rs | 10 +- src/php/pack.rs | 7 +- src/php/types/object.rs | 15 ++- src/php/types/string.rs | 257 ++++++++++++++---------------------- src/php/types/zval.rs | 16 +-- 7 files changed, 138 insertions(+), 188 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b57b5554ab..86759526e3 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -46,7 +46,7 @@ jobs: env: LIBCLANG_PATH: ${{ runner.temp }}/llvm-${{ matrix.llvm }}/lib EXT_PHP_RS_TEST: - run: cargo build --release --features alloc,closure + run: cargo build --release --all-features - name: Test guide examples env: CARGO_PKG_NAME: mdbook-tests diff --git a/src/php/boxed.rs b/src/php/boxed.rs index 2433ca924a..e50978cbec 100644 --- a/src/php/boxed.rs +++ b/src/php/boxed.rs @@ -18,6 +18,7 @@ //! [`emalloc`]: super::alloc::efree use std::{ + borrow::Borrow, fmt::Debug, mem::ManuallyDrop, ops::{Deref, DerefMut}, @@ -59,6 +60,7 @@ impl ZBox { } impl Drop for ZBox { + #[inline] fn drop(&mut self) { self.deref_mut().free() } @@ -67,6 +69,7 @@ impl Drop for ZBox { impl Deref for ZBox { type Target = T; + #[inline] fn deref(&self) -> &Self::Target { // SAFETY: All constructors ensure the contained pointer is well-aligned and dereferencable. unsafe { self.0.as_ref() } @@ -74,6 +77,7 @@ impl Deref for ZBox { } impl DerefMut for ZBox { + #[inline] fn deref_mut(&mut self) -> &mut Self::Target { // SAFETY: All constructors ensure the contained pointer is well-aligned and dereferencable. unsafe { self.0.as_mut() } @@ -81,11 +85,26 @@ impl DerefMut for ZBox { } impl Debug for ZBox { + #[inline] fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { self.deref().fmt(f) } } +impl Borrow for ZBox { + #[inline] + fn borrow(&self) -> &T { + self.deref() + } +} + +impl AsRef for ZBox { + #[inline] + fn as_ref(&self) -> &T { + self + } +} + /// Implemented on types that can be heap allocated using the Zend memory manager. These types are stored /// inside a [`ZBox`] when heap-allocated, and the [`free`] method is called when the box is dropped. /// diff --git a/src/php/class.rs b/src/php/class.rs index 7b100826dd..12824a7708 100644 --- a/src/php/class.rs +++ b/src/php/class.rs @@ -9,7 +9,7 @@ use crate::{ types::object::{ClassObject, ConstructorMeta, ConstructorResult, ZendObject}, }, }; -use std::{alloc::Layout, convert::TryInto, ffi::CString, fmt::Debug}; +use std::{alloc::Layout, convert::TryInto, ffi::CString, fmt::Debug, ops::DerefMut}; use crate::bindings::{ zend_class_entry, zend_declare_class_constant, zend_declare_property, @@ -22,7 +22,7 @@ use super::{ globals::ExecutorGlobals, types::{ object::RegisteredClass, - string::ZendString, + string::ZendStr, zval::{IntoZval, Zval}, }, }; @@ -43,10 +43,10 @@ impl ClassEntry { /// not be found or the class table has not been initialized. pub fn try_find(name: &str) -> Option<&'static Self> { ExecutorGlobals::get().class_table()?; - let mut name = ZendString::new(name, false).ok()?; + let mut name = ZendStr::new(name, false).ok()?; unsafe { - crate::bindings::zend_lookup_class_ex(name.as_mut_zend_str(), std::ptr::null_mut(), 0) + crate::bindings::zend_lookup_class_ex(name.deref_mut(), std::ptr::null_mut(), 0) .as_ref() } } @@ -337,7 +337,7 @@ impl ClassBuilder { /// /// Returns an [`Error`] variant if the class could not be registered. pub fn build(mut self) -> Result<&'static mut ClassEntry> { - self.ptr.name = ZendString::new_interned(&self.name, true)?.into_inner(); + self.ptr.name = ZendStr::new_interned(&self.name, true)?.into_raw(); self.methods.push(FunctionEntry::end()); let func = Box::into_raw(self.methods.into_boxed_slice()) as *const FunctionEntry; diff --git a/src/php/pack.rs b/src/php/pack.rs index df5620f277..1572ac067b 100644 --- a/src/php/pack.rs +++ b/src/php/pack.rs @@ -18,18 +18,13 @@ use crate::bindings::{ext_php_rs_zend_string_init, zend_string}; /// [`unpack`]: https://www.php.net/manual/en/function.unpack.php pub unsafe trait Pack: Clone { /// Packs a given vector into a Zend binary string. Can be passed to PHP and then unpacked - /// using the [`unpack`] function. Note you should probably use the [`set_binary`] method on the - /// [`Zval`] struct instead of this function directly, as there is currently no way to set a - /// [`ZendString`] on a [`Zval`] directly. + /// using the [`unpack`] function. /// /// # Parameters /// /// * `vec` - The vector to pack into a binary string. /// /// [`unpack`]: https://www.php.net/manual/en/function.unpack.php - /// [`Zval`]: crate::php::types::zval::Zval - /// [`ZendString`]: crate::php::types::string::ZendString - /// [`set_binary`]: crate::php::types::zval::Zval#method.set_binary fn pack_into(vec: Vec) -> *mut zend_string; /// Unpacks a given Zend binary string into a Rust vector. Can be used to pass data from `pack` diff --git a/src/php/types/object.rs b/src/php/types/object.rs index ee148a0932..a36c7300f9 100644 --- a/src/php/types/object.rs +++ b/src/php/types/object.rs @@ -33,13 +33,14 @@ use crate::{ flags::ZvalTypeFlags, function::FunctionBuilder, globals::ExecutorGlobals, - types::{array::OwnedHashTable, string::ZendString}, + types::array::OwnedHashTable, }, }; use super::{ props::Property, rc::PhpRc, + string::ZendStr, zval::{FromZval, IntoZval, Zval}, }; @@ -184,13 +185,13 @@ impl ZendObject { return Err(Error::InvalidProperty); } - let mut name = ZendString::new(name, false)?; + let mut name = ZendStr::new(name, false)?; let mut rv = Zval::new(); let zv = unsafe { self.handlers()?.read_property.ok_or(Error::InvalidScope)?( self.mut_ptr(), - name.as_mut_zend_str(), + name.deref_mut(), 1, std::ptr::null_mut(), &mut rv, @@ -209,13 +210,13 @@ impl ZendObject { /// * `name` - The name of the property. /// * `value` - The value to set the property to. pub fn set_property(&mut self, name: &str, value: impl IntoZval) -> Result<()> { - let mut name = ZendString::new(name, false)?; + let mut name = ZendStr::new(name, false)?; let mut value = value.into_zval(false)?; unsafe { self.handlers()?.write_property.ok_or(Error::InvalidScope)?( self, - name.as_mut_zend_str(), + name.deref_mut(), &mut value, std::ptr::null_mut(), ) @@ -234,12 +235,12 @@ impl ZendObject { /// * `name` - The name of the property. /// * `query` - The 'query' to classify if a property exists. pub fn has_property(&self, name: &str, query: PropertyQuery) -> Result { - let mut name = ZendString::new(name, false)?; + let mut name = ZendStr::new(name, false)?; Ok(unsafe { self.handlers()?.has_property.ok_or(Error::InvalidScope)?( self.mut_ptr(), - name.as_mut_zend_str(), + name.deref_mut(), query as _, std::ptr::null_mut(), ) diff --git a/src/php/types/string.rs b/src/php/types/string.rs index 8c5e7f75c7..b44403417e 100644 --- a/src/php/types/string.rs +++ b/src/php/types/string.rs @@ -2,13 +2,10 @@ //! contains the length of the string, meaning the string can contain the NUL character. use std::{ - borrow::{Borrow, Cow}, + borrow::Cow, convert::TryFrom, ffi::{CStr, CString}, fmt::Debug, - mem::ManuallyDrop, - ops::Deref, - ptr::NonNull, slice, }; @@ -23,117 +20,28 @@ use crate::{ zend_string_init_interned, }, errors::{Error, Result}, + php::boxed::{ZBox, ZBoxable}, }; /// A borrowed Zend-string. /// /// Although this object does implement [`Sized`], it is in fact not sized. As C cannot represent unsized /// types, an array of size 1 is used at the end of the type to represent the contents of the string, therefore -/// this type is actually unsized and has no valid constructors. See the owned variant [`ZendString`] to -/// create an owned version of a [`ZendStr`]. +/// this type is actually unsized. All constructors return [`ZBox`], the owned varaint. /// /// Once the `ptr_metadata` feature lands in stable rust, this type can potentially be changed to a DST using /// slices and metadata. See the tracking issue here: pub type ZendStr = zend_string; -// Clippy complains about there being no `is_empty` function when implementing on the alias `ZendStr` :( -// -#[allow(clippy::len_without_is_empty)] -impl ZendStr { - /// Returns the length of the string. - pub fn len(&self) -> usize { - self.len as usize - } - - /// Returns true if the string is empty, false otherwise. - pub fn is_empty(&self) -> bool { - self.len() == 0 - } - - /// Returns a reference to the underlying [`CStr`] inside the Zend string. - pub fn as_c_str(&self) -> &CStr { - // SAFETY: Zend strings store their readable length in a fat pointer. - unsafe { - let slice = slice::from_raw_parts(self.val.as_ptr() as *const u8, self.len() + 1); - CStr::from_bytes_with_nul_unchecked(slice) - } - } - - /// Attempts to return a reference to the underlying [`str`] inside the Zend string. - /// - /// Returns the [`None`] variant if the [`CStr`] contains non-UTF-8 characters. - pub fn as_str(&self) -> Option<&str> { - self.as_c_str().to_str().ok() - } -} - -impl Debug for ZendStr { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.as_c_str().fmt(f) - } -} - -impl ToOwned for ZendStr { - type Owned = ZendString; - - fn to_owned(&self) -> Self::Owned { - Self::Owned::from_c_str(self.as_c_str(), false) - } -} - -impl PartialEq for ZendStr { - #[inline] - fn eq(&self, other: &Self) -> bool { - self.as_c_str().eq(other.as_c_str()) - } -} - -impl<'a> From<&'a ZendStr> for &'a CStr { - fn from(value: &'a ZendStr) -> Self { - value.as_c_str() - } -} - -impl<'a> TryFrom<&'a ZendStr> for &'a str { - type Error = Error; - - fn try_from(value: &'a ZendStr) -> Result { - value.as_str().ok_or(Error::InvalidCString) - } -} - -impl<'a> TryFrom<&ZendStr> for String { - type Error = Error; - - fn try_from(value: &ZendStr) -> Result { - value - .as_str() - .map(|s| s.to_string()) - .ok_or(Error::InvalidCString) - } -} - -impl<'a> From<&'a ZendStr> for Cow<'a, ZendStr> { - fn from(value: &'a ZendStr) -> Self { - Cow::Borrowed(value) - } -} - -/// A type representing an owned Zend string, commonly used throughout the PHP API. -/// -/// The type contains an inner pointer to a [`ZendStr`], which is the DST that contains the contents -/// of the string. This type simply provides the required functions to handle the creation and deletion -/// of the internal string. -pub struct ZendString { - inner: NonNull, -} - // Adding to the Zend interned string hashtable is not atomic and can be contested when PHP is compiled with ZTS, // so an empty mutex is used to ensure no collisions occur on the Rust side. Not much we can do about collisions // on the PHP side. static INTERNED_LOCK: Mutex = Mutex::const_new(RawMutex::INIT, ()); -impl ZendString { +// Clippy complains about there being no `is_empty` function when implementing on the alias `ZendStr` :( +// +#[allow(clippy::len_without_is_empty)] +impl ZendStr { /// Creates a new Zend string from a [`str`]. /// /// # Parameters @@ -149,7 +57,7 @@ impl ZendString { /// # Panics /// /// Panics if the function was unable to allocate memory for the Zend string. - pub fn new(str: &str, persistent: bool) -> Result { + pub fn new(str: &str, persistent: bool) -> Result> { Ok(Self::from_c_str(&CString::new(str)?, persistent)) } @@ -163,13 +71,15 @@ impl ZendString { /// # Panics /// /// Panics if the function was unable to allocate memory for the Zend string. - pub fn from_c_str(str: &CStr, persistent: bool) -> Self { - let ptr = unsafe { - ext_php_rs_zend_string_init(str.as_ptr(), str.to_bytes().len() as _, persistent) - }; + pub fn from_c_str(str: &CStr, persistent: bool) -> ZBox { + unsafe { + let ptr = + ext_php_rs_zend_string_init(str.as_ptr(), str.to_bytes().len() as _, persistent); - Self { - inner: NonNull::new(ptr).expect("Failed to allocate for Zend string"), + ZBox::from_raw( + ptr.as_mut() + .expect("Failed to allocate memory for new Zend string"), + ) } } @@ -194,7 +104,7 @@ impl ZendString { /// # Panics /// /// Panics if the function was unable to allocate memory for the Zend string. - pub fn new_interned(str: &str, persistent: bool) -> Result { + pub fn new_interned(str: &str, persistent: bool) -> Result> { Ok(Self::interned_from_c_str(&CString::new(str)?, persistent)) } @@ -217,114 +127,143 @@ impl ZendString { /// /// * The function used to create interned strings has not been set. /// * The function could not allocate enough memory for the Zend string. - pub fn interned_from_c_str(str: &CStr, persistent: bool) -> Self { + pub fn interned_from_c_str(str: &CStr, persistent: bool) -> ZBox { let _lock = INTERNED_LOCK.lock(); - let ptr = unsafe { - zend_string_init_interned.expect("`zend_string_init_interned` not ready")( + + unsafe { + let ptr = zend_string_init_interned.expect("`zend_string_init_interned` not ready")( str.as_ptr(), str.to_bytes().len() as _, persistent, - ) - }; + ); - Self { - inner: NonNull::new(ptr).expect("Failed to allocate for Zend string"), + ZBox::from_raw( + ptr.as_mut() + .expect("Failed to allocate memory for new Zend string"), + ) } } - /// Returns a reference to the internal [`ZendStr`]. - pub fn as_zend_str(&self) -> &ZendStr { - // SAFETY: All constructors ensure a valid internal pointer. - unsafe { self.inner.as_ref() } + /// Returns the length of the string. + pub fn len(&self) -> usize { + self.len as usize + } + + /// Returns true if the string is empty, false otherwise. + pub fn is_empty(&self) -> bool { + self.len() == 0 } - /// Returns a reference to the internal [`ZendStr`]. - pub(crate) fn as_mut_zend_str(&mut self) -> &mut ZendStr { - // SAFETY: All constructors ensure a valid internal pointer. - unsafe { self.inner.as_mut() } + /// Returns a reference to the underlying [`CStr`] inside the Zend string. + pub fn as_c_str(&self) -> &CStr { + // SAFETY: Zend strings store their readable length in a fat pointer. + unsafe { + let slice = slice::from_raw_parts(self.val.as_ptr() as *const u8, self.len() + 1); + CStr::from_bytes_with_nul_unchecked(slice) + } } - /// Converts the owned Zend string into the internal pointer, bypassing the [`Drop`] - /// implementation. + /// Attempts to return a reference to the underlying [`str`] inside the Zend string. /// - /// The caller is responsible for freeing the resulting pointer using the `zend_string_release` - /// function. - pub fn into_inner(self) -> *mut ZendStr { - let this = ManuallyDrop::new(self); - this.inner.as_ptr() + /// Returns the [`None`] variant if the [`CStr`] contains non-UTF-8 characters. + pub fn as_str(&self) -> Option<&str> { + self.as_c_str().to_str().ok() } } -impl Drop for ZendString { - fn drop(&mut self) { - // SAFETY: All constructors ensure a valid internal pointer. - unsafe { ext_php_rs_zend_string_release(self.inner.as_ptr()) }; +unsafe impl ZBoxable for ZendStr { + fn free(&mut self) { + unsafe { ext_php_rs_zend_string_release(self) }; } } -impl Deref for ZendString { - type Target = ZendStr; - - fn deref(&self) -> &Self::Target { - self.as_zend_str() +impl Debug for ZendStr { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.as_c_str().fmt(f) } } -impl Debug for ZendString { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.as_zend_str().fmt(f) +impl ToOwned for ZendStr { + type Owned = ZBox; + + fn to_owned(&self) -> Self::Owned { + Self::from_c_str(self.as_c_str(), false) } } -impl Borrow for ZendString { +impl PartialEq for ZendStr { #[inline] - fn borrow(&self) -> &ZendStr { - self.deref() + fn eq(&self, other: &Self) -> bool { + self.as_c_str().eq(other.as_c_str()) } } -impl AsRef for ZendString { - #[inline] - fn as_ref(&self) -> &ZendStr { - self +impl<'a> From<&'a ZendStr> for &'a CStr { + fn from(value: &'a ZendStr) -> Self { + value.as_c_str() + } +} + +impl<'a> TryFrom<&'a ZendStr> for &'a str { + type Error = Error; + + fn try_from(value: &'a ZendStr) -> Result { + value.as_str().ok_or(Error::InvalidCString) + } +} + +impl<'a> TryFrom<&ZendStr> for String { + type Error = Error; + + fn try_from(value: &ZendStr) -> Result { + value + .as_str() + .map(|s| s.to_string()) + .ok_or(Error::InvalidCString) + } +} + +impl<'a> From<&'a ZendStr> for Cow<'a, ZendStr> { + fn from(value: &'a ZendStr) -> Self { + Cow::Borrowed(value) } } -impl From<&CStr> for ZendString { +impl From<&CStr> for ZBox { fn from(value: &CStr) -> Self { - Self::from_c_str(value, false) + ZendStr::from_c_str(value, false) } } -impl From for ZendString { +impl From for ZBox { fn from(value: CString) -> Self { - Self::from_c_str(&value, false) + ZendStr::from_c_str(&value, false) } } -impl TryFrom<&str> for ZendString { +impl TryFrom<&str> for ZBox { type Error = Error; fn try_from(value: &str) -> Result { - Self::new(value, false) + ZendStr::new(value, false) } } -impl TryFrom for ZendString { +impl TryFrom for ZBox { type Error = Error; fn try_from(value: String) -> Result { - Self::new(value.as_str(), false) + ZendStr::new(value.as_str(), false) } } -impl From for Cow<'_, ZendStr> { - fn from(value: ZendString) -> Self { +impl From> for Cow<'_, ZendStr> { + fn from(value: ZBox) -> Self { Cow::Owned(value) } } -impl From> for ZendString { +impl From> for ZBox { fn from(value: Cow<'_, ZendStr>) -> Self { value.into_owned() } diff --git a/src/php/types/zval.rs b/src/php/types/zval.rs index 3087ec85cb..7616db1ea9 100644 --- a/src/php/types/zval.rs +++ b/src/php/types/zval.rs @@ -14,14 +14,10 @@ use crate::{ zend_value, zval, zval_ptr_dtor, }, errors::{Error, Result}, - php::{exceptions::PhpException, pack::Pack}, + php::{boxed::ZBox, exceptions::PhpException, pack::Pack}, }; -use crate::php::{ - enums::DataType, - flags::ZvalTypeFlags, - types::{long::ZendLong, string::ZendString}, -}; +use crate::php::{enums::DataType, flags::ZvalTypeFlags, types::long::ZendLong}; use super::{ array::{HashTable, OwnedHashTable}, @@ -311,7 +307,7 @@ impl Zval { /// * `val` - The value to set the zval as. /// * `persistent` - Whether the string should persist between requests. pub fn set_string(&mut self, val: &str, persistent: bool) -> Result<()> { - self.set_zend_string(ZendString::new(val, persistent)?); + self.set_zend_string(ZendStr::new(val, persistent)?); Ok(()) } @@ -320,9 +316,9 @@ impl Zval { /// # Parameters /// /// * `val` - String content. - pub fn set_zend_string(&mut self, val: ZendString) { + pub fn set_zend_string(&mut self, val: ZBox) { self.change_type(ZvalTypeFlags::StringEx); - self.value.str_ = val.into_inner(); + self.value.str_ = val.into_raw(); } /// Sets the value of the zval as a binary string, which is represented in Rust as a vector. @@ -343,7 +339,7 @@ impl Zval { /// * `val` - The value to set the zval as. /// * `persistent` - Whether the string should persist between requests. pub fn set_interned_string(&mut self, val: &str, persistent: bool) -> Result<()> { - self.set_zend_string(ZendString::new_interned(val, persistent)?); + self.set_zend_string(ZendStr::new_interned(val, persistent)?); Ok(()) } From 550b980bf2410b9dd942c85e765f5d837250edcb Mon Sep 17 00:00:00 2001 From: David Cole Date: Mon, 4 Oct 2021 01:02:53 +1300 Subject: [PATCH 3/6] Replace `OwnedZendObject` with `ZBox` --- src/php/boxed.rs | 7 +- src/php/execution_data.rs | 13 ++- src/php/types/object.rs | 168 +++++++++++++++----------------------- 3 files changed, 80 insertions(+), 108 deletions(-) diff --git a/src/php/boxed.rs b/src/php/boxed.rs index e50978cbec..2361446c00 100644 --- a/src/php/boxed.rs +++ b/src/php/boxed.rs @@ -53,9 +53,10 @@ impl ZBox { /// /// The caller is responsible for managing the memory pointed to by the returned pointer, including /// freeing the memory. - pub fn into_raw(self) -> *mut T { - let this = ManuallyDrop::new(self); - this.0.as_ptr() + 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 dereferencable. + unsafe { this.0.as_mut() } } } diff --git a/src/php/execution_data.rs b/src/php/execution_data.rs index 30cc7a035f..ee7402459a 100644 --- a/src/php/execution_data.rs +++ b/src/php/execution_data.rs @@ -3,15 +3,22 @@ use crate::bindings::{zend_execute_data, ZEND_MM_ALIGNMENT, ZEND_MM_ALIGNMENT_MASK}; -use super::types::{ - object::{RegisteredClass, ZendClassObject, ZendObject}, - zval::Zval, +use super::{ + args::ArgParser, + types::{ + object::{RegisteredClass, ZendClassObject, ZendObject}, + zval::Zval, + }, }; /// Execution data passed when a function is called from Zend. pub type ExecutionData = zend_execute_data; impl ExecutionData { + pub fn parser(&mut self) -> (Option<&mut ZendClassObject>, ArgParser) { + (self.get_object(), ArgParser::new(self)) + } + /// Attempts to retrieve a reference to the underlying class object of the Zend object. /// /// Returns a [`ZendClassObject`] if the execution data contained a valid object of type `T`, diff --git a/src/php/types/object.rs b/src/php/types/object.rs index a36c7300f9..46ce44315c 100644 --- a/src/php/types/object.rs +++ b/src/php/types/object.rs @@ -26,6 +26,7 @@ use crate::{ }, errors::{Error, Result}, php::{ + boxed::{ZBox, ZBoxable}, class::ClassEntry, enums::DataType, exceptions::{PhpException, PhpResult}, @@ -44,14 +45,22 @@ use super::{ zval::{FromZval, IntoZval, Zval}, }; -/// A wrapper around [`ZendObject`] providing the correct [`Drop`] implementation required to not -/// leak memory. Dereferences to [`ZendObject`]. -/// -/// This type differs from [`ClassObject`] in the fact that this type is not aware of any Rust type attached -/// to the head of the [`ZendObject`]. It is possible to convert from a [`ClassObject`] to this type. -pub struct OwnedZendObject(NonNull); +pub type ZendObject = zend_object; +pub type ZendObjectHandlers = zend_object_handlers; + +/// Different ways to query if a property exists. +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +#[repr(u32)] +pub enum PropertyQuery { + /// Property exists and is not NULL. + Isset = ZEND_PROPERTY_ISSET, + /// Property is not empty. + NotEmpty = ZEND_ISEMPTY, + /// Property exists. + Exists = ZEND_PROPERTY_EXISTS, +} -impl OwnedZendObject { +impl ZendObject { /// Creates a new [`ZendObject`], returned inside an [`OwnedZendObject`] wrapper. /// /// # Parameters @@ -61,11 +70,16 @@ impl OwnedZendObject { /// # Panics /// /// Panics when allocating memory for the new object fails. - pub fn new(ce: &ClassEntry) -> Self { + pub fn new(ce: &ClassEntry) -> ZBox { // SAFETY: Using emalloc to allocate memory inside Zend arena. Casting `ce` to `*mut` is valid // as the function will not mutate `ce`. - let ptr = unsafe { zend_objects_new(ce as *const _ as *mut _) }; - Self(NonNull::new(ptr).expect("Failed to allocate Zend object")) + unsafe { + let ptr = zend_objects_new(ce as *const _ as *mut _); + ZBox::from_raw( + ptr.as_mut() + .expect("Failed to allocate memory for Zend object"), + ) + } } /// Creates a new `stdClass` instance, returned inside an [`OwnedZendObject`] wrapper. @@ -74,7 +88,7 @@ impl OwnedZendObject { /// /// Panics if allocating memory for the object fails, or if the `stdClass` class entry has not been /// registered with PHP yet. - pub fn new_stdclass() -> Self { + pub fn new_stdclass() -> ZBox { // SAFETY: This will be `NULL` until it is initialized. `as_ref()` checks for null, // so we can panic if it's null. Self::new(unsafe { @@ -84,75 +98,6 @@ impl OwnedZendObject { }) } - /// Consumes the [`OwnedZendObject`] wrapper, returning a mutable, static reference to the - /// underlying [`ZendObject`]. - /// - /// It is the callers responsibility to free the underlying memory that the returned reference - /// points to. - pub fn into_inner(self) -> &'static mut ZendObject { - let mut this = ManuallyDrop::new(self); - unsafe { this.0.as_mut() } - } -} - -impl Deref for OwnedZendObject { - type Target = ZendObject; - - fn deref(&self) -> &Self::Target { - unsafe { self.0.as_ref() } - } -} - -impl DerefMut for OwnedZendObject { - fn deref_mut(&mut self) -> &mut Self::Target { - unsafe { self.0.as_mut() } - } -} - -impl Drop for OwnedZendObject { - fn drop(&mut self) { - unsafe { ext_php_rs_zend_object_release(self.0.as_ptr()) } - } -} - -impl Debug for OwnedZendObject { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - unsafe { self.0.as_ref() }.fmt(f) - } -} - -impl IntoZval for OwnedZendObject { - const TYPE: DataType = DataType::Object(None); - - fn set_zval(self, zv: &mut Zval, _: bool) -> Result<()> { - let obj = self.into_inner(); - - // 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`. - obj.dec_count(); - zv.set_object(obj); - Ok(()) - } -} - -pub type ZendObject = zend_object; -pub type ZendObjectHandlers = zend_object_handlers; - -/// Different ways to query if a property exists. -#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] -#[repr(u32)] -pub enum PropertyQuery { - /// Property exists and is not NULL. - Isset = ZEND_PROPERTY_ISSET, - /// Property is not empty. - NotEmpty = ZEND_ISEMPTY, - /// Property exists. - Exists = ZEND_PROPERTY_EXISTS, -} - -impl ZendObject { /// Attempts to retrieve the class name of the object. pub fn get_class_name(&self) -> Result { unsafe { @@ -282,6 +227,30 @@ impl ZendObject { } } +unsafe impl ZBoxable for ZendObject { + fn free(&mut self) { + unsafe { ext_php_rs_zend_object_release(self) } + } +} + +impl Debug for ZendObject { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut dbg = f.debug_struct( + self.get_class_name() + .unwrap_or_else(|_| "ZendObject".to_string()) + .as_str(), + ); + + if let Ok(props) = self.get_properties() { + for (id, key, val) in props.iter() { + dbg.field(key.unwrap_or_else(|| id.to_string()).as_str(), val); + } + } + + dbg.finish() + } +} + impl<'a> FromZval<'a> for &'a ZendObject { const TYPE: DataType = DataType::Object(None); @@ -290,6 +259,19 @@ impl<'a> FromZval<'a> for &'a ZendObject { } } +impl IntoZval for ZBox { + const TYPE: DataType = DataType::Object(None); + + 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`. + self.dec_count(); + zv.set_object(self.into_raw()); + Ok(()) + } +} + /// `FromZendObject` is implemented by types which can be extracted from a Zend object. /// /// Normal usage is through the helper method `ZendObject::extract`: @@ -310,7 +292,7 @@ pub trait FromZendObject<'a>: Sized { /// to determine the type of object which is produced. pub trait IntoZendObject { /// Attempts to convert `self` into a Zend object. - fn into_zend_object(self) -> Result; + fn into_zend_object(self) -> Result>; } impl FromZendObject<'_> for String { @@ -349,24 +331,6 @@ impl FromZendObject<'_> for String { } } -impl Debug for ZendObject { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let mut dbg = f.debug_struct( - self.get_class_name() - .unwrap_or_else(|_| "ZendObject".to_string()) - .as_str(), - ); - - if let Ok(props) = self.get_properties() { - for (id, key, val) in props.iter() { - dbg.field(key.unwrap_or_else(|| id.to_string()).as_str(), val); - } - } - - dbg.finish() - } -} - /// Wrapper struct used to return a reference to a PHP object. pub struct ClassRef<'a, T: RegisteredClass + 'a> { ptr: &'a mut ZendClassObject, @@ -507,9 +471,9 @@ impl ClassObject { /// Converts the class object into an owned [`ZendObject`], removing any reference to /// the embedded struct type `T`. - pub fn into_owned_object(self) -> OwnedZendObject { + pub fn into_owned_object(self) -> ZBox { let mut this = ManuallyDrop::new(self); - OwnedZendObject((&mut this.deref_mut().std).into()) + unsafe { ZBox::from_raw(&mut this.std) } } } @@ -1178,7 +1142,7 @@ impl<'a, T: RegisteredClass> FromZval<'a> for &'a T { // } impl IntoZendObject for T { - fn into_zend_object(self) -> Result { + fn into_zend_object(self) -> Result> { Ok(ClassObject::new(self).into_owned_object()) } } From 00498df77912c82a744f779ff79f73ed5c69cb42 Mon Sep 17 00:00:00 2001 From: David Cole Date: Mon, 4 Oct 2021 01:25:26 +1300 Subject: [PATCH 4/6] Replace `ClassObject` with `ZBox` Fixed deserialization bug Panic when uninitialized - better than UB --- ext-php-rs-derive/src/zval.rs | 6 +- src/php/boxed.rs | 4 +- src/php/class.rs | 6 +- src/php/execution_data.rs | 13 +- src/php/types/object.rs | 323 +++++++++++++++------------------- 5 files changed, 152 insertions(+), 200 deletions(-) diff --git a/ext-php-rs-derive/src/zval.rs b/ext-php-rs-derive/src/zval.rs index b526f5b1e2..dad0dde40b 100644 --- a/ext-php-rs-derive/src/zval.rs +++ b/ext-php-rs-derive/src/zval.rs @@ -128,11 +128,13 @@ fn parse_struct( Ok(quote! { impl #into_impl_generics ::ext_php_rs::php::types::object::IntoZendObject for #ident #ty_generics #into_where_clause { fn into_zend_object(self) -> ::ext_php_rs::errors::Result< - ::ext_php_rs::php::types::object::OwnedZendObject + ::ext_php_rs::php::boxed::ZBox< + ::ext_php_rs::php::types::object::ZendObject + > > { use ::ext_php_rs::php::types::zval::IntoZval; - let mut obj = ::ext_php_rs::php::types::object::OwnedZendObject::new_stdclass(); + let mut obj = ::ext_php_rs::php::types::object::ZendObject::new_stdclass(); #(#into_fields)* ::ext_php_rs::errors::Result::Ok(obj) } diff --git a/src/php/boxed.rs b/src/php/boxed.rs index 2361446c00..7db06d0774 100644 --- a/src/php/boxed.rs +++ b/src/php/boxed.rs @@ -88,14 +88,14 @@ impl DerefMut for ZBox { impl Debug for ZBox { #[inline] fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.deref().fmt(f) + (&**self).fmt(f) } } impl Borrow for ZBox { #[inline] fn borrow(&self) -> &T { - self.deref() + &**self } } diff --git a/src/php/class.rs b/src/php/class.rs index 12824a7708..98e70bb0a0 100644 --- a/src/php/class.rs +++ b/src/php/class.rs @@ -6,7 +6,7 @@ use crate::{ exceptions::PhpException, execution_data::ExecutionData, function::FunctionBuilder, - types::object::{ClassObject, ConstructorMeta, ConstructorResult, ZendObject}, + types::object::{ConstructorMeta, ConstructorResult, ZendClassObject, ZendObject}, }, }; use std::{alloc::Layout, convert::TryInto, ffi::CString, fmt::Debug, ops::DerefMut}; @@ -277,8 +277,8 @@ impl ClassBuilder { extern "C" fn create_object(_: *mut ClassEntry) -> *mut ZendObject { // SAFETY: After calling this function, PHP will always call the constructor defined below, // which assumes that the object is uninitialized. - let obj = unsafe { ClassObject::::new_uninit() }; - obj.into_inner().get_mut_zend_obj() + let obj = unsafe { ZendClassObject::::new_uninit() }; + obj.into_raw().get_mut_zend_obj() } extern "C" fn constructor(ex: &mut ExecutionData, _: &mut Zval) { diff --git a/src/php/execution_data.rs b/src/php/execution_data.rs index ee7402459a..30cc7a035f 100644 --- a/src/php/execution_data.rs +++ b/src/php/execution_data.rs @@ -3,22 +3,15 @@ use crate::bindings::{zend_execute_data, ZEND_MM_ALIGNMENT, ZEND_MM_ALIGNMENT_MASK}; -use super::{ - args::ArgParser, - types::{ - object::{RegisteredClass, ZendClassObject, ZendObject}, - zval::Zval, - }, +use super::types::{ + object::{RegisteredClass, ZendClassObject, ZendObject}, + zval::Zval, }; /// Execution data passed when a function is called from Zend. pub type ExecutionData = zend_execute_data; impl ExecutionData { - pub fn parser(&mut self) -> (Option<&mut ZendClassObject>, ArgParser) { - (self.get_object(), ArgParser::new(self)) - } - /// Attempts to retrieve a reference to the underlying class object of the Zend object. /// /// Returns a [`ZendClassObject`] if the execution data contained a valid object of type `T`, diff --git a/src/php/types/object.rs b/src/php/types/object.rs index 46ce44315c..d9a291b6cf 100644 --- a/src/php/types/object.rs +++ b/src/php/types/object.rs @@ -61,7 +61,7 @@ pub enum PropertyQuery { } impl ZendObject { - /// Creates a new [`ZendObject`], returned inside an [`OwnedZendObject`] wrapper. + /// Creates a new [`ZendObject`], returned inside an [`ZBox`] wrapper. /// /// # Parameters /// @@ -82,7 +82,7 @@ impl ZendObject { } } - /// Creates a new `stdClass` instance, returned inside an [`OwnedZendObject`] wrapper. + /// Creates a new `stdClass` instance, returned inside an [`ZBox`] wrapper. /// /// # Panics /// @@ -353,208 +353,48 @@ impl<'a, T: RegisteredClass> IntoZval for ClassRef<'a, T> { } } -/// The owned variant of [`ZendClassObject`]. -pub struct ClassObject(NonNull>); - -impl ClassObject { - /// Creates a new [`ZendObject`] of type `T`, where `T` is a [`RegisteredClass`] in PHP, storing the given - /// value `val` inside the object. - /// - /// # Parameters - /// - /// * `val` - The value to store inside the object. - /// - /// # Panics - /// - /// Panics if memory was unable to be allocated for the new object. - pub fn new(val: T) -> Self { - unsafe { Self::internal_new(MaybeUninit::new(val), true) } - } - - /// Creates a new [`ZendObject`] of type `T`, with an uninitialized internal object. - /// - /// # Safety - /// - /// As the object is uninitialized, the caller must ensure the following until the internal object is - /// initialized: - /// - /// * The [`Drop`] implementation is never called. - /// * The [`Clone`] implementation is never called. - /// * The [`Debug`] implementation is never called. - /// * The object is never dereferenced to `T`. - /// - /// If any of these conditions are not met while not initialized, the corresponding function will panic. - /// Converting the object into its inner with the [`into_inner`] function is valid, however. - /// - /// [`into_inner`]: #method.into_inner - /// - /// # Panics - /// - /// Panics if memory was unable to be allocated for the new object. - pub unsafe fn new_uninit() -> Self { - Self::internal_new(MaybeUninit::uninit(), false) - } - - /// Creates a new [`ZendObject`] of type `T`, storing the given (and potentially uninitialized) `val` - /// inside the object. - /// - /// # Parameters - /// - /// * `val` - Value to store inside the object. See safety section. - /// * `init` - Whether the given `val` was initialized. - /// - /// # Safety - /// - /// Providing an initialized variant of [`MaybeUninit`] is safe. - /// - /// Providing an uninitalized variant of [`MaybeUninit`] is unsafe. As the object is uninitialized, - /// the caller must ensure the following until the internal object is initialized: - /// - /// * The [`Drop`] implementation is never called. - /// * The [`Clone`] implementation is never called. - /// * The [`Debug`] implementation is never called. - /// * The object is never dereferenced to `T`. - /// - /// If any of these conditions are not met while not initialized, the corresponding function will panic. - /// Converting the object into its inner with the [`into_inner`] function is valid, however. You can initialize - /// the object with the [`initialize`] function. - /// - /// [`into_inner`]: #method.into_inner - /// [`initialize`]: #method.initialize - /// - /// # Panics - /// - /// Panics if memory was unable to be allocated for the new object. - unsafe fn internal_new(val: MaybeUninit, init: bool) -> Self { - let size = mem::size_of::>(); - let meta = T::get_metadata(); - let ce = meta.ce() as *const _ as *mut _; - let obj = ext_php_rs_zend_object_alloc(size as _, ce) as *mut ZendClassObject; - let obj = obj - .as_mut() - .expect("Failed to allocate for new Zend object"); - - zend_object_std_init(&mut obj.std, ce); - object_properties_init(&mut obj.std, ce); - - obj.obj = val; - obj.init = init; - obj.std.handlers = meta.handlers(); - Self(obj.into()) - } - - /// Initializes the class object with the value `val`. - /// - /// # Parameters - /// - /// * `val` - The value to initialize the object with. - /// - /// # Returns - /// - /// Returns the old value in an [`Option`] if the object had already been initialized, [`None`] - /// otherwise. - pub fn initialize(&mut self, val: T) -> Option { - // Instead of using the `Deref` implementation, we write a wrapper function, as calling `deref()` - // on an uninitialized object will panic. - - // SAFETY: All constructors of `NewClassObject` guarantee that it will contain a valid pointer. - unsafe { self.0.as_mut() }.initialize(val) - } - - /// Converts the [`ClassObject`] into the inner pointer, which is a pointer to [`ZendClassObject`]. - /// The caller is responsible for freeing the returned pointer when finished. - pub fn into_inner(self) -> &'static mut ZendClassObject { - let mut this = ManuallyDrop::new(self); - // SAFETY: All constructors of `ClassObject` guarantee that it will contain a valid pointer. - unsafe { this.0.as_mut() } - } - - /// Converts the class object into an owned [`ZendObject`], removing any reference to - /// the embedded struct type `T`. - pub fn into_owned_object(self) -> ZBox { - let mut this = ManuallyDrop::new(self); +impl From>> for ZBox { + fn from(obj: ZBox>) -> Self { + let mut this = ManuallyDrop::new(obj); unsafe { ZBox::from_raw(&mut this.std) } } } -impl Default for ClassObject { +impl Default for ZBox> { fn default() -> Self { - Self::new(T::default()) + ZendClassObject::new(T::default()) } } -impl Clone for ClassObject { +impl Clone for ZBox> { fn clone(&self) -> Self { - if !self.init { - panic!("Attempted to clone uninitialized class object"); - } - // SAFETY: All constructors of `NewClassObject` guarantee that it will contain a valid pointer. // The constructor also guarantees that the internal `ZendClassObject` pointer will contain a valid, // initialized `obj`, therefore we can dereference both safely. - let mut new = Self::new(unsafe { self.0.as_ref().obj.assume_init_ref().clone() }); - unsafe { zend_objects_clone_members(&mut new.std, &self.std as *const _ as *mut _) }; - new + unsafe { + let mut new = ZendClassObject::new((&***self).clone()); + zend_objects_clone_members(&mut new.std, &self.std as *const _ as *mut _); + new + } } } -impl Debug for ClassObject { +impl Debug for ZendClassObject { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if !self.init { - panic!("Attempted to call `Debug` implementation on uninitialized class object"); - } - - // TODO(david): Implement `Debug` for `ZendClassObject`? - self.deref().obj.fmt(f) + (&**self).fmt(f) } } -impl IntoZval for ClassObject { +impl IntoZval for ZBox> { const TYPE: DataType = DataType::Object(Some(T::CLASS_NAME)); fn set_zval(self, zv: &mut Zval, _: bool) -> Result<()> { - let obj = self.into_inner(); + let obj = self.into_raw(); zv.set_object(&mut obj.std); Ok(()) } } -impl Deref for ClassObject { - type Target = ZendClassObject; - - fn deref(&self) -> &Self::Target { - // SAFETY: All constructors of `ClassObject` guarantee that it will contain a valid pointer. - let ptr = unsafe { self.0.as_ref() }; - if !ptr.init { - panic!("Attempted to access uninitialized class object"); - } - ptr - } -} - -impl DerefMut for ClassObject { - fn deref_mut(&mut self) -> &mut Self::Target { - // SAFETY: All constructors of `ClassObject` guarantee that it will contain a valid pointer. - let ptr = unsafe { self.0.as_mut() }; - if !ptr.init { - panic!("Attempted to access uninitialized class object"); - } - ptr - } -} - -impl Drop for ClassObject { - fn drop(&mut self) { - if !self.init { - panic!("Attempted to drop uninitialized class object"); - } - - // SAFETY: All constructors guarantee that `self` contains a valid pointer. Further, all constructors - // guarantee that the `std` field of `ZendClassObject` will be initialized. - unsafe { ext_php_rs_zend_object_release(&mut self.0.as_mut().std) } - } -} - /// Object constructor metadata. pub struct ConstructorMeta { /// Constructor function. @@ -642,6 +482,92 @@ pub struct ZendClassObject { } impl ZendClassObject { + /// Creates a new [`ZendClassObject`] of type `T`, where `T` is a [`RegisteredClass`] in PHP, storing the + /// given value `val` inside the object. + /// + /// # Parameters + /// + /// * `val` - The value to store inside the object. + /// + /// # Panics + /// + /// Panics if memory was unable to be allocated for the new object. + pub fn new(val: T) -> ZBox { + unsafe { Self::internal_new(MaybeUninit::new(val), true) } + } + + /// Creates a new [`ZendClassObject`] of type `T`, with an uninitialized internal object. + /// + /// # Safety + /// + /// As the object is uninitialized, the caller must ensure the following until the internal object is + /// initialized: + /// + /// * The [`Drop`] implementation is never called. + /// * The [`Clone`] implementation is never called. + /// * The [`Debug`] implementation is never called. + /// * The object is never dereferenced to `T`. + /// + /// If any of these conditions are not met while not initialized, the corresponding function will panic. + /// Converting the object into its inner pointer with the [`into_raw`] function is valid, however. + /// + /// [`into_raw`]: #method.into_raw + /// + /// # Panics + /// + /// Panics if memory was unable to be allocated for the new object. + pub unsafe fn new_uninit() -> ZBox { + Self::internal_new(MaybeUninit::uninit(), false) + } + + /// Creates a new [`ZendObject`] of type `T`, storing the given (and potentially uninitialized) `val` + /// inside the object. + /// + /// # Parameters + /// + /// * `val` - Value to store inside the object. See safety section. + /// * `init` - Whether the given `val` was initialized. + /// + /// # Safety + /// + /// Providing an initialized variant of [`MaybeUninit`] is safe. + /// + /// Providing an uninitalized variant of [`MaybeUninit`] is unsafe. As the object is uninitialized, + /// the caller must ensure the following until the internal object is initialized: + /// + /// * The [`Drop`] implementation is never called. + /// * The [`Clone`] implementation is never called. + /// * The [`Debug`] implementation is never called. + /// * The object is never dereferenced to `T`. + /// + /// If any of these conditions are not met while not initialized, the corresponding function will panic. + /// Converting the object into its inner with the [`into_raw`] function is valid, however. You can initialize + /// the object with the [`initialize`] function. + /// + /// [`into_raw`]: #method.into_raw + /// [`initialize`]: #method.initialize + /// + /// # Panics + /// + /// Panics if memory was unable to be allocated for the new object. + unsafe fn internal_new(val: MaybeUninit, init: bool) -> ZBox { + let size = mem::size_of::>(); + let meta = T::get_metadata(); + let ce = meta.ce() as *const _ as *mut _; + let obj = ext_php_rs_zend_object_alloc(size as _, ce) as *mut ZendClassObject; + let obj = obj + .as_mut() + .expect("Failed to allocate for new Zend object"); + + zend_object_std_init(&mut obj.std, ce); + object_properties_init(&mut obj.std, ce); + + obj.obj = val; + obj.init = init; + obj.std.handlers = meta.handlers(); + ZBox::from_raw(obj) + } + /// Initializes the class object with the value `val`. /// /// # Parameters @@ -738,6 +664,29 @@ impl ZendClassObject { } } +impl<'a, T: RegisteredClass> FromZval<'a> for &'a ZendClassObject { + const TYPE: DataType = DataType::Object(Some(T::CLASS_NAME)); + + fn from_zval(zval: &'a Zval) -> Option { + Self::from_zend_object(zval.object()?).ok() + } +} + +impl<'a, T: RegisteredClass> FromZendObject<'a> for &'a ZendClassObject { + fn from_zend_object(obj: &'a ZendObject) -> Result { + // TODO(david): replace with better error + ZendClassObject::from_zend_obj(obj).ok_or(Error::InvalidPointer) + } +} + +unsafe impl ZBoxable for ZendClassObject { + fn free(&mut self) { + // SAFETY: All constructors guarantee that `self` contains a valid pointer. Further, all constructors + // guarantee that the `std` field of `ZendClassObject` will be initialized. + unsafe { ext_php_rs_zend_object_release(&mut self.std) } + } +} + impl Drop for ZendClassObject { fn drop(&mut self) { // SAFETY: All constructors guarantee that `obj` is valid. @@ -749,6 +698,10 @@ impl Deref for ZendClassObject { type Target = T; fn deref(&self) -> &Self::Target { + if !self.init { + panic!("Attempted to access uninitialized class object"); + } + // SAFETY: All constructors guarantee that `obj` is valid. unsafe { self.obj.assume_init_ref() } } @@ -756,6 +709,10 @@ impl Deref for ZendClassObject { impl DerefMut for ZendClassObject { fn deref_mut(&mut self) -> &mut Self::Target { + if !self.init { + panic!("Attempted to access uninitialized class object"); + } + // SAFETY: All constructors guarantee that `obj` is valid. unsafe { self.obj.assume_init_mut() } } @@ -922,7 +879,7 @@ impl ZendObjectHandlers { let prop_name = member .as_ref() .ok_or("Invalid property name pointer given")?; - let self_ = obj.obj.assume_init_mut(); + let self_ = &mut **obj; let mut props = T::get_properties(); let prop = props.remove(prop_name.as_str().ok_or("Invalid property name given")?); @@ -969,7 +926,7 @@ impl ZendObjectHandlers { let prop_name = member .as_ref() .ok_or("Invalid property name pointer given")?; - let self_ = obj.obj.assume_init_mut(); + let self_ = &mut **obj; let mut props = T::get_properties(); let prop = props.remove(prop_name.as_str().ok_or("Invalid property name given")?); let value_mut = value.as_mut().ok_or("Invalid return zval given")?; @@ -1004,7 +961,7 @@ impl ZendObjectHandlers { .as_mut() .and_then(|obj| ZendClassObject::::from_zend_obj_mut(obj)) .ok_or("Invalid object pointer given")?; - let self_ = obj.obj.assume_init_mut(); + let self_ = &mut **obj; let struct_props = T::get_properties(); for (name, val) in struct_props.into_iter() { @@ -1054,7 +1011,7 @@ impl ZendObjectHandlers { .ok_or("Invalid property name pointer given")?; let props = T::get_properties(); let prop = props.get(prop_name.as_str().ok_or("Invalid property name given")?); - let self_ = obj.obj.assume_init_mut(); + let self_ = &mut **obj; match has_set_exists { // * 0 (has) whether property exists and is not NULL @@ -1112,7 +1069,7 @@ impl<'a, T: RegisteredClass> FromZendObject<'a> for &'a T { fn from_zend_object(obj: &'a ZendObject) -> Result { // TODO(david): Error is kinda wrong, should have something like `WrongObject` let cobj = ZendClassObject::::from_zend_obj(obj).ok_or(Error::InvalidPointer)?; - Ok(unsafe { cobj.obj.assume_init_ref() }) + Ok(&**cobj) } } @@ -1143,7 +1100,7 @@ impl<'a, T: RegisteredClass> FromZval<'a> for &'a T { impl IntoZendObject for T { fn into_zend_object(self) -> Result> { - Ok(ClassObject::new(self).into_owned_object()) + Ok(ZendClassObject::new(self).into()) } } From 82d32ceceea69bed76589c406990dbe3c5c47971 Mon Sep 17 00:00:00 2001 From: David Cole Date: Tue, 5 Oct 2021 00:48:33 +1300 Subject: [PATCH 5/6] Replace `OwnedHashTable` with `ZBox` --- build.rs | 1 + src/php/types/array.rs | 170 ++++++++++++++-------------------------- src/php/types/object.rs | 3 +- src/php/types/zval.rs | 14 +--- 4 files changed, 63 insertions(+), 125 deletions(-) diff --git a/build.rs b/build.rs index 9477dc7d56..4fbeed467f 100644 --- a/build.rs +++ b/build.rs @@ -86,6 +86,7 @@ fn main() { .rustfmt_bindings(true) .no_copy("_zend_value") .no_copy("_zend_string") + .no_copy("_zend_array") .layout_tests(env::var("EXT_PHP_RS_TEST").is_ok()); for binding in ALLOWED_BINDINGS.iter() { diff --git a/src/php/types/array.rs b/src/php/types/array.rs index 1be16ee908..0003fe8d7c 100644 --- a/src/php/types/array.rs +++ b/src/php/types/array.rs @@ -7,8 +7,6 @@ use std::{ ffi::CString, fmt::Debug, iter::FromIterator, - mem::ManuallyDrop, - ops::{Deref, DerefMut}, ptr::NonNull, u64, }; @@ -21,7 +19,10 @@ use crate::{ HT_MIN_SIZE, }, errors::{Error, Result}, - php::enums::DataType, + php::{ + boxed::{ZBox, ZBoxable}, + enums::DataType, + }, }; use super::zval::{FromZval, IntoZval, Zval}; @@ -30,6 +31,27 @@ use super::zval::{FromZval, IntoZval, Zval}; pub use crate::bindings::HashTable; impl HashTable { + /// Creates a new, empty, PHP associative array. + pub fn new() -> ZBox { + Self::with_capacity(HT_MIN_SIZE) + } + + /// Creates a new, empty, PHP associative array with an initial size. + /// + /// # Parameters + /// + /// * `size` - The size to initialize the array with. + pub fn with_capacity(size: u32) -> ZBox { + // SAFETY: PHP allocater handles the creation of the array. + unsafe { + let ptr = _zend_new_array(size); + ZBox::from_raw( + ptr.as_mut() + .expect("Failed to allocate memory for hashtable"), + ) + } + } + /// Returns the current number of elements in the array. pub fn len(&self) -> usize { self.nNumOfElements as usize @@ -184,11 +206,11 @@ impl HashTable { pub fn values(&self) -> Values { Values::new(self) } +} - /// Clones the hash table, returning an [`OwnedHashTable`]. - pub fn to_owned(&self) -> OwnedHashTable { - let ptr = unsafe { zend_array_dup(self as *const HashTable as *mut HashTable) }; - unsafe { OwnedHashTable::from_ptr(ptr) } +unsafe impl ZBoxable for HashTable { + fn free(&mut self) { + unsafe { zend_array_destroy(self) } } } @@ -203,6 +225,17 @@ impl Debug for HashTable { } } +impl ToOwned for HashTable { + type Owned = ZBox; + + fn to_owned(&self) -> Self::Owned { + unsafe { + let ptr = zend_array_dup(self as *const HashTable as *mut HashTable); + ZBox::from_raw(ptr) + } + } +} + /// Immutable iterator upon a reference to a hashtable. pub struct Iter<'a> { ht: &'a HashTable, @@ -315,106 +348,19 @@ impl<'a> DoubleEndedIterator for Values<'a> { } } -/// A container used to 'own' a Zend hashtable. Dereferences to a reference to [`HashTable`]. -/// -/// When this struct is dropped, it will also destroy the internal hashtable, unless the `into_raw` -/// function is used. -pub struct OwnedHashTable { - ptr: NonNull, -} - -impl OwnedHashTable { - /// Creates a new, empty, PHP associative array. - pub fn new() -> Self { - Self::with_capacity(HT_MIN_SIZE) - } - - /// Creates a new, empty, PHP associative array with an initial size. - /// - /// # Parameters - /// - /// * `size` - The size to initialize the array with. - pub fn with_capacity(size: u32) -> Self { - // SAFETY: PHP allocater handles the creation of the array. - unsafe { - let ptr = _zend_new_array(size); - Self::from_ptr(ptr) - } - } - - /// Creates an owned hashtable from a hashtable pointer, which will be freed when the - /// resulting Rust object is dropped. - /// - /// # Parameters - /// - /// * `ptr` - Hashtable pointer. - /// - /// # Panics - /// - /// Panics if the given pointer is null. - /// - /// # Safety - /// - /// Caller must ensure that the given pointer is a valid hashtable pointer, including - /// non-null and properly aligned. - pub unsafe fn from_ptr(ptr: *mut HashTable) -> Self { - Self { - ptr: NonNull::new(ptr).expect("Invalid hashtable pointer given"), - } - } - - /// Converts the owned Zend hashtable into the internal pointer, bypassing the [`Drop`] - /// implementation. - /// - /// The caller is responsible for freeing the resulting pointer using the `zend_array_destroy` - /// function. - pub fn into_inner(self) -> *mut HashTable { - let this = ManuallyDrop::new(self); - this.ptr.as_ptr() - } -} - -impl Deref for OwnedHashTable { - type Target = HashTable; - - fn deref(&self) -> &Self::Target { - // SAFETY: all constructors ensure a valid ptr is present - unsafe { self.ptr.as_ref() } - } -} - -impl DerefMut for OwnedHashTable { - fn deref_mut(&mut self) -> &mut Self::Target { - // SAFETY: all constructors ensure a valid, owned ptr is present - unsafe { self.ptr.as_mut() } - } -} - -impl Debug for OwnedHashTable { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.deref().fmt(f) - } -} - -impl Default for OwnedHashTable { +impl Default for ZBox { fn default() -> Self { - Self::new() + HashTable::new() } } -impl Clone for OwnedHashTable { +impl Clone for ZBox { fn clone(&self) -> Self { - self.deref().to_owned() - } -} - -impl Drop for OwnedHashTable { - fn drop(&mut self) { - unsafe { zend_array_destroy(self.ptr.as_mut()) }; + (**self).to_owned() } } -impl IntoZval for OwnedHashTable { +impl IntoZval for ZBox { const TYPE: DataType = DataType::Array; fn set_zval(self, zv: &mut Zval, _: bool) -> Result<()> { @@ -455,7 +401,7 @@ where } } -impl TryFrom> for OwnedHashTable +impl TryFrom> for ZBox where K: AsRef, V: IntoZval, @@ -463,9 +409,8 @@ where type Error = Error; fn try_from(value: HashMap) -> Result { - let mut ht = OwnedHashTable::with_capacity( - value.len().try_into().map_err(|_| Error::IntegerOverflow)?, - ); + let mut ht = + HashTable::with_capacity(value.len().try_into().map_err(|_| Error::IntegerOverflow)?); for (k, v) in value.into_iter() { ht.insert(k.as_ref(), v)?; @@ -521,16 +466,15 @@ where } } -impl TryFrom> for OwnedHashTable +impl TryFrom> for ZBox where T: IntoZval, { type Error = Error; fn try_from(value: Vec) -> Result { - let mut ht = OwnedHashTable::with_capacity( - value.len().try_into().map_err(|_| Error::IntegerOverflow)?, - ); + let mut ht = + HashTable::with_capacity(value.len().try_into().map_err(|_| Error::IntegerOverflow)?); for val in value.into_iter() { ht.push(val)?; @@ -564,9 +508,9 @@ where } } -impl FromIterator for OwnedHashTable { +impl FromIterator for ZBox { fn from_iter>(iter: T) -> Self { - let mut ht = OwnedHashTable::new(); + let mut ht = HashTable::new(); for item in iter.into_iter() { // Inserting a zval cannot fail, as `push` only returns `Err` if converting `val` to a zval // fails. @@ -576,9 +520,9 @@ impl FromIterator for OwnedHashTable { } } -impl FromIterator<(u64, Zval)> for OwnedHashTable { +impl FromIterator<(u64, Zval)> for ZBox { fn from_iter>(iter: T) -> Self { - let mut ht = OwnedHashTable::new(); + let mut ht = HashTable::new(); for (key, val) in iter.into_iter() { // Inserting a zval cannot fail, as `push` only returns `Err` if converting `val` to a zval // fails. @@ -588,9 +532,9 @@ impl FromIterator<(u64, Zval)> for OwnedHashTable { } } -impl<'a> FromIterator<(&'a str, Zval)> for OwnedHashTable { +impl<'a> FromIterator<(&'a str, Zval)> for ZBox { fn from_iter>(iter: T) -> Self { - let mut ht = OwnedHashTable::new(); + let mut ht = HashTable::new(); for (key, val) in iter.into_iter() { // Inserting a zval cannot fail, as `push` only returns `Err` if converting `val` to a zval // fails. diff --git a/src/php/types/object.rs b/src/php/types/object.rs index d9a291b6cf..cabaa63607 100644 --- a/src/php/types/object.rs +++ b/src/php/types/object.rs @@ -34,7 +34,6 @@ use crate::{ flags::ZvalTypeFlags, function::FunctionBuilder, globals::ExecutorGlobals, - types::array::OwnedHashTable, }, }; @@ -979,7 +978,7 @@ impl ZendObjectHandlers { let props = zend_std_get_properties(object) .as_mut() - .or_else(|| OwnedHashTable::new().into_inner().as_mut()) + .or_else(|| Some(HashTable::new().into_raw())) .expect("Failed to get property hashtable"); if let Err(e) = internal::(object, props) { diff --git a/src/php/types/zval.rs b/src/php/types/zval.rs index 7616db1ea9..a4925edd74 100644 --- a/src/php/types/zval.rs +++ b/src/php/types/zval.rs @@ -19,13 +19,7 @@ use crate::{ use crate::php::{enums::DataType, flags::ZvalTypeFlags, types::long::ZendLong}; -use super::{ - array::{HashTable, OwnedHashTable}, - callable::Callable, - object::ZendObject, - rc::PhpRc, - string::ZendStr, -}; +use super::{array::HashTable, callable::Callable, object::ZendObject, rc::PhpRc, string::ZendStr}; /// Zend value. Represents most data types that are in the Zend engine. pub type Zval = zval; @@ -421,7 +415,7 @@ impl Zval { /// # Parameters /// /// * `val` - The value to set the zval as. - pub fn set_array>(&mut self, val: T) -> Result<()> { + pub fn set_array, Error = Error>>(&mut self, val: T) -> Result<()> { self.set_hashtable(val.try_into()?); Ok(()) } @@ -431,9 +425,9 @@ impl Zval { /// # Parameters /// /// * `val` - The value to set the zval as. - pub fn set_hashtable(&mut self, val: OwnedHashTable) { + pub fn set_hashtable(&mut self, val: ZBox) { self.change_type(ZvalTypeFlags::ArrayEx); - self.value.arr = val.into_inner(); + self.value.arr = val.into_raw(); } /// Sets the value of the zval as a pointer. From af7a2e3811fa59f8839e004ad661868d1518ce64 Mon Sep 17 00:00:00 2001 From: David Cole Date: Tue, 5 Oct 2021 16:51:27 +1300 Subject: [PATCH 6/6] Remove `MaybeUninit` from `ZendClassObject` --- src/php/types/object.rs | 67 +++++++++++++---------------------------- 1 file changed, 21 insertions(+), 46 deletions(-) diff --git a/src/php/types/object.rs b/src/php/types/object.rs index cabaa63607..425bc0cd38 100644 --- a/src/php/types/object.rs +++ b/src/php/types/object.rs @@ -471,12 +471,10 @@ where fn get_properties<'a>() -> HashMap<&'static str, Property<'a, Self>>; } -/// Representation of a Zend class object in memory. Usually seen through its owned variant -/// of [`ClassObject`]. +/// Representation of a Zend class object in memory. #[repr(C)] pub struct ZendClassObject { - obj: MaybeUninit, - init: bool, + obj: Option, std: zend_object, } @@ -492,7 +490,7 @@ impl ZendClassObject { /// /// Panics if memory was unable to be allocated for the new object. pub fn new(val: T) -> ZBox { - unsafe { Self::internal_new(MaybeUninit::new(val), true) } + unsafe { Self::internal_new(Some(val)) } } /// Creates a new [`ZendClassObject`] of type `T`, with an uninitialized internal object. @@ -502,10 +500,9 @@ impl ZendClassObject { /// As the object is uninitialized, the caller must ensure the following until the internal object is /// initialized: /// - /// * The [`Drop`] implementation is never called. + /// * The object is never dereferenced to `T`. /// * The [`Clone`] implementation is never called. /// * The [`Debug`] implementation is never called. - /// * The object is never dereferenced to `T`. /// /// If any of these conditions are not met while not initialized, the corresponding function will panic. /// Converting the object into its inner pointer with the [`into_raw`] function is valid, however. @@ -516,7 +513,7 @@ impl ZendClassObject { /// /// Panics if memory was unable to be allocated for the new object. pub unsafe fn new_uninit() -> ZBox { - Self::internal_new(MaybeUninit::uninit(), false) + Self::internal_new(None) } /// Creates a new [`ZendObject`] of type `T`, storing the given (and potentially uninitialized) `val` @@ -534,10 +531,9 @@ impl ZendClassObject { /// Providing an uninitalized variant of [`MaybeUninit`] is unsafe. As the object is uninitialized, /// the caller must ensure the following until the internal object is initialized: /// - /// * The [`Drop`] implementation is never called. + /// * The object is never dereferenced to `T`. /// * The [`Clone`] implementation is never called. /// * The [`Debug`] implementation is never called. - /// * The object is never dereferenced to `T`. /// /// If any of these conditions are not met while not initialized, the corresponding function will panic. /// Converting the object into its inner with the [`into_raw`] function is valid, however. You can initialize @@ -549,7 +545,7 @@ impl ZendClassObject { /// # Panics /// /// Panics if memory was unable to be allocated for the new object. - unsafe fn internal_new(val: MaybeUninit, init: bool) -> ZBox { + unsafe fn internal_new(val: Option) -> ZBox { let size = mem::size_of::>(); let meta = T::get_metadata(); let ce = meta.ce() as *const _ as *mut _; @@ -561,8 +557,11 @@ impl ZendClassObject { zend_object_std_init(&mut obj.std, ce); object_properties_init(&mut obj.std, ce); - obj.obj = val; - obj.init = init; + // SAFETY: `obj` is non-null and well aligned as it is a reference. + // As the data in `obj.obj` is uninitalized, we don't want to drop + // the data, but directly override it. + ptr::write(&mut obj.obj, val); + obj.std.handlers = meta.handlers(); ZBox::from_raw(obj) } @@ -578,16 +577,7 @@ impl ZendClassObject { /// Returns the old value in an [`Option`] if the object had already been initialized, [`None`] /// otherwise. pub fn initialize(&mut self, val: T) -> Option { - let old = Some(mem::replace(&mut self.obj, MaybeUninit::new(val))).and_then(|v| { - if self.init { - Some(unsafe { v.assume_init() }) - } else { - None - } - }); - self.init = true; - - old + self.obj.replace(val) } /// Returns a reference to the [`ZendClassObject`] of a given object `T`. Returns [`None`] @@ -686,34 +676,21 @@ unsafe impl ZBoxable for ZendClassObject { } } -impl Drop for ZendClassObject { - fn drop(&mut self) { - // SAFETY: All constructors guarantee that `obj` is valid. - unsafe { std::ptr::drop_in_place(self.obj.as_mut_ptr()) }; - } -} - impl Deref for ZendClassObject { type Target = T; fn deref(&self) -> &Self::Target { - if !self.init { - panic!("Attempted to access uninitialized class object"); - } - - // SAFETY: All constructors guarantee that `obj` is valid. - unsafe { self.obj.assume_init_ref() } + self.obj + .as_ref() + .expect("Attempted to access uninitalized class object") } } impl DerefMut for ZendClassObject { fn deref_mut(&mut self) -> &mut Self::Target { - if !self.init { - panic!("Attempted to access uninitialized class object"); - } - - // SAFETY: All constructors guarantee that `obj` is valid. - unsafe { self.obj.assume_init_mut() } + self.obj + .as_mut() + .expect("Attempted to access uninitalized class object") } } @@ -848,10 +825,8 @@ impl ZendObjectHandlers { .and_then(|obj| ZendClassObject::::from_zend_obj_mut(obj)) .expect("Invalid object pointer given for `free_obj`"); - // Manually drop the object as it is wrapped with `MaybeUninit`. - if obj.init { - ptr::drop_in_place(obj.obj.as_mut_ptr()); - } + // Manually drop the object as we don't want to free the underlying memory. + ptr::drop_in_place(&mut obj.obj); zend_object_std_dtor(object) }