Skip to content

Fix property_info sizing for locally shadowed trait properties#21358

Open
iluuu1994 wants to merge 1 commit intophp:PHP-8.5from
iluuu1994:gh-20672
Open

Fix property_info sizing for locally shadowed trait properties#21358
iluuu1994 wants to merge 1 commit intophp:PHP-8.5from
iluuu1994:gh-20672

Conversation

@iluuu1994
Copy link
Member

Previously, static trait properties would always redeclare locally declared static properties to make sure an inherited static property stops sharing a common slot with the parent. This would leave holes in property_info, creating issues for this code:

zend_hash_extend(&ce->properties_info,
    zend_hash_num_elements(&ce->properties_info) +
    zend_hash_num_elements(&parent_ce->properties_info), 0);

where zend_hash_num_elements(&ce->properties_info) + zend_hash_num_elements(&parent_ce->properties_info) is supposed to extend the hash table enough to hold all additional properties coming from parent. However, if ce->properties_info contains holes this might not be enough, given all parent properties are appended at nNumUsed.

This could be fixed by further extending the hash table, but we can also avoid the holes in properties_info completely. This is now possible because traits are bound before performing parent class inheritance, declaring the static property and avoiding the defensive copying of these properties.

Fixes GH-20672

Previously, static trait properties would always redeclare locally declared
static properties to make sure an inherited static property stops sharing a
common slot with the parent. This would leave holes in property_info, creating
issues for this code:

    zend_hash_extend(&ce->properties_info,
        zend_hash_num_elements(&ce->properties_info) +
        zend_hash_num_elements(&parent_ce->properties_info), 0);

where zend_hash_num_elements(&ce->properties_info) +
zend_hash_num_elements(&parent_ce->properties_info) is supposed to extend the
hash table enough to hold all additional properties coming from parent. However,
if ce->properties_info contains holes this might not be enough, given all parent
properties are appended at nNumUsed.

This could be fixed by further extending the hash table, but we can also avoid
the holes in properties_info completely. This is now possible because traits are
bound before performing parent class inheritance, declaring the static property
and avoiding the defensive copying of these properties.

Fixes phpGH-20672
use T;
}

?>
Copy link
Member Author

@iluuu1994 iluuu1994 Mar 6, 2026

Choose a reason for hiding this comment

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

I forgot the ===DONE=== that are usually added to tests with no output. I'll do that before merging.

Copy link
Member

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Nice find. This assumption that we can skip the static check here is definitely correct. I'm not certain whether this fixes all possible cases though.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Mar 6, 2026

I'm not certain whether this fixes all possible cases though.

Fair point. Any property hitting the two if ((property_info_ptr = zend_hash_find_ptr(&ce->properties_info, name)) != NULL) { conditions in zend_declare_typed_property() could recreate the same issue. However, at least in our internal tests, both of these branches are dead. FWICT, the same also goes for internal classes.

We can replace these branches with asserts on master.

@bwoebi
Copy link
Member

bwoebi commented Mar 6, 2026

@iluuu1994 I haven't checked whether they should be indeed dead, but if they are supposed to, then yes, please merge it and replace it by asserts on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants