Repository navigation
Fix GH-24577: CE-cache flag on interned class name visible before map_ptr offset - #24585
prateekbhujel wants to merge 2 commits into
Conversation
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().
5c21092 to
4f811bd
Compare
ndossche
left a comment
ndossche
left a comment
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.
|
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 |
|
Can you update the PR description to accurately reflect the current version of the PR's code please? |
|
Done, updated the description to reflect the memory barrier changes. |
Fixes GH-24577.
Problem
When OPcache assigns a class cache slot (
map_ptroffset) to an interned class name string in shared memory, it stores the offset intype_name->gc.refcountand marks the string with theIS_STR_CLASS_NAME_MAP_PTRflag.Without memory barriers, the compiler or CPU can reorder these operations (or make the flag visible to concurrent worker processes before the offset is written). In
Zend/zend.c, the flag was also being set before the refcount in source order.If another worker process checks
ZSTR_HAS_CE_CACHE()and seesIS_STR_CLASS_NAME_MAP_PTRwhilegc.refcountis still2(the default refcount for SHM interned strings), it treats2as a valid cache offset and dereferences unaligned memory atmap_ptr_base + 2, causing a SIGSEGV.Solution
Use release and acquire memory barriers to ensure the offset is always written and visible before the flag:
zend_persist.c(zend_accel_pending_ce_cache_commit),ZendAccelerator.c(accel_new_interned_string), andzend.c(zend_alloc_ce_cache), writeGC_SET_REFCOUNTfirst, issueZEND_ATOMIC_FENCE_RELEASE(), and then setIS_STR_CLASS_NAME_MAP_PTR.ZendAccelerator.c(accel_interned_strings_restore_state), clear the flag first, issueZEND_ATOMIC_FENCE_RELEASE(), and then resetgc.refcountto 2.ZSTR_HAS_CE_CACHE(), issueZEND_ATOMIC_FENCE_ACQUIRE()whenIS_STR_CLASS_NAME_MAP_PTRis detected, ensuring the reader always sees the publishedmap_ptroffset.ext/opcache/tests/gh24577.phpt, which pre-populates shared interned strings with refcount 2 and verifies concurrent readers and writers under load.