From 1fa44f7dc4c28b6a3add2c80db99b3f501bdfbef Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Sun, 6 Jul 2025 00:31:05 +0200 Subject: [PATCH 1/3] Fix GH-19044: Protected properties are not scoped according to their prototype --- NEWS | 2 ++ Zend/tests/gh19044.phpt | 26 +++++++++++++++++++ Zend/tests/property_hooks/gh19044-1.phpt | 26 +++++++++++++++++++ Zend/tests/property_hooks/gh19044-2.phpt | 30 ++++++++++++++++++++++ Zend/tests/property_hooks/gh19044-3.phpt | 24 ++++++++++++++++++ Zend/tests/property_hooks/gh19044-4.phpt | 28 +++++++++++++++++++++ Zend/tests/property_hooks/gh19044-5.phpt | 32 ++++++++++++++++++++++++ Zend/tests/property_hooks/gh19044.phpt | 26 +++++++++++++++++++ Zend/zend_object_handlers.c | 23 +++++++++++++---- 9 files changed, 212 insertions(+), 5 deletions(-) create mode 100644 Zend/tests/gh19044.phpt create mode 100644 Zend/tests/property_hooks/gh19044-1.phpt create mode 100644 Zend/tests/property_hooks/gh19044-2.phpt create mode 100644 Zend/tests/property_hooks/gh19044-3.phpt create mode 100644 Zend/tests/property_hooks/gh19044-4.phpt create mode 100644 Zend/tests/property_hooks/gh19044-5.phpt create mode 100644 Zend/tests/property_hooks/gh19044.phpt diff --git a/NEWS b/NEWS index 3467754039cb8..7d044646a6186 100644 --- a/NEWS +++ b/NEWS @@ -11,6 +11,8 @@ PHP NEWS . Fixed bug GH-18907 (Leak when creating cycle in hook). (ilutov) . Fix OSS-Fuzz #427814456. (nielsdos) . Fix OSS-Fuzz #428983568 and #428760800. (nielsdos) + . Fixed bug GH-19044 (Protected properties are not scoped according to their + prototype). (Bob) - Curl: . Fix memory leaks when returning refcounted value from curl callback. diff --git a/Zend/tests/gh19044.phpt b/Zend/tests/gh19044.phpt new file mode 100644 index 0000000000000..3477dd3c8b08a --- /dev/null +++ b/Zend/tests/gh19044.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-19044: Protected properties must be scoped according to their prototype +--FILE-- +foo; } +} + +var_dump(C2::foo(new C2)); +var_dump(C2::foo(new C1)); + +?> +--EXPECT-- +int(2) +int(1) diff --git a/Zend/tests/property_hooks/gh19044-1.phpt b/Zend/tests/property_hooks/gh19044-1.phpt new file mode 100644 index 0000000000000..133727c73def8 --- /dev/null +++ b/Zend/tests/property_hooks/gh19044-1.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-19044: Protected properties must be scoped according to their prototype (common ancestor has a protected setter) +--FILE-- + 1; set {} } +} + +class GrandC1 extends C1 { + public protected(set) mixed $foo { get => 2; set {} } +} + +class C2 extends C1 { + static function foo($c) { return $c->foo += 1; } +} + +var_dump(C2::foo(new GrandC1)); + +?> +--EXPECT-- +int(3) diff --git a/Zend/tests/property_hooks/gh19044-2.phpt b/Zend/tests/property_hooks/gh19044-2.phpt new file mode 100644 index 0000000000000..550b1b64df8ad --- /dev/null +++ b/Zend/tests/property_hooks/gh19044-2.phpt @@ -0,0 +1,30 @@ +--TEST-- +GH-19044: Protected properties must be scoped according to their prototype (common ancestor does not have a setter) +--FILE-- + 1; } +} + +class GrandC1 extends C1 { + public protected(set) mixed $foo { get => 2; set {} } +} + +class C2 extends C1 { + static function foo($c) { return $c->foo += 1; } +} + +var_dump(C2::foo(new GrandC1)); + +?> +--EXPECTF-- +Fatal error: Uncaught Error: Cannot modify protected(set) property GrandC1::$foo from scope C2 in %s:%d +Stack trace: +#0 %s(%d): C2::foo(Object(GrandC1)) +#1 {main} + thrown in %s on line %d diff --git a/Zend/tests/property_hooks/gh19044-3.phpt b/Zend/tests/property_hooks/gh19044-3.phpt new file mode 100644 index 0000000000000..3d4f6789a5a37 --- /dev/null +++ b/Zend/tests/property_hooks/gh19044-3.phpt @@ -0,0 +1,24 @@ +--TEST-- +GH-19044: Protected properties must be scoped according to their prototype (abstract parent defining visibility only takes precedence) +--FILE-- + 2; set {} } +} + +class C2 extends P { + public mixed $foo = 1; + + static function foo($c) { return $c->foo += 1; } +} + +var_dump(C2::foo(new C1)); + +?> +--EXPECT-- +int(3) diff --git a/Zend/tests/property_hooks/gh19044-4.phpt b/Zend/tests/property_hooks/gh19044-4.phpt new file mode 100644 index 0000000000000..e1abf47390831 --- /dev/null +++ b/Zend/tests/property_hooks/gh19044-4.phpt @@ -0,0 +1,28 @@ +--TEST-- +GH-19044: Protected properties must be scoped according to their prototype (abstract parent sets protected(set) with not having grandparent a setter - both inherit from parent) +--FILE-- + 2; set {} } +} + +class C2 extends P { + public mixed $foo = 1; + + static function foo($c) { return $c->foo += 1; } +} + +var_dump(C2::foo(new C1)); + +?> +--EXPECT-- +int(3) diff --git a/Zend/tests/property_hooks/gh19044-5.phpt b/Zend/tests/property_hooks/gh19044-5.phpt new file mode 100644 index 0000000000000..80e110859a57a --- /dev/null +++ b/Zend/tests/property_hooks/gh19044-5.phpt @@ -0,0 +1,32 @@ +--TEST-- +GH-19044: Protected properties must be scoped according to their prototype (abstract parent sets protected(set) with not having grandparent a setter - one inherits from grandparent) +--FILE-- + 2; set {} } +} + +class C2 extends GP { + public mixed $foo = 1; + + static function foo($c) { return $c->foo += 1; } +} + +var_dump(C2::foo(new C1)); + +?> +--EXPECTF-- +Fatal error: Uncaught Error: Cannot modify protected(set) property C1::$foo from scope C2 in %s:%d +Stack trace: +#0 %s(%d): C2::foo(Object(C1)) +#1 {main} + thrown in %s on line %d diff --git a/Zend/tests/property_hooks/gh19044.phpt b/Zend/tests/property_hooks/gh19044.phpt new file mode 100644 index 0000000000000..2fa62e4619f56 --- /dev/null +++ b/Zend/tests/property_hooks/gh19044.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-19044: Protected properties must be scoped according to their prototype (hooks variation) +--FILE-- +foo; } +} + +var_dump(C2::foo(new C2)); +var_dump(C2::foo(new C1)); + +?> +--EXPECT-- +int(2) +int(1) diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index 2ddaeae96e999..47ed7eb362a19 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -282,7 +282,20 @@ static zend_always_inline bool is_derived_class(const zend_class_entry *child_cl static zend_never_inline int is_protected_compatible_scope(const zend_class_entry *ce, const zend_class_entry *scope) /* {{{ */ { return scope && - (is_derived_class(ce, scope) || is_derived_class(scope, ce)); + (ce == scope || is_derived_class(ce, scope) || is_derived_class(scope, ce)); +} +/* }}} */ + +static zend_never_inline int is_asymmetric_set_protected_property_compatible_scope(const zend_property_info *info, const zend_class_entry *scope) /* {{{ */ +{ + zend_class_entry *ce; + if (!(info->prototype->flags & ZEND_ACC_PROTECTED_SET) && info->hooks && info->hooks[ZEND_PROPERTY_HOOK_SET]) { + zend_function *hookfn = info->hooks[ZEND_PROPERTY_HOOK_SET]; + ce = hookfn->common.prototype ? hookfn->common.prototype->common.scope : hookfn->common.scope; + } else { + ce = info->prototype->ce; + } + return is_protected_compatible_scope(ce, scope); } /* }}} */ @@ -419,7 +432,7 @@ static zend_always_inline uintptr_t zend_get_property_offset(zend_class_entry *c } } else { ZEND_ASSERT(flags & ZEND_ACC_PROTECTED); - if (UNEXPECTED(!is_protected_compatible_scope(property_info->ce, scope))) { + if (UNEXPECTED(!is_protected_compatible_scope(property_info->prototype->ce, scope))) { goto wrong; } } @@ -514,7 +527,7 @@ ZEND_API zend_property_info *zend_get_property_info(const zend_class_entry *ce, } } else { ZEND_ASSERT(flags & ZEND_ACC_PROTECTED); - if (UNEXPECTED(!is_protected_compatible_scope(property_info->ce, scope))) { + if (UNEXPECTED(!is_protected_compatible_scope(property_info->prototype->ce, scope))) { goto wrong; } } @@ -585,7 +598,7 @@ ZEND_API bool ZEND_FASTCALL zend_asymmetric_property_has_set_access(const zend_p return true; } return EXPECTED((prop_info->flags & ZEND_ACC_PROTECTED_SET) - && is_protected_compatible_scope(prop_info->ce, scope)); + && is_asymmetric_set_protected_property_compatible_scope(prop_info, scope)); } static void zend_property_guard_dtor(zval *el) /* {{{ */ { @@ -2030,7 +2043,7 @@ ZEND_API zval *zend_std_get_static_property_with_info(zend_class_entry *ce, zend zend_class_entry *scope = get_fake_or_executed_scope(); if (property_info->ce != scope) { if (UNEXPECTED(property_info->flags & ZEND_ACC_PRIVATE) - || UNEXPECTED(!is_protected_compatible_scope(property_info->ce, scope))) { + || UNEXPECTED(!is_protected_compatible_scope(property_info->prototype->ce, scope))) { if (type != BP_VAR_IS) { zend_bad_property_access(property_info, ce, property_name); } From 825ee3f8d9b706822e6592032d67f56aa51f9d14 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Sun, 6 Jul 2025 15:34:01 +0200 Subject: [PATCH 2/3] Adjust after review --- Zend/tests/asymmetric_visibility/gh19044.phpt | 28 +++++++++++++++++++ Zend/tests/property_hooks/gh19044-6.phpt | 28 +++++++++++++++++++ Zend/zend_object_handlers.c | 21 ++++++++++---- 3 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 Zend/tests/asymmetric_visibility/gh19044.phpt create mode 100644 Zend/tests/property_hooks/gh19044-6.phpt diff --git a/Zend/tests/asymmetric_visibility/gh19044.phpt b/Zend/tests/asymmetric_visibility/gh19044.phpt new file mode 100644 index 0000000000000..adedb79e08c73 --- /dev/null +++ b/Zend/tests/asymmetric_visibility/gh19044.phpt @@ -0,0 +1,28 @@ +--TEST-- +GH-19044: Protected properties must be scoped according to their prototype (protected(set) on non-hooked property) +--FILE-- + 42; } +} + +class C1 extends P { + public protected(set) mixed $foo = 1; +} + +class C2 extends P { + public protected(set) mixed $foo; + + static function foo($c) { return $c->foo += 1; } +} + +var_dump(C2::foo(new C1)); + +?> +--EXPECTF-- +Fatal error: Uncaught Error: Cannot modify protected(set) property C1::$foo from scope C2 in %s:%d +Stack trace: +#0 %s(%d): C2::foo(Object(C1)) +#1 {main} + thrown in %s on line %d diff --git a/Zend/tests/property_hooks/gh19044-6.phpt b/Zend/tests/property_hooks/gh19044-6.phpt new file mode 100644 index 0000000000000..43332698fe9f7 --- /dev/null +++ b/Zend/tests/property_hooks/gh19044-6.phpt @@ -0,0 +1,28 @@ +--TEST-- +GH-19044: Protected properties must be scoped according to their prototype (abstract parent has implicit set hook) +--FILE-- + $this->foo; } +} + +class C1 extends P { + public protected(set) mixed $foo = 1; +} + +class C2 extends P { + public protected(set) mixed $foo; + + static function foo($c) { return $c->foo += 1; } +} + +var_dump(C2::foo(new C1)); + +?> +--EXPECT-- +int(2) diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index 47ed7eb362a19..424051ca1db2f 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -286,14 +286,25 @@ static zend_never_inline int is_protected_compatible_scope(const zend_class_entr } /* }}} */ -static zend_never_inline int is_asymmetric_set_protected_property_compatible_scope(const zend_property_info *info, const zend_class_entry *scope) /* {{{ */ +static int is_asymmetric_set_protected_property_compatible_scope(const zend_property_info *info, const zend_class_entry *scope) /* {{{ */ { zend_class_entry *ce; - if (!(info->prototype->flags & ZEND_ACC_PROTECTED_SET) && info->hooks && info->hooks[ZEND_PROPERTY_HOOK_SET]) { - zend_function *hookfn = info->hooks[ZEND_PROPERTY_HOOK_SET]; - ce = hookfn->common.prototype ? hookfn->common.prototype->common.scope : hookfn->common.scope; + /* we need to identify the common protected(set) ancestor: if the prototype has the protected(set), it's straightforward */ + if (info->prototype->flags & ZEND_ACC_PROTECTED_SET) { + ce = info->prototype->ce; + } else if (info->hooks && info->hooks[ZEND_PROPERTY_HOOK_SET]) { + /* shortcut: the visibility of hooks cannot be overwritten */ + zend_function *hook = info->hooks[ZEND_PROPERTY_HOOK_SET]; + ce = zend_get_function_root_class(hook); } else { - ce = info->prototype->ce; + /* we do not have an easy way to find the ancestor which introduces the protected(set), let's iterate */ + do { + ce = info->ce; + if (!ce->parent->properties_info_table) { + break; + } + info = ce->parent->properties_info_table[OBJ_PROP_TO_NUM(info->offset)]; + } while (info->flags & ZEND_ACC_PROTECTED_SET); } return is_protected_compatible_scope(ce, scope); } From cc51bf8949706c6bedddfa2dc22e1998ae46ed3a Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Sun, 6 Jul 2025 19:26:38 +0200 Subject: [PATCH 3/3] Simplify to using prototype even for asymmetric visibility --- Zend/tests/asymmetric_visibility/gh19044.phpt | 8 ++---- Zend/tests/property_hooks/gh19044-2.phpt | 8 ++---- Zend/tests/property_hooks/gh19044-5.phpt | 8 ++---- Zend/zend_object_handlers.c | 26 +------------------ 4 files changed, 7 insertions(+), 43 deletions(-) diff --git a/Zend/tests/asymmetric_visibility/gh19044.phpt b/Zend/tests/asymmetric_visibility/gh19044.phpt index adedb79e08c73..253e315d5b945 100644 --- a/Zend/tests/asymmetric_visibility/gh19044.phpt +++ b/Zend/tests/asymmetric_visibility/gh19044.phpt @@ -20,9 +20,5 @@ class C2 extends P { var_dump(C2::foo(new C1)); ?> ---EXPECTF-- -Fatal error: Uncaught Error: Cannot modify protected(set) property C1::$foo from scope C2 in %s:%d -Stack trace: -#0 %s(%d): C2::foo(Object(C1)) -#1 {main} - thrown in %s on line %d +--EXPECT-- +int(43) diff --git a/Zend/tests/property_hooks/gh19044-2.phpt b/Zend/tests/property_hooks/gh19044-2.phpt index 550b1b64df8ad..ed73c60dc5a3a 100644 --- a/Zend/tests/property_hooks/gh19044-2.phpt +++ b/Zend/tests/property_hooks/gh19044-2.phpt @@ -22,9 +22,5 @@ class C2 extends C1 { var_dump(C2::foo(new GrandC1)); ?> ---EXPECTF-- -Fatal error: Uncaught Error: Cannot modify protected(set) property GrandC1::$foo from scope C2 in %s:%d -Stack trace: -#0 %s(%d): C2::foo(Object(GrandC1)) -#1 {main} - thrown in %s on line %d +--EXPECT-- +int(3) diff --git a/Zend/tests/property_hooks/gh19044-5.phpt b/Zend/tests/property_hooks/gh19044-5.phpt index 80e110859a57a..773682961ab41 100644 --- a/Zend/tests/property_hooks/gh19044-5.phpt +++ b/Zend/tests/property_hooks/gh19044-5.phpt @@ -24,9 +24,5 @@ class C2 extends GP { var_dump(C2::foo(new C1)); ?> ---EXPECTF-- -Fatal error: Uncaught Error: Cannot modify protected(set) property C1::$foo from scope C2 in %s:%d -Stack trace: -#0 %s(%d): C2::foo(Object(C1)) -#1 {main} - thrown in %s on line %d +--EXPECT-- +int(3) diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index 424051ca1db2f..13a74e1725c28 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -286,30 +286,6 @@ static zend_never_inline int is_protected_compatible_scope(const zend_class_entr } /* }}} */ -static int is_asymmetric_set_protected_property_compatible_scope(const zend_property_info *info, const zend_class_entry *scope) /* {{{ */ -{ - zend_class_entry *ce; - /* we need to identify the common protected(set) ancestor: if the prototype has the protected(set), it's straightforward */ - if (info->prototype->flags & ZEND_ACC_PROTECTED_SET) { - ce = info->prototype->ce; - } else if (info->hooks && info->hooks[ZEND_PROPERTY_HOOK_SET]) { - /* shortcut: the visibility of hooks cannot be overwritten */ - zend_function *hook = info->hooks[ZEND_PROPERTY_HOOK_SET]; - ce = zend_get_function_root_class(hook); - } else { - /* we do not have an easy way to find the ancestor which introduces the protected(set), let's iterate */ - do { - ce = info->ce; - if (!ce->parent->properties_info_table) { - break; - } - info = ce->parent->properties_info_table[OBJ_PROP_TO_NUM(info->offset)]; - } while (info->flags & ZEND_ACC_PROTECTED_SET); - } - return is_protected_compatible_scope(ce, scope); -} -/* }}} */ - static zend_never_inline zend_property_info *zend_get_parent_private_property(zend_class_entry *scope, const zend_class_entry *ce, zend_string *member) /* {{{ */ { zval *zv; @@ -609,7 +585,7 @@ ZEND_API bool ZEND_FASTCALL zend_asymmetric_property_has_set_access(const zend_p return true; } return EXPECTED((prop_info->flags & ZEND_ACC_PROTECTED_SET) - && is_asymmetric_set_protected_property_compatible_scope(prop_info, scope)); + && is_protected_compatible_scope(prop_info->prototype->ce, scope)); } static void zend_property_guard_dtor(zval *el) /* {{{ */ {