Framework: Race condition on session engine causing unexpected behavior on concurrent requests

Created on 11 Aug 2014 · 49Comments · Source: laravel/framework

The FileSessionHandler introduced as part of the new Session Engine in Laravel 4.1 doesn't perform any file locking during read and write operations on the underlying session file. This creates the opportunity for a race condition, which is more likely to occur when a large number of single session concurrent requests are made. This bug is present in Laravel versions 4.1 and newer.

The race condition occurs when a session is being written to the file. Currently, the write process causes the session file to be truncated before it is written to. As there is no lock on the file, if a read session file operation were to occur after the truncation but before the write, the data read will be empty. This will cause the session store to replace the existing session data with a new, empty session.

The solution is twofold:
1) The write method in FileSessionHandler should call file_put_contents with the LOCK_EX flag
2) The read method in FileSessionHandler should obtain a read lock before reading the file (not using file_get_contents).

If it were desired to continue to use the Filesystem class, one could add an optional argument to get() and put(), $lock=false. If $lock is set to true, then Laravel will obtain a lock before reading and writing. FileSessionHandler could then be modified to pass $lock = true.

bug

Source

PhilipChen

Most helpful comment

Using native PHP sessions is not a good solution. Native PHP sessions output cookies straight to the headers and there is no way to change that. It breaks the entire request/response wrapping we are trying to achieve.

taylorotwell on 30 Dec 2014

👍2

All 49 comments

Bump

avi123 on 26 Aug 2014

Bump again. This is a pretty serious issue with ajax applications.

avi123 on 29 Aug 2014

Bump. This is causing huge issues for our clients

Boogydown on 25 Sep 2014

Can anyone provide a pull request to fix this?

GrahamCampbell on 26 Dec 2014

Taylor has pushed a fix in d6648e2f19ab4c9f8806968b077f7e36c4939730. Feedback would be appreciated.

Ping @PhilipChen, @avi123, @Boogydown.

GrahamCampbell on 29 Dec 2014

Assuming this to be fixed. Ping me if this is not the case.

GrahamCampbell on 29 Dec 2014

@GrahamCampbell The fix is not complete. While it fixes concurrent writes, it doesn't fix the issue where a read can occur during the write process. More specifically, reads must explicitly obtain a lock before opening the file.

PhilipChen on 29 Dec 2014

No, we've totally locked it while we're writing to it. Php will wait to read the file while another thread is writing.

GrahamCampbell on 29 Dec 2014

Could you try this with the redis driver and tell me if you observe the same behaviour please?

GrahamCampbell on 29 Dec 2014

When you say "The fix is not complete", have you actually tested it. That would be really helpful to get to the bottom of this.

GrahamCampbell on 29 Dec 2014

@PhilipChen is correct. I just simulated locally with two concurrent PHP scripts.

The issue is:
1) The session driver calls the Illuminate FileSystem get function to read the session file:
https://github.com/laravel/framework/blob/d6648e2f19ab4c9f8806968b077f7e36c4939730/src/Illuminate/Session/FileSessionHandler.php#L58

2) The Illuminate FileSystem get function calls file_get_contents to read the file contents:
https://github.com/laravel/framework/blob/d6648e2f19ab4c9f8806968b077f7e36c4939730/src/Illuminate/Filesystem/Filesystem.php#L29

3) file_get_contents does not respect locks, as it uses the equivalent of fopen with rb flags:
https://github.com/php/php-src/blob/PHP-5.5.12/ext/standard/file.c#L540

avi123 on 29 Dec 2014

Ok, thanks for investigating. Could you propose a fix in a PR to 4.2 please.

GrahamCampbell on 29 Dec 2014

