Changing process environment unsafe on multithreaded servers

[toddbc]

When using Dotenv on a multithreaded webserver, such as through Apache on Windows with the winnt mpm or on Linux/Unix with an event or worker mpm, using putenv and getenv is actually unsafe.

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.

See laravel/framework#8187 for a deeper description of the effects of these problems.

The only real benefit of modifying the environment using putenv() is that forked child processes will inherit the environment. For the most part, modifying $_ENV would be enough (however, using getenv() would no longer be enough in that case.)

[janhartigan]

Thanks for looking into this more deeply than I was capable of, @toddbc

And another issue thread here: laravel/framework#8191

[toddbc]

I actually happened to already know about putenv()'s threadsafety issues, I just didn't think about that when it occurred. As soon as I heard it was happening on Linux / event mpm, I knew this was definitely the issue.

PHP itself could "fix" this, but it would still leak env between threads. In a situation where Dotenv is used with a virtual host that has multiple .env files (e.g. subdirectories, w/e), this would still be unsafe. In that case (if Dotenv chooses not to fix this and rely on PHP to fix it) I would at least recommend noting in Dotenv's (and therefore Laravel's) documentation that it is not safe to use in this fashion.

Do you know if there's already a PHP bug open about this? The best they could do, that I can think of, would be persist the putenv hash for the entire process (e.g. MSHUTDOWN) and mutex getenv/putenv so they cannot be called concurrently (getenv calls concurrent to other getenv calls are probably / most likely safe, but two putenvs or a putenv and a getenv are not.)

[janhartigan]

I took care in my original PR to not make too many assumptions about the underlying cause, because I didn't truly understand it. As such, I'm not really sure about what existing PHP issues there are for this. In any case, I have learned a lot so far by reading your explanations.

[vlucas]

Dotenv was never meant to be used in production. I even say this explicitly in the README, but many many people continue to use it in production anyways. The entire point of dotenv is to emulate set environment variables for local development and testing, to get them out of your source control system. This is why you don't check-in the file - it's not even supposed to exist or be parsed in production. These environment variables should already be set and ready to be used on your server (in the Apache/nginx process, for instance). I have never supported or recommended using dotenv in production, and I myself do not use it in production. On production, I set my ENV vars in either in my nginx virtual host config, or define them in the control panel of a cloud hosting provider like Heroku, etc. This is the way it was meant to be done.

I do appreciate the very good explanation of what is going on, but this issue is a direct result of using dotenv in a way it was not meant to be used. I am not going to be a total ass about it, so I am open for possible fixes and suggestions for those who still wish to use it in production, but that is where it stands right now. Ideas?

[toddbc]

Okay, fair enough about the leaking bit - might want to make sure @taylorotwell knows this is not intended to be used in production, as Laravel's configuration documentation clearly seems to suggest using it in production.

This is actually only affecting me in a local environment, though. Although it is being used in production in my case, that's using php-fpm which is not multithreaded, so there's no issue there.

One option would be to add a method, like Dotenv::makeMutable(), that suppresses the use of putenv(). This wouldn't break backwards compatibility, but would provide a way for consumers to opt-in to functionality that is safe in multithreaded environments (getenv() would not work, though.)

Unfortunately, the situation with fixing it in PHP is a bit tricky. When a single process handles multiple requests, resetting on request shutdown makes some sense, and there's probably software depending on that behavior. On the other hand, it's a complete no-go when it's multithreaded.

[GrahamCampbell]

@toddbc You don't HAVE to use it in production with laravel you know. You just can, if you want to.

[toddbc]

Sure, you can remove or replace DetectEnvironment. I didn't notice anywhere in the documentation that suggested doing so.

Or I guess just leave it and not use the file.

[progmars]

While debugging, I found out that in my case when things break down for concurrent requests, the $key cannot be found nor in getenv(), nor in $_ENV and not even in $_SERVER.
The only thing that worked was to modify Dotenv class.
I added:

 protected static $cache = [];

then modified setEnvironmentVariable adding the following block right after list($name, $value) ...

    // workaround for 
    // https://github.com/laravel/framework/pull/8187
    // and
    // https://github.com/vlucas/phpdotenv/issues/76

    // don't rely upon findEnvironmentVariable because there might be situations
    // when getenv() or $_ENV is set, but $cached[$name] is not, 
    // and then for later requests getenv() and $_ENV are both empty and also no value in cache,
    // therefore this additional fix is here to ensure that we always cache the value

    // but first keep the value while we haven't updated the cache because after that the value will be returned from the cache
    // thus completely ignoring ENV, which is not what we intended
    $oldVal = static::findEnvironmentVariable($name);

    if (static::$immutable === false || !array_key_exists($name, static::$cache)){
        static::$cache[$name] = $value;
    }

and then in findEnvironmentVariable I added

case array_key_exists($name, static::$cache):
            return static::$cache[$name];

and call Dotenv::findEnvironmentVariable($key) everywhere where you would normally call getenv() (for example, in Laravel's helpers.php env() function, and replacing 'false' check with 'null' check).

Although Dotenv was not meant for production, I don't want to change our deployment and configuration workflow.

With my workaround I was able to run apache bench and also concurrent AJAX requests (queued with setInterval() ) for an hour and did not get any issues with Dotenv (before my fixes, I had about one crash each minute). So, now it seems Dotenv's findEnvironmentVariable() is more reliable than PHP's getenv().

[progmars]

Here is my workaround patch for Dotenv (but it includes also Laravel):
https://gist.github.com/progmars/db5b8e2331e8723dd637

[GrahamCampbell]

Starting in laravel 5.2, we only use dotenv to populate the config cache, then we don't load the environment on any requests. I recommend everyone does something like this.