From cdf7f91259d5e0e261832d0edffadf3760575223 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 4 May 2015 14:02:27 +0200 Subject: [PATCH 01/35] expose locking provider in the server container --- lib/private/server.php | 18 ++++++++++++++++++ lib/public/iservercontainer.php | 8 ++++++++ 2 files changed, 26 insertions(+) diff --git a/lib/private/server.php b/lib/private/server.php index aeea4a6485e9..7c8e59c8aa14 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -43,6 +43,7 @@ use OC\Diagnostics\NullQueryLogger; use OC\Diagnostics\EventLogger; use OC\Diagnostics\QueryLogger; +use OC\Lock\MemcacheLockingProvider; use OC\Mail\Mailer; use OC\Memcache\ArrayCache; use OC\Http\Client\ClientService; @@ -420,6 +421,13 @@ public function __construct($webRoot) { $this->getLogger() ); }); + $this->registerService('LockingProvider', function (Server $c) { + /** @var \OC\Memcache\Factory $memcacheFactory */ + $memcacheFactory = $c->getMemCacheFactory(); + return new MemcacheLockingProvider( + $memcacheFactory->createDistributed('lock') + ); + }); } /** @@ -908,4 +916,14 @@ public function getCommandBus(){ public function getTrustedDomainHelper() { return $this->query('TrustedDomainHelper'); } + + /** + * Get the locking provider + * + * @return \OCP\Lock\ILockingProvider + * @since 8.1.0 + */ + public function getLockingProvider() { + return $this->query('LockingProvider'); + } } diff --git a/lib/public/iservercontainer.php b/lib/public/iservercontainer.php index 5dd545aed373..8f0bede6cc81 100644 --- a/lib/public/iservercontainer.php +++ b/lib/public/iservercontainer.php @@ -413,4 +413,12 @@ public function getCommandBus(); * @since 8.1.0 */ public function getMailer(); + + /** + * Get the locking provider + * + * @return \OCP\Lock\ILockingProvider + * @since 8.1.0 + */ + public function getLockingProvider(); } From 536e187e5125aefec75037648181afc42df2a9d0 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 4 May 2015 14:21:34 +0200 Subject: [PATCH 02/35] add locking to the storage api --- apps/files_sharing/lib/sharedstorage.php | 26 +++++++++++++++++++ lib/private/files/storage/common.php | 20 ++++++++++++++ lib/private/files/storage/storage.php | 15 +++++++++++ lib/private/files/storage/wrapper/jail.php | 20 ++++++++++++++ lib/private/files/storage/wrapper/wrapper.php | 20 ++++++++++++++ lib/public/files/storage.php | 16 ++++++++++++ 6 files changed, 117 insertions(+) diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index ee86787c1815..ad6958363967 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -31,6 +31,9 @@ use OC\Files\Filesystem; use OCA\Files_Sharing\ISharedStorage; +use OCA\Files_Sharing\Propagator; +use OCA\Files_Sharing\SharedMount; +use OCP\Lock\ILockingProvider; /** * Convert target path to source path and pass the function call to the correct storage provider @@ -608,4 +611,27 @@ public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceIntern list($targetStorage, $targetInternalPath) = $this->resolvePath($targetInternalPath); return $targetStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + * @throws \OCP\Lock\LockedException + */ + public function acquireLock($path, $type, ILockingProvider $provider) { + /** @var \OCP\Files\Storage $targetStorage */ + list($targetStorage, $targetInternalPath) = $this->resolvePath($path); + $targetStorage->acquireLock($targetInternalPath, $type, $provider); + } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function releaseLock($path, $type, ILockingProvider $provider) { + /** @var \OCP\Files\Storage $targetStorage */ + list($targetStorage, $targetInternalPath) = $this->resolvePath($path); + $targetStorage->releaseLock($targetInternalPath, $type, $provider); + } } diff --git a/lib/private/files/storage/common.php b/lib/private/files/storage/common.php index 1257a14dd043..045011725ed9 100644 --- a/lib/private/files/storage/common.php +++ b/lib/private/files/storage/common.php @@ -43,6 +43,7 @@ use OCP\Files\InvalidCharacterInPathException; use OCP\Files\InvalidPathException; use OCP\Files\ReservedWordException; +use OCP\Lock\ILockingProvider; /** * Storage backend class for providing common filesystem operation methods @@ -621,4 +622,23 @@ public function getMetaData($path) { return $data; } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + * @throws \OCP\Lock\LockedException + */ + public function acquireLock($path, $type, ILockingProvider $provider) { + $provider->acquireLock($this->getId() . '::' . $path, $type); + } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function releaseLock($path, $type, ILockingProvider $provider) { + $provider->releaseLock($this->getId() . '::' . $path, $type); + } } diff --git a/lib/private/files/storage/storage.php b/lib/private/files/storage/storage.php index 07b5633c9088..8b34908e610e 100644 --- a/lib/private/files/storage/storage.php +++ b/lib/private/files/storage/storage.php @@ -21,6 +21,7 @@ */ namespace OC\Files\Storage; +use OCP\Lock\ILockingProvider; /** * Provide a common interface to all different storage options @@ -76,4 +77,18 @@ public function getStorageCache(); */ public function getMetaData($path); + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + * @throws \OCP\Lock\LockedException + */ + public function acquireLock($path, $type, ILockingProvider $provider); + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function releaseLock($path, $type, ILockingProvider $provider); } diff --git a/lib/private/files/storage/wrapper/jail.php b/lib/private/files/storage/wrapper/jail.php index b86b4e6405d8..229d12e82fa5 100644 --- a/lib/private/files/storage/wrapper/jail.php +++ b/lib/private/files/storage/wrapper/jail.php @@ -23,6 +23,7 @@ namespace OC\Files\Storage\Wrapper; use OC\Files\Cache\Wrapper\CacheJail; +use OCP\Lock\ILockingProvider; /** * Jail to a subdirectory of the wrapped storage @@ -424,4 +425,23 @@ public function getWatcher($path = '', $storage = null) { public function getETag($path) { return $this->storage->getETag($this->getSourcePath($path)); } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + * @throws \OCP\Lock\LockedException + */ + public function acquireLock($path, $type, ILockingProvider $provider) { + $this->storage->acquireLock($this->getSourcePath($path), $type, $provider); + } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function releaseLock($path, $type, ILockingProvider $provider) { + $this->storage->releaseLock($this->getSourcePath($path), $type, $provider); + } } diff --git a/lib/private/files/storage/wrapper/wrapper.php b/lib/private/files/storage/wrapper/wrapper.php index 14024addec4e..6aca771c0bf9 100644 --- a/lib/private/files/storage/wrapper/wrapper.php +++ b/lib/private/files/storage/wrapper/wrapper.php @@ -25,6 +25,7 @@ namespace OC\Files\Storage\Wrapper; use OCP\Files\InvalidPathException; +use OCP\Lock\ILockingProvider; class Wrapper implements \OC\Files\Storage\Storage { /** @@ -541,4 +542,23 @@ public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceIntern public function getMetaData($path) { return $this->storage->getMetaData($path); } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + * @throws \OCP\Lock\LockedException + */ + public function acquireLock($path, $type, ILockingProvider $provider) { + $this->storage->acquireLock($path, $type, $provider); + } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function releaseLock($path, $type, ILockingProvider $provider) { + $this->storage->releaseLock($path, $type, $provider); + } } diff --git a/lib/public/files/storage.php b/lib/public/files/storage.php index b89fb49a4be9..ea1da575959e 100644 --- a/lib/public/files/storage.php +++ b/lib/public/files/storage.php @@ -33,6 +33,7 @@ // This means that they should be used by apps instead of the internal ownCloud classes namespace OCP\Files; use OCP\Files\InvalidPathException; +use OCP\Lock\ILockingProvider; /** * Provide a common interface to all different storage options @@ -413,4 +414,19 @@ public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceIntern * @since 8.1.0 */ public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath); + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + * @throws \OCP\Lock\LockedException + */ + public function acquireLock($path, $type, ILockingProvider $provider); + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function releaseLock($path, $type, ILockingProvider $provider); } From bf7002bc655967dd8dd1970db45e5affce47c3c3 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 4 May 2015 15:05:09 +0200 Subject: [PATCH 03/35] add locking to the view apo --- lib/private/files/view.php | 73 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 63af2b616cff..c82f8e1fafeb 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -47,6 +47,7 @@ use OCP\Files\InvalidCharacterInPathException; use OCP\Files\InvalidPathException; use OCP\Files\ReservedWordException; +use OCP\Lock\ILockingProvider; /** * Class to provide access to ownCloud filesystem via a "view", and methods for @@ -71,6 +72,11 @@ class View { /** @var \OC\Files\Cache\Updater */ protected $updater; + /** + * @var \OCP\Lock\ILockingProvider + */ + private $lockingProvider; + /** * @param string $root * @throws \Exception If $root contains an invalid path @@ -79,12 +85,13 @@ public function __construct($root = '') { if (is_null($root)) { throw new \InvalidArgumentException('Root can\'t be null'); } - if(!Filesystem::isValidPath($root)) { + if (!Filesystem::isValidPath($root)) { throw new \Exception(); } $this->fakeRoot = $root; $this->updater = new Updater($this); + $this->lockingProvider = \OC::$server->getLockingProvider(); } public function getAbsolutePath($path = '/') { @@ -137,7 +144,7 @@ public function getRelativePath($path) { return $path; } - if (rtrim($path,'/') === rtrim($this->fakeRoot, '/')) { + if (rtrim($path, '/') === rtrim($this->fakeRoot, '/')) { return '/'; } @@ -1553,4 +1560,66 @@ public function verifyPath($path, $fileName) { throw new InvalidPathException($l10n->t('File name is too long')); } } + + /** + * get all parent folders of $path + * + * @param string $path + * @return string[] + */ + private function getParents($path) { + $parts = explode('/', $path); + + // remove the singe file + array_pop($parts); + $result = array('/'); + $resultPath = ''; + foreach ($parts as $part) { + if ($part) { + $resultPath .= '/' . $part; + $result[] = $resultPath; + } + } + return $result; + } + + private function lockPath($path, $type) { + $mount = $this->getMount($path); + $mount->getStorage()->acquireLock($mount->getInternalPath($path), $type, $this->lockingProvider); + } + + private function unlockPath($path, $type) { + $mount = $this->getMount($path); + $mount->getStorage()->releaseLock($mount->getInternalPath($path), $type, $this->lockingProvider); + } + + /** + * Lock a path and all it's parents + * + * @param string $path + * @param int $type + */ + public function lockFile($path, $type) { + $this->lockPath($path, $type); + + $parents = $this->getParents($path); + foreach ($parents as $parent) { + $this->lockPath($parent, ILockingProvider::LOCK_SHARED); + } + } + + /** + * Unlock a path and all it's parents + * + * @param string $path + * @param int $type + */ + public function unlockFile($path, $type) { + $this->unlockPath($path, $type); + + $parents = $this->getParents($path); + foreach ($parents as $parent) { + $this->unlockPath($parent, ILockingProvider::LOCK_SHARED); + } + } } From e64360e72db8514b305c628a38cd30809fb2913d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 4 May 2015 15:08:32 +0200 Subject: [PATCH 04/35] always use arraycache for unit tests --- lib/private/server.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/server.php b/lib/private/server.php index 7c8e59c8aa14..900d754012c9 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -224,7 +224,7 @@ public function __construct($webRoot) { $this->registerService('MemCacheFactory', function (Server $c) { $config = $c->getConfig(); - if($config->getSystemValue('installed', false)) { + if($config->getSystemValue('installed', false) && !(defined('PHPUNIT_RUN') && PHPUNIT_RUN)) { $v = \OC_App::getAppVersions(); $v['core'] = implode('.', \OC_Util::getVersion()); $version = implode(',', $v); From 7e418c7d699718b6d934de6184f6c96fdf9437d6 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 5 May 2015 13:48:49 +0200 Subject: [PATCH 05/35] high level locking wip --- lib/private/files/view.php | 64 +++++++++++++++++++++++++++++++++++--- tests/lib/files/view.php | 27 ++++++++++++++++ 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index c82f8e1fafeb..eb6fd0bb7aa6 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -41,6 +41,7 @@ namespace OC\Files; +use Icewind\Streams\CallbackWrapper; use OC\Files\Cache\Updater; use OC\Files\Mount\MoveableMount; use OCP\Files\FileNameTooLongException; @@ -636,6 +637,9 @@ public function rename($path1, $path2) { $internalPath1 = $mount1->getInternalPath($absolutePath1); $internalPath2 = $mount2->getInternalPath($absolutePath2); + $this->lockFile($path1, ILockingProvider::LOCK_EXCLUSIVE); + $this->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); + if ($internalPath1 === '' and $mount1 instanceof MoveableMount) { if ($this->isTargetAllowed($absolutePath2)) { /** @@ -656,6 +660,10 @@ public function rename($path1, $path2) { } else { $result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2); } + + $this->unlockFile($path1, ILockingProvider::LOCK_EXCLUSIVE); + $this->unlockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); + if ((Cache\Scanner::isPartialFile($path1) && !Cache\Scanner::isPartialFile($path2)) && $result !== false) { // if it was a rename from a part file to a regular file it was a write and not a rename operation $this->updater->update($path2); @@ -734,6 +742,9 @@ public function copy($path1, $path2, $preserveMtime = false) { $internalPath1 = $mount1->getInternalPath($absolutePath1); $storage2 = $mount2->getStorage(); $internalPath2 = $mount2->getInternalPath($absolutePath2); + + $this->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); + if ($mount1->getMountPoint() == $mount2->getMountPoint()) { if ($storage1) { $result = $storage1->copy($internalPath1, $internalPath2); @@ -744,6 +755,9 @@ public function copy($path1, $path2, $preserveMtime = false) { $result = $storage2->copyFromStorage($storage1, $internalPath1, $internalPath2); } $this->updater->update($path2); + + $this->unlockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); + if ($this->shouldEmitHooks() && $result !== false) { \OC_Hook::emit( Filesystem::CLASSNAME, @@ -939,10 +953,24 @@ private function basicOperation($operation, $path, $hooks = array(), $extraParam $run = $this->runHooks($hooks, $path); list($storage, $internalPath) = Filesystem::resolvePath($absolutePath . $postFix); if ($run and $storage) { - if (!is_null($extraParam)) { - $result = $storage->$operation($internalPath, $extraParam); + if (in_array('write', $hooks) || in_array('delete', $hooks)) { + $this->lockFile($path, ILockingProvider::LOCK_EXCLUSIVE); } else { - $result = $storage->$operation($internalPath); + $this->lockFile($path, ILockingProvider::LOCK_SHARED); + } + try { + if (!is_null($extraParam)) { + $result = $storage->$operation($internalPath, $extraParam); + } else { + $result = $storage->$operation($internalPath); + } + } catch (\Exception $e) { + if (in_array('write', $hooks) || in_array('delete', $hooks)) { + $this->lockFile($path, ILockingProvider::LOCK_EXCLUSIVE); + } else { + $this->lockFile($path, ILockingProvider::LOCK_SHARED); + } + throw $e; } if (in_array('delete', $hooks) and $result) { @@ -955,6 +983,23 @@ private function basicOperation($operation, $path, $hooks = array(), $extraParam $this->updater->update($path, $extraParam); } + if ($operation === 'fopen') { + $result = CallbackWrapper::wrap($result, null, null, function () use ($hooks, $path) { + if (in_array('write', $hooks)) { + $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); + } else { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + } + }); + } else { + if (in_array('write', $hooks) || in_array('delete', $hooks)) { + $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); + } else { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + } + } + + if ($this->shouldEmitHooks($path) && $result !== false) { if ($operation != 'fopen') { //no post hooks for fopen, the file stream is still open $this->runHooks($hooks, $path, true); @@ -1568,6 +1613,9 @@ public function verifyPath($path, $fileName) { * @return string[] */ private function getParents($path) { + if (!$path) { + return []; + } $parts = explode('/', $path); // remove the singe file @@ -1585,12 +1633,16 @@ private function getParents($path) { private function lockPath($path, $type) { $mount = $this->getMount($path); - $mount->getStorage()->acquireLock($mount->getInternalPath($path), $type, $this->lockingProvider); + if ($mount) { + $mount->getStorage()->acquireLock($mount->getInternalPath($this->getAbsolutePath($path)), $type, $this->lockingProvider); + } } private function unlockPath($path, $type) { $mount = $this->getMount($path); - $mount->getStorage()->releaseLock($mount->getInternalPath($path), $type, $this->lockingProvider); + if ($mount) { + $mount->getStorage()->releaseLock($mount->getInternalPath($this->getAbsolutePath($path)), $type, $this->lockingProvider); + } } /** @@ -1600,6 +1652,7 @@ private function unlockPath($path, $type) { * @param int $type */ public function lockFile($path, $type) { + $path = rtrim($path, '/'); $this->lockPath($path, $type); $parents = $this->getParents($path); @@ -1615,6 +1668,7 @@ public function lockFile($path, $type) { * @param int $type */ public function unlockFile($path, $type) { + $path = rtrim($path, '/'); $this->unlockPath($path, $type); $parents = $this->getParents($path); diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php index 6bc635571384..993107424738 100644 --- a/tests/lib/files/view.php +++ b/tests/lib/files/view.php @@ -11,6 +11,7 @@ use OC\Files\Storage\Common; use OC\Files\Mount\MountPoint; use OC\Files\Storage\Temporary; +use OCP\Lock\ILockingProvider; class TemporaryNoTouch extends \OC\Files\Storage\Temporary { public function touch($path, $mtime = null) { @@ -1080,4 +1081,30 @@ public function testGetRelativePathOnNull() { public function testNullAsRoot() { new \OC\Files\View(null); } + + /** + * e.g. reading from a folder that's being renamed + * + * @expectedException \OCP\Lock\LockedException + */ + public function testReadFromWriteLockedPath() { + $view = new \OC\Files\View(); + $storage = new Temporary(array()); + Filesystem::mount($storage, [], '/'); + $view->lockFile('/foo/bar', ILockingProvider::LOCK_EXCLUSIVE); + $view->lockFile('/foo/bar/asd', ILockingProvider::LOCK_SHARED); + } + + /** + * e.g. writing a file that's being downloaded + * + * @expectedException \OCP\Lock\LockedException + */ + public function testWriteToReadLockedFile() { + $view = new \OC\Files\View(); + $storage = new Temporary(array()); + Filesystem::mount($storage, [], '/'); + $view->lockFile('/foo/bar', ILockingProvider::LOCK_SHARED); + $view->lockFile('/foo/bar', ILockingProvider::LOCK_EXCLUSIVE); + } } From a1a25a9b5bddab05b6e8250557e8b7b6e17da1ff Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 11 May 2015 13:36:25 +0200 Subject: [PATCH 06/35] fix unlocking when moving mount points --- lib/private/files/view.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index eb6fd0bb7aa6..9540a2e2a9c6 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -664,6 +664,11 @@ public function rename($path1, $path2) { $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); + } + if ((Cache\Scanner::isPartialFile($path1) && !Cache\Scanner::isPartialFile($path2)) && $result !== false) { // if it was a rename from a part file to a regular file it was a write and not a rename operation $this->updater->update($path2); From d519aba878cca25acbe0aecc39efa9cb76bda4bb Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 11 May 2015 17:22:39 +0200 Subject: [PATCH 07/35] fix test --- tests/lib/files/view.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php index 993107424738..06a42d634312 100644 --- a/tests/lib/files/view.php +++ b/tests/lib/files/view.php @@ -1090,7 +1090,7 @@ public function testNullAsRoot() { public function testReadFromWriteLockedPath() { $view = new \OC\Files\View(); $storage = new Temporary(array()); - Filesystem::mount($storage, [], '/'); + \OC\Files\Filesystem::mount($storage, [], '/'); $view->lockFile('/foo/bar', ILockingProvider::LOCK_EXCLUSIVE); $view->lockFile('/foo/bar/asd', ILockingProvider::LOCK_SHARED); } @@ -1103,7 +1103,7 @@ public function testReadFromWriteLockedPath() { public function testWriteToReadLockedFile() { $view = new \OC\Files\View(); $storage = new Temporary(array()); - Filesystem::mount($storage, [], '/'); + \OC\Files\Filesystem::mount($storage, [], '/'); $view->lockFile('/foo/bar', ILockingProvider::LOCK_SHARED); $view->lockFile('/foo/bar', ILockingProvider::LOCK_EXCLUSIVE); } From 5edf294ce597c59c204c37e9fb753807ab40082f Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 13 May 2015 11:03:38 +0200 Subject: [PATCH 08/35] Add CAS methods to Null memcache This prevents breaking ownCloud completely when memcache is not enabled and the locking code is triggered --- lib/private/memcache/null.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/private/memcache/null.php b/lib/private/memcache/null.php index 3b8ce0b42da8..c9b07f48ce29 100644 --- a/lib/private/memcache/null.php +++ b/lib/private/memcache/null.php @@ -39,6 +39,18 @@ public function remove($key) { return true; } + public function inc($key) { + return true; + } + + public function dec($key) { + return true; + } + + public function cas($key, $old, $new) { + return true; + } + public function clear($prefix = '') { return true; } From 0775e9c1ca95a3f93b1069610fd89a0801f9f897 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 13 May 2015 11:36:40 +0200 Subject: [PATCH 09/35] Use md5 for lock key --- lib/private/files/storage/common.php | 4 ++-- lib/private/files/view.php | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/lib/private/files/storage/common.php b/lib/private/files/storage/common.php index 045011725ed9..1c54f7cbc142 100644 --- a/lib/private/files/storage/common.php +++ b/lib/private/files/storage/common.php @@ -630,7 +630,7 @@ public function getMetaData($path) { * @throws \OCP\Lock\LockedException */ public function acquireLock($path, $type, ILockingProvider $provider) { - $provider->acquireLock($this->getId() . '::' . $path, $type); + $provider->acquireLock(md5($this->getId() . '::' . $path), $type); } /** @@ -639,6 +639,6 @@ public function acquireLock($path, $type, ILockingProvider $provider) { * @param \OCP\Lock\ILockingProvider $provider */ public function releaseLock($path, $type, ILockingProvider $provider) { - $provider->releaseLock($this->getId() . '::' . $path, $type); + $provider->releaseLock(md5($this->getId() . '::' . $path), $type); } } diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 9540a2e2a9c6..166989ed77aa 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -1639,14 +1639,26 @@ private function getParents($path) { private function lockPath($path, $type) { $mount = $this->getMount($path); if ($mount) { - $mount->getStorage()->acquireLock($mount->getInternalPath($this->getAbsolutePath($path)), $type, $this->lockingProvider); + $mount->getStorage()->acquireLock( + $mount->getInternalPath( + $this->getAbsolutePath($path) + ), + $type, + $this->lockingProvider + ); } } private function unlockPath($path, $type) { $mount = $this->getMount($path); if ($mount) { - $mount->getStorage()->releaseLock($mount->getInternalPath($this->getAbsolutePath($path)), $type, $this->lockingProvider); + $mount->getStorage()->releaseLock( + $mount->getInternalPath( + $this->getAbsolutePath($path) + ), + $type, + $this->lockingProvider + ); } } From 8d53dc803f978f0b2e5db1c3503dc5fb0d55b0c5 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 13 May 2015 12:12:39 +0200 Subject: [PATCH 10/35] Use md5 + prefix for file locking keys in memcache Also trim slashes from paths to make sure the locks are based on the same paths. --- lib/private/files/storage/common.php | 4 ++-- lib/private/files/view.php | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/private/files/storage/common.php b/lib/private/files/storage/common.php index 1c54f7cbc142..f0c9e1bfa050 100644 --- a/lib/private/files/storage/common.php +++ b/lib/private/files/storage/common.php @@ -630,7 +630,7 @@ public function getMetaData($path) { * @throws \OCP\Lock\LockedException */ public function acquireLock($path, $type, ILockingProvider $provider) { - $provider->acquireLock(md5($this->getId() . '::' . $path), $type); + $provider->acquireLock('files/' . md5($this->getId() . '::' . trim($path, '/')), $type); } /** @@ -639,6 +639,6 @@ public function acquireLock($path, $type, ILockingProvider $provider) { * @param \OCP\Lock\ILockingProvider $provider */ public function releaseLock($path, $type, ILockingProvider $provider) { - $provider->releaseLock(md5($this->getId() . '::' . $path), $type); + $provider->releaseLock('files/' . md5($this->getId() . '::' . trim($path, '/')), $type); } } diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 166989ed77aa..b499bbb36415 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -1621,9 +1621,11 @@ private function getParents($path) { if (!$path) { return []; } + + $path = trim($path, '/'); $parts = explode('/', $path); - // remove the singe file + // remove the single file array_pop($parts); $result = array('/'); $resultPath = ''; From 35c377f7a9a0413bea5fa6768467c6e5f7c2d83e Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 13 May 2015 13:37:18 +0200 Subject: [PATCH 11/35] phpdoc and minor issues --- lib/private/files/storage/storage.php | 4 ++-- lib/private/files/view.php | 26 +++++++++++++++++--------- lib/public/files/storage.php | 4 ++-- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/lib/private/files/storage/storage.php b/lib/private/files/storage/storage.php index 8b34908e610e..642544bad37d 100644 --- a/lib/private/files/storage/storage.php +++ b/lib/private/files/storage/storage.php @@ -78,7 +78,7 @@ public function getStorageCache(); public function getMetaData($path); /** - * @param string $path + * @param string $path The path of the file to acquire the lock for * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE * @param \OCP\Lock\ILockingProvider $provider * @throws \OCP\Lock\LockedException @@ -86,7 +86,7 @@ public function getMetaData($path); public function acquireLock($path, $type, ILockingProvider $provider); /** - * @param string $path + * @param string $path The path of the file to release the lock for * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE * @param \OCP\Lock\ILockingProvider $provider */ diff --git a/lib/private/files/view.php b/lib/private/files/view.php index b499bbb36415..b8bbd60f0289 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -971,9 +971,9 @@ private function basicOperation($operation, $path, $hooks = array(), $extraParam } } catch (\Exception $e) { if (in_array('write', $hooks) || in_array('delete', $hooks)) { - $this->lockFile($path, ILockingProvider::LOCK_EXCLUSIVE); + $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); } else { - $this->lockFile($path, ILockingProvider::LOCK_SHARED); + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } throw $e; } @@ -1638,6 +1638,10 @@ private function getParents($path) { return $result; } + /** + * @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 + */ private function lockPath($path, $type) { $mount = $this->getMount($path); if ($mount) { @@ -1651,6 +1655,10 @@ private function lockPath($path, $type) { } } + /** + * @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 + */ private function unlockPath($path, $type) { $mount = $this->getMount($path); if ($mount) { @@ -1665,13 +1673,13 @@ private function unlockPath($path, $type) { } /** - * Lock a path and all it's parents + * Lock a path and all its parents up to the root of the view * - * @param string $path - * @param int $type + * @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 */ public function lockFile($path, $type) { - $path = rtrim($path, '/'); + $path = '/' . trim($path, '/'); $this->lockPath($path, $type); $parents = $this->getParents($path); @@ -1681,10 +1689,10 @@ public function lockFile($path, $type) { } /** - * Unlock a path and all it's parents + * Unlock a path and all its parents up to the root of the view * - * @param string $path - * @param int $type + * @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 */ public function unlockFile($path, $type) { $path = rtrim($path, '/'); diff --git a/lib/public/files/storage.php b/lib/public/files/storage.php index ea1da575959e..68d00fab34cc 100644 --- a/lib/public/files/storage.php +++ b/lib/public/files/storage.php @@ -416,7 +416,7 @@ public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceIntern public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath); /** - * @param string $path + * @param string $path The path of the file to acquire the lock for * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE * @param \OCP\Lock\ILockingProvider $provider * @throws \OCP\Lock\LockedException @@ -424,7 +424,7 @@ public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceIntern public function acquireLock($path, $type, ILockingProvider $provider); /** - * @param string $path + * @param string $path The path of the file to acquire the lock for * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE * @param \OCP\Lock\ILockingProvider $provider */ From 1270c6800dd439e411278764e3a2ddb2f68040ac Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 13 May 2015 13:54:15 +0200 Subject: [PATCH 12/35] dont lock on meta data operations --- lib/private/files/view.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index b8bbd60f0289..b1333b4150cd 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -960,7 +960,7 @@ private function basicOperation($operation, $path, $hooks = array(), $extraParam if ($run and $storage) { if (in_array('write', $hooks) || in_array('delete', $hooks)) { $this->lockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } else { + } else if (in_array('read', $hooks)) { $this->lockFile($path, ILockingProvider::LOCK_SHARED); } try { @@ -972,7 +972,7 @@ private function basicOperation($operation, $path, $hooks = array(), $extraParam } catch (\Exception $e) { if (in_array('write', $hooks) || in_array('delete', $hooks)) { $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } else { + } else if (in_array('read', $hooks)) { $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } throw $e; @@ -992,14 +992,14 @@ private function basicOperation($operation, $path, $hooks = array(), $extraParam $result = CallbackWrapper::wrap($result, null, null, function () use ($hooks, $path) { if (in_array('write', $hooks)) { $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } else { + } else if (in_array('read', $hooks)) { $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } }); } else { if (in_array('write', $hooks) || in_array('delete', $hooks)) { $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } else { + } else if (in_array('read', $hooks)) { $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } } From 2d63fd77de1cbfdb0a3474cadf99f1dcf4962d5f Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 13 May 2015 14:07:18 +0200 Subject: [PATCH 13/35] dont apply callback wrapper when fopen failed --- lib/private/files/view.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index b1333b4150cd..6bf864c00aba 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -988,7 +988,7 @@ private function basicOperation($operation, $path, $hooks = array(), $extraParam $this->updater->update($path, $extraParam); } - if ($operation === 'fopen') { + if ($operation === 'fopen' and $result) { $result = CallbackWrapper::wrap($result, null, null, function () use ($hooks, $path) { if (in_array('write', $hooks)) { $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); From e08423f95631a1b06f2de0216c5765552092075c Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 19 May 2015 17:12:09 +0200 Subject: [PATCH 14/35] release all locks on shutdown --- lib/base.php | 2 ++ lib/private/lock/memcachelockingprovider.php | 27 ++++++++++++++++++++ lib/public/lock/ilockingprovider.php | 5 ++++ 3 files changed, 34 insertions(+) diff --git a/lib/base.php b/lib/base.php index b7f19c96406d..09159dc22aae 100644 --- a/lib/base.php +++ b/lib/base.php @@ -666,6 +666,8 @@ public static function init() { //make sure temporary files are cleaned up $tmpManager = \OC::$server->getTempManager(); register_shutdown_function(array($tmpManager, 'clean')); + $lockProvider = \OC::$server->getLockingProvider(); + register_shutdown_function(array($lockProvider, 'releaseAll')); if ($systemConfig->getValue('installed', false) && !self::checkUpgrade(false)) { if (\OC::$server->getConfig()->getAppValue('core', 'backgroundjobs_mode', 'ajax') == 'ajax') { diff --git a/lib/private/lock/memcachelockingprovider.php b/lib/private/lock/memcachelockingprovider.php index 9c8c72354624..b2938e3a8ce9 100644 --- a/lib/private/lock/memcachelockingprovider.php +++ b/lib/private/lock/memcachelockingprovider.php @@ -31,6 +31,11 @@ class MemcacheLockingProvider implements ILockingProvider { */ private $memcache; + private $acquiredLocks = [ + 'shared' => [], + 'exclusive' => [] + ]; + /** * @param \OCP\IMemcache $memcache */ @@ -64,11 +69,16 @@ public function acquireLock($path, $type) { if (!$this->memcache->inc($path)) { throw new LockedException($path); } + if (!isset($this->acquiredLocks['shared'][$path])) { + $this->acquiredLocks['shared'][$path] = 0; + } + $this->acquiredLocks['shared'][$path]++; } else { $this->memcache->add($path, 0); if (!$this->memcache->cas($path, 0, 'exclusive')) { throw new LockedException($path); } + $this->acquiredLocks['exclusive'][$path] = true; } } @@ -79,8 +89,25 @@ public function acquireLock($path, $type) { public function releaseLock($path, $type) { if ($type === self::LOCK_SHARED) { $this->memcache->dec($path); + $this->acquiredLocks['shared'][$path]--; } else if ($type === self::LOCK_EXCLUSIVE) { $this->memcache->cas($path, 'exclusive', 0); + unset($this->acquiredLocks['exclusive'][$path]); + } + } + + /** + * release all lock acquired by this instance + */ + public function releaseAll() { + foreach ($this->acquiredLocks['shared'] as $path => $count) { + for ($i = 0; $i < $count; $i++) { + $this->releaseLock($path, self::LOCK_SHARED); + } + } + + foreach ($this->acquiredLocks['exclusive'] as $path => $hasLock) { + $this->releaseLock($path, self::LOCK_EXCLUSIVE); } } } diff --git a/lib/public/lock/ilockingprovider.php b/lib/public/lock/ilockingprovider.php index 0b17580faac4..18fa47fa6311 100644 --- a/lib/public/lock/ilockingprovider.php +++ b/lib/public/lock/ilockingprovider.php @@ -44,4 +44,9 @@ public function acquireLock($path, $type); * @param int $type self::LOCK_SHARED or self::LOCK_EXCLUSIVE */ public function releaseLock($path, $type); + + /** + * release all lock acquired by this instance + */ + public function releaseAll(); } From b98dd3ceb84f1372415fd69bc7992d0bba954f3f Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 20 May 2015 13:08:56 +0200 Subject: [PATCH 15/35] release all locks after test --- tests/lib/testcase.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/lib/testcase.php b/tests/lib/testcase.php index c1ebfb025cf6..d8595c83a91d 100644 --- a/tests/lib/testcase.php +++ b/tests/lib/testcase.php @@ -43,6 +43,7 @@ protected function setUp() { protected function tearDown() { $hookExceptions = \OC_Hook::$thrownExceptions; \OC_Hook::$thrownExceptions = []; + \OC::$server->getLockingProvider()->releaseAll(); if(!empty($hookExceptions)) { throw $hookExceptions[0]; } From f0b86727299728bb7243e99584a21038c54123cd Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 20 May 2015 15:22:53 +0200 Subject: [PATCH 16/35] fix locking root of a view --- lib/private/files/view.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 6bf864c00aba..db904e57cb93 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -1618,11 +1618,11 @@ public function verifyPath($path, $fileName) { * @return string[] */ private function getParents($path) { + $path = trim($path, '/'); if (!$path) { return []; } - $path = trim($path, '/'); $parts = explode('/', $path); // remove the single file From 006eaa84aa25b6301fc75cdf2fbf2594e21b4271 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 20 May 2015 16:25:41 +0200 Subject: [PATCH 17/35] dont release shared lock if we dont have any --- lib/private/lock/memcachelockingprovider.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/private/lock/memcachelockingprovider.php b/lib/private/lock/memcachelockingprovider.php index b2938e3a8ce9..1334d0073142 100644 --- a/lib/private/lock/memcachelockingprovider.php +++ b/lib/private/lock/memcachelockingprovider.php @@ -88,8 +88,10 @@ public function acquireLock($path, $type) { */ public function releaseLock($path, $type) { if ($type === self::LOCK_SHARED) { - $this->memcache->dec($path); - $this->acquiredLocks['shared'][$path]--; + if (isset($this->acquiredLocks['shared'][$path]) and $this->acquiredLocks['shared'][$path] > 0) { + $this->memcache->dec($path); + $this->acquiredLocks['shared'][$path]--; + } } else if ($type === self::LOCK_EXCLUSIVE) { $this->memcache->cas($path, 'exclusive', 0); unset($this->acquiredLocks['exclusive'][$path]); From 6df502a5aa29b137d17bb7c17cb2552719386203 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 21 May 2015 13:06:20 +0200 Subject: [PATCH 18/35] Fix Null memcache fallback to match interface --- lib/private/memcache/null.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/private/memcache/null.php b/lib/private/memcache/null.php index c9b07f48ce29..982f7edc80fe 100644 --- a/lib/private/memcache/null.php +++ b/lib/private/memcache/null.php @@ -22,7 +22,7 @@ namespace OC\Memcache; -class Null extends Cache { +class Null extends Cache implements \OCP\IMemcache { public function get($key) { return null; } @@ -39,11 +39,15 @@ public function remove($key) { return true; } - public function inc($key) { + public function add($key, $value, $ttl = 0) { return true; } - public function dec($key) { + public function inc($key, $step = 1) { + return true; + } + + public function dec($key, $step = 1) { return true; } From c72ea9f7d72d3ab22ff56195235808b17cecb0ba Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 21 May 2015 15:57:33 +0200 Subject: [PATCH 19/35] unit test for releaseall --- 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 08d879da8bba..337aa4cea7ea 100644 --- a/tests/lib/lock/lockingprovider.php +++ b/tests/lib/lock/lockingprovider.php @@ -107,6 +107,30 @@ public function testExclusiveLockAfterSharedReleased() { $this->assertTrue($this->instance->isLocked('foo', ILockingProvider::LOCK_EXCLUSIVE)); } + public function testReleaseAll() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('bar', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('asd', ILockingProvider::LOCK_EXCLUSIVE); + + $this->instance->releaseAll(); + + $this->assertFalse($this->instance->isLocked('foo', ILockingProvider::LOCK_SHARED)); + $this->assertFalse($this->instance->isLocked('bar', ILockingProvider::LOCK_SHARED)); + $this->assertFalse($this->instance->isLocked('asd', ILockingProvider::LOCK_EXCLUSIVE)); + } + + public function testReleaseAfterReleaseAll() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + + $this->instance->releaseAll(); + + $this->assertFalse($this->instance->isLocked('foo', ILockingProvider::LOCK_SHARED)); + + $this->instance->releaseLock('foo', ILockingProvider::LOCK_SHARED); + } + /** * @expectedException \OCP\Lock\LockedException From 2f4f468399d316157979f747d2418fb5cff8d3e0 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 21 May 2015 16:11:10 +0200 Subject: [PATCH 20/35] Added config switch for file locking --- config/config.sample.php | 13 +++++ lib/private/lock/nooplockingprovider.php | 60 ++++++++++++++++++++++++ lib/private/server.php | 15 ++++-- 3 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 lib/private/lock/nooplockingprovider.php diff --git a/config/config.sample.php b/config/config.sample.php index ed86dd941315..a9fafe7b4f12 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1026,6 +1026,19 @@ */ 'max_filesize_animated_gifs_public_sharing' => 10, + +/** + * Enables the EXPERIMENTAL file locking. + * This is disabled by default as it is experimental. + * + * Prevents concurrent processes to access the same files + * at the same time. Can help prevent side effects that would + * be caused by concurrent operations. + * + * WARNING: EXPERIMENTAL + */ +'filelocking.enabled' => false, + /** * This entry is just here to show a warning in case somebody copied the sample * configuration. DO NOT ADD THIS SWITCH TO YOUR CONFIGURATION! diff --git a/lib/private/lock/nooplockingprovider.php b/lib/private/lock/nooplockingprovider.php new file mode 100644 index 000000000000..0ca8edb4d006 --- /dev/null +++ b/lib/private/lock/nooplockingprovider.php @@ -0,0 +1,60 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Lock; + +use OCP\Lock\ILockingProvider; + +/** + * Locking provider that does nothing. + * + * To be used when locking is disabled. + */ +class NoopLockingProvider implements ILockingProvider { + + /** + * {@inheritdoc} + */ + public function isLocked($path, $type) { + return false; + } + + /** + * {@inheritdoc} + */ + public function acquireLock($path, $type) { + // do nothing + } + + /** + * {@inheritdoc} + */ + public function releaseLock($path, $type) { + // do nothing + } + + /** + * release all lock acquired by this instance + */ + public function releaseAll() { + // do nothing + } +} diff --git a/lib/private/server.php b/lib/private/server.php index 900d754012c9..88f57e73a394 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -44,6 +44,7 @@ use OC\Diagnostics\EventLogger; use OC\Diagnostics\QueryLogger; use OC\Lock\MemcacheLockingProvider; +use OC\Lock\NoopLockingProvider; use OC\Mail\Mailer; use OC\Memcache\ArrayCache; use OC\Http\Client\ClientService; @@ -422,11 +423,15 @@ public function __construct($webRoot) { ); }); $this->registerService('LockingProvider', function (Server $c) { - /** @var \OC\Memcache\Factory $memcacheFactory */ - $memcacheFactory = $c->getMemCacheFactory(); - return new MemcacheLockingProvider( - $memcacheFactory->createDistributed('lock') - ); + if ($c->getConfig()->getSystemValue('filelocking.enabled', false)) { + /** @var \OC\Memcache\Factory $memcacheFactory */ + $memcacheFactory = $c->getMemCacheFactory(); + $memcache = $memcacheFactory->createDistributed('lock'); + if (!($memcache instanceof \OC\Memcache\Null)) { + return new MemcacheLockingProvider($memcache); + } + } + return new NoopLockingProvider(); }); } From 668fafd4d2e56ee47ec67ba0a9768b7e5c3ccafd Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 21 May 2015 17:52:13 +0200 Subject: [PATCH 21/35] close file handle after sending sabre response --- lib/private/connector/sabre/filesplugin.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/private/connector/sabre/filesplugin.php b/lib/private/connector/sabre/filesplugin.php index 3c79f5a7a2ac..09d931be6060 100644 --- a/lib/private/connector/sabre/filesplugin.php +++ b/lib/private/connector/sabre/filesplugin.php @@ -89,6 +89,12 @@ public function initialize(\Sabre\DAV\Server $server) { $this->server->on('afterBind', array($this, 'sendFileIdHeader')); $this->server->on('afterWriteContent', array($this, 'sendFileIdHeader')); $this->server->on('afterMethod:GET', [$this,'httpGet']); + $this->server->on('afterResponse', function($request, ResponseInterface $response) { + $body = $response->getBody(); + if (is_resource($body)) { + fclose($body); + } + }); } /** From 437c0b55a625ce3c2c6a81740fda8cb7a0bf1998 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 22 May 2015 13:43:44 +0200 Subject: [PATCH 22/35] unlock source file when we cant lock the target in a rename --- lib/private/files/view.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index db904e57cb93..d6cc62dff276 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -49,6 +49,7 @@ use OCP\Files\InvalidPathException; use OCP\Files\ReservedWordException; use OCP\Lock\ILockingProvider; +use OCP\Lock\LockedException; /** * Class to provide access to ownCloud filesystem via a "view", and methods for @@ -638,7 +639,12 @@ public function rename($path1, $path2) { $internalPath2 = $mount2->getInternalPath($absolutePath2); $this->lockFile($path1, ILockingProvider::LOCK_EXCLUSIVE); - $this->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); + try { + $this->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); + } catch (LockedException $e) { + $this->unlockFile($path1, ILockingProvider::LOCK_EXCLUSIVE); + throw $e; + } if ($internalPath1 === '' and $mount1 instanceof MoveableMount) { if ($this->isTargetAllowed($absolutePath2)) { From 72847dbc7794f250fb59ddf5e679d226909ec065 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 22 May 2015 13:52:42 +0200 Subject: [PATCH 23/35] always use locking in unit tests --- lib/private/server.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/server.php b/lib/private/server.php index 88f57e73a394..551012d8355b 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -423,7 +423,7 @@ public function __construct($webRoot) { ); }); $this->registerService('LockingProvider', function (Server $c) { - if ($c->getConfig()->getSystemValue('filelocking.enabled', false)) { + if ($c->getConfig()->getSystemValue('filelocking.enabled', false) or (defined('PHPUNIT_RUN') && PHPUNIT_RUN)) { /** @var \OC\Memcache\Factory $memcacheFactory */ $memcacheFactory = $c->getMemCacheFactory(); $memcache = $memcacheFactory->createDistributed('lock'); From 6b965d71d170759a6c4c5d755a37f891f0166912 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 26 May 2015 14:41:37 +0200 Subject: [PATCH 24/35] add seperate config option for locking memcache backend --- config/config.sample.php | 6 ++++++ lib/private/memcache/factory.php | 23 ++++++++++++++++++++++- lib/private/server.php | 5 +++-- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index a9fafe7b4f12..23a27fa3ec0d 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1039,6 +1039,12 @@ */ 'filelocking.enabled' => false, +/** + * Memory caching backend for file locking + * Because most memcache backends can clean values without warning using redis is recommended + */ +'memcache.locking' => '\OC\Memcache\Redis', + /** * This entry is just here to show a warning in case somebody copied the sample * configuration. DO NOT ADD THIS SWITCH TO YOUR CONFIGURATION! diff --git a/lib/private/memcache/factory.php b/lib/private/memcache/factory.php index bbfd775c7749..6e9aaa11d85e 100644 --- a/lib/private/memcache/factory.php +++ b/lib/private/memcache/factory.php @@ -46,13 +46,19 @@ class Factory implements ICacheFactory { */ private $distributedCacheClass; + /** + * @var string $lockingCacheClass + */ + private $lockingCacheClass; + /** * @param string $globalPrefix * @param string|null $localCacheClass * @param string|null $distributedCacheClass + * @param string|null $lockingCacheClass */ public function __construct($globalPrefix, - $localCacheClass = null, $distributedCacheClass = null) + $localCacheClass = null, $distributedCacheClass = null, $lockingCacheClass = null) { $this->globalPrefix = $globalPrefix; @@ -62,8 +68,23 @@ public function __construct($globalPrefix, if (!($distributedCacheClass && $distributedCacheClass::isAvailable())) { $distributedCacheClass = $localCacheClass; } + if (!($lockingCacheClass && $lockingCacheClass::isAvailable())) { + // dont fallback since the fallback might not be suitable for storing lock + $lockingCacheClass = '\OC\Memcache\Null'; + } $this->localCacheClass = $localCacheClass; $this->distributedCacheClass = $distributedCacheClass; + $this->lockingCacheClass = $lockingCacheClass; + } + + /** + * create a cache instance for storing locks + * + * @param string $prefix + * @return \OCP\IMemcache + */ + public function createLocking($prefix = '') { + return new $this->lockingCacheClass($this->globalPrefix . '/' . $prefix); } /** diff --git a/lib/private/server.php b/lib/private/server.php index 551012d8355b..ab1506fa13ac 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -234,7 +234,8 @@ public function __construct($webRoot) { $prefix = md5($instanceId.'-'.$version.'-'.$path); return new \OC\Memcache\Factory($prefix, $config->getSystemValue('memcache.local', null), - $config->getSystemValue('memcache.distributed', null) + $config->getSystemValue('memcache.distributed', null), + $config->getSystemValue('memcache.locking', null) ); } @@ -426,7 +427,7 @@ public function __construct($webRoot) { if ($c->getConfig()->getSystemValue('filelocking.enabled', false) or (defined('PHPUNIT_RUN') && PHPUNIT_RUN)) { /** @var \OC\Memcache\Factory $memcacheFactory */ $memcacheFactory = $c->getMemCacheFactory(); - $memcache = $memcacheFactory->createDistributed('lock'); + $memcache = $memcacheFactory->createLocking('lock'); if (!($memcache instanceof \OC\Memcache\Null)) { return new MemcacheLockingProvider($memcache); } From 72776b165f3017057f7eb56534d346357d048db7 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 26 May 2015 15:12:49 +0200 Subject: [PATCH 25/35] use arraycache for locking in unit tests --- lib/private/server.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/server.php b/lib/private/server.php index ab1506fa13ac..aea5be5afa64 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -240,6 +240,7 @@ public function __construct($webRoot) { } return new \OC\Memcache\Factory('', + new ArrayCache(), new ArrayCache(), new ArrayCache() ); From 8665a98744a8d3545859b809fc479ea216e45176 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 27 May 2015 15:19:46 +0200 Subject: [PATCH 26/35] add locking for non-chunking webdav upload --- lib/private/connector/sabre/file.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index 8e4460ef3b55..325206f060bf 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -45,6 +45,7 @@ use OCP\Files\LockNotAcquiredException; use OCP\Files\NotPermittedException; use OCP\Files\StorageNotAvailableException; +use OCP\Lock\ILockingProvider; use Sabre\DAV\Exception; use Sabre\DAV\Exception\BadRequest; use Sabre\DAV\Exception\Forbidden; @@ -110,6 +111,8 @@ public function put($data) { $partFilePath = $this->path; } + $this->fileView->lockFile($this->path, ILockingProvider::LOCK_EXCLUSIVE); + // the part file and target file might be on a different storage in case of a single file storage (e.g. single file share) /** @var \OC\Files\Storage\Storage $partStorage */ list($partStorage, $internalPartPath) = $this->fileView->resolvePath($partFilePath); @@ -232,6 +235,8 @@ public function put($data) { throw new ServiceUnavailable("Failed to check file size: " . $e->getMessage()); } + $this->fileView->unlockFile($this->path, ILockingProvider::LOCK_EXCLUSIVE); + return '"' . $this->info->getEtag() . '"'; } From ba174ac626ae54b75565870fdeb55398436599e4 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 29 May 2015 09:59:20 +0200 Subject: [PATCH 27/35] Convert LockedException to FileLocked in Sabre connector For Sabre to be able to return the proper error code instead of 500, the LockedException is now rethrown as FileLocked exception in the Sabre connector --- lib/private/connector/sabre/file.php | 14 +++++++++++++- lib/private/connector/sabre/objecttree.php | 8 ++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php index 325206f060bf..3e1b29a4f285 100644 --- a/lib/private/connector/sabre/file.php +++ b/lib/private/connector/sabre/file.php @@ -46,6 +46,7 @@ use OCP\Files\NotPermittedException; use OCP\Files\StorageNotAvailableException; use OCP\Lock\ILockingProvider; +use OCP\Lock\LockedException; use Sabre\DAV\Exception; use Sabre\DAV\Exception\BadRequest; use Sabre\DAV\Exception\Forbidden; @@ -80,6 +81,7 @@ class File extends Node implements IFile { * @throws Exception * @throws EntityTooLarge * @throws ServiceUnavailable + * @throws FileLocked * @return string|null */ public function put($data) { @@ -111,7 +113,11 @@ public function put($data) { $partFilePath = $this->path; } - $this->fileView->lockFile($this->path, ILockingProvider::LOCK_EXCLUSIVE); + try { + $this->fileView->lockFile($this->path, ILockingProvider::LOCK_EXCLUSIVE); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); + } // the part file and target file might be on a different storage in case of a single file storage (e.g. single file share) /** @var \OC\Files\Storage\Storage $partStorage */ @@ -257,6 +263,8 @@ public function get() { throw new ServiceUnavailable("Encryption not ready: " . $e->getMessage()); } catch (StorageNotAvailableException $e) { throw new ServiceUnavailable("Failed to open file: " . $e->getMessage()); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); } } @@ -278,6 +286,8 @@ public function delete() { } } catch (StorageNotAvailableException $e) { throw new ServiceUnavailable("Failed to unlink: " . $e->getMessage()); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); } } @@ -383,6 +393,8 @@ private function createFileChunked($data) { return $info->getEtag(); } catch (StorageNotAvailableException $e) { throw new ServiceUnavailable("Failed to put file: " . $e->getMessage()); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); } } diff --git a/lib/private/connector/sabre/objecttree.php b/lib/private/connector/sabre/objecttree.php index 17d9aff8f689..ed42a31336ab 100644 --- a/lib/private/connector/sabre/objecttree.php +++ b/lib/private/connector/sabre/objecttree.php @@ -26,10 +26,12 @@ namespace OC\Connector\Sabre; use OC\Connector\Sabre\Exception\InvalidPath; +use OC\Connector\Sabre\Exception\FileLocked; use OC\Files\FileInfo; use OC\Files\Mount\MoveableMount; use OCP\Files\StorageInvalidException; use OCP\Files\StorageNotAvailableException; +use OCP\Lock\LockedException; class ObjectTree extends \Sabre\DAV\Tree { @@ -221,8 +223,10 @@ public function move($sourcePath, $destinationPath) { if (!$renameOkay) { throw new \Sabre\DAV\Exception\Forbidden(''); } - } catch (\OCP\Files\StorageNotAvailableException $e) { + } catch (StorageNotAvailableException $e) { throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage()); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); } $this->markDirty($sourceDir); @@ -258,7 +262,7 @@ public function copy($source, $destination) { try { $this->fileView->copy($source, $destination); - } catch (\OCP\Files\StorageNotAvailableException $e) { + } catch (StorageNotAvailableException $e) { throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage()); } From 0451a6652d7dcac3887bdf760593482eaa19e681 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 29 May 2015 10:55:25 +0200 Subject: [PATCH 28/35] Move locking exceptions --- lib/private/connector/sabre/directory.php | 17 +++++++++++++---- lib/private/connector/sabre/objecttree.php | 2 ++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/private/connector/sabre/directory.php b/lib/private/connector/sabre/directory.php index ef35b300ea2e..82e1b55d763d 100644 --- a/lib/private/connector/sabre/directory.php +++ b/lib/private/connector/sabre/directory.php @@ -28,6 +28,8 @@ namespace OC\Connector\Sabre; use OC\Connector\Sabre\Exception\InvalidPath; +use OC\Connector\Sabre\Exception\FileLocked; +use OCP\Lock\LockedException; class Directory extends \OC\Connector\Sabre\Node implements \Sabre\DAV\ICollection, \Sabre\DAV\IQuota { @@ -102,6 +104,8 @@ public function createFile($name, $data = null) { return $node->put($data); } catch (\OCP\Files\StorageNotAvailableException $e) { throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage()); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); } } @@ -127,6 +131,8 @@ public function createDirectory($name) { throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage()); } catch (\OCP\Files\InvalidPathException $ex) { throw new InvalidPath($ex->getMessage()); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); } } @@ -211,11 +217,14 @@ public function delete() { throw new \Sabre\DAV\Exception\Forbidden(); } - if (!$this->fileView->rmdir($this->path)) { - // assume it wasn't possible to remove due to permission issue - throw new \Sabre\DAV\Exception\Forbidden(); + try { + if (!$this->fileView->rmdir($this->path)) { + // assume it wasn't possible to remove due to permission issue + throw new \Sabre\DAV\Exception\Forbidden(); + } + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); } - } /** diff --git a/lib/private/connector/sabre/objecttree.php b/lib/private/connector/sabre/objecttree.php index ed42a31336ab..c56fd7ee4db4 100644 --- a/lib/private/connector/sabre/objecttree.php +++ b/lib/private/connector/sabre/objecttree.php @@ -264,6 +264,8 @@ public function copy($source, $destination) { $this->fileView->copy($source, $destination); } catch (StorageNotAvailableException $e) { throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage()); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); } list($destinationDir,) = \Sabre\HTTP\URLUtil::splitPath($destination); From 270a10b7545ba8fdf8d8736ae6b967929875cfdd Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 29 May 2015 11:24:06 +0200 Subject: [PATCH 29/35] Return 423 instead of 503 for locked files --- lib/private/connector/sabre/exception/filelocked.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/connector/sabre/exception/filelocked.php b/lib/private/connector/sabre/exception/filelocked.php index 2405059bfb1a..1657a7ae3768 100644 --- a/lib/private/connector/sabre/exception/filelocked.php +++ b/lib/private/connector/sabre/exception/filelocked.php @@ -42,6 +42,6 @@ public function __construct($message = "", $code = 0, Exception $previous = null */ public function getHTTPCode() { - return 503; + return 423; } } From 43772e2a9a4b1400e7e3a8874130ac817aa64058 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Fri, 29 May 2015 11:54:05 +0200 Subject: [PATCH 30/35] Adding information on file locking status to admin section --- settings/admin.php | 8 ++++++++ settings/templates/admin.php | 37 ++++++++++++++++++++++++------------ 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/settings/admin.php b/settings/admin.php index 5720bd9f99cb..f2e01adab118 100644 --- a/settings/admin.php +++ b/settings/admin.php @@ -29,6 +29,8 @@ * */ +use OC\Lock\NoopLockingProvider; + OC_Util::checkAdminUser(); OC_App::setActiveNavigationEntry("admin"); @@ -175,6 +177,11 @@ $template->assign('filesExternal', $filesExternal); $template->assign('updaterAppPanel', $updaterAppPanel); $template->assign('ocDefaultEncryptionModulePanel', $ocDefaultEncryptionModulePanel); +if (\OC::$server->getLockingProvider() instanceof NoopLockingProvider) { + $template->assign('fileLockingEnabled', false); +} else { + $template->assign('fileLockingEnabled', true); +} $formsMap = array_map(function ($form) { if (preg_match('%(]*>.*?)%i', $form, $regs)) { @@ -200,6 +207,7 @@ $formsAndMore[] = ['anchor' => 'backgroundjobs', 'section-name' => $l->t('Cron')]; $formsAndMore[] = ['anchor' => 'mail_general_settings', 'section-name' => $l->t('Email server')]; $formsAndMore[] = ['anchor' => 'log-section', 'section-name' => $l->t('Log')]; +$formsAndMore[] = ['anchor' => 'server-status', 'section-name' => $l->t('Server Status')]; $formsAndMore[] = ['anchor' => 'admin-tips', 'section-name' => $l->t('Tips & tricks')]; if ($updaterAppPanel) { $formsAndMore[] = ['anchor' => 'updater', 'section-name' => $l->t('Updates')]; diff --git a/settings/templates/admin.php b/settings/templates/admin.php index f9a99b589afd..3d253d4cbbd5 100644 --- a/settings/templates/admin.php +++ b/settings/templates/admin.php @@ -7,6 +7,7 @@ /** * @var array $_ * @var \OCP\IL10N $l + * @var OC_Defaults $theme */ style('settings', 'settings'); @@ -15,32 +16,32 @@ vendor_script('select2/select2'); vendor_style('select2/select2'); -$levels = array('Debug', 'Info', 'Warning', 'Error', 'Fatal'); -$levelLabels = array( +$levels = ['Debug', 'Info', 'Warning', 'Error', 'Fatal']; +$levelLabels = [ $l->t( 'Everything (fatal issues, errors, warnings, info, debug)' ), $l->t( 'Info, warnings, errors and fatal issues' ), $l->t( 'Warnings, errors and fatal issues' ), $l->t( 'Errors and fatal issues' ), $l->t( 'Fatal issues only' ), -); +]; -$mail_smtpauthtype = array( +$mail_smtpauthtype = [ '' => $l->t('None'), 'LOGIN' => $l->t('Login'), 'PLAIN' => $l->t('Plain'), 'NTLM' => $l->t('NT LAN Manager'), -); +]; -$mail_smtpsecure = array( +$mail_smtpsecure = [ '' => $l->t('None'), 'ssl' => $l->t('SSL'), 'tls' => $l->t('TLS'), -); +]; -$mail_smtpmode = array( +$mail_smtpmode = [ 'php', 'smtp', -); +]; if ($_['sendmail_is_available']) { $mail_smtpmode[] = 'sendmail'; } @@ -137,7 +138,7 @@ ?>
t('We strongly suggest installing the required packages on your system to support one of the following locales: %s.', array($locales))); + p($l->t('We strongly suggest installing the required packages on your system to support one of the following locales: %s.', [$locales])); ?> - t("Last cron job execution: %s.", array($relative_time)));?> + t("Last cron job execution: %s.", [$relative_time]));?> - t("Last cron job execution: %s. Something seems wrong.", array($relative_time)));?> + t("Last cron job execution: %s. Something seems wrong.", [$relative_time]));?> @@ -527,6 +528,18 @@
  • t('Hardening and security guidance'));?> ↗
    • +
    +

    t('Server Status'));?>

    +
      +
    • + t('Experimental File Lock is enabled.')); + } else { + p($l->t('Experimental File Lock is disabled.')); + } ?> +
      • +
    +

    t('Version'));?>

    From a1372b2fb5acc51eacf70012a6702a96c78e61ff Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 29 May 2015 14:34:21 +0200 Subject: [PATCH 31/35] add method to atomically change between shared and exclusive lock --- lib/private/lock/memcachelockingprovider.php | 20 ++++++++ lib/public/lock/ilockingprovider.php | 9 ++++ tests/lib/lock/lockingprovider.php | 53 ++++++++++++++++++++ 3 files changed, 82 insertions(+) diff --git a/lib/private/lock/memcachelockingprovider.php b/lib/private/lock/memcachelockingprovider.php index 1334d0073142..368ff4503256 100644 --- a/lib/private/lock/memcachelockingprovider.php +++ b/lib/private/lock/memcachelockingprovider.php @@ -98,6 +98,26 @@ public function releaseLock($path, $type) { } } + /** + * Change the type of an existing lock + * + * @param string $path + * @param int $targetType self::LOCK_SHARED or self::LOCK_EXCLUSIVE + * @throws \OCP\Lock\LockedException + */ + public function changeLock($path, $targetType) { + if ($targetType === self::LOCK_SHARED) { + if (!$this->memcache->cas($path, 'exclusive', 1)) { + throw new LockedException($path); + } + } else if ($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')) { + throw new LockedException($path); + } + } + } + /** * release all lock acquired by this instance */ diff --git a/lib/public/lock/ilockingprovider.php b/lib/public/lock/ilockingprovider.php index 18fa47fa6311..6a963b9d7aa5 100644 --- a/lib/public/lock/ilockingprovider.php +++ b/lib/public/lock/ilockingprovider.php @@ -45,6 +45,15 @@ public function acquireLock($path, $type); */ public function releaseLock($path, $type); + /** + * Change the type of an existing lock + * + * @param string $path + * @param int $targetType self::LOCK_SHARED or self::LOCK_EXCLUSIVE + * @throws \OCP\Lock\LockedException + */ + public function changeLock($path, $targetType); + /** * release all lock acquired by this instance */ diff --git a/tests/lib/lock/lockingprovider.php b/tests/lib/lock/lockingprovider.php index 337aa4cea7ea..efd6e1939f26 100644 --- a/tests/lib/lock/lockingprovider.php +++ b/tests/lib/lock/lockingprovider.php @@ -158,4 +158,57 @@ public function testLockedExceptionHasPathForExclusive() { $this->assertEquals('foo', $e->getPath()); } } + + public function testChangeLockToExclusive() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->changeLock('foo', ILockingProvider::LOCK_EXCLUSIVE); + $this->assertFalse($this->instance->isLocked('foo', ILockingProvider::LOCK_SHARED)); + $this->assertTrue($this->instance->isLocked('foo', ILockingProvider::LOCK_EXCLUSIVE)); + } + + public function testChangeLockToShared() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_EXCLUSIVE); + $this->instance->changeLock('foo', ILockingProvider::LOCK_SHARED); + $this->assertFalse($this->instance->isLocked('foo', ILockingProvider::LOCK_EXCLUSIVE)); + $this->assertTrue($this->instance->isLocked('foo', ILockingProvider::LOCK_SHARED)); + } + + /** + * @expectedException \OCP\Lock\LockedException + */ + public function testChangeLockToExclusiveDoubleShared() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->changeLock('foo', ILockingProvider::LOCK_EXCLUSIVE); + } + + /** + * @expectedException \OCP\Lock\LockedException + */ + public function testChangeLockToExclusiveNoShared() { + $this->instance->changeLock('foo', ILockingProvider::LOCK_EXCLUSIVE); + } + + /** + * @expectedException \OCP\Lock\LockedException + */ + public function testChangeLockToExclusiveFromExclusive() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_EXCLUSIVE); + $this->instance->changeLock('foo', ILockingProvider::LOCK_EXCLUSIVE); + } + + /** + * @expectedException \OCP\Lock\LockedException + */ + public function testChangeLockToSharedNoExclusive() { + $this->instance->changeLock('foo', ILockingProvider::LOCK_SHARED); + } + + /** + * @expectedException \OCP\Lock\LockedException + */ + public function testChangeLockToSharedFromShared() { + $this->instance->acquireLock('foo', ILockingProvider::LOCK_SHARED); + $this->instance->changeLock('foo', ILockingProvider::LOCK_SHARED); + } } From 661c9e2444c097aad7e47b77df4fc2b10c346d04 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 29 May 2015 14:40:06 +0200 Subject: [PATCH 32/35] add changeLock to the storage api --- apps/files_sharing/lib/sharedstorage.php | 11 +++++++++++ lib/private/files/storage/common.php | 9 +++++++++ lib/private/files/storage/storage.php | 8 ++++++++ lib/private/files/storage/wrapper/jail.php | 9 +++++++++ lib/private/files/storage/wrapper/wrapper.php | 9 +++++++++ lib/public/files/storage.php | 8 ++++++++ 6 files changed, 54 insertions(+) diff --git a/apps/files_sharing/lib/sharedstorage.php b/apps/files_sharing/lib/sharedstorage.php index ad6958363967..bf61dda37187 100644 --- a/apps/files_sharing/lib/sharedstorage.php +++ b/apps/files_sharing/lib/sharedstorage.php @@ -634,4 +634,15 @@ public function releaseLock($path, $type, ILockingProvider $provider) { list($targetStorage, $targetInternalPath) = $this->resolvePath($path); $targetStorage->releaseLock($targetInternalPath, $type, $provider); } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function changeLock($path, $type, ILockingProvider $provider) { + /** @var \OCP\Files\Storage $targetStorage */ + list($targetStorage, $targetInternalPath) = $this->resolvePath($path); + $targetStorage->changeLock($targetInternalPath, $type, $provider); + } } diff --git a/lib/private/files/storage/common.php b/lib/private/files/storage/common.php index f0c9e1bfa050..847cb8492fef 100644 --- a/lib/private/files/storage/common.php +++ b/lib/private/files/storage/common.php @@ -641,4 +641,13 @@ public function acquireLock($path, $type, ILockingProvider $provider) { public function releaseLock($path, $type, ILockingProvider $provider) { $provider->releaseLock('files/' . md5($this->getId() . '::' . trim($path, '/')), $type); } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function changeLock($path, $type, ILockingProvider $provider) { + $provider->changeLock('files/' . md5($this->getId() . '::' . trim($path, '/')), $type); + } } diff --git a/lib/private/files/storage/storage.php b/lib/private/files/storage/storage.php index 642544bad37d..bd809099e1f9 100644 --- a/lib/private/files/storage/storage.php +++ b/lib/private/files/storage/storage.php @@ -91,4 +91,12 @@ public function acquireLock($path, $type, ILockingProvider $provider); * @param \OCP\Lock\ILockingProvider $provider */ public function releaseLock($path, $type, ILockingProvider $provider); + + /** + * @param string $path The path of the file to change the lock for + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + * @throws \OCP\Lock\LockedException + */ + public function changeLock($path, $type, ILockingProvider $provider); } diff --git a/lib/private/files/storage/wrapper/jail.php b/lib/private/files/storage/wrapper/jail.php index 229d12e82fa5..2857e74de464 100644 --- a/lib/private/files/storage/wrapper/jail.php +++ b/lib/private/files/storage/wrapper/jail.php @@ -444,4 +444,13 @@ public function acquireLock($path, $type, ILockingProvider $provider) { public function releaseLock($path, $type, ILockingProvider $provider) { $this->storage->releaseLock($this->getSourcePath($path), $type, $provider); } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function changeLock($path, $type, ILockingProvider $provider) { + $this->storage->changeLock($this->getSourcePath($path), $type, $provider); + } } diff --git a/lib/private/files/storage/wrapper/wrapper.php b/lib/private/files/storage/wrapper/wrapper.php index 6aca771c0bf9..d1414880beb0 100644 --- a/lib/private/files/storage/wrapper/wrapper.php +++ b/lib/private/files/storage/wrapper/wrapper.php @@ -561,4 +561,13 @@ public function acquireLock($path, $type, ILockingProvider $provider) { public function releaseLock($path, $type, ILockingProvider $provider) { $this->storage->releaseLock($path, $type, $provider); } + + /** + * @param string $path + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + */ + public function changeLock($path, $type, ILockingProvider $provider) { + $this->storage->changeLock($path, $type, $provider); + } } diff --git a/lib/public/files/storage.php b/lib/public/files/storage.php index 68d00fab34cc..ee160c50e89e 100644 --- a/lib/public/files/storage.php +++ b/lib/public/files/storage.php @@ -429,4 +429,12 @@ public function acquireLock($path, $type, ILockingProvider $provider); * @param \OCP\Lock\ILockingProvider $provider */ public function releaseLock($path, $type, ILockingProvider $provider); + + /** + * @param string $path The path of the file to change the lock for + * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE + * @param \OCP\Lock\ILockingProvider $provider + * @throws \OCP\Lock\LockedException + */ + public function changeLock($path, $type, ILockingProvider $provider); } From ce04cf6610ffd823ef1cb5ab3ee215b69afffcb4 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 29 May 2015 16:35:49 +0200 Subject: [PATCH 33/35] shared lock around hooks --- lib/private/files/view.php | 53 ++++++++++++++++---- lib/private/lock/memcachelockingprovider.php | 4 ++ 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index d6cc62dff276..b98842f5eb74 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -534,6 +534,7 @@ public function file_put_contents($path, $data) { and !Filesystem::isFileBlacklisted($path) ) { $path = $this->getRelativePath($absolutePath); + $exists = $this->file_exists($path); $run = true; if ($this->shouldEmitHooks($path)) { @@ -613,6 +614,10 @@ public function rename($path1, $path2) { if ($path1 == null or $path2 == null) { return false; } + + $this->lockFile($path1, ILockingProvider::LOCK_SHARED); + $this->lockFile($path2, ILockingProvider::LOCK_SHARED); + $run = true; if ($this->shouldEmitHooks() && (Cache\Scanner::isPartialFile($path1) && !Cache\Scanner::isPartialFile($path2))) { // if it was a rename from a part file to a regular file it was a write and not a rename operation @@ -638,13 +643,8 @@ public function rename($path1, $path2) { $internalPath1 = $mount1->getInternalPath($absolutePath1); $internalPath2 = $mount2->getInternalPath($absolutePath2); - $this->lockFile($path1, ILockingProvider::LOCK_EXCLUSIVE); - try { - $this->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); - } catch (LockedException $e) { - $this->unlockFile($path1, ILockingProvider::LOCK_EXCLUSIVE); - throw $e; - } + $this->changeLock($path1, ILockingProvider::LOCK_EXCLUSIVE); + $this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE); if ($internalPath1 === '' and $mount1 instanceof MoveableMount) { if ($this->isTargetAllowed($absolutePath2)) { @@ -702,6 +702,8 @@ public function rename($path1, $path2) { } return $result; } else { + $this->unlockFile($path1, ILockingProvider::LOCK_SHARED); + $this->unlockFile($path2, ILockingProvider::LOCK_SHARED); return false; } } else { @@ -733,6 +735,10 @@ public function copy($path1, $path2, $preserveMtime = false) { return false; } $run = true; + + $this->lockFile($path2, ILockingProvider::LOCK_SHARED); + $this->lockFile($path1, ILockingProvider::LOCK_SHARED); + $exists = $this->file_exists($path2); if ($this->shouldEmitHooks()) { \OC_Hook::emit( @@ -754,7 +760,7 @@ public function copy($path1, $path2, $preserveMtime = false) { $storage2 = $mount2->getStorage(); $internalPath2 = $mount2->getInternalPath($absolutePath2); - $this->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); + $this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE); if ($mount1->getMountPoint() == $mount2->getMountPoint()) { if ($storage1) { @@ -768,6 +774,7 @@ public function copy($path1, $path2, $preserveMtime = false) { $this->updater->update($path2); $this->unlockFile($path2, ILockingProvider::LOCK_EXCLUSIVE); + $this->unlockFile($path1, ILockingProvider::LOCK_SHARED); if ($this->shouldEmitHooks() && $result !== false) { \OC_Hook::emit( @@ -782,6 +789,8 @@ public function copy($path1, $path2, $preserveMtime = false) { } return $result; } else { + $this->unlockFile($path2, ILockingProvider::LOCK_SHARED); + $this->unlockFile($path1, ILockingProvider::LOCK_SHARED); return false; } } else { @@ -961,13 +970,16 @@ private function basicOperation($operation, $path, $hooks = array(), $extraParam return false; } + if(in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks)) { + // always a shared lock during pre-hooks so the hook can read the file + $this->lockFile($path, ILockingProvider::LOCK_SHARED); + } + $run = $this->runHooks($hooks, $path); list($storage, $internalPath) = Filesystem::resolvePath($absolutePath . $postFix); if ($run and $storage) { if (in_array('write', $hooks) || in_array('delete', $hooks)) { - $this->lockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } else if (in_array('read', $hooks)) { - $this->lockFile($path, ILockingProvider::LOCK_SHARED); + $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); } try { if (!is_null($extraParam)) { @@ -1017,6 +1029,8 @@ private function basicOperation($operation, $path, $hooks = array(), $extraParam } } return $result; + } else { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } } return null; @@ -1661,6 +1675,23 @@ private function lockPath($path, $type) { } } + /** + * @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 + */ + private function changeLock($path, $type) { + $mount = $this->getMount($path); + if ($mount) { + $mount->getStorage()->changeLock( + $mount->getInternalPath( + $this->getAbsolutePath($path) + ), + $type, + $this->lockingProvider + ); + } + } + /** * @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 diff --git a/lib/private/lock/memcachelockingprovider.php b/lib/private/lock/memcachelockingprovider.php index 368ff4503256..26957f8ceda2 100644 --- a/lib/private/lock/memcachelockingprovider.php +++ b/lib/private/lock/memcachelockingprovider.php @@ -110,11 +110,15 @@ public function changeLock($path, $targetType) { if (!$this->memcache->cas($path, 'exclusive', 1)) { throw new LockedException($path); } + unset($this->acquiredLocks['exclusive'][$path]); + $this->acquiredLocks['shared'][$path]++; } else if ($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')) { throw new LockedException($path); } + $this->acquiredLocks['exclusive'][$path] = true; + $this->acquiredLocks['shared'][$path]--; } } From 8902e2be733242bf02cb13ba6b374cdb45aa9e2f Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 1 Jun 2015 13:25:27 +0200 Subject: [PATCH 34/35] fix nooplockingprovider --- lib/private/lock/nooplockingprovider.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/private/lock/nooplockingprovider.php b/lib/private/lock/nooplockingprovider.php index 0ca8edb4d006..4f33b881555c 100644 --- a/lib/private/lock/nooplockingprovider.php +++ b/lib/private/lock/nooplockingprovider.php @@ -51,10 +51,17 @@ public function releaseLock($path, $type) { // do nothing } - /** - * release all lock acquired by this instance + /**1 + * {@inheritdoc} */ public function releaseAll() { // do nothing } + + /** + * {@inheritdoc} + */ + public function changeLock($path, $targetType) { + // do nothing + } } From 2104c2ffdd4fb2f63c4150bd2ffd0ac35d5ea859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Mon, 1 Jun 2015 14:10:00 +0200 Subject: [PATCH 35/35] Fixing undefined index 'foo' --- lib/private/lock/memcachelockingprovider.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/private/lock/memcachelockingprovider.php b/lib/private/lock/memcachelockingprovider.php index 26957f8ceda2..3f32ab1d8c5c 100644 --- a/lib/private/lock/memcachelockingprovider.php +++ b/lib/private/lock/memcachelockingprovider.php @@ -111,6 +111,9 @@ public function changeLock($path, $targetType) { throw new LockedException($path); } unset($this->acquiredLocks['exclusive'][$path]); + if (!isset($this->acquiredLocks['shared'][$path])) { + $this->acquiredLocks['shared'][$path] = 0; + } $this->acquiredLocks['shared'][$path]++; } else if ($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