From 614acc9419bb7d36d39c39369ee2e8828792788a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 6 May 2020 15:32:44 +0200 Subject: [PATCH] add locking to resolve concurent move to trashbin conflicts uses a lock to prevent two requests from moving a file to the trashbin concurrently (causing sql duplicate key errors) Signed-off-by: Robin Appelman --- apps/files_trashbin/lib/Trashbin.php | 52 +++++++++++++++++------ apps/files_trashbin/tests/StorageTest.php | 50 ++++++++++++++++++---- 2 files changed, 81 insertions(+), 21 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 4ceac3b04c..a85f761abb 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -47,10 +47,13 @@ use OC\Files\Filesystem; use OC\Files\View; use OCA\Files_Trashbin\AppInfo\Application; use OCA\Files_Trashbin\Command\Expire; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; +use OCP\Lock\ILockingProvider; +use OCP\Lock\LockedException; use OCP\User; class Trashbin { @@ -220,8 +223,8 @@ class Trashbin { public static function move2trash($file_path, $ownerOnly = false) { // get the user for which the filesystem is setup $root = Filesystem::getRoot(); - list(, $user) = explode('/', $root); - list($owner, $ownerPath) = self::getUidAndFilename($file_path); + [, $user] = explode('/', $root); + [$owner, $ownerPath] = self::getUidAndFilename($file_path); // if no owner found (ex: ext storage + share link), will use the current user's trashbin then if (is_null($owner)) { @@ -245,15 +248,36 @@ class Trashbin { $filename = $path_parts['basename']; $location = $path_parts['dirname']; - $timestamp = time(); + /** @var ITimeFactory $timeFactory */ + $timeFactory = \OC::$server->query(ITimeFactory::class); + $timestamp = $timeFactory->getTime(); + + $lockingProvider = \OC::$server->getLockingProvider(); // disable proxy to prevent recursive calls $trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp; + $gotLock = false; + + while (!$gotLock) { + try { + /** @var \OC\Files\Storage\Storage $trashStorage */ + [$trashStorage, $trashInternalPath] = $ownerView->resolvePath($trashPath); + + $trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + $gotLock = true; + } catch (LockedException $e) { + // a file with the same name is being deleted concurrently + // nudge the timestamp a bit to resolve the conflict + + $timestamp = $timestamp + 1; + + $trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp; + } + } - /** @var \OC\Files\Storage\Storage $trashStorage */ - list($trashStorage, $trashInternalPath) = $ownerView->resolvePath($trashPath); /** @var \OC\Files\Storage\Storage $sourceStorage */ - list($sourceStorage, $sourceInternalPath) = $ownerView->resolvePath('/files/' . $ownerPath); + [$sourceStorage, $sourceInternalPath] = $ownerView->resolvePath('/files/' . $ownerPath); + try { $moveSuccessful = true; if ($trashStorage->file_exists($trashInternalPath)) { @@ -296,6 +320,8 @@ class Trashbin { } } + $trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + self::scheduleExpire($user); // if owner !== user we also need to update the owners trash size @@ -345,9 +371,9 @@ class Trashbin { */ private static function move(View $view, $source, $target) { /** @var \OC\Files\Storage\Storage $sourceStorage */ - list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source); + [$sourceStorage, $sourceInternalPath] = $view->resolvePath($source); /** @var \OC\Files\Storage\Storage $targetStorage */ - list($targetStorage, $targetInternalPath) = $view->resolvePath($target); + [$targetStorage, $targetInternalPath] = $view->resolvePath($target); /** @var \OC\Files\Storage\Storage $ownerTrashStorage */ $result = $targetStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); @@ -367,9 +393,9 @@ class Trashbin { */ private static function copy(View $view, $source, $target) { /** @var \OC\Files\Storage\Storage $sourceStorage */ - list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source); + [$sourceStorage, $sourceInternalPath] = $view->resolvePath($source); /** @var \OC\Files\Storage\Storage $targetStorage */ - list($targetStorage, $targetInternalPath) = $view->resolvePath($target); + [$targetStorage, $targetInternalPath] = $view->resolvePath($target); /** @var \OC\Files\Storage\Storage $ownerTrashStorage */ $result = $targetStorage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); @@ -465,7 +491,7 @@ class Trashbin { $target = Filesystem::normalizePath('/' . $location . '/' . $uniqueFilename); - list($owner, $ownerPath) = self::getUidAndFilename($target); + [$owner, $ownerPath] = self::getUidAndFilename($target); // file has been deleted in between if (empty($ownerPath)) { @@ -731,7 +757,7 @@ class Trashbin { $dirContent = Helper::getTrashFiles('/', $user, 'mtime'); // delete all files older then $retention_obligation - list($delSize, $count) = self::deleteExpiredFiles($dirContent, $user); + [$delSize, $count] = self::deleteExpiredFiles($dirContent, $user); $availableSpace += $delSize; @@ -868,7 +894,7 @@ class Trashbin { //force rescan of versions, local storage may not have updated the cache if (!self::$scannedVersions) { /** @var \OC\Files\Storage\Storage $storage */ - list($storage,) = $view->resolvePath('/'); + [$storage,] = $view->resolvePath('/'); $storage->getScanner()->scan('files_trashbin/versions'); self::$scannedVersions = true; } diff --git a/apps/files_trashbin/tests/StorageTest.php b/apps/files_trashbin/tests/StorageTest.php index 3b27e13e88..ec0c3d522a 100644 --- a/apps/files_trashbin/tests/StorageTest.php +++ b/apps/files_trashbin/tests/StorageTest.php @@ -37,12 +37,14 @@ use OC\Files\Storage\Temporary; use OCA\Files_Trashbin\Events\MoveToTrashEvent; use OCA\Files_Trashbin\Storage; use OCA\Files_Trashbin\Trash\ITrashManager; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\Cache\ICache; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\Node; use OCP\ILogger; use OCP\IUserManager; +use OCP\Lock\ILockingProvider; use Symfony\Component\EventDispatcher\EventDispatcherInterface; /** @@ -107,7 +109,7 @@ class StorageTest extends \Test\TestCase { public function testSingleStorageDeleteFile() { $this->assertTrue($this->userView->file_exists('test.txt')); $this->userView->unlink('test.txt'); - list($storage,) = $this->userView->resolvePath('test.txt'); + [$storage,] = $this->userView->resolvePath('test.txt'); $storage->getScanner()->scan(''); // make sure we check the storage $this->assertFalse($this->userView->getFileInfo('test.txt')); @@ -124,7 +126,7 @@ class StorageTest extends \Test\TestCase { public function testSingleStorageDeleteFolder() { $this->assertTrue($this->userView->file_exists('folder/inside.txt')); $this->userView->rmdir('folder'); - list($storage,) = $this->userView->resolvePath('folder/inside.txt'); + [$storage,] = $this->userView->resolvePath('folder/inside.txt'); $storage->getScanner()->scan(''); // make sure we check the storage $this->assertFalse($this->userView->getFileInfo('folder')); @@ -213,7 +215,7 @@ class StorageTest extends \Test\TestCase { $this->userView->unlink('test.txt'); // rescan trash storage - list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + [$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin'); $rootStorage->getScanner()->scan(''); // check if versions are in trashbin @@ -242,7 +244,7 @@ class StorageTest extends \Test\TestCase { $this->userView->rmdir('folder'); // rescan trash storage - list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + [$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin'); $rootStorage->getScanner()->scan(''); // check if versions are in trashbin @@ -296,7 +298,7 @@ class StorageTest extends \Test\TestCase { $recipientView->unlink('share/test.txt'); // rescan trash storage for both users - list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + [$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin'); $rootStorage->getScanner()->scan(''); // check if versions are in trashbin for both users @@ -350,7 +352,7 @@ class StorageTest extends \Test\TestCase { $recipientView->rmdir('share/folder'); // rescan trash storage - list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + [$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin'); $rootStorage->getScanner()->scan(''); // check if versions are in trashbin for owner @@ -407,7 +409,7 @@ class StorageTest extends \Test\TestCase { $this->assertTrue($this->userView->file_exists('substorage/test.txt')); // rescan trash storage - list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + [$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin'); $rootStorage->getScanner()->scan(''); // versions were moved too @@ -448,7 +450,7 @@ class StorageTest extends \Test\TestCase { $this->assertTrue($this->userView->file_exists('substorage/folder/inside.txt')); // rescan trash storage - list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + [$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin'); $rootStorage->getScanner()->scan(''); // versions were moved too @@ -606,4 +608,36 @@ class StorageTest extends \Test\TestCase { $this->addToAssertionCount(1); } } + + public function testTrashbinCollision() { + $this->userView->file_put_contents('test.txt', 'foo'); + $this->userView->file_put_contents('folder/test.txt', 'bar'); + + $timeFactory = $this->createMock(ITimeFactory::class); + $timeFactory->method('getTime') + ->willReturn(1000); + + $lockingProvider = \OC::$server->getLockingProvider(); + + $this->overwriteService(ITimeFactory::class, $timeFactory); + + $this->userView->unlink('test.txt'); + + $this->assertTrue($this->rootView->file_exists('/' . $this->user . '/files_trashbin/files/test.txt.d1000')); + + /** @var \OC\Files\Storage\Storage $trashStorage */ + [$trashStorage, $trashInternalPath] = $this->rootView->resolvePath('/' . $this->user . '/files_trashbin/files/test.txt.d1000'); + + /// simulate a concurrent delete + $trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + + $this->userView->unlink('folder/test.txt'); + + $trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); + + $this->assertTrue($this->rootView->file_exists($this->user . '/files_trashbin/files/test.txt.d1001')); + + $this->assertEquals('foo', $this->rootView->file_get_contents($this->user . '/files_trashbin/files/test.txt.d1000')); + $this->assertEquals('bar', $this->rootView->file_get_contents($this->user . '/files_trashbin/files/test.txt.d1001')); + } }