opcache: free transient linked class copy on inheritance-cache race (ZTS)#14
Open
EdmondDantes wants to merge 1 commit into
Open
opcache: free transient linked class copy on inheritance-cache race (ZTS)#14EdmondDantes wants to merge 1 commit into
EdmondDantes wants to merge 1 commit into
Conversation
…ZTS)
In a multi-threaded (ZTS) SAPI where worker threads share the opcache SHM
inheritance cache but each keeps its own class_table, two threads can link
the same immutable class concurrently and leak one linked copy.
zend_do_link_class() links an immutable (SHM) class in three phases:
A. early check: zend_inheritance_cache_get(). On hit, return the cached
class directly; no working copy is made.
B. on miss: zend_lazy_class_load() emalloc's a private, writable copy of
the class and links it against its parent.
C. late insert: zend_inheritance_cache_add() stores the linked class in
SHM. On the MISS path zend_persist_class_entry() copies the tables into
SHM via zend_shared_memdup_free(), which frees the working copy.
The leak is the C-HIT path. It is reachable only when A missed but, by the
time we reach C, the entry already exists, i.e. another thread linked and
cached the same class in between. zend_accel_inheritance_cache_add() then
returns the existing entry->ce without calling zend_persist_class_entry(),
so zend_shared_memdup_free() never runs and the losing thread's working
copy (default properties/static members, function table, properties_info,
constants table, interfaces) is abandoned without being freed.
In a single-process SAPI this path is unreachable: a class is linked once,
A and C see the same empty cache, so C always MISSes and the copy is freed
during persist. It only surfaces in long-lived multi-threaded ZTS runtimes
with concurrent class linking, where the copy accumulates on the worker's
heap for the lifetime of the thread.
Fix: on the cache-HIT return in zend_accel_inheritance_cache_add(), free
the caller's unreachable working copy via destroy_zend_class() before
returning the shared entry. This mirrors the MISS path's implicit free and
covers both zend_inheritance_cache_add() call sites.
Found while investigating unbounded memory growth across worker reloads in
a threaded async HTTP server; reduced to a stock opcache + ZTS race. Under
a DEBUG (report_memleaks) build the leak drops from dozens of blocks per
retired worker to zero, with no double-free or crash under reload/traffic
stress.
8e809e7 to
2bc1de0
Compare
Author
|
The issue with the test. As for reproducing it in a test, the problem only occurs when PHP is used with threads; specifically, PHP threads. Since threading isn't a built-in PHP feature, I'm not sure yet how to write a proper test for it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In a multi-threaded (ZTS) SAPI where worker threads share the opcache SHM inheritance cache but each keeps its own
class_table, two threads that link the same immutable class concurrently can leak one linked class copy. The leak is small per class (~1 KB) but accumulates for the life of every worker thread, so long-lived servers grow memory steadily — most visibly across worker reloads, where each fresh worker re-links the whole framework against an already-populated shared cache.The offending code is entirely stock opcache/Zend — no async/threading extension is involved in the leaking path. It just needs concurrent class linking on top of a shared SHM inheritance cache.
How the leak happens
zend_do_link_class()links an immutable (SHM) class in three phases:zend_inheritance_cache_get,zend_inheritance.c): on hit, return the cached linked class directly; no working copy is made.zend_lazy_class_load()emallocs a private, writable copy of the class (default properties, static members, function table, properties_info, constants table, …) and links it against its parent.zend_inheritance_cache_add): store the linked class into SHM. On the MISS path,zend_persist_class_entry()copies the tables into SHM viazend_shared_memdup_free()— the_freesuffix frees the working copy's original tables. No leak.The leak is the C-HIT path. It is reachable only when A missed but by the time we reach C the entry already exists — i.e. another thread linked and cached the same class in between:
The caller then does
Z_CE_P(zv) = entry->ceand drops its ownce. Sincezend_persist_class_entry()never ran for it, nothing frees itsemalloc'd tables.Why single-process PHP never hits it
In a classic single-process SAPI a given class is linked once: phase A and phase C observe the same (empty) cache state, so C always MISSes and the copy is freed during persist. The C-HIT branch is only reachable as a race between two threads linking the same class — i.e. a ZTS-only condition. opcache is designed to be shared across ZTS threads, so this is a latent concurrency bug rather than a design limitation.
The fix
On the cache-HIT return in
zend_accel_inheritance_cache_add(), free the caller's now-unreachable working copy withdestroy_zend_class()before returning the shared entry. This mirrors what the MISS path already does implicitly (viazend_shared_memdup_freein persist) and, being insidecache_add, covers bothzend_inheritance_cache_add()call sites (normal linking and the delayed-early-binding path).Testing
Reproduced and verified under a DEBUG (
report_memleaks=1, Zend MM) ZTS build driving a threaded HTTP server (16 workers) with concurrent request traffic and repeated worker reloads:zend_lazy_class_load/ inheritanceemallocblocks as leaked. Instrumentation showed the leaked classes are exactly the ones that took the cache-HIT branch (MISS→freed,HIT→leaked).=== Total 0 memory leaks detected ===across 20 reloads under load; no double-free, assertion, or crash.Notes for reviewers
zend_accel_inheritance_cache_add()because only there is the HIT vs MISS distinction known: on MISS the copy's tables are already freed by persist, so freeing at the call site would double-free. If you'd prefer a different structure (e.g. signalling HIT/MISS back tozend_do_link_class), happy to adjust.