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

[devnexen]

…st).

Fix #23637

zend_accel_inheritance_cache_add() linked the new entry into proto->inheritance_cache before updating ZCSG(map_ptr_last), so another process could take that entry in zend_accel_inheritance_cache_get(), extend its map_ptr table only up to the previous value, and cache a class whose methods' run_time_cache offsets sit past its own CG(map_ptr_last). Calling such a method read an uninitialised slot, kept by
zend_init_func_run_time_cache() as it is not NULL, which an fcall observer then dereferenced. The entry is now published last.

The store order alone does not hold on weakly ordered architectures, hence the fences. The acquire sits right after ce->inheritance_cache is read, not before the ZCSG(map_ptr_last) test: it pairs with the release only when sequenced after the load reading the published pointer, and it also has to cover the entry walk in zend_accel_inheritance_cache_find().

[devnexen]

ping :)

[arnaud-lb]

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]

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 ?

[arnaud-lb]

Looks good to me!