Fix GH-19044: Protected properties are not scoped according to their prototype #19046
+279
−5
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| --TEST-- | ||
| GH-19044: Protected properties must be scoped according to their prototype (protected(set) on non-hooked property) | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| class P { | ||
| public mixed $foo { get => 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| --TEST-- | ||
| GH-19044: Protected properties must be scoped according to their prototype | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| abstract class P { | ||
| protected $foo; | ||
| } | ||
|
|
||
| class C1 extends P { | ||
| protected $foo = 1; | ||
| } | ||
|
|
||
| class C2 extends P { | ||
| protected $foo = 2; | ||
|
|
||
| static function foo($c) { return $c->foo; } | ||
| } | ||
|
|
||
| var_dump(C2::foo(new C2)); | ||
| var_dump(C2::foo(new C1)); | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| int(2) | ||
| int(1) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| --TEST-- | ||
| GH-19044: Protected properties must be scoped according to their prototype (common ancestor has a protected setter) | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| abstract class P { | ||
| abstract public mixed $foo { get; } | ||
| } | ||
|
|
||
| class C1 extends P { | ||
| public protected(set) mixed $foo { get => 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) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| --TEST-- | ||
| GH-19044: Protected properties must be scoped according to their prototype (common ancestor does not have a setter) | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| abstract class P { | ||
| abstract public mixed $foo { get; } | ||
| } | ||
|
|
||
| class C1 extends P { | ||
| public mixed $foo { get => 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| --TEST-- | ||
| GH-19044: Protected properties must be scoped according to their prototype (abstract parent defining visibility only takes precedence) | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| abstract class P { | ||
| abstract protected(set) mixed $foo { get; set; } | ||
| } | ||
|
|
||
| class C1 extends P { | ||
| public protected(set) mixed $foo { get => 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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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-- | ||
| <?php | ||
|
|
||
| abstract class GP { | ||
| abstract mixed $foo { get; } | ||
| } | ||
|
|
||
| abstract class P extends GP { | ||
| abstract protected(set) mixed $foo { get; set; } | ||
| } | ||
|
|
||
| class C1 extends P { | ||
| public protected(set) mixed $foo { get => 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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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-- | ||
| <?php | ||
|
|
||
| abstract class GP { | ||
| abstract mixed $foo { get; } | ||
| } | ||
|
|
||
| abstract class P extends GP { | ||
| abstract protected(set) mixed $foo { get; set; } | ||
| } | ||
|
|
||
| class C1 extends P { | ||
| public protected(set) mixed $foo { get => 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| --TEST-- | ||
| GH-19044: Protected properties must be scoped according to their prototype (abstract parent has implicit set hook) | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| abstract class GP { | ||
| public abstract mixed $foo { get; } | ||
| } | ||
|
|
||
| class P extends GP { | ||
| public protected(set) mixed $foo { get => $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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| --TEST-- | ||
| GH-19044: Protected properties must be scoped according to their prototype (hooks variation) | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| abstract class P { | ||
| abstract protected $foo { get; } | ||
| } | ||
|
|
||
| class C1 extends P { | ||
| protected $foo = 1; | ||
| } | ||
|
|
||
| class C2 extends P { | ||
| protected $foo = 2; | ||
|
|
||
| static function foo($c) { return $c->foo; } | ||
| } | ||
|
|
||
| var_dump(C2::foo(new C2)); | ||
| var_dump(C2::foo(new C1)); | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| int(2) | ||
| int(1) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -282,7 +282,31 @@ 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 && | ||
| (ce == scope || is_derived_class(ce, scope) || is_derived_class(scope, ce)); | ||
| } | ||
| /* }}} */ | ||
|
|
||
| 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]) { | ||
|
There was a problem hiding this comment. I believe this is wrong still. The <?php
abstract class GP {
public abstract mixed $foo { get; }
}
class P extends GP {
public protected(set) mixed $foo;
}
class C1 extends P {
public protected(set) mixed $foo = 1 { set { parent::$foo::set($value); } }
}
class C2 extends P {
public protected(set) mixed $foo;
static function foo($c) { return $c->foo += 1; }
}
var_dump(C2::foo(new C1));
// Fatal error: Uncaught Error: Cannot modify protected(set) property C1::$foo from scope C2 in %s:%dThere was a problem hiding this comment. Oh, I assumed that you cannot put a setter in a child when the parent is a bare property for some reason, well, yeah, then let's just remove that branch. |
||
| /* 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)]; | ||
|
There was a problem hiding this comment. It's not safe to index into There was a problem hiding this comment. Yes, it's not safe, but that led to #19053. There was a problem hiding this comment. So yeah, the only way to iterate is then the very expensive hashtable lookup? At that point I think maybe we should simply admit defeat and go by the prototype only... |
||
| } while (info->flags & ZEND_ACC_PROTECTED_SET); | ||
|
There was a problem hiding this comment. Shouldn't we stop this loop before setting <?php
abstract class GP {
public abstract mixed $foo { get; }
public $dummy;
}
class P1 extends GP {
public protected(set) mixed $foo;
}
class P2 extends GP {
public protected(set) mixed $foo;
}
class C1 extends P1 {
public protected(set) mixed $foo = 1;
}
class C2 extends P2 {
public protected(set) mixed $foo;
static function foo($c) { return $c->foo += 1; }
}
var_dump(C2::foo(new C1));This crashes due to out-of-bounds first due to the error above, but I believe after fixing that this would mistakenly allow the assignment. |
||
| } | ||
| return is_protected_compatible_scope(ce, scope); | ||
| } | ||
| /* }}} */ | ||
|
|
||
|
|
@@ -419,7 +443,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->prototype->ce, scope))) { | ||
| goto wrong; | ||
| } | ||
| } | ||
|
|
@@ -514,7 +538,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->prototype->ce, scope))) { | ||
| goto wrong; | ||
| } | ||
| } | ||
|
|
@@ -585,7 +609,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)); | ||
| } | ||
|
|
||
| static void zend_property_guard_dtor(zval *el) /* {{{ */ { | ||
|
|
@@ -2030,7 +2054,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->prototype->ce, scope))) { | ||
| if (type != BP_VAR_IS) { | ||
| zend_bad_property_access(property_info, ce, property_name); | ||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming is quite confusing. It's the child of C1, grand implies grand-child or grand-parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C1 = Child 1, hence GrandC1 :-D (Just like GrandP in some other test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how exactly to rename them, but feel free to just push to my branch for that.