Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Fix registration of internal readonly child classes #15459

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

Merged
kocsismate merged 2 commits into php:master from kocsismate:readonly-extend
Aug 24, 2024

Conversation

Copy link
Member

@kocsismate kocsismate commented Aug 17, 2024 •
edited
Loading

Currently, internal classes are registered with the following code:

INIT_CLASS_ENTRY(ce, "InternalClass", class_InternalClass_methods);
class_entry = zend_register_internal_class_ex(&ce, NULL);
class_entry->ce_flags |= ...;

This has worked well so far, except if InternalClass is a readonly class. It is because some inheritance checks are run by zend_register_internal_class_ex, before ZEND_ACC_READONLY_CLASS is added to ce_flags.

The issue is fixed by adding a zend_register_internal_class_with_flags() zend API function that stubs can use from now on. This function makes sure to add the flags before running any checks. Since the new API is not available in lower PHP versions, gen_stub.php has to keep support for the existing API for PHP 8.4-.

The fix is tested in #14461 where I originally found the issue.

zeriyoshi reacted with thumbs up emoji
@TimWolla TimWolla removed their request for review August 19, 2024 13:57
Copy link
Member Author

@nielsdos @iluuu1994 May I get a review from you?

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Changes seem fine, just 2 remarks/questions about the stub generator

$code .= "#if (PHP_VERSION_ID >= " . PHP_84_VERSION_ID . ")\n";
}

$code .= "\tclass_entry = zend_register_internal_class_with_flags(&ce, " . (isset($this->extends[0]) ? "class_entry_" . str_replace("\\", "_", $this->extends[0]->toString()) : "NULL") . ", " . (implode("", $flagCodes) ?: 0) . ");\n";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$code .= "\tclass_entry = zend_register_internal_class_with_flags(&ce, " . (isset($this->extends[0]) ? "class_entry_" . str_replace("\\", "_", $this->extends[0]->toString()) : "NULL") . ", " . (implode("", $flagCodes) ?: 0) . ");\n";
$code .= "\tclass_entry = zend_register_internal_class_with_flags(&ce, " . (isset($this->extends[0]) ? "class_entry_" . str_replace("\\", "_", $this->extends[0]->toString()) : "NULL") . ", " . ($flags ?: 0) . ");\n";

$code .= "#else\n";

$code .= "\tclass_entry = zend_register_internal_class_ex(&ce, " . (isset($this->extends[0]) ? "class_entry_" . str_replace("\\", "_", $this->extends[0]->toString()) : "NULL") . ");\n";
$code .= "\tclass_entry->ce_flags |= " . implode("", $flagCodes) . "\n";
Copy link
Member

Choose a reason for hiding this comment

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

Can use $flags too.
Also, don't you need to check if $flags !== "" here?

Copy link
Member Author

@kocsismate kocsismate Aug 24, 2024

Choose a reason for hiding this comment

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

Yes, I definitely should have checked it.

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.

The changes look correct to me.

@kocsismate kocsismate merged commit 8d12f66 into php:master Aug 24, 2024
10 checks passed
@kocsismate kocsismate deleted the readonly-extend branch August 24, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adoy adoy adoy approved these changes

@iluuu1994 iluuu1994 iluuu1994 approved these changes

@nielsdos nielsdos nielsdos approved these changes

@Girgias Girgias Awaiting requested review from Girgias Girgias is a code owner

@zeriyoshi zeriyoshi Awaiting requested review from zeriyoshi zeriyoshi is a code owner

Assignees
No one assigned
Labels
Category: Build System Category: Engine Extension: com_dotnet Extension: curl Extension: date Extension: dba Extension: dom Extension: enchant Extension: ffi Extension: fileinfo Extension: ftp Extension: gd Extension: gmp Extension: hash Extension: intl Extension: json Extension: ldap Extension: libxml Extension: mysqli Extension: odbc Extension: openssl Extension: pdo (core) Extension: pdo_dblib Extension: pdo_firebird Extension: pdo_mysql Extension: pdo_odbc Extension: pdo_pgsql Extension: pdo_sqlite Extension: pgsql Extension: phar Extension: random Extension: reflection Extension: session Extension: shmop Extension: simplexml Extension: snmp Extension: soap Extension: sockets Extension: sodium Extension: spl Extension: sqlite3 Extension: standard Extension: sysvmsg Extension: sysvsem Extension: sysvshm Extension: tidy Extension: tokenizer Extension: xml Extension: xmlreader Extension: xmlwriter Extension: xsl Extension: zend_test Extension: zip Extension: zlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /