Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add locking to resolve concurent move to trashbin conflicts #20841

Merged
merged 1 commit into from
May 6, 2020
Merged

add locking to resolve concurent move to trashbin conflicts #20841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 39 additions & 13 deletions apps/files_trashbin/lib/Trashbin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,13 @@
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 {
Expand Down Expand Up @@ -220,8 +223,8 @@ private static function copyFilesToUser($sourcePath, $owner, $targetPath, $user,
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)) {
Expand All @@ -245,15 +248,36 @@ public static function move2trash($file_path, $ownerOnly = false) {

$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)) {
Expand Down Expand Up @@ -296,6 +320,8 @@ public static function move2trash($file_path, $ownerOnly = false) {
}
}

$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);

self::scheduleExpire($user);

// if owner !== user we also need to update the owners trash size
Expand Down Expand Up @@ -345,9 +371,9 @@ private static function retainVersions($filename, $owner, $ownerPath, $timestamp
*/
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);
Expand All @@ -367,9 +393,9 @@ private static function move(View $view, $source, $target) {
*/
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);
Expand Down Expand Up @@ -465,7 +491,7 @@ private static function restoreVersions(View $view, $file, $filename, $uniqueFil

$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)) {
Expand Down Expand Up @@ -731,7 +757,7 @@ public static function expire($user) {
$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;

Expand Down Expand Up @@ -868,7 +894,7 @@ private static function getVersionsFromTrash($filename, $timestamp, $user) {
//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;
}
Expand Down
50 changes: 42 additions & 8 deletions apps/files_trashbin/tests/StorageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@
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;

/**
Expand Down Expand Up @@ -107,7 +109,7 @@ protected function tearDown(): void {
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'));

Expand All @@ -124,7 +126,7 @@ public function testSingleStorageDeleteFile() {
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'));

Expand Down Expand Up @@ -213,7 +215,7 @@ public function testDeleteVersionsOfFile() {
$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
Expand Down Expand Up @@ -242,7 +244,7 @@ public function testDeleteVersionsOfFolder() {
$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
Expand Down Expand Up @@ -296,7 +298,7 @@ public function testDeleteVersionsOfFileAsRecipient() {
$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
Expand Down Expand Up @@ -350,7 +352,7 @@ public function testDeleteVersionsOfFolderAsRecipient() {
$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
Expand Down Expand Up @@ -407,7 +409,7 @@ public function testKeepFileAndVersionsWhenMovingFileBetweenStorages() {
$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
Expand Down Expand Up @@ -448,7 +450,7 @@ public function testKeepFileAndVersionsWhenMovingFolderBetweenStorages() {
$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
Expand Down Expand Up @@ -606,4 +608,36 @@ public function testSingleStorageDeleteFileLoggedOut() {
$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'));
}
}