Merge pull request #17070 from owncloud/lock-movemountbug

Lock correct paths when moving mount

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

lib/private/files/view.php

@@ -628,8 +628,8 @@ class View {
 				return false;
 			}
 
-			$this->lockFile($path1, ILockingProvider::LOCK_SHARED);
+			$this->lockFile($path1, ILockingProvider::LOCK_SHARED, true);
-			$this->lockFile($path2, ILockingProvider::LOCK_SHARED);
+			$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($path1, ILockingProvider::LOCK_EXCLUSIVE, true);
-				$this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE);
+				$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($path1, ILockingProvider::LOCK_EXCLUSIVE, true);
-				$this->unlockFile($path2, ILockingProvider::LOCK_EXCLUSIVE);
+				$this->unlockFile($path2, ILockingProvider::LOCK_EXCLUSIVE, true);
-
-				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);
-				}
 
 				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($path1, ILockingProvider::LOCK_SHARED, true);
-				$this->unlockFile($path2, ILockingProvider::LOCK_SHARED);
+				$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) {

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}', ''],