[pavimus]

Prepared with an AI assistant (Claude), which also ran the reproduction and the tests on my machine; I reviewed the result. The text below is AI-generated (quoted, as asked in CONTRIBUTING.md).

While working on GH-8739 for threaded SAPIs we found that Apache with a threaded MPM and mod_php ZTS crashes during warm-up even without any OPcache restart. This is a small, separate fix.

accel_unlock_all() runs at the end of every request (and in RINIT for a "stuck count") and
unlocks the whole lock file (l_start = 0, l_len = 0). fcntl() record locks belong to the
process, so under ZTS a thread ending its request also releases

• byte 0, the SHM write lock that another thread of the same process holds inside
  zend_shared_alloc_lock() (that thread still holds zts_lock and keeps writing), and
• byte 2, the restart lock.

With several multi-threaded processes sharing the cache (Apache event/worker MPM + mod_php ZTS)
another process can then take the write lock, and two processes modify SHM at the same time.

Evidence (AArch64 Linux, Apache 2.4 event, 3 children x 32 threads, 128M SHM, no restart):

• A debug counter of threads inside zend_shared_alloc_lock(), kept in shared memory, saw two
  processes writing concurrently in 4 of 5 cold starts; with this change 0 of 5.
• Children crashed before the first restart in 6 of 13 stock runs (SIGSEGV in
  accel_find_interned_string() on a garbage string table position, and in
  zend_hash_find_bucket() on a class method table); 0 of 13 with this change.

Bytes 0 and 2 are always released by the thread that took them (zend_shared_alloc_unlock(),
zend_shared_alloc_safe_unlock() in post_deactivate, accel_restart_leave()), so under ZTS only
the reader lock (byte 1) is released. NTS is unchanged.

This does not fix the restart race of GH-8739 (threads of the restarting process are still
invisible to the F_GETLK probe); that is a separate PR for GH-8739, which builds on this commit.

🤖 Generated with Claude Code

[ArtUkrainskiy]

Reproduced the over-release deterministically, no race needed. Thread A holds the byte-0 write lock, thread B of the same process runs accel_unlock_all, and another process can then take byte 0. The byte-1 unlock keeps it held. fcntl locks are per-process, so this is arch-independent, not just aarch64.

Reachable only under ZTS with a threaded MPM and several processes on one cache, like Apache event/worker with mod_php ZTS. The trigger is many threads compiling uncached scripts at once, so warm-up on deploy or restart. php-fpm and single-process workers stay unaffected.

The fix loses nothing, since byte 0 and byte 2 are released by their own paths, zend_shared_alloc_safe_unlock and accel_restart_leave. Narrowing accel_unlock_all to the reader lock is enough. LGTM.

[arnaud-lb]

Unfortunately, releasing the read lock is also incorrect: This allows a restart to be performed while other threads are actively using SHM. I believe that the problem is not limited to accel_unlock_all(): accel_deactivate_sub() has the same issue.

We should guard the unlock in accel_deactivate_sub() behind a process-wide counter in ZTS builds, so that we unlock only when the counter falls to zero (and lock only when 0->1). We likely need a mutex to serialize the increment + lock operations.

Then we should remove accel_unlock_all(), and use accel_deactivate_now() instead.

[pavimus]

Thanks @arnaud-lb, you are right: the reader lock is per process, so dropping it at the end of one thread's request (here, or in accel_deactivate_sub() via accelerator_shm_read_unlock()) lets a restart start while other threads still use SHM.

This is what #24276 (GH-8739) does, built on top of this commit:

• threads are counted in a process-local atomic counter; byte 1 is locked on 0 -> 1 and unlocked on 1 -> 0, both under a process-local mutex (other changes are a CAS);
• under ZTS a thread becomes a reader only in RINIT, so accel_activate_add() fails, and the thread's reference is released once at the end of the request instead of unlocking the whole file;
• the restart takes the mutex and a write lock on byte 1 for its whole duration instead of the F_GETLK probe.

Since this PR alone is incomplete, I'm closing it in favour of #24276. Would you have time to look at #24276?