Properly handle copy/move failures in cross storage copy/move

2 files changed, 24 insertions, 8 deletions

diff --git a/lib/private/files/storage/common.php b/lib/private/files/storage/common.php
index ca898bcc0b3..66ed713e22d 100644
--- a/lib/private/files/storage/common.php
+++ b/lib/private/files/storage/common.php
@@ -537,7 +537,7 @@ abstract class Common implements Storage {
 			$dh = $sourceStorage->opendir($sourceInternalPath);
 			$result = $this->mkdir($targetInternalPath);
 			if (is_resource($dh)) {
-				while (($file = readdir($dh)) !== false) {
+				while ($result and ($file = readdir($dh)) !== false) {
 					if (!Filesystem::isIgnoredDir($file)) {
 						$result &= $this->copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file);
 					}
@@ -547,13 +547,20 @@ abstract class Common implements Storage {
 			$source = $sourceStorage->fopen($sourceInternalPath, 'r');
 			$target = $this->fopen($targetInternalPath, 'w');
 			list(, $result) = \OC_Helper::streamCopy($source, $target);
-			if ($preserveMtime) {
+			if ($result and $preserveMtime) {
 				$this->touch($targetInternalPath, $sourceStorage->filemtime($sourceInternalPath));
 			}
 			fclose($source);
 			fclose($target);
+
+			if (!$result) {
+				// delete partially written target file
+				$this->unlink($targetInternalPath);
+				// delete cache entry that was created by fopen
+				$this->getCache()->remove($targetInternalPath);
+			}
 		}
-		return $result;
+		return (bool)$result;
 	}
 
 	/**
diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php
index 3b5e3aa3b08..269b8b23e7d 100644
--- a/tests/lib/files/view.php
+++ b/tests/lib/files/view.php
@@ -944,8 +944,19 @@ class View extends \Test\TestCase {
 
 	private function doTestCopyRenameFail($operation) {
 		$storage1 = new Temporary(array());
-		$storage2 = new Temporary(array());
-		$storage2 = new \OC\Files\Storage\Wrapper\Quota(array('storage' => $storage2, 'quota' => 9));
+		/** @var \PHPUnit_Framework_MockObject_MockObject | \OC\Files\Storage\Temporary $storage2 */
+		$storage2 = $this->getMockBuilder('\Test\Files\TemporaryNoCross')
+			->setConstructorArgs([[]])
+			->setMethods(['fopen'])
+			->getMock();
+
+		$storage2->expects($this->any())
+			->method('fopen')
+			->will($this->returnCallback(function($path, $mode) use($storage2) {
+				$source = fopen($storage2->getSourcePath($path), $mode);
+				return \OC\Files\Stream\Quota::wrap($source, 9);
+			}));
+
 		$storage1->mkdir('sub');
 		$storage1->file_put_contents('foo.txt', '0123456789ABCDEFGH');
 		$storage1->mkdir('dirtomove');
@@ -976,10 +987,8 @@ class View extends \Test\TestCase {
 		$this->assertFalse($view->$operation('/test/dirtomove/', '/test/sub/storage/dirtomove/'));
 		// since the move failed, the full source tree is kept
 		$this->assertTrue($storage1->file_exists('dirtomove/indir1.txt'));
-		// but the target file stays
-		$this->assertTrue($storage2->file_exists('dirtomove/indir1.txt'));
-		// second file not moved/copied
 		$this->assertTrue($storage1->file_exists('dirtomove/indir2.txt'));
+		// second file not moved/copied
 		$this->assertFalse($storage2->file_exists('dirtomove/indir2.txt'));
 		$this->assertFalse($storage2->getCache()->get('dirtomove/indir2.txt'));