Skip to content

[WIP] Attempt to fix opcache_reset() races #14803

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

Open
wants to merge 1 commit into
base: PHP-8.2
Choose a base branch
from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jul 4, 2024

This is related to both #8739 and #14471. In particular to the latter.

I observe that there are data races possible against &ZCSG(hash) between zend_accel_hash_find_entry and zend_accel_add_key. We use the bucket return value from zend_accel_hash_find_entry and use it as a data pointer for zend_accel_add_key. However, if the &ZCSG(hash) table is modified in between those calls, then we end up with inconsistent data in the hash table. This is possible for example with opcache_reset().

We can observe this is the case by adding memset(accel_hash->hash_entries, 0xf0, sizeof(zend_accel_hash_entry )*accel_hash->max_num_entries); at the bottom of zend_accel_hash_clean which causes a dereference at address 0xf0f0f0f0f0f0f110 when walking the bucket chain in zend_accel_hash_find_entry.

I ran the test from GH-14471 with opcache.interned_strings_buffer=0 and it is much more stable. Prior to this patch it would constantly crash after a while. With the interned strings buffer not zero I still observe crashes. Even with the buffer size 0, not all issues are fixed yet, see #8739 (comment) for example. I would need some feedback or guidance.

One problem I think is still there is when returning a string from the hash table with zend_string *str = zend_string_copy(persistent_script->script.filename);, if that string is in the interned hash table then nothing is preventing a restart from clearing out the filename string.

Comment on lines +2067 to +2068
zend_shared_alloc_lock();

Copy link
Member

Choose a reason for hiding this comment

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

Please think about a different solution. This one should make a big slowdown.

opcache should be able to do lock-free cache lookups.
Probably, the modification of ZCSG(hash) should be done more careful (to keep consistency for concurrent readers)

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