We fixed this locally by creating a custom session handler. Our goal was to mimic standard PHP session behavior, whereby a session is blocking until session_write_close (or in Lavarel's case, Session::save()) is called. We can do a PR of this if that behavior is of interest.

avi123 on 29 Dec 2014

Yes please. That was what we intended the behaviour to be.

GrahamCampbell on 29 Dec 2014

https://github.com/laravel/framework/pull/6848

avi123 on 29 Dec 2014

Can't we use the Symfony NativeFile Driver, which uses the php session driver, but different location?

barryvdh on 29 Dec 2014

I think Laravel removed support for the NativeFile Driver via their interface

avi123 on 29 Dec 2014

Could we not wrap it?

GrahamCampbell on 29 Dec 2014

This would be a good idea in laravel 5.

GrahamCampbell on 29 Dec 2014

Probably be better than the proposed solution i've just had a look at. It seems a bit over kill.

GrahamCampbell on 29 Dec 2014

Aren't they both just implementing the SessionHandlerInterface from php?

barryvdh on 29 Dec 2014

It's possible you could wrap it. We developed this for our own needs, where we have multiple concurrent api requests that create all sorts of race conditions if not properly handled. I've stripped out some applicatoin specifics, but this works for our needs. If there's something lighter weight, go for it.

avi123 on 29 Dec 2014

Aren't they both just implementing the SessionHandlerInterface from php?

Yeh, they are. Switching to symfony's is maybe the best solution.

GrahamCampbell on 29 Dec 2014

As an aside, the GC fix addresses a different Laravel issue that was partially fixed in Symfony but still had an edge case, I believe

avi123 on 29 Dec 2014

taylorotwell on 30 Dec 2014

👍2

Can someone answer the question if this occurs with the Memcached or Redis session drivers?

taylorotwell on 30 Dec 2014

So why doesn't Symfony have this problem?

It does seem a tricky issue, in Symfony the Native handler was removed and then added back again (https://github.com/symfony/symfony/pull/4899), also PDO (https://github.com/symfony/symfony/pull/10908) took some time (but we're also not using that). (+more issues, https://github.com/symfony/symfony/pull/4668 etc)

Maybe @drak could explain more, as he was involved in those changes?

barryvdh on 30 Dec 2014

someone would have to check if those drivers have atomic read/writes

avi123 on 30 Dec 2014

No they don't have locking. Neither does Symfony's Memcached driver. I'm mainly curious _why_ a series of concurrent _presumably_ idempotent operations (GET requests) would be adding values to the session anyways?

The session token mysteriously changing on a series of requests is indeed confusing and should be fixed though. @avi123 can you determine if the CSRF token thing occurs using the Memcached or Redis driver?

taylorotwell on 30 Dec 2014

@taylorotwell Probably not related, but with the new (L5) PreviousUrl functionality, you are writing on every request. Is that something to consider, cause it means writing on every GET request also, right?

The PDO issue states:

This is probably the first Session Handler for databases that actually works with locking.
And the issue about using the native handler (https://github.com/symfony/symfony/pull/4668) looks pretty similar ('Losing session in case of many concurrent requests')

barryvdh on 30 Dec 2014

@barryvdh we don't write the Previous URL on AJAX requests, which seems to be the most common issue here (main page requests followed by a lot of concurrent AJAX requests).

taylorotwell on 30 Dec 2014

First, I believe the Laravel request lifecycle calls a Session::start at the beginning of every request and a Session::save at the end of every request:

https://github.com/laravel/framework/blob/4.2/src/Illuminate/Session/Middleware.php#L58

So regardless of the HTTP method, you are ALWAYS forcing a session read and a session write. Now, there are two separate issues here:

1) General writing of data to sessions
2) CSRF specific problem (the subject of this bug)

The first issue occurs if you have multiple concurrent POSTs/PUTs, or even sequential asynchronous requests that happen in close succession such that the second request starts prior to the first request completing. The issue here is that changes to the session in the second request can be lost if the first request doesn't lock the session, but instead does a read, and then a full write at the completion of the request (I believe the method that the Session Store takes). This is the impetus for using a locking session driver in general.

The second issue is specific to the FileSessionHandler implemented by Laravel. Because session reads DO NOT respect file write locks, a request calling Session::start that HAPPENS to read the session file at the exact moment that the write call in the other request is between ftruncate and fwrite will receive NO DATA, thereby assuming that NO session exists and creating a new one, hence the new token. Because this has to occur at that exact moment, it does not always occur, and is more likely to occur when many short requests happen concurrently. If you were to want to recreate, I would suggest manually replacing file_put_contents with a fopen (mode c), ftruncate, and fwrite (as per https://github.com/php/php-src/blob/PHP-5.5.12/ext/standard/file.c#L567) and place a sleep between ftruncate and fwrite - you should see the same behavior.

Unfortunately, I do not have the bandwidth to test other drivers, however, I imagine if they support atomic updates you should be good to go.

avi123 on 30 Dec 2014

I'm mainly interested in fixing #2 at the moment. Forcing a sequential execution of requests by implementing locking on for example a Memcached driver is perhaps fixing a "problem" that shouldn't exist in the first place based on your application structure.

taylorotwell on 30 Dec 2014

If you add a call to $this->close() to the end of the read() method in https://github.com/laravel/framework/pull/6848, then requests will no longer be blocking, but the CSRF issue should be fixed. I can add if you guys are planning on merging that PR.

avi123 on 30 Dec 2014

@taylorotwell We are experiencing the same issue on Redis, I don't think it would be lock issue because redis is single thread and atomic, I used the method suggested in #6777 to debug and here is the log:

[2015-03-26 13:12:06] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:06] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:06] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:06] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:06] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:06] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:07] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:07] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:07] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:07] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:07] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:08] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:08] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:08] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:08] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:08] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:09] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:09] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:10] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:10] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:10] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:10] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:10] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:11] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:11] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:11] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:11] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:11] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []
[2015-03-26 13:12:11] production.INFO: ["k9OqsVTnE0QkphMjGLxfF8nnClidNXsDZo7i9H8C","9uIpGse9rdQlZJGs8NYMFH4s8KsWkvDsZ2efk5Fv","1a85f2d0adb856b874dda75f05c75ea8a7ad0c43"] [] []

