Project

General

Profile

Actions

Bug #95171

closed

Concurrent requests might fail because of PHP threaded getenv/putenv implementation

Added by Robert Kärner almost 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Should have
Assignee:
Category:
System/Bootstrap/Configuration
Start date:
2021-09-10
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
8
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:

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:

  1. 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.
  2. 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.
  3. 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:

Related issues 2 (0 open2 closed)

Related to TYPO3 Core - Bug #94996: Drop PackageStates.php in Composer modeClosedHelmut Hummel2021-08-25

Actions
Related to TYPO3 Core - Bug #95237: Usage of Composer 2.1 runtime API while only requireing 2.0Closed2021-09-16

Actions
Actions #1

Updated by Helmut Hummel almost 3 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.

Actions #2

Updated by Helmut Hummel almost 3 years ago

  • Category changed from composer to System/Bootstrap/Configuration
Actions #3

Updated by Helmut Hummel almost 3 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.

Actions #4

Updated by Robert Kärner almost 3 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.

Actions #5

Updated by Helmut Hummel almost 3 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?

Actions #6

Updated by Robert Kärner almost 3 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.

Actions #7

Updated by Helmut Hummel almost 3 years ago

  • Related to Bug #94996: Drop PackageStates.php in Composer mode added
Actions #8

Updated by Gerrit Code Review almost 3 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

Actions #9

Updated by Gerrit Code Review almost 3 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

Actions #10

Updated by Helmut Hummel almost 3 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #11

Updated by Gerrit Code Review almost 3 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

Actions #12

Updated by Helmut Hummel almost 3 years ago

  • Related to Bug #95237: Usage of Composer 2.1 runtime API while only requireing 2.0 added
Actions #13

Updated by Helmut Hummel almost 3 years ago

  • Status changed from Under Review to Resolved
Actions #14

Updated by Benni Mack over 2 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF