From 17a64360e53798cdc6c6479aa40643c8aeb4ff3e Mon Sep 17 00:00:00 2001
From: Bjoern Schiessle <schiessle@owncloud.com>
Date: Tue, 22 Sep 2015 11:18:42 +0200
Subject: [PATCH] catch excexptions during the copy operation and make sure
 that we free the lock correctly

---
 lib/private/files/view.php | 97 +++++++++++++++++++++-----------------
 tests/lib/files/view.php   | 43 +++++++++++++++++
 2 files changed, 98 insertions(+), 42 deletions(-)

diff --git a/lib/private/files/view.php b/lib/private/files/view.php
index 9afa9d40b2..c04ca2ef46 100644
--- a/lib/private/files/view.php
+++ b/lib/private/files/view.php
@@ -759,59 +759,72 @@ class View {
 
 			$this->lockFile($path2, ILockingProvider::LOCK_SHARED);
 			$this->lockFile($path1, ILockingProvider::LOCK_SHARED);
+			$lockTypePath1 = ILockingProvider::LOCK_SHARED;
+			$lockTypePath2 = ILockingProvider::LOCK_SHARED;
 
-			$exists = $this->file_exists($path2);
-			if ($this->shouldEmitHooks()) {
-				\OC_Hook::emit(
-					Filesystem::CLASSNAME,
-					Filesystem::signal_copy,
-					array(
-						Filesystem::signal_param_oldpath => $this->getHookPath($path1),
-						Filesystem::signal_param_newpath => $this->getHookPath($path2),
-						Filesystem::signal_param_run => &$run
-					)
-				);
-				$this->emit_file_hooks_pre($exists, $path2, $run);
-			}
-			if ($run) {
-				$mount1 = $this->getMount($path1);
-				$mount2 = $this->getMount($path2);
-				$storage1 = $mount1->getStorage();
-				$internalPath1 = $mount1->getInternalPath($absolutePath1);
-				$storage2 = $mount2->getStorage();
-				$internalPath2 = $mount2->getInternalPath($absolutePath2);
+			try {
 
-				$this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE);
-
-				if ($mount1->getMountPoint() == $mount2->getMountPoint()) {
-					if ($storage1) {
-						$result = $storage1->copy($internalPath1, $internalPath2);
-					} else {
-						$result = false;
-					}
-				} else {
-					$result = $storage2->copyFromStorage($storage1, $internalPath1, $internalPath2);
-				}
-
-				$this->updater->update($path2);
-
-				$this->changeLock($path2, ILockingProvider::LOCK_SHARED);
-
-				if ($this->shouldEmitHooks() && $result !== false) {
+				$exists = $this->file_exists($path2);
+				if ($this->shouldEmitHooks()) {
 					\OC_Hook::emit(
 						Filesystem::CLASSNAME,
-						Filesystem::signal_post_copy,
+						Filesystem::signal_copy,
 						array(
 							Filesystem::signal_param_oldpath => $this->getHookPath($path1),
-							Filesystem::signal_param_newpath => $this->getHookPath($path2)
+							Filesystem::signal_param_newpath => $this->getHookPath($path2),
+							Filesystem::signal_param_run => &$run
 						)
 					);
-					$this->emit_file_hooks_post($exists, $path2);
+					$this->emit_file_hooks_pre($exists, $path2, $run);
 				}
+				if ($run) {
+					$mount1 = $this->getMount($path1);
+					$mount2 = $this->getMount($path2);
+					$storage1 = $mount1->getStorage();
+					$internalPath1 = $mount1->getInternalPath($absolutePath1);
+					$storage2 = $mount2->getStorage();
+					$internalPath2 = $mount2->getInternalPath($absolutePath2);
 
-				$this->unlockFile($path2, ILockingProvider::LOCK_SHARED);
-				$this->unlockFile($path1, ILockingProvider::LOCK_SHARED);
+					$this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE);
+					$lockTypePath2 = ILockingProvider::LOCK_EXCLUSIVE;
+
+					if ($mount1->getMountPoint() == $mount2->getMountPoint()) {
+						if ($storage1) {
+							$result = $storage1->copy($internalPath1, $internalPath2);
+						} else {
+							$result = false;
+						}
+					} else {
+						$result = $storage2->copyFromStorage($storage1, $internalPath1, $internalPath2);
+					}
+
+					$this->updater->update($path2);
+
+					$this->changeLock($path2, ILockingProvider::LOCK_SHARED);
+					$lockTypePath2 = ILockingProvider::LOCK_SHARED;
+
+					if ($this->shouldEmitHooks() && $result !== false) {
+						\OC_Hook::emit(
+							Filesystem::CLASSNAME,
+							Filesystem::signal_post_copy,
+							array(
+								Filesystem::signal_param_oldpath => $this->getHookPath($path1),
+								Filesystem::signal_param_newpath => $this->getHookPath($path2)
+							)
+						);
+						$this->emit_file_hooks_post($exists, $path2);
+					}
+
+				}
+			} catch (\Exception $e) {
+				$this->unlockFile($path2, $lockTypePath2);
+				$this->unlockFile($path1, $lockTypePath1);
+				throw $e;
 			}
+
+			$this->unlockFile($path2, $lockTypePath2);
+			$this->unlockFile($path1, $lockTypePath1);
+
 		}
 		return $result;
 	}
diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php
index bb42f385fc..618f29fc6f 100644
--- a/tests/lib/files/view.php
+++ b/tests/lib/files/view.php
@@ -1817,6 +1817,49 @@ class View extends \Test\TestCase {
 		$this->assertNull($this->getFileLockType($view, $targetPath), 'Target file not locked after operation');
 	}
 
+	/**
+	 * simulate a failed copy operation.
+	 * We expect that we catch the exception, free the lock and re-throw it.
+	 *
+	 * @expectedException \Exception
+	 */
+	public function testLockFileCopyException() {
+		$view = new \OC\Files\View('/' . $this->user . '/files/');
+
+		$storage = $this->getMockBuilder('\OC\Files\Storage\Temporary')
+			->setMethods(['copy'])
+			->getMock();
+
+		$sourcePath = 'original.txt';
+		$targetPath = 'target.txt';
+
+		\OC\Files\Filesystem::mount($storage, array(), $this->user . '/');
+		$storage->mkdir('files');
+		$view->file_put_contents($sourcePath, 'meh');
+
+		$storage->expects($this->once())
+			->method('copy')
+			->will($this->returnCallback(
+				function() {
+					throw new \Exception();
+				}
+			));
+
+		$this->connectMockHooks('copy', $view, $sourcePath, $lockTypeSourcePre, $lockTypeSourcePost);
+		$this->connectMockHooks('copy', $view, $targetPath, $lockTypeTargetPre, $lockTypeTargetPost);
+
+		$this->assertNull($this->getFileLockType($view, $sourcePath), 'Source file not locked before operation');
+		$this->assertNull($this->getFileLockType($view, $targetPath), 'Target file not locked before operation');
+
+		try {
+			$view->copy($sourcePath, $targetPath);
+		} catch (\Exception $e) {
+			$this->assertNull($this->getFileLockType($view, $sourcePath), 'Source file not locked after operation');
+			$this->assertNull($this->getFileLockType($view, $targetPath), 'Target file not locked after operation');
+			throw $e;
+		}
+	}
+
 	/**
 	 * Test rename operation: unlock first path when second path was locked
 	 */