Skip to content

Don't call __set() on uninitialized typed properties (Z_EXTRA variant) #4856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Oct 24, 2019

Same as #4854 but based on Z_EXTRA rather than TYPE_FLAG.

Zend/zend_API.c Outdated
@@ -1264,12 +1264,14 @@ static zend_always_inline void _object_properties_init(zend_object *object, zend
if (UNEXPECTED(class_type->type == ZEND_INTERNAL_CLASS)) {
do {
ZVAL_COPY_OR_DUP(dst, src);
Z_EXTRA_P(dst) = Z_EXTRA_P(src);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to introduce special macros instead of generic Z_EXTRA_P(), to easily find all relevant usages.

@@ -270,7 +270,7 @@ static void zend_class_copy_ctor(zend_class_entry **pce)
end = src + ce->default_properties_count;
ce->default_properties_table = dst;
for (; src != end; src++, dst++) {
ZVAL_COPY_VALUE(dst, src);
*dst = *src;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments required

@@ -1158,7 +1158,7 @@ ZEND_API void zend_do_inheritance_ex(zend_class_entry *ce, zend_class_entry *par
do {
dst--;
src--;
ZVAL_COPY_VALUE(dst, src);
*dst = *src; /* Copy Z_EXTRA as well */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and later: better to keep the existing code under #if 0 with comments about optimization (or introduce special macro ZVAL_COPY_PROP_VALUE)

nikic added 2 commits October 25, 2019 15:51
Also don't set it for static properties -- this is only relevant
for instance properties, as static properties have no __set().
@nikic
Copy link
Member Author

nikic commented Oct 25, 2019

Merged as f1848a4.

@nikic nikic closed this Oct 25, 2019
@Majkl578
Copy link
Contributor

Ad:

For PHP 8, we should fine a way to forbid unsetting of declared
properties entirely, and provide a different way to achieve lazy
initialization.

I suppose this is just another case that could be solved by having property accessors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants