[iloveleon19]

Fixes #24319.

Prepared with an AI assistant; I reviewed the change and the test results. The text below is AI-generated (quoted, as asked in CONTRIBUTING.md).

zend_accel_hash_update() links a new entry into its bucket with plain stores while holding the SHM lock, but zend_accel_hash_find_ex() reads the hash without the lock. On weakly ordered CPUs a reader can reach an entry whose key/indirect/data stores are not visible yet. Details and observations are in #24319.

This uses the fence macros added for GH-23637 (873a997):

• release fence before accel_hash->hash_table[index] = entry, and before updating data/indirect of an existing entry;
• acquire fence after loading a non-NULL bucket head in zend_accel_hash_find_ex() and in accelerator_get_scripts() (opcache_get_status()).

Testing so far

• The same change backported to 8.2.32 (on top of the PHP-FPM worker SIGSEGV: run_time_cache map_ptr offset beyond CG(map_ptr_last) when a class is reached through the CE cache #23637 fix): 0 crashes in 4368 reloads, versus 9 crashes of this kind in 2607 reloads without it (FrankenPHP worker mode, aarch64, see the issue for the caveats).
• This branch's version builds as a shared extension against PHP 8.4.26 ZTS with 873a997 applied, with no new warnings in the touched files, and loads fine.
• An A/B reload run on PHP 8.4.23 + 873a997, with and without this change (aarch64, FrankenPHP 1.12.4), is in progress; I will post the numbers here.

There is no phpt: this is a cross-thread ordering issue that we can only reproduce under load. The direct→indirect conversion of an existing entry (two fields) is not made atomic by this change; it is listed as an open point in the issue.

[morrisonlevi]

@iluuu1994 and @iloveleon19: There are broader issues here that fences won't fix. The writer does this:

entry->data = target_entry;  // data now points to a hash entry
entry->indirect = true;      // announces that interpretation

OPcache writers block other writers but not readers. Even if x86_64/aarch64 made those stores visible in exactly that order, another core can run between them. No memory reordering is necessary for the issue:

Writer                           Reader
------                           ------
data = target_entry
                                 reads indirect == false
                                 reads data == target_entry
                                 treats target_entry as a compiled script
indirect = true

There is also a less localized race:

1. A reader obtains a direct entry from zend_accel_hash_find_entry().
2. Another process converts that entry into an alias.
3. The original reader loads entry->data and casts it to zend_persistent_script *. But data now points to another zend_accel_hash_entry. PHP interprets the hash entry's hash_value as the script's filename pointer. When persistent_compile_file() adds the supposed filename to EG(included_files), zend_hash_add() dereferences the hash value as a zend_string * and crashes.

I verified these problems on both aarch64 and x86_64.

We can use a tagged pointer to fix this, since we only need a flag for "Is this direct or indirect?" and OPcache already aligns zend_persistent_script and zend_accel_hash_entry.

[iloveleon19]

Prepared with an AI assistant; I reviewed it. The text below is AI-generated (quoted, as asked in CONTRIBUTING.md).

Thanks @morrisonlevi, agreed. The fences in this PR only cover publishing a new entry. They cannot make the two-field data/indirect update atomic, and as you show, that race does not need any reordering. I listed the direct→indirect conversion as an open point in #24319, but it should not have been left out of this PR.

Your second race also matches what we saw. Besides the key == NULL crashes, one of our cores shows a reader that took the indirect == 0 branch on an entry that is indirect in memory, and another run crashed in zend_hash_add() called from persistent_compile_file().

A tagged pointer sounds right to me: one word that says "script or alias", written with a single store and decoded by readers from a single load. Two questions before I rework this:

1. Would you (or @iluuu1994) rather make the tagged-pointer change yourselves, or should I update Fix GH-24319: opcache hash entries can be seen before they are initialised #24320? I'm happy to do it.
2. Should the publication fences for new entries stay (for the key == NULL case on weakly ordered CPUs), with the tagged pointer on top of them?

The PHP 8.4 numbers I mentioned in the description went to the private advisory after @iluuu1994's note on #24319, so I'm not posting them here.

[morrisonlevi]

@iloveleon19 I already prepared a PR for it: #25258.