From e92aeade640355894989b888a67812002ed06c4d Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 22 Jul 2026 11:07:03 -0400 Subject: [PATCH 1/3] fix(lock): reject invalid lock types and modernize memcache provider Replace the magic exclusive-lock value with a named constant, simplify TTL handling, reject invalid lock types explicitly, clarify lock-release and conversion logic, and improve concurrency documentation around shared-lock TTL restoration. Assisted-by: GitHubCopilot:GPT-5.6 Signed-off-by: Josh --- lib/private/Lock/MemcacheLockingProvider.php | 152 +++++++++++-------- 1 file changed, 92 insertions(+), 60 deletions(-) diff --git a/lib/private/Lock/MemcacheLockingProvider.php b/lib/private/Lock/MemcacheLockingProvider.php index 51a3ca26ec764..c6c69f3e31f40 100644 --- a/lib/private/Lock/MemcacheLockingProvider.php +++ b/lib/private/Lock/MemcacheLockingProvider.php @@ -14,7 +14,12 @@ use OCP\IMemcacheTTL; use OCP\Lock\LockedException; +/** + * Locking provider that stores locks in memcache. + */ class MemcacheLockingProvider extends AbstractLockingProvider { + private const EXCLUSIVE_LOCK_VALUE = 'exclusive'; + /** @var array */ private array $oldTTLs = []; @@ -27,78 +32,93 @@ public function __construct( } private function setTTL(string $path, ?int $ttl = null, mixed $compare = null): void { - if (is_null($ttl)) { + if ($ttl === null) { $ttl = $this->ttl; } - if ($this->memcache instanceof IMemcacheTTL) { - if ($compare !== null) { - $this->memcache->compareSetTTL($path, $compare, $ttl); - } else { - $this->memcache->setTTL($path, $ttl); - } + + if (!$this->memcache instanceof IMemcacheTTL) { + return; + } + + if ($compare !== null) { + $this->memcache->compareSetTTL($path, $compare, $ttl); + return; } + + $this->memcache->setTTL($path, $ttl); } + /** + * @return int Actual TTL, or -1 when TTL support is unavailable or no TTL is returned + */ private function getTTL(string $path): int { - if ($this->memcache instanceof IMemcacheTTL) { - $ttl = $this->memcache->getTTL($path); - return $ttl === false ? -1 : $ttl; - } else { + if (!$this->memcache instanceof IMemcacheTTL) { return -1; } + + $ttl = $this->memcache->getTTL($path); + return $ttl === false ? -1 : $ttl; } #[\Override] public function isLocked(string $path, int $type): bool { + $this->assertValidLockType($type); + $lockValue = $this->memcache->get($path); - if ($type === self::LOCK_SHARED) { - return is_int($lockValue) && $lockValue > 0; - } elseif ($type === self::LOCK_EXCLUSIVE) { - return $lockValue === 'exclusive'; - } else { - return false; - } + + return match ($type) { + self::LOCK_SHARED => is_int($lockValue) && $lockValue > 0, + self::LOCK_EXCLUSIVE => $lockValue === self::EXCLUSIVE_LOCK_VALUE, + }; } #[\Override] public function acquireLock(string $path, int $type, ?string $readablePath = null): void { + $this->assertValidLockType($type); + if ($type === self::LOCK_SHARED) { - // save the old TTL to for `restoreTTL` + // Save the previous TTL for restoreTTL(). $this->oldTTLs[$path] = [ 'ttl' => $this->getTTL($path), - 'time' => $this->timeFactory->getTime() + 'time' => $this->timeFactory->getTime(), ]; if (!$this->memcache->inc($path)) { throw new LockedException($path, null, $this->getExistingLockForException($path), $readablePath); } - } else { - // when getting exclusive locks, we know there are no old TTLs to restore + } elseif ($type === self::LOCK_EXCLUSIVE) { + // An exclusive lock has no TTL to restore. $this->memcache->add($path, 0); - // ttl is updated automatically when the `set` succeeds - if (!$this->memcache->cas($path, 0, 'exclusive')) { + // The TTL is updated automatically when the compare-and-set succeeds. + if (!$this->memcache->cas($path, 0, self::EXCLUSIVE_LOCK_VALUE)) { throw new LockedException($path, null, $this->getExistingLockForException($path), $readablePath); } unset($this->oldTTLs[$path]); } + $this->setTTL($path); $this->markAcquire($path, $type); } #[\Override] public function releaseLock(string $path, int $type): void { + $this->assertValidLockType($type); + if ($type === self::LOCK_SHARED) { $ownSharedLockCount = $this->getOwnSharedLockCount($path); $newValue = 0; - if ($ownSharedLockCount === 0) { // if we are not holding the lock, don't try to release it + // If we do not hold the lock, do not try to release it. + if ($ownSharedLockCount === 0) { return; } + // If we own the only shared lock, remove it atomically. if ($ownSharedLockCount === 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 + $removed = $this->memcache->cad($path, 1); + if (!$removed) { + // Another owner holds a shared lock; decrement only our share. $newValue = $this->memcache->dec($path); } } else { - // if we own more than one lock ourselves just decrease + // If we hold more than one shared lock, decrement only our count. $newValue = $this->memcache->dec($path); } @@ -108,79 +128,91 @@ public function releaseLock(string $path, int $type): void { unset($this->oldTTLs[$path]); } - // if we somehow release more locks then exists, reset the lock + // If we (somehow) release more locks than exist, reset the lock. if ($newValue < 0) { $this->memcache->cad($path, $newValue); } } elseif ($type === self::LOCK_EXCLUSIVE) { - $this->memcache->cad($path, 'exclusive'); + $this->memcache->cad($path, self::EXCLUSIVE_LOCK_VALUE); } + $this->markRelease($path, $type); } #[\Override] public function changeLock(string $path, int $targetType): void { + $this->assertValidLockType($targetType); + if ($targetType === self::LOCK_SHARED) { - if (!$this->memcache->cas($path, 'exclusive', 1)) { + if (!$this->memcache->cas($path, self::EXCLUSIVE_LOCK_VALUE, 1)) { throw new LockedException($path, null, $this->getExistingLockForException($path)); } } elseif ($targetType === self::LOCK_EXCLUSIVE) { - // we can only change a shared lock to an exclusive if there's only a single owner of the shared lock - if (!$this->memcache->cas($path, 1, 'exclusive')) { + // A shared lock can only be upgraded to exclusive when the shared lock has a single owner. + if (!$this->memcache->cas($path, 1, self::EXCLUSIVE_LOCK_VALUE)) { $this->restoreTTL($path); throw new LockedException($path, null, $this->getExistingLockForException($path)); } unset($this->oldTTLs[$path]); } + $this->setTTL($path); $this->markChange($path, $targetType); } /** - * With shared locks, each time the lock is acquired, the ttl for the path is reset. + * With shared locks, each acquisition resets the path's TTL. * - * Due to this "ttl extension" when a shared lock isn't freed correctly for any reason - * the lock won't expire until no shared locks are required for the path for 1h. - * This can lead to a client repeatedly trying to upload a file, and failing forever - * because the lock never gets the opportunity to expire. + * A side effect of this automatic TTL extension is that a shared lock not + * released for any reason may never expire if shared locks continue to be + * acquired for the path. With the default one-hour TTL, a client repeatedly + * trying to upload a file can therefore fail indefinitely because the lock + * never has an opportunity to expire. * - * To help the lock expire in this case, we lower the TTL back to what it was before we - * took the shared lock *only* if nobody else got a shared lock after we did. + * To help the lock expire in this case, restore the path's previous TTL, but + * only if no other concurrent owner, such as another request, acquired a + * shared lock after this one. * - * This doesn't handle all cases where multiple requests are acquiring shared locks - * but it should handle some of the more common ones and not hurt things further + * This does not cover every concurrent shared-lock scenario, but mitigates + * common cases without making them worse. */ private function restoreTTL(string $path): void { - if (isset($this->oldTTLs[$path])) { - $saved = $this->oldTTLs[$path]; - $elapsed = $this->timeFactory->getTime() - $saved['time']; + if (!isset($this->oldTTLs[$path])) { + return; + } - // old value to compare to when setting ttl in case someone else changes the lock in the middle of this function - $value = $this->memcache->get($path); + $saved = $this->oldTTLs[$path]; + $elapsed = $this->timeFactory->getTime() - $saved['time']; - $currentTtl = $this->getTTL($path); + // Value to compare when updating the TTL in case another request changes the lock. + $value = $this->memcache->get($path); - // what the old ttl would be given the time elapsed since we acquired the lock - // note that if this gets negative the key will be expired directly when we set the ttl - $remainingOldTtl = $saved['ttl'] - $elapsed; - // what the currently ttl would be if nobody else acquired a lock since we did (+1 to cover rounding errors) - $expectedTtl = $this->ttl - $elapsed + 1; + $currentTtl = $this->getTTL($path); - // check if another request has acquired a lock (and didn't release it yet) - if ($currentTtl <= $expectedTtl) { - $this->setTTL($path, $remainingOldTtl, $value); - } + // Account for elapsed time since acquiring the shared lock. + $remainingOldTtl = $saved['ttl'] - $elapsed; + + // Allow one second for rounding when detecting a concurrent acquisition. + $expectedTtl = $this->ttl - $elapsed + 1; + + // Restore the TTL only if no other request has acquired the lock since. + if ($currentTtl <= $expectedTtl) { + // A negative remaining TTL expires the key. + $this->setTTL($path, $remainingOldTtl, $value); } } private function getExistingLockForException(string $path): string { $existing = $this->memcache->get($path); + if (!$existing) { return 'none'; - } elseif ($existing === 'exclusive') { + } + + if ($existing === self::EXCLUSIVE_LOCK_VALUE) { return $existing; - } else { - return $existing . ' shared locks'; } + + return $existing . ' shared locks'; } } From 75fff30caa61bc234499ceff70d428c68643d353 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 22 Jul 2026 12:33:45 -0400 Subject: [PATCH 2/3] fix(lock): reject invalid lock types and modernize db provider Clarify retained shared-lock bookkeeping, align query construction, reject invalid lock types explicitly, and remove redundant inherited documentation. Assisted-by: GitHubCopilot:GPT-5.6 Signed-off-by: Josh --- lib/private/Lock/DBLockingProvider.php | 195 ++++++++++++++----------- 1 file changed, 108 insertions(+), 87 deletions(-) diff --git a/lib/private/Lock/DBLockingProvider.php b/lib/private/Lock/DBLockingProvider.php index 8cefabc662f45..1fb794221fe73 100644 --- a/lib/private/Lock/DBLockingProvider.php +++ b/lib/private/Lock/DBLockingProvider.php @@ -1,5 +1,6 @@ */ private array $sharedLocks = []; public function __construct( @@ -31,47 +32,49 @@ public function __construct( } /** - * Check if we have an open shared lock for a path + * Whether this request tracks a shared lock retained in the database. */ protected function isLocallyLocked(string $path): bool { - return isset($this->sharedLocks[$path]) && $this->sharedLocks[$path]; + return $this->sharedLocks[$path] ?? false; } - /** @inheritDoc */ #[\Override] protected function markAcquire(string $path, int $targetType): void { parent::markAcquire($path, $targetType); - if ($this->cacheSharedLocks) { - if ($targetType === self::LOCK_SHARED) { - $this->sharedLocks[$path] = true; - } + + if (!$this->cacheSharedLocks) { + return; + } + + if ($targetType === self::LOCK_SHARED) { + $this->sharedLocks[$path] = true; } } - /** - * Change the type of an existing tracked lock - */ #[\Override] protected function markChange(string $path, int $targetType): void { parent::markChange($path, $targetType); - if ($this->cacheSharedLocks) { - if ($targetType === self::LOCK_SHARED) { - $this->sharedLocks[$path] = true; - } elseif ($targetType === self::LOCK_EXCLUSIVE) { - $this->sharedLocks[$path] = false; - } + + if (!$this->cacheSharedLocks) { + return; + } + + if ($targetType === self::LOCK_SHARED) { + $this->sharedLocks[$path] = true; + } elseif ($targetType === self::LOCK_EXCLUSIVE) { + $this->sharedLocks[$path] = false; } } /** - * Insert a file locking row if it does not exists. + * Insert a file-lock row if it does not already exist. */ protected function initLockField(string $path, int $lock = 0): int { $expire = $this->getExpireTime(); return $this->connection->insertIgnoreConflict('file_locks', [ 'key' => $path, 'lock' => $lock, - 'ttl' => $expire + 'ttl' => $expire, ]); } @@ -79,39 +82,43 @@ protected function getExpireTime(): int { return $this->timeFactory->getTime() + $this->ttl; } - /** @inheritDoc */ #[\Override] public function isLocked(string $path, int $type): bool { + $this->assertValidLockType($type); + if ($this->hasAcquiredLock($path, $type)) { return true; } + $query = $this->connection->getQueryBuilder(); $query->select('lock') ->from('file_locks') ->where($query->expr()->eq('key', $query->createNamedParameter($path))); + $result = $query->executeQuery(); $lockValue = (int)$result->fetchOne(); + if ($type === self::LOCK_SHARED) { - if ($this->isLocallyLocked($path)) { - // if we have a shared lock we kept open locally but it's released we always have at least 1 shared lock in the db - return $lockValue > 1; - } else { - return $lockValue > 0; - } - } elseif ($type === self::LOCK_EXCLUSIVE) { - return $lockValue === -1; - } else { - return false; + // A shared lock retained for this request remains in the database after it is + // released from active bookkeeping. + return $this->isLocallyLocked($path) + ? $lockValue > 1 + : $lockValue > 0; } + + return $lockValue === -1; } - /** @inheritDoc */ #[\Override] public function acquireLock(string $path, int $type, ?string $readablePath = null): void { + $this->assertValidLockType($type); + $expire = $this->getExpireTime(); + if ($type === self::LOCK_SHARED) { if (!$this->isLocallyLocked($path)) { $result = $this->initLockField($path, 1); + if ($result <= 0) { $query = $this->connection->getQueryBuilder(); $query->update('file_locks') @@ -124,12 +131,20 @@ public function acquireLock(string $path, int $type, ?string $readablePath = nul } else { $result = 1; } - } else { + } elseif ($type === self::LOCK_EXCLUSIVE) { $existing = 0; - if ($this->hasAcquiredLock($path, ILockingProvider::LOCK_SHARED) === false && $this->isLocallyLocked($path)) { + + // A shared lock retained for this request may remain in the database after it + // has been released from active bookkeeping. + if ( + $this->hasAcquiredLock($path, self::LOCK_SHARED) === false + && $this->isLocallyLocked($path) + ) { $existing = 1; } + $result = $this->initLockField($path, -1); + if ($result <= 0) { $query = $this->connection->getQueryBuilder(); $query->update('file_locks') @@ -140,85 +155,94 @@ public function acquireLock(string $path, int $type, ?string $readablePath = nul $result = $query->executeStatement(); } } + if ($result !== 1) { throw new LockedException($path, null, null, $readablePath); } + $this->markAcquire($path, $type); } - /** @inheritDoc */ #[\Override] public function releaseLock(string $path, int $type): void { + $this->assertValidLockType($type); + $this->markRelease($path, $type); - // we keep shared locks till the end of the request so we can re-use them + // Shared locks remain in the database until the end of the request so they + // can be reused. if ($type === self::LOCK_EXCLUSIVE) { - $qb = $this->connection->getQueryBuilder(); - $qb->update('file_locks') - ->set('lock', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)) - ->where($qb->expr()->eq('key', $qb->createNamedParameter($path))) - ->andWhere($qb->expr()->eq('lock', $qb->createNamedParameter(-1, IQueryBuilder::PARAM_INT))); - $qb->executeStatement(); + $query = $this->connection->getQueryBuilder(); + $query->update('file_locks') + ->set('lock', $query->createNamedParameter(0, IQueryBuilder::PARAM_INT)) + ->where($query->expr()->eq('key', $query->createNamedParameter($path))) + ->andWhere($query->expr()->eq('lock', $query->createNamedParameter(-1, IQueryBuilder::PARAM_INT))); + $query->executeStatement(); } elseif (!$this->cacheSharedLocks) { - $qb = $this->connection->getQueryBuilder(); - $qb->update('file_locks') - ->set('lock', $qb->func()->subtract('lock', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT))) - ->where($qb->expr()->eq('key', $qb->createNamedParameter($path))) - ->andWhere($qb->expr()->gt('lock', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))); - $qb->executeStatement(); + $query = $this->connection->getQueryBuilder(); + $query->update('file_locks') + ->set('lock', $query->func()->subtract('lock', $query->createNamedParameter(1, IQueryBuilder::PARAM_INT))) + ->where($query->expr()->eq('key', $query->createNamedParameter($path))) + ->andWhere($query->expr()->gt('lock', $query->createNamedParameter(0, IQueryBuilder::PARAM_INT))); + $query->executeStatement(); } } - /** @inheritDoc */ #[\Override] public function changeLock(string $path, int $targetType): void { + $this->assertValidLockType($targetType); + $expire = $this->getExpireTime(); + if ($targetType === self::LOCK_SHARED) { - $qb = $this->connection->getQueryBuilder(); - $result = $qb->update('file_locks') - ->set('lock', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)) - ->set('ttl', $qb->createNamedParameter($expire, IQueryBuilder::PARAM_INT)) - ->where($qb->expr()->andX( - $qb->expr()->eq('key', $qb->createNamedParameter($path)), - $qb->expr()->eq('lock', $qb->createNamedParameter(-1, IQueryBuilder::PARAM_INT)) - ))->executeStatement(); - } else { - // since we only keep one shared lock in the db we need to check if we have more than one shared lock locally manually - if (isset($this->acquiredLocks['shared'][$path]) && $this->acquiredLocks['shared'][$path] > 1) { + $query = $this->connection->getQueryBuilder(); + $query->update('file_locks') + ->set('lock', $query->createNamedParameter(1, IQueryBuilder::PARAM_INT)) + ->set('ttl', $query->createNamedParameter($expire, IQueryBuilder::PARAM_INT)) + ->where($query->expr()->andX( + $query->expr()->eq('key', $query->createNamedParameter($path)), + $query->expr()->eq('lock', $query->createNamedParameter(-1, IQueryBuilder::PARAM_INT)))); + $result = $query->executeStatement(); + } elseif ($targetType === self::LOCK_EXCLUSIVE) { + // The database retains one shared lock per request, so an upgrade is only + // possible when this request holds exactly one shared lock. + if ($this->getOwnSharedLockCount($path) > 1) { throw new LockedException($path); } - $qb = $this->connection->getQueryBuilder(); - $result = $qb->update('file_locks') - ->set('lock', $qb->createNamedParameter(-1, IQueryBuilder::PARAM_INT)) - ->set('ttl', $qb->createNamedParameter($expire, IQueryBuilder::PARAM_INT)) - ->where($qb->expr()->andX( - $qb->expr()->eq('key', $qb->createNamedParameter($path)), - $qb->expr()->eq('lock', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)) - ))->executeStatement(); + + $query = $this->connection->getQueryBuilder(); + $query->update('file_locks') + ->set('lock', $query->createNamedParameter(-1, IQueryBuilder::PARAM_INT)) + ->set('ttl', $query->createNamedParameter($expire, IQueryBuilder::PARAM_INT)) + ->where($query->expr()->andX( + $query->expr()->eq('key', $query->createNamedParameter($path)), + $query->expr()->eq('lock', $query->createNamedParameter(1, IQueryBuilder::PARAM_INT)))); + $result = $query->executeStatement(); } + if ($result !== 1) { throw new LockedException($path); } + $this->markChange($path, $targetType); } - /** @inheritDoc */ public function cleanExpiredLocks(): void { $expire = $this->timeFactory->getTime(); + try { - $qb = $this->connection->getQueryBuilder(); - $qb->delete('file_locks') - ->where($qb->expr()->lt('ttl', $qb->createNamedParameter($expire, IQueryBuilder::PARAM_INT))) - ->executeStatement(); + $query = $this->connection->getQueryBuilder(); + $query->delete('file_locks') + ->where($query->expr()->lt('ttl', $query->createNamedParameter($expire, IQueryBuilder::PARAM_INT))); + $query->executeStatement(); } catch (\Exception $e) { - // If the table is missing, the clean up was successful + // If the table is missing, cleanup was successful. if ($this->connection->tableExists('file_locks')) { throw $e; } } } - /** @inheritDoc */ #[\Override] public function releaseAll(): void { parent::releaseAll(); @@ -226,23 +250,20 @@ public function releaseAll(): void { if (!$this->cacheSharedLocks) { return; } - // since we keep shared locks we need to manually clean those - $lockedPaths = array_keys($this->sharedLocks); - $lockedPaths = array_filter($lockedPaths, function ($path) { - return $this->sharedLocks[$path]; - }); + // Shared locks remain in the database until the end of the request. + $lockedPaths = array_keys(array_filter($this->sharedLocks)); $chunkedPaths = array_chunk($lockedPaths, 100); - $qb = $this->connection->getQueryBuilder(); - $qb->update('file_locks') - ->set('lock', $qb->func()->subtract('lock', $qb->expr()->literal(1))) - ->where($qb->expr()->in('key', $qb->createParameter('chunk'))) - ->andWhere($qb->expr()->gt('lock', new Literal(0))); + $query = $this->connection->getQueryBuilder(); + $query->update('file_locks') + ->set('lock', $query->func()->subtract('lock', $query->expr()->literal(1))) + ->where($query->expr()->in('key', $query->createParameter('chunk'))) + ->andWhere($query->expr()->gt('lock', new Literal(0))); foreach ($chunkedPaths as $chunk) { - $qb->setParameter('chunk', $chunk, IQueryBuilder::PARAM_STR_ARRAY); - $qb->executeStatement(); + $query->setParameter('chunk', $chunk, IQueryBuilder::PARAM_STR_ARRAY); + $query->executeStatement(); } } } From a1cda605e358e1407430aa55d290173cdb3f5e98 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 22 Jul 2026 12:37:17 -0400 Subject: [PATCH 3/3] test(lock): add lock type validation rejection checks to LockingProvider Signed-off-by: Josh --- tests/lib/Lock/LockingProvider.php | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/lib/Lock/LockingProvider.php b/tests/lib/Lock/LockingProvider.php index 74172889e4d0b..a22ea00a80440 100644 --- a/tests/lib/Lock/LockingProvider.php +++ b/tests/lib/Lock/LockingProvider.php @@ -240,4 +240,28 @@ public function testReleaseNonExistingShared(): void { $this->assertTrue($this->instance->isLocked('foo', ILockingProvider::LOCK_EXCLUSIVE)); $this->instance->releaseLock('foo', ILockingProvider::LOCK_EXCLUSIVE); } + + public function testIsLockedWithInvalidType(): void { + $this->expectException(\InvalidArgumentException::class); + + $this->instance->isLocked('foo', 0); + } + + public function testAcquireLockWithInvalidType(): void { + $this->expectException(\InvalidArgumentException::class); + + $this->instance->acquireLock('foo', 0); + } + + public function testReleaseLockWithInvalidType(): void { + $this->expectException(\InvalidArgumentException::class); + + $this->instance->releaseLock('foo', 0); + } + + public function testChangeLockWithInvalidType(): void { + $this->expectException(\InvalidArgumentException::class); + + $this->instance->changeLock('foo', 0); + } }