Skip to content

Fix persisting of inherited class constants #14114

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 1 commit into from

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented May 2, 2024

Class constants are inherited to user classes without cloning. Thus, internal class constants should not be persisted at all. Simply keep pointing to the internal class constant. If the internal class constants are persisted, the attempt to free the attributes hash map after persistence will fail, because it's persistently allocated.

Fixes GH-14109

This will need to be rebased to PHP-8.2. I target master for now only because ZendAttributeTest doesn't exist on lower branches, and I was too lazy to backport.

@iluuu1994 iluuu1994 requested a review from TimWolla May 2, 2024 19:14
@iluuu1994 iluuu1994 marked this pull request as ready for review May 2, 2024 22:38
@iluuu1994 iluuu1994 requested a review from dstogov as a code owner May 2, 2024 22:38
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

I can't comment on the fix, but I can confirm that the failing test cases for #[\Deprecated] are fixed with this patch and that the added test looks good to me.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

Please double check if this doesn't make troubles for Windows (different workers may have internal constants at different addresses) and run tests with "--repeat 3" before merging this.

Personally, I would move code from zend_persist_class_entry() into zend_persist_class_constant(), because it already contains logic related to decision about persistence. With your patch part of this logic is duplicated.

@iluuu1994 iluuu1994 force-pushed the gh-14109 branch 3 times, most recently from 1522e6b to 054e46c Compare May 3, 2024 12:01
@iluuu1994
Copy link
Member Author

@dstogov Thanks your your comment.

Please double check if this doesn't make troubles for Windows (different workers may have internal constants at different addresses)

As hinted by you in the private chat, UPDATE_IS_CACHEABLE() on Windows prevents placing classes into inheritance cache when the parent (or any dependency) is an internal class.

// TODO: ASLR may cause different addresses in different workers ???
# define UPDATE_IS_CACHEABLE(ce) do { \
is_cacheable &= (ce)->ce_flags; \
} while (0)

Hence, such a class is not be possible on Windows. I had to add an additional clause for c->ce->ce_flags & ZEND_ACC_IMMUTABLE, because with inheritance cache, the parent class constant may also refer to a class constant of another immutable class.

and run tests with "--repeat 3" before merging this.

Done!

Personally, I would move code from zend_persist_class_entry() into zend_persist_class_constant(), because it already contains logic related to decision about persistence. With your patch part of this logic is duplicated.

I added it there only because orig_ce is not available from zend_persist_class_constant(). I can move it there and pass it as a param if you prefer.

@iluuu1994
Copy link
Member Author

Custom internal attributes are not supported in stubs until PHP 8.3. I will merge this change into PHP 8.2 and then commit the test separately for PHP 8.3+.

@iluuu1994
Copy link
Member Author

According to the failed build, I'll still need CONST_OWNED.

Class constants are inherited to user classes without cloning. Thus, internal
class constants should not be persisted at all. Simply keep pointing to the
internal class constant.

Fixes phpGH-14109
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.

heap-buffer-overflow with opcache when extending an internal class with class constant having attributes
3 participants