[Pkg-owncloud-commits] [owncloud] 398/457: Webdav PUT small file lock must be shared during hooks
David Prévot
taffit at moszumanska.debian.org
Sun Jun 28 20:06:55 UTC 2015
This is an automated email from the git hooks/post-receive script.
taffit pushed a commit to branch stable8
in repository owncloud.
commit 4497aa4c68051ff75d1587dc1b01676926dfb466
Author: Vincent Petry <pvince81 at owncloud.com>
Date: Fri Jun 12 18:50:49 2015 +0200
Webdav PUT small file lock must be shared during hooks
Fixed code path for Webdav PUT of small files to use shared locks during
hook execution, and exclusive during the file operation
This makes it possible for versions to be copied by accessing the file
in a post_write hook.
---
lib/private/connector/sabre/file.php | 16 +++++++-
lib/private/files/view.php | 4 +-
tests/lib/connector/sabre/file.php | 80 ++++++++++++++++++++++++++++++++++++
tests/lib/testcase.php | 35 ++++++++++++++++
4 files changed, 132 insertions(+), 3 deletions(-)
diff --git a/lib/private/connector/sabre/file.php b/lib/private/connector/sabre/file.php
index 3e1b29a..5aef30c 100644
--- a/lib/private/connector/sabre/file.php
+++ b/lib/private/connector/sabre/file.php
@@ -114,7 +114,7 @@ class File extends Node implements IFile {
}
try {
- $this->fileView->lockFile($this->path, ILockingProvider::LOCK_EXCLUSIVE);
+ $this->fileView->lockFile($this->path, ILockingProvider::LOCK_SHARED);
} catch (LockedException $e) {
throw new FileLocked($e->getMessage(), $e->getCode(), $e);
}
@@ -192,6 +192,12 @@ class File extends Node implements IFile {
));
}
+ try {
+ $this->fileView->changeLock($this->path, ILockingProvider::LOCK_EXCLUSIVE);
+ } catch (LockedException $e) {
+ throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+ }
+
if ($needsPartFile) {
// rename to correct path
try {
@@ -213,6 +219,12 @@ class File extends Node implements IFile {
// since we skipped the view we need to scan and emit the hooks ourselves
$partStorage->getScanner()->scanFile($internalPath);
+ try {
+ $this->fileView->changeLock($this->path, ILockingProvider::LOCK_SHARED);
+ } catch (LockedException $e) {
+ throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+ }
+
if ($view) {
$this->fileView->getUpdater()->propagate($hookPath);
if (!$exists) {
@@ -241,7 +253,7 @@ class File extends Node implements IFile {
throw new ServiceUnavailable("Failed to check file size: " . $e->getMessage());
}
- $this->fileView->unlockFile($this->path, ILockingProvider::LOCK_EXCLUSIVE);
+ $this->fileView->unlockFile($this->path, ILockingProvider::LOCK_SHARED);
return '"' . $this->info->getEtag() . '"';
}
diff --git a/lib/private/files/view.php b/lib/private/files/view.php
index a206eab..99db05f 100644
--- a/lib/private/files/view.php
+++ b/lib/private/files/view.php
@@ -1683,12 +1683,14 @@ class View {
}
/**
+ * Change the lock 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
* @return bool False if the path is excluded from locking, true otherwise
* @throws \OCP\Lock\LockedException if the path is already locked
*/
- private function changeLock($path, $type) {
+ public function changeLock($path, $type) {
$absolutePath = $this->getAbsolutePath($path);
if (!$this->shouldLockFile($absolutePath)) {
return false;
diff --git a/tests/lib/connector/sabre/file.php b/tests/lib/connector/sabre/file.php
index 6602a2d..834698d 100644
--- a/tests/lib/connector/sabre/file.php
+++ b/tests/lib/connector/sabre/file.php
@@ -404,4 +404,84 @@ class File extends \Test\TestCase {
$params[Filesystem::signal_param_path]
);
}
+
+ /**
+ * Test whether locks are set before and after the operation
+ */
+ public function testPutLocking() {
+ $view = new \OC\Files\View('/' . $this->user . '/files/');
+
+ $path = 'test-locking.txt';
+ $info = new \OC\Files\FileInfo(
+ '/' . $this->user . '/files/' . $path,
+ null,
+ null,
+ ['permissions' => \OCP\Constants::PERMISSION_ALL],
+ null
+ );
+
+ $file = new \OC\Connector\Sabre\File($view, $info);
+
+ $this->assertFalse(
+ $this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_SHARED),
+ 'File unlocked before put'
+ );
+ $this->assertFalse(
+ $this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE),
+ 'File unlocked before put'
+ );
+
+ $wasLockedPre = false;
+ $wasLockedPost = false;
+ $eventHandler = $this->getMockBuilder('\stdclass')
+ ->setMethods(['writeCallback', 'postWriteCallback'])
+ ->getMock();
+
+ // both pre and post hooks might need access to the file,
+ // so only shared lock is acceptable
+ $eventHandler->expects($this->once())
+ ->method('writeCallback')
+ ->will($this->returnCallback(
+ function() use ($view, $path, &$wasLockedPre){
+ $wasLockedPre = $this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_SHARED);
+ $wasLockedPre = $wasLockedPre && !$this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE);
+ }
+ ));
+ $eventHandler->expects($this->once())
+ ->method('postWriteCallback')
+ ->will($this->returnCallback(
+ function() use ($view, $path, &$wasLockedPost){
+ $wasLockedPost = $this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_SHARED);
+ $wasLockedPost = $wasLockedPost && !$this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE);
+ }
+ ));
+
+ \OCP\Util::connectHook(
+ Filesystem::CLASSNAME,
+ Filesystem::signal_write,
+ $eventHandler,
+ 'writeCallback'
+ );
+ \OCP\Util::connectHook(
+ Filesystem::CLASSNAME,
+ Filesystem::signal_post_write,
+ $eventHandler,
+ 'postWriteCallback'
+ );
+
+ $this->assertNotEmpty($file->put($this->getStream('test data')));
+
+ $this->assertTrue($wasLockedPre, 'File was locked during pre-hooks');
+ $this->assertTrue($wasLockedPost, 'File was locked during post-hooks');
+
+ $this->assertFalse(
+ $this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_SHARED),
+ 'File unlocked after put'
+ );
+ $this->assertFalse(
+ $this->isFileLocked($view, $path, \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE),
+ 'File unlocked after put'
+ );
+ }
+
}
diff --git a/tests/lib/testcase.php b/tests/lib/testcase.php
index 8551eda..d385a90 100644
--- a/tests/lib/testcase.php
+++ b/tests/lib/testcase.php
@@ -242,4 +242,39 @@ abstract class TestCase extends \PHPUnit_Framework_TestCase {
\OC_Util::setupFS($user);
}
}
+
+ /**
+ * Check if the given path is locked with a given type
+ *
+ * @param \OC\Files\View $view view
+ * @param string $path path to check
+ * @param int $type lock type
+ *
+ * @return boolean true if the file is locked with the
+ * given type, false otherwise
+ */
+ protected function isFileLocked($view, $path, $type) {
+ // Note: this seems convoluted but is necessary because
+ // the format of the lock key depends on the storage implementation
+ // (in our case mostly md5)
+
+ if ($type === \OCP\Lock\ILockingProvider::LOCK_SHARED) {
+ // to check if the file has a shared lock, try acquiring an exclusive lock
+ $checkType = \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE;
+ } else {
+ // a shared lock cannot be set if exclusive lock is in place
+ $checkType = \OCP\Lock\ILockingProvider::LOCK_SHARED;
+ }
+ try {
+ $view->lockFile($path, $checkType);
+ // no exception, which means the lock of $type is not set
+ // clean up
+ $view->unlockFile($path, $checkType);
+ return false;
+ } catch (\OCP\Lock\LockedException $e) {
+ // we could not acquire the counter-lock, which means
+ // the lock of $type was in place
+ return true;
+ }
+ }
}
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-owncloud/owncloud.git
More information about the Pkg-owncloud-commits
mailing list