you can see that the token is regenerated out of nowhere.

ammont on 26 Mar 2015

The problem is still relevant for me on Laravel 5. Even more, sometimes .env file contents can be lost for AJAX-intensive applications:
http://stackoverflow.com/questions/31295126/laravel-5-losing-sessions-and-env-configuration-values-in-ajax-intensive-applic

progmars on 8 Jul 2015

Even more, sometimes .env file contents can be lost for AJAX-intensive applications:

This is known, and has been discussed here. You MUST run the config:cache command to resolve this.

GrahamCampbell on 8 Jul 2015

Thanks, I thought that Dotenv issue is also related to file locking issues but it's not. I managed to fix Dotenv by using an array as a fallback as I described here: https://github.com/laravel/framework/pull/8187#issuecomment-86790971
but I'm not sure if that is the best solution. At least, it made Dotenv more reliable for me and now I'm not afraid to use it on production servers (which, most probably, wouldn't have the issue if using PHP CGI configuration). I also added a comment on Dotenv GitHub, so they can decide what to do with it.

Regarding the session issues, I guess, I'll have to look for an alternative or patched session drivers.

progmars on 10 Jul 2015

I just tried this approach. Modify Store.php:

add

protected $rawInitialData;

modify constructor:

$this->metaBag = new MetadataBag('_sf2_meta', 60); // <- had to add all arguments, lazy hard-coded 60, but could take from session config, if that makes sense

modify readFromHandler():

    $data = $this->handler->read($this->getId()); // original code, as is

    if ($data)
    {       
            $data = $this->prepareForUnserialize($data);
            $this->rawInitialData = $data; // <- keep raw string cached as is, but prepared - this takes care for EncryptedStore

            $data = @unserialize($data);

modify save():

    public function save()
    {
        $this->addBagDataToSession();

        $this->ageFlashData();

        // cache new datastring
        $serialized = serialize($this->attributes);

        if($serialized !== $this->rawInitialData){
            $this->handler->write($this->getId(), $this->prepareForStorage($serialized)); // <- modified and wrapped with "if" to write to file only if there are any changes
        }

Of course, there should be cleaner way to check if objects are dirty than comparing strings, but that would require much more code.

These fixes do not solve the issue, but at least reduce concurrent write/read scenarios if your AJAX requests are just reading. I now am running my stress test with insane amount of AJAX requests (some of them fail on Chrome with net:ERR_INSUFFICIENT_RESOURCES) but for an hour already I don't have any session failures at all. And as a bonus, performance might also increase a bit - less redundant writing to disk.

progmars on 10 Jul 2015

@progmars are your concurrent AJAX stress tests all read only for the session data? I'm just making sure that I understand that this fix is for read only sessions to prevent partial files from coming in but will presumably still break if you're changing things.

loren138 on 10 Jul 2015

@loren138 yes, they are read only; and things still will break the same as before if session data is changed in concurrent requests.
I just updated the original code to work if 'encrypt' => true in config/session.php. I'm working on a proper patch. Still, this is just a workaround. For a proper fix you should use an improved session driver with file locking or database transactions etc. In that case my fixes won't harm, but just ensure less unnecessary session writes.

progmars on 10 Jul 2015

So, here is my quick patch to make this issue much less frequent (or fix it completely, if you don't write to session yourself on every request):
(different patches for different Laravel versions)
https://gist.github.com/progmars/960b848170ff4ecd580a
https://gist.github.com/progmars/76598c982179bc335ebb

progmars on 10 Jul 2015

Does anyone got a solution to this problem? And why the issue occur only on Chrome browser?

sebastianvirlan on 14 Apr 2016

@sebastianvirlan In my experience, this occurs not only on Chrome but also in other apps which create lots of parallel requests to a Laravel web application. I have seen the same issue also when running Apache Bench ab tool.

The best solution would be to use some other, more concurrency-safe session driver. In my case it was enough to have my workarounds which at least prevent Laravel from rewriting session on every request, even when no session data has actually changed.

If anyone wants to better understand the core of the issue, I suggest this old and in-depth article:
http://thwartedefforts.org/2006/11/11/race-conditions-with-ajax-and-php-sessions/ (you can scroll down to The Default PHP Session Handler).

progmars on 15 Apr 2016

@progmars even if I use driver file, database, redis I get same.

https://www.youtube.com/watch?v=P_M9qf3W1CI

Look at chrome requests. I also see that if I start the server from a ubuntu and access the ip from a windows chrome the problem will not occur anymore.

sebastianvirlan on 18 Apr 2016

@sebastianvirlan We had the same issue, here is my complete explanation about the issue:
http://stackoverflow.com/questions/27938723/laravel-session-expires-randomly/29895324#29895324

ammont on 18 Apr 2016

I read your answer. Very sad that this happen. So from the developers of laravel what we have? We will have any fix? Because this is a very serious problem, I can make a clean laravel project with this script:

<script type="text/javascript">
    $.ajaxSetup({
        headers: {
            'X-CSRF-TOKEN': $('meta[name="csrf-token"]').attr('content')
        },
        data: {
            _token: $('meta[name="csrf-token"]').attr('content')
        }
    });

    for (var i = 0; i <= 50; i++) {
        getUsers();
    }
    function getUsers()
    {
        $.ajax({
            url: '/getUsers',
            type: 'POST',
            data: {field: 'field'},
            success: function(data){

            }
        });
    }
</script>

and a POST route

Route::POST('/getUsers', 'HomeController@users');

And the issue still occur random (but as I discover last days only if I start the server on windows).

sebastianvirlan on 18 Apr 2016

I have tried several suggestions for fix this issue, but in my case, using a route outside from the web, auth groups solved the 401 error on multiple ajax request. May be obvious for the experts here, but this workaround present some potencial risk for sensitive data. Hope this comment helps to find a definitive solution