From 35047a23000fa4788a06332428aa0f844394816f Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 22 Jun 2015 12:38:48 +0200 Subject: [PATCH 1/2] Fix locked paths in the moveMount case When moving a mount point directly, the lock must be applied on the local mount point path instead of the attached storage root. Other operations will still lock the attached storage root. --- lib/private/files/view.php | 80 +++++++++++++++++++++++---------- tests/lib/files/view.php | 91 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 23 deletions(-) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 73daf8a141..61adc6246f 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -628,8 +628,8 @@ class View { return false; } - $this->lockFile($path1, ILockingProvider::LOCK_SHARED); - $this->lockFile($path2, ILockingProvider::LOCK_SHARED); + $this->lockFile($path1, ILockingProvider::LOCK_SHARED, true); + $this->lockFile($path2, ILockingProvider::LOCK_SHARED, true); $run = true; if ($this->shouldEmitHooks() && (Cache\Scanner::isPartialFile($path1) && !Cache\Scanner::isPartialFile($path2))) { @@ -656,8 +656,8 @@ class View { $internalPath1 = $mount1->getInternalPath($absolutePath1); $internalPath2 = $mount2->getInternalPath($absolutePath2); - $this->changeLock($path1, ILockingProvider::LOCK_EXCLUSIVE); - $this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE); + $this->changeLock($path1, ILockingProvider::LOCK_EXCLUSIVE, true); + $this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE, true); if ($internalPath1 === '' and $mount1 instanceof MoveableMount) { if ($this->isTargetAllowed($absolutePath2)) { @@ -670,12 +670,14 @@ class View { } else { $result = false; } + // moving a file/folder within the same mount point } elseif ($storage1 == $storage2) { if ($storage1) { $result = $storage1->rename($internalPath1, $internalPath2); } else { $result = false; } + // moving a file/folder between storages (from $storage1 to $storage2) } else { $result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2); } @@ -693,13 +695,8 @@ class View { } } - $this->unlockFile($path1, ILockingProvider::LOCK_EXCLUSIVE); - $this->unlockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); - - if ($internalPath1 === '' and $mount1 instanceof MoveableMount) { - // since $path2 now points to a different storage we need to unlock the path on the old storage separately - $storage2->releaseLock($internalPath2, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider); - } + $this->unlockFile($path1, ILockingProvider::LOCK_EXCLUSIVE, true); + $this->unlockFile($path2, ILockingProvider::LOCK_EXCLUSIVE, true); if ((Cache\Scanner::isPartialFile($path1) && !Cache\Scanner::isPartialFile($path2)) && $result !== false) { if ($this->shouldEmitHooks()) { @@ -719,8 +716,8 @@ class View { } return $result; } else { - $this->unlockFile($path1, ILockingProvider::LOCK_SHARED); - $this->unlockFile($path2, ILockingProvider::LOCK_SHARED); + $this->unlockFile($path1, ILockingProvider::LOCK_SHARED, true); + $this->unlockFile($path2, ILockingProvider::LOCK_SHARED, true); return false; } } else { @@ -1699,22 +1696,51 @@ class View { return $result; } + /** + * Returns the mount point for which to lock + * + * @param string $absolutePath absolute path + * @param bool $useParentMount true to return parent mount instead of whatever + * is mounted directly on the given path, false otherwise + * @return \OC\Files\Mount\MountPoint mount point for which to apply locks + */ + private function getMountForLock($absolutePath, $useParentMount = false) { + $results = []; + $mount = Filesystem::getMountManager()->find($absolutePath); + if (!$mount) { + return $results; + } + + if ($useParentMount) { + // find out if something is mounted directly on the path + $internalPath = $mount->getInternalPath($absolutePath); + if ($internalPath === '') { + // resolve the parent mount instead + $mount = Filesystem::getMountManager()->find(dirname($absolutePath)); + } + } + + return $mount; + } + /** * Lock the given path * * @param string $path the path of the file to lock, relative to the view * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage + * * @return bool False if the path is excluded from locking, true otherwise * @throws \OCP\Lock\LockedException if the path is already locked */ - private function lockPath($path, $type) { + private function lockPath($path, $type, $lockMountPoint = false) { $absolutePath = $this->getAbsolutePath($path); $absolutePath = Filesystem::normalizePath($absolutePath); if (!$this->shouldLockFile($absolutePath)) { return false; } - $mount = $this->getMount($path); + $mount = $this->getMountForLock($absolutePath, $lockMountPoint); if ($mount) { try { $mount->getStorage()->acquireLock( @@ -1739,10 +1765,12 @@ class View { * * @param string $path the path of the file to lock, relative to the view * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage + * * @return bool False if the path is excluded from locking, true otherwise * @throws \OCP\Lock\LockedException if the path is already locked */ - public function changeLock($path, $type) { + public function changeLock($path, $type, $lockMountPoint = false) { $path = Filesystem::normalizePath($path); $absolutePath = $this->getAbsolutePath($path); $absolutePath = Filesystem::normalizePath($absolutePath); @@ -1750,7 +1778,7 @@ class View { return false; } - $mount = $this->getMount($path); + $mount = $this->getMountForLock($absolutePath, $lockMountPoint); if ($mount) { try { $mount->getStorage()->changeLock( @@ -1775,16 +1803,18 @@ class View { * * @param string $path the path of the file to unlock, relative to the view * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage + * * @return bool False if the path is excluded from locking, true otherwise */ - private function unlockPath($path, $type) { + private function unlockPath($path, $type, $lockMountPoint = false) { $absolutePath = $this->getAbsolutePath($path); $absolutePath = Filesystem::normalizePath($absolutePath); if (!$this->shouldLockFile($absolutePath)) { return false; } - $mount = $this->getMount($path); + $mount = $this->getMountForLock($absolutePath, $lockMountPoint); if ($mount) { $mount->getStorage()->releaseLock( $mount->getInternalPath($absolutePath), @@ -1801,16 +1831,18 @@ class View { * * @param string $path the path of the file to lock relative to the view * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage + * * @return bool False if the path is excluded from locking, true otherwise */ - public function lockFile($path, $type) { + public function lockFile($path, $type, $lockMountPoint = false) { $absolutePath = $this->getAbsolutePath($path); $absolutePath = Filesystem::normalizePath($absolutePath); if (!$this->shouldLockFile($absolutePath)) { return false; } - $this->lockPath($path, $type); + $this->lockPath($path, $type, $lockMountPoint); $parents = $this->getParents($path); foreach ($parents as $parent) { @@ -1825,16 +1857,18 @@ class View { * * @param string $path the path of the file to lock relative to the view * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage + * * @return bool False if the path is excluded from locking, true otherwise */ - public function unlockFile($path, $type) { + public function unlockFile($path, $type, $lockMountPoint = false) { $absolutePath = $this->getAbsolutePath($path); $absolutePath = Filesystem::normalizePath($absolutePath); if (!$this->shouldLockFile($absolutePath)) { return false; } - $this->unlockPath($path, $type); + $this->unlockPath($path, $type, $lockMountPoint); $parents = $this->getParents($path); foreach ($parents as $parent) { diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php index 9862026495..42768a0b27 100644 --- a/tests/lib/files/view.php +++ b/tests/lib/files/view.php @@ -1162,6 +1162,97 @@ class View extends \Test\TestCase { $this->assertFalse($view->lockFile($pathPrefix . '/foo/bar', ILockingProvider::LOCK_EXCLUSIVE)); } + /** + * Test that locks are on mount point paths instead of mount root + */ + public function testLockLocalMountPointPathInsteadOfStorageRoot() { + $lockingProvider = \OC::$server->getLockingProvider(); + $view = new \OC\Files\View('/testuser/files/'); + $storage = new Temporary([]); + \OC\Files\Filesystem::mount($storage, [], '/'); + $mountedStorage = new Temporary([]); + \OC\Files\Filesystem::mount($mountedStorage, [], '/testuser/files/mountpoint'); + + $this->assertTrue( + $view->lockFile('/mountpoint', ILockingProvider::LOCK_EXCLUSIVE, true), + 'Can lock mount point' + ); + + // no exception here because storage root was not locked + $mountedStorage->acquireLock('', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + + $thrown = false; + try { + $storage->acquireLock('/testuser/files/mountpoint', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + } catch (\OCP\Lock\LockedException $e) { + $thrown = true; + } + $this->assertTrue($thrown, 'Mount point path was locked on root storage'); + + $lockingProvider->releaseAll(); + } + + /** + * Test that locks are on mount point paths and also mount root when requested + */ + public function testLockStorageRootButNotLocalMountPoint() { + $lockingProvider = \OC::$server->getLockingProvider(); + $view = new \OC\Files\View('/testuser/files/'); + $storage = new Temporary([]); + \OC\Files\Filesystem::mount($storage, [], '/'); + $mountedStorage = new Temporary([]); + \OC\Files\Filesystem::mount($mountedStorage, [], '/testuser/files/mountpoint'); + + $this->assertTrue( + $view->lockFile('/mountpoint', ILockingProvider::LOCK_EXCLUSIVE, false), + 'Can lock mount point' + ); + + $thrown = false; + try { + $mountedStorage->acquireLock('', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + } catch (\OCP\Lock\LockedException $e) { + $thrown = true; + } + $this->assertTrue($thrown, 'Mount point storage root was locked on original storage'); + + // local mount point was not locked + $storage->acquireLock('/testuser/files/mountpoint', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + + $lockingProvider->releaseAll(); + } + + /** + * Test that locks are on mount point paths and also mount root when requested + */ + public function testLockMountPointPathFailReleasesBoth() { + $lockingProvider = \OC::$server->getLockingProvider(); + $view = new \OC\Files\View('/testuser/files/'); + $storage = new Temporary([]); + \OC\Files\Filesystem::mount($storage, [], '/'); + $mountedStorage = new Temporary([]); + \OC\Files\Filesystem::mount($mountedStorage, [], '/testuser/files/mountpoint.txt'); + + // this would happen if someone is writing on the mount point + $mountedStorage->acquireLock('', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + + $thrown = false; + try { + // this actually acquires two locks, one on the mount point and one no the storage root, + // but the one on the storage root will fail + $view->lockFile('/mountpoint.txt', ILockingProvider::LOCK_SHARED); + } catch (\OCP\Lock\LockedException $e) { + $thrown = true; + } + $this->assertTrue($thrown, 'Cannot acquire shared lock because storage root is already locked'); + + // from here we expect that the lock on the local mount point was released properly + // so acquiring an exclusive lock will succeed + $storage->acquireLock('/testuser/files/mountpoint.txt', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + + $lockingProvider->releaseAll(); + } + public function dataLockPaths() { return [ ['/testuser/{folder}', ''], From 8859004a2bb1a65d71553b55562c79d9a20cfb3e Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 24 Jun 2015 15:42:00 +0200 Subject: [PATCH 2/2] Rollback folder scan if an entry was locked --- lib/private/files/cache/scanner.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/private/files/cache/scanner.php b/lib/private/files/cache/scanner.php index 12aa05277a..50609e1e20 100644 --- a/lib/private/files/cache/scanner.php +++ b/lib/private/files/cache/scanner.php @@ -357,6 +357,11 @@ class Scanner extends BasicEmitter { // log and ignore \OC_Log::write('core', 'Exception while scanning file "' . $child . '": ' . $ex->getMessage(), \OC_Log::DEBUG); $exceptionOccurred = true; + } catch (\OCP\Lock\LockedException $e) { + if ($this->useTransactions) { + \OC_DB::rollback(); + } + throw $e; } } $removedChildren = \array_diff(array_keys($existingChildren), $newChildren);