implement r/w locks to improve performance

[withinboredom]

While investigating some performance issues with FrankenPHP, @AlliBalliBaba discovered that environment access takes an exclusive lock before accessing the environment. This PR makes a small ABI break to tsrm_env_unlock and tsrm_env_lock to indicate whether the environment will be modified. This will take a shared lock for reading (so that multiple threads can read at once) and an exclusive lock on writing.

This change should increase performance for ZTS builds (especially those using modern frameworks that make heavy use of environment variables): dunglas/frankenphp#1080 (comment) increasing performance by ~2x under load for the litespeed SAPI.

[cmb69]

Thank you! That looks like a good idea, but I wonder why we would export tsrm_rwlock_alloc() and tsrm_rwlock_free(); aren't these supposed to be only used in TSRM.c? And on Windows, there's no need to alloc() and free() the structure; static RWLOCK_T tsrm_env_mutex = SRWLOCK_INIT; should be sufficient. Don't know if something similar is possible with pthreads.

[withinboredom]

That looks like a good idea, but I wonder why we would export tsrm_rwlock_alloc() and tsrm_rwlock_free(); aren't these supposed to be only used in TSRM.c?

Related regular locks are exported as well, and used outside TSRM (such as the curl extension), so this allows other things to benefit from rwlocks.

And on Windows, there's no need to alloc() and free() the structure; static RWLOCK_T tsrm_env_mutex = SRWLOCK_INIT; should be sufficient. Don't know if something similar is possible with pthreads.

I'm not aware of anything similar in Pthreads (I'll have to check), but doing an alloc() step is easier to reason about and maintain, IMHO. For example, if in the future we need to do other book-keeping, we can do it in the alloc function.

[iluuu1994]

@arnaud-lb or @nielsdos might be best equipped to review this.

[arnaud-lb]

I think that php-src shouldn't access/modify environ after startup. There are a few reasons for that:

• environ is shared by all threads (edit: especially I mean requests), so there will be race conditions. E.g. a request changes an env var and executes something that uses it, but an other request changes the env var again in the middle of this sequence. (The fact that changes are visible by concurrent and subsequent requests also violates the share-nothing architecture.)
• The lock only protects accesses in php-src, and not in 3rd party code. If environ is used by a C extension / C library while php-src modifies it, there will be memory corruptions.
• If php-src maintains its own copy of environ, and reads/writes only from that, there is no need to modify environ. This might as well be faster (hash table and no locking).

One case where it might be useful/necessary to update environ is fork/exec. We can always sync environ safely in the fork.

[bwoebi]

@arnaud-lb There are some annoying edge cases whereby the libraries imported by php extensions have configurations which are read from environ. E.g. gd, see https://www.php.net/manual/en/function.imagettftext.php, $fontfile parameter.
But aside from that, I agree.

[arnaud-lb]

@bwoebi this falls in my second bullet point. This is unsafe in ZTS, as GD will access environ without synchronization. putenv("GDFONTPATH=.") may reallocate environ itself and/or the GDFONTPATH=. entry, even when the value isn't changed, so getenv("GDFONTPATH") in a concurrent request will use-after-free.

[cmb69]

This is unsafe in ZTS, as GD will access environ without synchronization.

But even if there was synchronization, separate requests could influence each other, couldn't they?

Wrt to GDFONTPATH and the whole font lookup in libgd: as is, that can't work with relative paths under ZTS due to VCWD. We need to re-implement the whole font lookup logic in ext/gd, and only pass a single absolute font path to gdImageStringFT(). That would avoid the need to change the global environment for this case.

[withinboredom]

@arnaud-lb I largely agree with you, but php is used by more than just requests (cli tools, job workers, etc). The goal here is to prevent an exclusive lock when simply reading the environment, whether that is a sapi, extension, or user code.