Skip to content

Fix GH-12232: FPM: segfault dynamically loading extension without opcache - #12277

Closed
bukka wants to merge 1 commit into
php:PHP-8.1from
bukka:fpm_config_extension_istr_switch
Closed

bukka wants to merge 1 commit into
php:PHP-8.1from
bukka:fpm_config_extension_istr_switch

Conversation

@bukka

@bukka bukka commented Sep 22, 2023

Copy link
Copy Markdown
Member

This is alternative fix that uses zend_interned_strings_switch_storage before using php_dl to make sure that permanent internet string storage is used instead of request storage. Suggested by @dstogov in #12233

@bukka
bukka changed the base branch from master to PHP-8.1 September 22, 2023 21:30
@bukka
bukka force-pushed the fpm_config_extension_istr_switch branch from f399c42 to 93d2776 Compare September 22, 2023 21:31
@bukka
bukka requested a review from dstogov October 2, 2023 10:27
Comment thread sapi/fpm/tests/gh12232-php-value-extension.phpt Outdated
Comment thread sapi/fpm/fpm/fpm_php.c Outdated

@dstogov dstogov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure, if the fact that extension interned strings are not going to be copied by accel_copy_permanent_strings() may make critical problems.

A potential problem, that during caching, opcache may keep a reference to such string and then another process may use this "invalid" reference. Can you please check if this is possible?

@bukka

bukka commented Oct 5, 2023

Copy link
Copy Markdown
Member Author

@dstogov Ok let me first check if I understood correctly what you see as a potential problem. Is it that the interned string
(module name in our case) stored in permanent hash table and opcache could potentially cache the string reference
to shm which could then be used by other procs and cause a crash? That means that opcache would have to relay on the
IS_STR_INTERNED flag instead of checking accel interned range?

If so, I have done some checking and reading of opcache code. From what I see all strings should go through
zend_accel_store_interned_string (specifically the ones in zend_persist_zval where I would be looking for this
sort of issue) which however uses IS_ACCEL_INTERNED check so it should be ok as it checks the memory range and should not treat permanent interned string as accell interned string (meaning it should not store its reference but copy the whole
string instead).

Are there any other places that I should check or did I miss anything in the above?

…opcache

Also fixes incorrect assertion in ini init that php_dl is always
temporary.

Closes phpGH-12277
@dstogov

dstogov commented Oct 9, 2023

Copy link
Copy Markdown
Member

Are there any other places that I should check or did I miss anything in the above?

I think your analyses should be enough.

@bukka bukka closed this in 0217be4 Oct 14, 2023
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.

3 participants