Fixed possible crashes, because of inconsistent PCRE cache and opcache SHM reset #4161
Conversation
|
@nikic please take a look. |
|
The originally proposed patch was a bit simpler https://gist.github.com/tony2001/b03b49ab53e0c0c7924cee8bb6b92c61 |
|
|
||
| /* Trivia */ | ||
| add_assoc_bool(return_value, "opcache_enabled", ZCG(enabled) && (ZCG(counted) || ZCSG(accelerator_enabled))); | ||
| add_assoc_bool(return_value, "opcache_enabled", ZCG(enabled) && ZCG(accelerator_enabled)); |
There was a problem hiding this comment.
Can the ZCG(enabled) && part be dropped?
| realpath_cache_clean(); | ||
|
|
||
| accel_reset_pcre_cache(); | ||
| ZCG(pcre_reseted) = 0; |
There was a problem hiding this comment.
I don't really understand what's going on here. The cache is reset in the line above, but pcre_reseted is set to 0. Why is it necessary to reset the cache again?
There was a problem hiding this comment.
Ah, I think I got it. We already finished this restart, so this resets pcre_reseted=0 for the next one.
There was a problem hiding this comment.
Right. We reseted PCRE cache and set ZCG(pcre_reseted) = 1, to avoid multiple resets during restart pending, then reseted once again to repopulated cache with interned keys, and set ZCG(pcre_reseted) = 0 to be prepared for next restart.
|
Hello, this has been committed to the repo? Should be closed? |
|
For reference: 5c4d125 |
|
I'm looking into an issue we're having that might be related to this commit. Deactivating opcache or going back to 7.2.18 or 7.3.5 "solve" the problem. Could this be a side effect of this commit? |
|
@mweimerskirch Yes, this is https://bugs.php.net/bug.php?id=78106. We're trying to figure out what the problem is right now. |
Opcache reset may remove interned strings, that are used as keys in per-process PCRE caches.
PHP workers clear per-process PCRE cache on startup of next request after opcache reset, but crashes are possible in requests started before the actual reset.
To fix the problem, we reset PCRE caches on request startup, if we see restart pending event.
We also decide, if process may use opcache SHM once, by copying ZCSG(accelerator_enabled) into ZSG(accelerator_enabled) on request startup.