From f06d773484db70293492053f0d90731d0074ee1c Mon Sep 17 00:00:00 2001 From: Vasily Zorin Date: Mon, 22 Dec 2025 22:30:27 +0700 Subject: [PATCH] fix(class): property visibility checks #375 --- allowed_bindings.rs | 1 + crates/macros/src/function.rs | 3 +- docsrs_bindings.rs | 7 ++ src/builders/ini.rs | 3 +- src/builders/sapi.rs | 28 ++++-- src/closure.rs | 3 +- src/embed/mod.rs | 5 +- src/enum_.rs | 9 +- src/types/array/conversions/mod.rs | 6 +- src/types/array/mod.rs | 8 +- src/types/zval.rs | 4 +- src/zend/handlers.rs | 140 +++++++++++++++++++++++++- tests/src/integration/array/mod.rs | 3 +- tests/src/integration/class/class.php | 34 +++++++ tests/src/integration/class/mod.rs | 46 +++++++++ tests/src/integration/mod.rs | 3 +- 16 files changed, 270 insertions(+), 33 deletions(-) diff --git a/allowed_bindings.rs b/allowed_bindings.rs index 42dbb8750..de86cb339 100644 --- a/allowed_bindings.rs +++ b/allowed_bindings.rs @@ -128,6 +128,7 @@ bind! { zend_resource, zend_string, zend_string_init_interned, + zend_throw_error, zend_throw_exception_ex, zend_throw_exception_object, zend_type, diff --git a/crates/macros/src/function.rs b/crates/macros/src/function.rs index 853d6664b..60773f9a1 100644 --- a/crates/macros/src/function.rs +++ b/crates/macros/src/function.rs @@ -972,7 +972,8 @@ fn expr_to_php_stub(expr: &Expr) -> String { } } -/// Returns true if the given type is nullable in PHP (i.e., it's an `Option`). +/// Returns true if the given type is nullable in PHP (i.e., it's an +/// `Option`). /// /// Note: Having a default value does NOT make a type nullable. A parameter with /// a default value is optional (can be omitted), but passing `null` explicitly diff --git a/docsrs_bindings.rs b/docsrs_bindings.rs index 29a2ee8c6..85df21abf 100644 --- a/docsrs_bindings.rs +++ b/docsrs_bindings.rs @@ -1202,6 +1202,13 @@ unsafe extern "C" { pub static mut zend_interrupt_function: ::std::option::Option; } +unsafe extern "C" { + pub fn zend_throw_error( + exception_ce: *mut zend_class_entry, + format: *const ::std::os::raw::c_char, + ... + ); +} unsafe extern "C" { pub static mut zend_standard_class_def: *mut zend_class_entry; } diff --git a/src/builders/ini.rs b/src/builders/ini.rs index ca71a53b5..f42aabde9 100644 --- a/src/builders/ini.rs +++ b/src/builders/ini.rs @@ -218,7 +218,8 @@ impl AsRef for IniBuilder { } } -// Ensure the C buffer is properly deinitialized when the builder goes out of scope. +// Ensure the C buffer is properly deinitialized when the builder goes out of +// scope. impl Drop for IniBuilder { fn drop(&mut self) { if !self.value.is_null() { diff --git a/src/builders/sapi.rs b/src/builders/sapi.rs index 1de957209..a9fc8898f 100644 --- a/src/builders/sapi.rs +++ b/src/builders/sapi.rs @@ -168,7 +168,8 @@ impl SapiBuilder { /// /// # Parameters /// - /// * `func` - The function to be called when PHP gets an environment variable. + /// * `func` - The function to be called when PHP gets an environment + /// variable. pub fn getenv_function(mut self, func: SapiGetEnvFunc) -> Self { self.module.getenv = Some(func); self @@ -196,7 +197,8 @@ impl SapiBuilder { /// Sets the send headers function for this SAPI /// - /// This function is called once when all headers are finalized and ready to send. + /// This function is called once when all headers are finalized and ready to + /// send. /// /// # Arguments /// @@ -230,7 +232,8 @@ impl SapiBuilder { /// /// # Parameters /// - /// * `func` - The function to be called when PHP registers server variables. + /// * `func` - The function to be called when PHP registers server + /// variables. pub fn register_server_variables_function( mut self, func: SapiRegisterServerVariablesFunc, @@ -291,8 +294,8 @@ impl SapiBuilder { /// Sets the pre-request init function for this SAPI /// - /// This function is called before request activation and before POST data is read. - /// It is typically used for .user.ini processing. + /// This function is called before request activation and before POST data + /// is read. It is typically used for .user.ini processing. /// /// # Parameters /// @@ -455,7 +458,8 @@ pub type SapiGetUidFunc = extern "C" fn(uid: *mut uid_t) -> c_int; /// A function to be called when PHP gets the gid pub type SapiGetGidFunc = extern "C" fn(gid: *mut gid_t) -> c_int; -/// A function to be called before request activation (used for .user.ini processing) +/// A function to be called before request activation (used for .user.ini +/// processing) #[cfg(php85)] pub type SapiPreRequestInitFunc = extern "C" fn() -> c_int; @@ -485,8 +489,9 @@ mod test { extern "C" fn test_getenv(_name: *const c_char, _name_length: usize) -> *mut c_char { ptr::null_mut() } - // Note: C-variadic functions are unstable in Rust, so we can't test this properly - // extern "C" fn test_sapi_error(_type: c_int, _error_msg: *const c_char, _args: ...) {} + // Note: C-variadic functions are unstable in Rust, so we can't test this + // properly extern "C" fn test_sapi_error(_type: c_int, _error_msg: *const + // c_char, _args: ...) {} extern "C" fn test_send_header(_header: *mut sapi_header_struct, _server_context: *mut c_void) { } extern "C" fn test_send_headers(_sapi_headers: *mut sapi_headers_struct) -> c_int { @@ -633,9 +638,10 @@ mod test { ); } - // Note: Cannot test sapi_error_function because C-variadic functions are unstable in Rust - // The sapi_error field accepts a function with variadic arguments which cannot be - // created in stable Rust. However, the builder method itself works correctly. + // Note: Cannot test sapi_error_function because C-variadic functions are + // unstable in Rust The sapi_error field accepts a function with variadic + // arguments which cannot be created in stable Rust. However, the builder + // method itself works correctly. #[test] fn test_send_header_function() { diff --git a/src/closure.rs b/src/closure.rs index f184ff8f4..f58c9a6a0 100644 --- a/src/closure.rs +++ b/src/closure.rs @@ -116,7 +116,8 @@ impl Closure { /// function. /// /// If the class has already been built, this function returns early without - /// doing anything. This allows for safe repeated calls in test environments. + /// doing anything. This allows for safe repeated calls in test + /// environments. /// /// # Panics /// diff --git a/src/embed/mod.rs b/src/embed/mod.rs index 97df7e0aa..e0648cb3c 100644 --- a/src/embed/mod.rs +++ b/src/embed/mod.rs @@ -289,8 +289,9 @@ mod tests { #[test] fn test_eval_bailout() { Embed::run(|| { - // TODO: For PHP 8.5, this needs to be replaced, as `E_USER_ERROR` is deprecated. - // Currently, this seems to still be the best way to trigger a bailout. + // TODO: For PHP 8.5, this needs to be replaced, as `E_USER_ERROR` is + // deprecated. Currently, this seems to still be the best way + // to trigger a bailout. let result = Embed::eval("trigger_error(\"Fatal error\", E_USER_ERROR);"); assert!(result.is_err()); diff --git a/src/enum_.rs b/src/enum_.rs index 5868a876d..8bcbd6970 100644 --- a/src/enum_.rs +++ b/src/enum_.rs @@ -1,4 +1,5 @@ -//! This module defines the `PhpEnum` trait and related types for Rust enums that are exported to PHP. +//! This module defines the `PhpEnum` trait and related types for Rust enums +//! that are exported to PHP. use std::ptr; use crate::{ @@ -19,7 +20,8 @@ pub trait RegisteredEnum { /// # Errors /// - /// - [`Error::InvalidProperty`] if the enum does not have a case with the given name, an error is returned. + /// - [`Error::InvalidProperty`] if the enum does not have a case with the + /// given name, an error is returned. fn from_name(name: &str) -> Result where Self: Sized; @@ -125,7 +127,8 @@ impl EnumCase { } } -/// Represents the discriminant of an enum case in PHP, which can be either an integer or a string. +/// Represents the discriminant of an enum case in PHP, which can be either an +/// integer or a string. #[derive(Debug, PartialEq, Eq)] pub enum Discriminant { /// An integer discriminant. diff --git a/src/types/array/conversions/mod.rs b/src/types/array/conversions/mod.rs index fbcc4c7f0..4d4cb4086 100644 --- a/src/types/array/conversions/mod.rs +++ b/src/types/array/conversions/mod.rs @@ -1,8 +1,8 @@ //! Collection type conversions for `ZendHashTable`. //! -//! This module provides conversions between Rust collection types and PHP arrays -//! (represented as `ZendHashTable`). Each collection type has its own module for -//! better organization and maintainability. +//! This module provides conversions between Rust collection types and PHP +//! arrays (represented as `ZendHashTable`). Each collection type has its own +//! module for better organization and maintainability. //! //! ## Supported Collections //! diff --git a/src/types/array/mod.rs b/src/types/array/mod.rs index a43e024a3..71f93d3b0 100644 --- a/src/types/array/mod.rs +++ b/src/types/array/mod.rs @@ -446,9 +446,11 @@ impl ZendHashTable { } ArrayKey::String(key) => { unsafe { + // Use raw bytes directly since zend_hash_str_update takes a length. + // This allows keys with embedded null bytes (e.g. PHP property mangling). zend_hash_str_update( self, - CString::new(key.as_str())?.as_ptr(), + key.as_str().as_ptr().cast(), key.len(), &raw mut val, ) @@ -456,7 +458,9 @@ impl ZendHashTable { } ArrayKey::Str(key) => { unsafe { - zend_hash_str_update(self, CString::new(key)?.as_ptr(), key.len(), &raw mut val) + // Use raw bytes directly since zend_hash_str_update takes a length. + // This allows keys with embedded null bytes (e.g. PHP property mangling). + zend_hash_str_update(self, key.as_ptr().cast(), key.len(), &raw mut val) }; } } diff --git a/src/types/zval.rs b/src/types/zval.rs index a96310bfa..de9ed6775 100644 --- a/src/types/zval.rs +++ b/src/types/zval.rs @@ -518,8 +518,8 @@ impl Zval { self.get_type() == DataType::Ptr } - /// Returns true if the zval is a scalar value (integer, float, string, or bool), - /// false otherwise. + /// Returns true if the zval is a scalar value (integer, float, string, or + /// bool), false otherwise. /// /// This is equivalent to PHP's `is_scalar()` function. #[must_use] diff --git a/src/zend/handlers.rs b/src/zend/handlers.rs index 6f4f680cd..9c43255ee 100644 --- a/src/zend/handlers.rs +++ b/src/zend/handlers.rs @@ -1,14 +1,15 @@ -use std::{ffi::c_void, mem::MaybeUninit, os::raw::c_int, ptr}; +use std::{ffi::CString, ffi::c_void, mem::MaybeUninit, os::raw::c_int, ptr}; use crate::{ class::RegisteredClass, exception::PhpResult, ffi::{ - std_object_handlers, zend_is_true, zend_object_handlers, zend_object_std_dtor, + 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_std_get_properties, zend_std_has_property, zend_std_read_property, - zend_std_write_property, + zend_std_write_property, zend_throw_error, }, - flags::ZvalTypeFlags, + flags::{PropertyFlags, ZvalTypeFlags}, types::{ZendClassObject, ZendHashTable, ZendObject, ZendStr, Zval}, }; @@ -108,6 +109,19 @@ impl ZendObjectHandlers { Ok(match prop { Some(prop_info) => { + // Check visibility before allowing access + let object_ce = unsafe { (*object).ce }; + if !unsafe { check_property_access(prop_info.flags, object_ce) } { + let is_private = prop_info.flags.contains(PropertyFlags::Private); + unsafe { + throw_property_access_error( + T::CLASS_NAME, + prop_name.as_str()?, + is_private, + ); + } + return Ok(rv); + } prop_info.prop.get(self_, rv_mut)?; rv } @@ -158,6 +172,19 @@ impl ZendObjectHandlers { Ok(match prop { Some(prop_info) => { + // Check visibility before allowing access + let object_ce = unsafe { (*object).ce }; + if !unsafe { check_property_access(prop_info.flags, object_ce) } { + let is_private = prop_info.flags.contains(PropertyFlags::Private); + unsafe { + throw_property_access_error( + T::CLASS_NAME, + prop_name.as_str()?, + is_private, + ); + } + return Ok(value); + } prop_info.prop.set(self_, value_mut)?; value } @@ -198,7 +225,19 @@ impl ZendObjectHandlers { if val.prop.get(self_, &mut zv).is_err() { continue; } - props.insert(name, zv).map_err(|e| { + + // Mangle property name according to visibility for debug output + // PHP convention: private = "\0ClassName\0propName", protected = + // "\0*\0propName" + let mangled_name = if val.flags.contains(PropertyFlags::Private) { + format!("\0{}\0{name}", T::CLASS_NAME) + } else if val.flags.contains(PropertyFlags::Protected) { + format!("\0*\0{name}") + } else { + name.to_string() + }; + + props.insert(mangled_name.as_str(), zv).map_err(|e| { format!("Failed to insert value into properties hashtable: {e:?}") })?; } @@ -309,3 +348,94 @@ impl ZendObjectHandlers { } } } + +/// Gets the current calling scope from the executor globals. +/// +/// # Safety +/// +/// Must only be called during PHP execution when executor globals are valid. +#[inline] +unsafe fn get_calling_scope() -> *const zend_class_entry { + let eg = unsafe { ext_php_rs_executor_globals().as_ref() }; + let Some(eg) = eg else { + return ptr::null(); + }; + let execute_data = eg.current_execute_data; + + if execute_data.is_null() { + return ptr::null(); + } + + let func = unsafe { (*execute_data).func }; + if func.is_null() { + return ptr::null(); + } + + // Access the common.scope field through the union + unsafe { (*func).common.scope } +} + +/// Checks if the calling scope has access to a property with the given flags. +/// +/// Returns `true` if access is allowed, `false` otherwise. +/// +/// # Safety +/// +/// Must only be called during PHP execution when executor globals are valid. +/// The `object_ce` pointer must be valid. +#[inline] +unsafe fn check_property_access(flags: PropertyFlags, object_ce: *const zend_class_entry) -> bool { + // Public properties are always accessible + if !flags.contains(PropertyFlags::Private) && !flags.contains(PropertyFlags::Protected) { + return true; + } + + let calling_scope = unsafe { get_calling_scope() }; + + if flags.contains(PropertyFlags::Private) { + // Private: must be called from the exact same class + return calling_scope == object_ce; + } + + if flags.contains(PropertyFlags::Protected) { + // Protected: must be called from same class or a subclass + if calling_scope.is_null() { + return false; + } + + // Same class check + if calling_scope == object_ce { + return true; + } + + // Check if calling_scope is a subclass of object_ce + // or if object_ce is a subclass of calling_scope (for parent access) + unsafe { + instanceof_function_slow(calling_scope, object_ce) + || instanceof_function_slow(object_ce, calling_scope) + } + } else { + true + } +} + +/// Throws an error for invalid property access. +/// +/// # Safety +/// +/// Must only be called during PHP execution. +/// +/// # Panics +/// +/// Panics if the error message cannot be converted to a `CString`. +unsafe fn throw_property_access_error(class_name: &str, prop_name: &str, is_private: bool) { + let visibility = if is_private { "private" } else { "protected" }; + let message = CString::new(format!( + "Cannot access {visibility} property {class_name}::${prop_name}" + )) + .expect("Failed to create error message"); + + unsafe { + zend_throw_error(ptr::null_mut(), message.as_ptr()); + } +} diff --git a/tests/src/integration/array/mod.rs b/tests/src/integration/array/mod.rs index 2e3f39be2..2cc5e1964 100644 --- a/tests/src/integration/array/mod.rs +++ b/tests/src/integration/array/mod.rs @@ -47,7 +47,8 @@ pub fn test_optional_array_ref(arr: Option<&ZendHashTable>) -> i64 { arr.map_or(-1, |ht| i64::try_from(ht.len()).unwrap_or(i64::MAX)) } -/// Test that `Option<&mut ZendHashTable>` works correctly (anti-regression for issue #515) +/// Test that `Option<&mut ZendHashTable>` works correctly (anti-regression for +/// issue #515) #[php_function] pub fn test_optional_array_mut_ref(arr: Option<&mut ZendHashTable>) -> i64 { match arr { diff --git a/tests/src/integration/class/class.php b/tests/src/integration/class/class.php index 13b98b962..afcccb8d1 100644 --- a/tests/src/integration/class/class.php +++ b/tests/src/integration/class/class.php @@ -122,3 +122,37 @@ // Test returning &Self (immutable reference) $selfRef = $builder2->getSelf(); assert($selfRef === $builder2, 'getSelf should return $this'); + +// Test property visibility (Issue #375) +$visibilityObj = new TestPropertyVisibility(42, 'private_data', 'protected_data'); + +// Test public property - should work +assert($visibilityObj->publicNum === 42, 'Public property read should work'); +$visibilityObj->publicNum = 100; +assert($visibilityObj->publicNum === 100, 'Public property write should work'); + +// Test accessing private property through class methods - should work +assert($visibilityObj->getPrivate() === 'private_data', 'Private property access via method should work'); +$visibilityObj->setPrivate('new_private'); +assert($visibilityObj->getPrivate() === 'new_private', 'Private property set via method should work'); + +// Test accessing protected property through class methods - should work +assert($visibilityObj->getProtected() === 'protected_data', 'Protected property access via method should work'); +$visibilityObj->setProtected('new_protected'); +assert($visibilityObj->getProtected() === 'new_protected', 'Protected property set via method should work'); + +// Test that direct access to private property throws an error +assert_exception_thrown(fn() => $visibilityObj->privateStr, 'Reading private property should throw'); +assert_exception_thrown(fn() => $visibilityObj->privateStr = 'test', 'Writing private property should throw'); + +// Test that direct access to protected property throws an error +assert_exception_thrown(fn() => $visibilityObj->protectedStr, 'Reading protected property should throw'); +assert_exception_thrown(fn() => $visibilityObj->protectedStr = 'test', 'Writing protected property should throw'); + +// Test var_dump shows mangled names for private/protected properties +ob_start(); +var_dump($visibilityObj); +$output = ob_get_clean(); +assert(strpos($output, 'publicNum') !== false, 'var_dump should show public property'); +// Private properties should show as ClassName::propertyName in var_dump +// Protected properties should show with * prefix diff --git a/tests/src/integration/class/mod.rs b/tests/src/integration/class/mod.rs index 6a547ca46..070ff13ac 100644 --- a/tests/src/integration/class/mod.rs +++ b/tests/src/integration/class/mod.rs @@ -277,6 +277,51 @@ impl FluentBuilder { } } +/// Test class for property visibility (Issue #375) +#[php_class] +pub struct TestPropertyVisibility { + /// Public property - accessible from anywhere + #[php(prop)] + pub public_num: i32, + /// Private property - only accessible from within the class + #[php(prop, flags = ext_php_rs::flags::PropertyFlags::Private)] + pub private_str: String, + /// Protected property - accessible from class and subclasses + #[php(prop, flags = ext_php_rs::flags::PropertyFlags::Protected)] + pub protected_str: String, +} + +#[php_impl] +impl TestPropertyVisibility { + pub fn __construct(public_num: i32, private_str: String, protected_str: String) -> Self { + Self { + public_num, + private_str, + protected_str, + } + } + + /// Method to access private property from within the class + pub fn get_private(&self) -> String { + self.private_str.clone() + } + + /// Method to access protected property from within the class + pub fn get_protected(&self) -> String { + self.protected_str.clone() + } + + /// Method to set private property from within the class + pub fn set_private(&mut self, value: String) { + self.private_str = value; + } + + /// Method to set protected property from within the class + pub fn set_protected(&mut self, value: String) { + self.protected_str = value; + } +} + pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { builder .class::() @@ -287,6 +332,7 @@ pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { .class::() .class::() .class::() + .class::() .function(wrap_function!(test_class)) .function(wrap_function!(throw_exception)) } diff --git a/tests/src/integration/mod.rs b/tests/src/integration/mod.rs index d58388e5a..e8dedccc0 100644 --- a/tests/src/integration/mod.rs +++ b/tests/src/integration/mod.rs @@ -39,7 +39,8 @@ mod test { command.arg("--release"); // Build features list dynamically based on compiled features - // Note: Using vec_init_then_push pattern here is intentional due to conditional compilation + // Note: Using vec_init_then_push pattern here is intentional due to conditional + // compilation #[allow(clippy::vec_init_then_push)] { let mut features = vec![];