Race condition on session engine causing unexpected behavior on concurrent requests

[PhilipChen]

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.

[avi123]

Bump

[avi123]

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

[Boogydown]

Bump. This is causing huge issues for our clients

[GrahamCampbell]

Can anyone provide a pull request to fix this?

[GrahamCampbell]

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

Ping @PhilipChen, @avi123, @Boogydown.

[GrahamCampbell]

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

[PhilipChen]

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

[GrahamCampbell]

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]

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

[GrahamCampbell]

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.

[avi123]

@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:
   

   framework/src/Illuminate/Session/FileSessionHandler.php

   Line 58 in d6648e2

   return $this->files->get($path);

2. The Illuminate FileSystem get function calls file_get_contents to read the file contents:
   

   framework/src/Illuminate/Filesystem/Filesystem.php

   Line 29 in d6648e2

   if ($this->isFile($path)) return file_get_contents($path);

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

[GrahamCampbell]

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

[avi123]

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.

[GrahamCampbell]

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

[avi123]

#6848

[barryvdh]

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

[avi123]

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

[GrahamCampbell]

Could we not wrap it?

[GrahamCampbell]

This would be a good idea in laravel 5.

[GrahamCampbell]

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

[barryvdh]

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

[avi123]

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.

[GrahamCampbell]

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

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

[taylorotwell]

@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).

[avi123]

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.

[taylorotwell]

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.

[avi123]

If you add a call to $this->close() to the end of the read() method in #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.

[ammont]

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

[progmars]

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

[GrahamCampbell]

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.

[progmars]

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: #8187 (comment)
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]

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.

[loren138]

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

[progmars]

@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]

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

[sebastianvirlan]

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

[progmars]

@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).

[sebastianvirlan]

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

[ammont]

@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

[sebastianvirlan]

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

[Angel-Gonzalez]

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

[sebastianvirlan]

Did not found a solution yet :(

@Angel-Gonzalez your solution does not solve the issue in my case.

#13949