Atomic locks do not provide reliable mutual exclusion when using the file cache driver

[hbgl]

• Laravel Version: 9.23.0
• PHP Version: 8.0.15
• Database Driver & Version: none

Description:

The locks returned by FileStore are instances of the class CacheLock. CacheLock's implementation of the acquire method is not atomic when the lock timeout is 0 (the default):

framework/src/Illuminate/Cache/CacheLock.php

Lines 30 to 50 in 053840f

	/**
	* Attempt to acquire the lock.
	*
	* @return bool
	*/
	public function acquire()
	{
	if (method_exists($this->store, 'add') && $this->seconds > 0) {
	return $this->store->add(
	$this->name, $this->owner, $this->seconds
	);
	}
	if (! is_null($this->store->get($this->name))) {
	return false;
	}
	return ($this->seconds > 0)
	? $this->store->put($this->name, $this->owner, $this->seconds)
	: $this->store->forever($this->name, $this->owner);
	}

When the lock timeout is 0, first a get and then a put is performed. When combined, these operations are not atomic and thus multiple actors can enter the critical section protected by the lock. The possibility for race conditions was already discussed in the PR #35139 that introduced the change.

Proposed quick fix

A quick fix would be to always call $this->store->add even if $this->seconds <= 0 because the store can handle 0 timeouts just fine:

public function acquire() 
{ 
    return $this->store->add($this->name, $this->owner, $this->seconds); 
}

There is no need to even check that the add method exists because it is the stores responsibility to return a compatible lock instance.

Proposed proper fix

The proper fix would be to write a custom lock implementation for each store. There already exists DatabaseLock, RedisLock, DynamoDbLock etc. which can guarantee mutual exclusion because their implementation is tailored to the specific store. There is no good way to write an abstract lock implementation that CacheLock attempts to be because the Store contract does not provide the necessary atomic primitives. Calling an undocumented add method on the store, which may or may not be atomic, seems like the wrong thing to do.

Steps To Reproduce:

I created a a repo that can pretty reliably reproduce the race condition:
https://github.com/hbgl/laravel-test-locks

Instructions:

git clone https://github.com/hbgl/laravel-test-locks
cd laravel-test-locks
composer install
php artisan app:test-locks --workers=2 --iterations=1000 --cache-driver=file

[driesvints]

Thanks @hbgl, this was a well written issue. We'd very much appreciate a PR for one of the solutions you propose 👍