Skip to content

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Aug 22, 2025

Fix #19548

@alexandre-daubois alexandre-daubois marked this pull request as ready for review August 22, 2025 08:54
@iluuu1994 iluuu1994 self-assigned this Aug 22, 2025
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

I think this can be fixed in a much simpler way. We can rely on the fact that an unmet inherited #[Override] property will have already errored when linking the parent. Hence, it's sufficient if we remove the ZEND_ACC_OVERRIDE flag only when the property belongs to the currently linked class.

diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c
index bbc7a767e1b..8679a53e8ff 100644
--- a/Zend/zend_inheritance.c
+++ b/Zend/zend_inheritance.c
@@ -1564,7 +1564,9 @@ static void do_inherit_property(zend_property_info *parent_info, zend_string *ke
 						ZSTR_VAL(parent_info->ce->name));
 			}
 
-			child_info->flags &= ~ZEND_ACC_OVERRIDE;
+			if (child_info->ce == ce) {
+				child_info->flags &= ~ZEND_ACC_OVERRIDE;
+			}
 		}
 	} else {
 		zend_function **hooks = parent_info->hooks;

@alexandre-daubois alexandre-daubois changed the title Fix GH-19548: Add defensive copy-on-write for hooks inheritance when CE is part of opcache shared memory Fix GH-19548: remove ZEND_ACC_OVERRIDE on property inheritance only if it's not part of shared memory Aug 22, 2025
@alexandre-daubois
Copy link
Member Author

Oh right, It makes sense given where we are in the inheritance "lifecycle". Thanks, it works really well

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Same for the other test, but lgtm otherwise. Thanks!

Comment on lines 3 to 7
--EXTENSIONS--
opcache
--INI--
opcache.enable_cli=1
opcache.protect_memory=1
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved to ext/opcache/tests, or this section should be removed entirely. Tests are ran with and without opcache, and opcache.protect_memory=1 is always enabled by run-tests.php.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes of course, tests are cleaned up a bit now 👍

@iluuu1994
Copy link
Member

Thank you @alexandre-daubois!

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.

SEGV do_inherit_property zend_inheritance.c

2 participants