From 3d5acbb1d0944ba00fee9af72f7253e6fc7f787e Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 5 Jul 2018 14:39:10 +0200 Subject: [PATCH] prevent lock values from going negative with memcache backend This can be caused by the code releasing more locks then it acquires, once the lock value becomes negative it's likely that it will never be able to change into an exclusive lock again. Signed-off-by: Robin Appelman --- lib/private/Lock/MemcacheLockingProvider.php | 10 ++++++++-- tests/lib/Lock/LockingProvider.php | 12 ++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/private/Lock/MemcacheLockingProvider.php b/lib/private/Lock/MemcacheLockingProvider.php index 55a82b2278..4d1b3dc0bc 100644 --- a/lib/private/Lock/MemcacheLockingProvider.php +++ b/lib/private/Lock/MemcacheLockingProvider.php @@ -90,14 +90,20 @@ class MemcacheLockingProvider extends AbstractLockingProvider { */ public function releaseLock(string $path, int $type) { if ($type === self::LOCK_SHARED) { + $newValue = 0; if ($this->getOwnSharedLockCount($path) === 1) { $removed = $this->memcache->cad($path, 1); // if we're the only one having a shared lock we can remove it in one go if (!$removed) { //someone else also has a shared lock, decrease only - $this->memcache->dec($path); + $newValue = $this->memcache->dec($path); } } else { // if we own more than one lock ourselves just decrease - $this->memcache->dec($path); + $newValue = $this->memcache->dec($path); + } + + // if we somehow release more locks then exists, reset the lock + if ($newValue < 0) { + $this->memcache->cad($path, $newValue); } } else if ($type === self::LOCK_EXCLUSIVE) { $this->memcache->cad($path, 'exclusive'); diff --git a/tests/lib/Lock/LockingProvider.php b/tests/lib/Lock/LockingProvider.php index 49742baa56..9c0461e2e6 100644 --- a/tests/lib/Lock/LockingProvider.php +++ b/tests/lib/Lock/LockingProvider.php @@ -243,4 +243,16 @@ abstract class LockingProvider extends TestCase { $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); $this->instance->changeLock('foo', ILockingProvider::LOCK_SHARED); } + + public function testReleaseNonExistingShared() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->releaseLock('foo', ILockingProvider::LOCK_SHARED); + + // releasing a lock once to many should not result in a locked state + $this->instance->releaseLock('foo', ILockingProvider::LOCK_SHARED); + + $this->instance->acquireLock('foo', ILockingProvider::LOCK_EXCLUSIVE); + $this->assertTrue($this->instance->isLocked('foo', ILockingProvider::LOCK_EXCLUSIVE)); + $this->instance->releaseLock('foo', ILockingProvider::LOCK_EXCLUSIVE); + } }