Changing process environment unsafe on multithreaded servers

[vlucas]

Changing process environment unsafe on multithreaded servers

[bappy004]

For lumen, we dont have config:cache command available. Any suggestions what to do for lumen?

[GrahamCampbell]

ONLY use env inside your config files, and then you MUST use the cache command to setup the config files. Then there are ZERO env issues in production.

[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?

[imbrish]

As this issue is widely referenced, I'll mention a monkey patch to solve the problem in local development, see laravel/framework#8191 (comment). In production always use php artisan config:cache.

[blubcow]

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.

@vlucas Very funny... then this library is basically useless. These dotenv bugs are happening mostly on development environments anyway. Telling us not to use it when publishing is a huge slap in the face >.<

[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.

[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.

[spescina]

Sorry @GrahamCampbell, I can't understand your words. It seems to me that laravel does not encourage to not use dotenv in production.
What do you mean with

Starting in laravel 5.2, we only use dotenv to populate the config cache

?

[GrahamCampbell]

It seems to me that laravel does not encourage to not use dotenv in production.

Yes it does. Taylor and I set up Laravel 5.2 in this way so it is safe to use.

[spescina]

I understand it's safe and from now on I will setup laravel in production in this way. I'm asking why there's no indication of this practice in the docs. I can only read that using config cache is optional and that it's a performance feature, just like route cache. If this package is unsafe, there should be at least a warning in the docs. Am I wrong?

[blubcow]

At least I got some success by

• starting the session always before initializing php-dotenv
• writing the ENV encoded into the session, and reading it if it's empty

Fix works on Windows, PHP 5.6.11 for me.
For production I'll use server side environment variables from now on. Thanks :)

[GrahamCampbell]

I think it's likely the following example will work in a multithreaded environment. This avoids using the adapter that would have called putenv and getenv, which are not threadsafe.

<?php

use Dotenv\Environment\Adapter\EnvConstAdapter;
use Dotenv\Environment\Adapter\ServerConstAdapter;
use Dotenv\Environment\DotenvFactory;
use Dotenv\Dotenv;

$factory = new DotenvFactory([new EnvConstAdapter(), new ServerConstAdapter()]);

Dotenv::create($path, null, $factory)->load();