Repository navigation
Fix GH-24577: CE-cache flag on interned class name visible before map_ptr offset - #24585
Open
prateekbhujel wants to merge 2 commits into
Open
prateekbhujel wants to merge 2 commits into
prateekbhujel wants to merge 2 commits into
Conversation
prateekbhujel
force-pushed
the
prateek/fix-gh24577-ce-cache-race
branch
2 times, most recently
from
October 23, 2026 04:27
4ac1363 to
5c21092
Compare
When OPcache persists a script into shared memory, zend_accel_pending_ce_cache_commit() (and zend_alloc_ce_cache() in Zend/zend.c) writes the allocated map_ptr offset into type_name->gc.refcount and sets IS_STR_CLASS_NAME_MAP_PTR on the shared interned string. Because these are plain non-atomic stores to adjacent struct fields, the compiler or weakly ordered CPU can make IS_STR_CLASS_NAME_MAP_PTR visible to concurrent worker processes before the map_ptr offset overwrites the initial SHM interned string refcount of 2. Previously, ZSTR_VALID_CE_CACHE() only checked (GC_REFCOUNT(s) - 1) / sizeof(void *) < CG(map_ptr_last), which accepted refcount 2 ((2 - 1) / 8 == 0 < CG(map_ptr_last)) and dereferenced an unaligned address at map_ptr_base + 2. Require GC_REFCOUNT(s) > 2 in ZSTR_VALID_CE_CACHE() and ZSTR_SET_CE_CACHE_EX(), and order GC_SET_REFCOUNT before GC_ADD_FLAGS in zend_alloc_ce_cache().
prateekbhujel
force-pushed
the
prateek/fix-gh24577-ce-cache-race
branch
from
October 24, 2026 03:47
5c21092 to
4f811bd
Compare
ndossche
reviewed
Oct 24, 2026
ndossche
left a comment
Member
There was a problem hiding this comment.
I think that if a solution using memory barriers is feasible, we should do that instead of the hardening. Nothing prevents the compiler or CPU from reordering the assignments again.
This will require adding barriers in a few places as there are multiple places where GC_ADD_FLAGS(type_name, IS_STR_CLASS_NAME_MAP_PTR); is done
Replace hardening check with explicit release and acquire memory barriers to prevent compiler and CPU reordering between GC_SET_REFCOUNT and GC_ADD_FLAGS(..., IS_STR_CLASS_NAME_MAP_PTR) across all interned class cache writers and readers.
Contributor
Author
|
Agreed, memory barriers are definitely the right approach here instead of the refcount guard. I've reverted the
Verified the emitted store ordering in disassembly and |
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.
Fixes GH-24577.
What was broken
Under concurrent traffic on a cold OPcache with
opcache.interned_strings_buffer > 0, worker processes can race when one child persists class definitions that attach a CE-cache slot to an already-shared interned class-name string while another child looks up that same class name.PHP reuses
type_name->gc.refcountto store themap_ptroffset and tags the string withIS_STR_CLASS_NAME_MAP_PTR. However:ext/opcache/zend_persist.c(zend_accel_pending_ce_cache_commit()),GC_SET_REFCOUNTandGC_ADD_FLAGS(type_name, IS_STR_CLASS_NAME_MAP_PTR)are plain non-atomic stores to adjacent struct fields (gc.refcountandgc.u.type_info), which compilers (e.g. GCC 12/15) or weakly ordered CPUs can emit or make visible with the flag store before the offset store. InZend/zend.c(zend_alloc_ce_cache()),GC_ADD_FLAGSalso precededGC_SET_REFCOUNTin source order.ZSTR_VALID_CE_CACHE()inZend/zend_types.honly checked(GC_REFCOUNT(s) - 1) / sizeof(void*) < CG(map_ptr_last). Because OPcache SHM interned strings initialize with refcount2,(2 - 1) / sizeof(void*) == 0 < CG(map_ptr_last)evaluated to true.IS_STR_CLASS_NAME_MAP_PTRwhilegc.refcountwas still2treated2as a valid offset and dereferenced 8 unaligned bytes at(map_ptr_base + 2), crashing inobject_init_ex(SIGSEGV).ZSTR_SET_CE_CACHE_EX(s, ce, 0)(which bypasses normal validation) had no guard against writing to(map_ptr_base + 2)if encountered whilegc.refcount <= 2.The fix
ZSTR_VALID_CE_CACHE(): CheckGC_REFCOUNT(s) > 2. Validmap_ptroffsets are strictly greater than 2 and always odd (k * sizeof(void*) + 1), so any refcount<= 2is rejected as a cache miss without adding atomic barrier overhead to the hot class-lookup path.ZSTR_SET_CE_CACHE_EX(): Guard the setter withEXPECTED(GC_REFCOUNT(s) > 2)so an uncommitted offset (<= 2) is never written to even whenvalidate == 0.Zend/zend.c: OrderGC_SET_REFCOUNTbeforeGC_ADD_FLAGSinzend_alloc_ce_cache()to matchext/opcache/zend_persist.c.ext/opcache/tests/gh24577.phpt, which first caches a separate consumer script in OPcache SHM so the target class-name strings exist in the shared interned strings buffer withrefcount = 2, then releases a barrier-synchronized Writer (persisting the class declarations) and Readers (probing the pre-existing shared interned strings).