Skip to content

ext/opcache: inheritance cache entry published before ZCSG(map_ptr_la… #23676

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.

Sign up for GitHub

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

Closed
devnexen wants to merge 1 commit into from
Closed

ext/opcache: inheritance cache entry published before ZCSG(map_ptr_la… #23676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Zend/zend_atomic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,23 @@ ZEND_API bool zend_atomic_bool_load(const zend_atomic_bool *obj);
ZEND_API int zend_atomic_int_load(const zend_atomic_int *obj);
#endif

#if defined(HAVE_C11_ATOMICS)
# define ZEND_ATOMIC_FENCE_RELEASE() __c11_atomic_thread_fence(__ATOMIC_RELEASE)
# define ZEND_ATOMIC_FENCE_ACQUIRE() __c11_atomic_thread_fence(__ATOMIC_ACQUIRE)
#elif defined(HAVE_GNUC_ATOMICS)
# define ZEND_ATOMIC_FENCE_RELEASE() __atomic_thread_fence(__ATOMIC_RELEASE)
# define ZEND_ATOMIC_FENCE_ACQUIRE() __atomic_thread_fence(__ATOMIC_ACQUIRE)
#elif defined(HAVE_SYNC_ATOMICS)
# define ZEND_ATOMIC_FENCE_RELEASE() __sync_synchronize()
# define ZEND_ATOMIC_FENCE_ACQUIRE() __sync_synchronize()
#elif defined(ZEND_WIN32)
# define ZEND_ATOMIC_FENCE_RELEASE() MemoryBarrier()
# define ZEND_ATOMIC_FENCE_ACQUIRE() MemoryBarrier()
#else
# define ZEND_ATOMIC_FENCE_RELEASE() ((void)0)
# define ZEND_ATOMIC_FENCE_ACQUIRE() ((void)0)
#endif

END_EXTERN_C()

#endif
7 changes: 6 additions & 1 deletion ext/opcache/ZendAccelerator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2302,6 +2302,10 @@ static zend_class_entry* zend_accel_inheritance_cache_get(zend_class_entry *ce,
bool needs_autoload;
zend_inheritance_cache_entry *entry = ce->inheritance_cache;

if (entry) {
ZEND_ATOMIC_FENCE_ACQUIRE();
Comment on lines +2305 to +2306

@arnaud-lb arnaud-lb Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure about the placement of this fence.

My understanding of __ATOMIC_ACQUIRE is that it prevents reads/writes after it from being reordered before any read before it. And __ATOMIC_RELEASE prevents reads/writes before it from being reordered after writes that follow it.

The fence in

	ZCSG(map_ptr_last) = CG(map_ptr_last);
	ZEND_ATOMIC_FENCE_RELEASE();
	proto->inheritance_cache = entry;

ensures that the write to ZCSG(map_ptr_last) happens before proto->inheritance_cache = entry, so if another process/thread sees that entry it will also see the updated ZCSG(map_ptr_last).

The acquire fence however, should ensure that ZCSG(map_ptr_last) is read after finding the final entry (after following some ->next pointers), so it should be placed just before reading ZCSG(map_ptr_last) a few lines below:

    if (ZCSG(map_ptr_last) > CG(map_ptr_last)) {

@devnexen devnexen Sep 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the review. I put it there on purpose. The fence pairs with the release as long as it comes after the load that reads the published pointer, i.e. entry = ce->inheritance_cache. So your placement would work for map_ptr_last too.

But then zend_accel_inheritance_cache_find() would not be covered. It reads entry->parent, entry->dependencies, entry->next and so on, which _add() writes before publishing. On ARM the address dependency saves us in practice, but the C11 model does not guarantee it, and map_ptr_last has no such dependency anyway.

Entries reached via ->next are fine as well: they are prepended under the SHM lock, so seeing the head covers the older ones. wdyt ?

}

while (entry) {
entry = zend_accel_inheritance_cache_find(entry, ce, parent, traits_and_interfaces, &needs_autoload);
if (entry) {
Expand Down Expand Up @@ -2458,12 +2462,13 @@ static zend_class_entry* zend_accel_inheritance_cache_add(zend_class_entry *ce,
entry->num_warnings = EG(num_errors);
entry->warnings = zend_persist_warnings(EG(num_errors), EG(errors));
entry->next = proto->inheritance_cache;
proto->inheritance_cache = entry;

EG(num_errors) = 0;
EG(errors) = NULL;

ZCSG(map_ptr_last) = CG(map_ptr_last);
ZEND_ATOMIC_FENCE_RELEASE();
proto->inheritance_cache = entry;

zend_shared_alloc_destroy_xlat_table();

Expand Down
Loading