Bug #95171
closed
Concurrent requests might fail because of PHP threaded getenv/putenv implementation
100%
Description
When concurrent requests are made to the backend, they have a good chance of failing (especially on Windows), because of PHP threaded getenv/putenv implementation.
This is because:
- The environment is per process. This means that two instances of PHP will share the same environment in a multithreaded server, rather than each using a unique one.
- The functions putenv() and getenv() are not required to be re-entrant or thread safe. What this means is that if two threads happen to call them at the same time (either on different cores, or from a context switch in the middle of the function), bad things can happen.
- putenv() (in C) takes a pointer and references the memory, rather than copying it (depending on implementation; on a Mac it does make a copy iirc, even though this violates the spec.) PHP keeps this around in a hash table, which is destroyed at the end of the request. This may cause the environment to be cleared while another thread is running and using it.
It causes the core not being able to access various environment variables like 'TYPO3_PATH_COMPOSER_ROOT'.
Since the latest deprecation of PackageStates.php in composer mode this is causing various issues all over the place.
I.e. if 'TYPO3_PATH_COMPOSER_ROOT' cannot be read (returning false), relative composer package paths cannot be expanded to absolute ones, resulting in:
- Missing template exceptions in the BE
- BE icons not loading
- Missing site configuration
- probably many more
The core should not rely on getenv() to get environment variables, and probably fall back to $_ENV.
More information and explanation can be found at:
Updated by Helmut Hummel over 2 years ago
- Category changed from System/Bootstrap/Configuration to composer
- Status changed from New to Accepted
- Assignee set to Helmut Hummel
- Target version set to Candidate for patchlevel
- TYPO3 Version changed from 11 to 8
Thanks for the detailed report. Yes, this is definitively an issue that needs to be fixed.
I'm wondering, why this isn't an issue that was reported beforehand, as putenv is done for paths (in Composer mode) for a pretty long time already.
My suggestion here is to not use the environment to store paths at all, avoid all calls to putenv (currently only used in tests and in the Composer plugin) and implement a canonical way to fetch environment variables, where it makes sense to still have/ read them.
Updated by Helmut Hummel over 2 years ago
- Category changed from composer to System/Bootstrap/Configuration
Updated by Helmut Hummel over 2 years ago
Helmut Hummel wrote in #note-1:
I'm wondering, why this isn't an issue that was reported beforehand, as putenv is done for paths (in Composer mode) for a pretty long time already.
tl;dr: this issue "only" exists for multi threaded web servers (on Windows or Linux with Apache with PHP as module and mpm-event worker). Since these aren't that common, we haven't received any reports so far.
Updated by Robert Kärner over 2 years ago
The issue has existed (silently) for some time, but it has only recently become noticeable as https://forge.typo3.org/issues/94996 was implemented.
Since then, TYPO3\CMS\Core\Package\Package relies heavily on getenv('TYPO3_PATH_COMPOSER_ROOT') to get the package path, which it didn't before.
I was able to reproduce this with a test project set up with Composer just before and after the commit in question.
Updated by Helmut Hummel over 2 years ago
Robert Kärner wrote in #note-4:
The issue has existed (silently) for some time, but it has only recently become noticeable as https://forge.typo3.org/issues/94996 was implemented.
Since then, TYPO3\CMS\Core\Package\Package relies heavily on getenv('TYPO3_PATH_COMPOSER_ROOT') to get the package path, which it didn't before.I was able to reproduce this with a test project set up with Composer just before and after the commit in question.
Two base paths are fetched from environment (and populated with setenv) in Composer mode since TYPO3 9.5:
https://github.com/TYPO3/typo3/blob/d6ed6058945d7961d11363091de87edf9f2dbd7c/typo3/sysext/core/Classes/Core/SystemEnvironmentBuilder.php#L215
https://github.com/TYPO3/typo3/blob/d6ed6058945d7961d11363091de87edf9f2dbd7c/typo3/sysext/core/Classes/Core/SystemEnvironmentBuilder.php#L300
https://github.com/TYPO3/typo3/blob/d6ed6058945d7961d11363091de87edf9f2dbd7c/typo3/sysext/core/Classes/Core/SystemEnvironmentBuilder.php#L310
My question is: why did this not lead to issues like you are seeing before?
Updated by Robert Kärner over 2 years ago
Helmut Hummel wrote in #note-5:
My question is: why did this not lead to issues like you are seeing before?
Good question. I assume it's related to timing.
As far as I understand, the environment builder runs during bootstrap, so all threads have everything set up when the first request finishes.
Now, getenv() might be called later during execution, as package paths are lazily expanded from relative to absolute.
I mean, it's definitely possible that this issue occurred on our machines before.
But since we get caching exceptions on local from time to time ("Could not remove file xxx from cache: Access denied"), we were always blaming our filesystem for random exceptions.
Since 11.4 it started getting out of hand, like we couldn't even save the Site Configuration in the backend, so I started investigating.
Updated by Helmut Hummel over 2 years ago
- Related to Bug #94996: Drop PackageStates.php in Composer mode added
Updated by Gerrit Code Review over 2 years ago
- Status changed from Accepted to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/71075
Updated by Gerrit Code Review over 2 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/71075
Updated by Helmut Hummel over 2 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset f3056b2f787f4ad10b228538fa3e7e2f90c6f71c.
Updated by Gerrit Code Review over 2 years ago
- Status changed from Resolved to Under Review
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/71083
Updated by Helmut Hummel over 2 years ago
- Related to Bug #95237: Usage of Composer 2.1 runtime API while only requireing 2.0 added
Updated by Helmut Hummel over 2 years ago
- Status changed from Under Review to Resolved
Updated by