Skip to content

Commit 614acc9

Browse files
icewind1991rullzer
icewind1991
authored and
rullzer
committed
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 <[email protected]>
1 parent 06732cf commit 614acc9

2 files changed

Lines changed: 81 additions & 21 deletions

File tree

‎apps/files_trashbin/lib/Trashbin.php‎

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,13 @@
4747
use OC\Files\View;
4848
use OCA\Files_Trashbin\AppInfo\Application;
4949
use OCA\Files_Trashbin\Command\Expire;
50+
use OCP\AppFramework\Utility\ITimeFactory;
5051
use OCP\Files\File;
5152
use OCP\Files\Folder;
5253
use OCP\Files\NotFoundException;
5354
use OCP\Files\NotPermittedException;
55+
use OCP\Lock\ILockingProvider;
56+
use OCP\Lock\LockedException;
5457
use OCP\User;
5558

5659
class Trashbin {
@@ -220,8 +223,8 @@ private static function copyFilesToUser($sourcePath, $owner, $targetPath, $user,
220223
public static function move2trash($file_path, $ownerOnly = false) {
221224
// get the user for which the filesystem is setup
222225
$root = Filesystem::getRoot();
223-
list(, $user) = explode('/', $root);
224-
list($owner, $ownerPath) = self::getUidAndFilename($file_path);
226+
[, $user] = explode('/', $root);
227+
[$owner, $ownerPath] = self::getUidAndFilename($file_path);
225228

226229
// if no owner found (ex: ext storage + share link), will use the current user's trashbin then
227230
if (is_null($owner)) {
@@ -245,15 +248,36 @@ public static function move2trash($file_path, $ownerOnly = false) {
245248

246249
$filename = $path_parts['basename'];
247250
$location = $path_parts['dirname'];
248-
$timestamp = time();
251+
/** @var ITimeFactory $timeFactory */
252+
$timeFactory = \OC::$server->query(ITimeFactory::class);
253+
$timestamp = $timeFactory->getTime();
254+
255+
$lockingProvider = \OC::$server->getLockingProvider();
249256

250257
// disable proxy to prevent recursive calls
251258
$trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp;
259+
$gotLock = false;
260+
261+
while (!$gotLock) {
262+
try {
263+
/** @var \OC\Files\Storage\Storage $trashStorage */
264+
[$trashStorage, $trashInternalPath] = $ownerView->resolvePath($trashPath);
265+
266+
$trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
267+
$gotLock = true;
268+
} catch (LockedException $e) {
269+
// a file with the same name is being deleted concurrently
270+
// nudge the timestamp a bit to resolve the conflict
271+
272+
$timestamp = $timestamp + 1;
273+
274+
$trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp;
275+
}
276+
}
252277

253-
/** @var \OC\Files\Storage\Storage $trashStorage */
254-
list($trashStorage, $trashInternalPath) = $ownerView->resolvePath($trashPath);
255278
/** @var \OC\Files\Storage\Storage $sourceStorage */
256-
list($sourceStorage, $sourceInternalPath) = $ownerView->resolvePath('/files/' . $ownerPath);
279+
[$sourceStorage, $sourceInternalPath] = $ownerView->resolvePath('/files/' . $ownerPath);
280+
257281
try {
258282
$moveSuccessful = true;
259283
if ($trashStorage->file_exists($trashInternalPath)) {
@@ -296,6 +320,8 @@ public static function move2trash($file_path, $ownerOnly = false) {
296320
}
297321
}
298322

323+
$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
324+
299325
self::scheduleExpire($user);
300326

301327
// if owner !== user we also need to update the owners trash size
@@ -345,9 +371,9 @@ private static function retainVersions($filename, $owner, $ownerPath, $timestamp
345371
*/
346372
private static function move(View $view, $source, $target) {
347373
/** @var \OC\Files\Storage\Storage $sourceStorage */
348-
list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source);
374+
[$sourceStorage, $sourceInternalPath] = $view->resolvePath($source);
349375
/** @var \OC\Files\Storage\Storage $targetStorage */
350-
list($targetStorage, $targetInternalPath) = $view->resolvePath($target);
376+
[$targetStorage, $targetInternalPath] = $view->resolvePath($target);
351377
/** @var \OC\Files\Storage\Storage $ownerTrashStorage */
352378

353379
$result = $targetStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
@@ -367,9 +393,9 @@ private static function move(View $view, $source, $target) {
367393
*/
368394
private static function copy(View $view, $source, $target) {
369395
/** @var \OC\Files\Storage\Storage $sourceStorage */
370-
list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source);
396+
[$sourceStorage, $sourceInternalPath] = $view->resolvePath($source);
371397
/** @var \OC\Files\Storage\Storage $targetStorage */
372-
list($targetStorage, $targetInternalPath) = $view->resolvePath($target);
398+
[$targetStorage, $targetInternalPath] = $view->resolvePath($target);
373399
/** @var \OC\Files\Storage\Storage $ownerTrashStorage */
374400

375401
$result = $targetStorage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
@@ -465,7 +491,7 @@ private static function restoreVersions(View $view, $file, $filename, $uniqueFil
465491

466492
$target = Filesystem::normalizePath('/' . $location . '/' . $uniqueFilename);
467493

468-
list($owner, $ownerPath) = self::getUidAndFilename($target);
494+
[$owner, $ownerPath] = self::getUidAndFilename($target);
469495

470496
// file has been deleted in between
471497
if (empty($ownerPath)) {
@@ -731,7 +757,7 @@ public static function expire($user) {
731757
$dirContent = Helper::getTrashFiles('/', $user, 'mtime');
732758

733759
// delete all files older then $retention_obligation
734-
list($delSize, $count) = self::deleteExpiredFiles($dirContent, $user);
760+
[$delSize, $count] = self::deleteExpiredFiles($dirContent, $user);
735761

736762
$availableSpace += $delSize;
737763

@@ -868,7 +894,7 @@ private static function getVersionsFromTrash($filename, $timestamp, $user) {
868894
//force rescan of versions, local storage may not have updated the cache
869895
if (!self::$scannedVersions) {
870896
/** @var \OC\Files\Storage\Storage $storage */
871-
list($storage,) = $view->resolvePath('/');
897+
[$storage,] = $view->resolvePath('/');
872898
$storage->getScanner()->scan('files_trashbin/versions');
873899
self::$scannedVersions = true;
874900
}

‎apps/files_trashbin/tests/StorageTest.php‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,14 @@
3737
use OCA\Files_Trashbin\Events\MoveToTrashEvent;
3838
use OCA\Files_Trashbin\Storage;
3939
use OCA\Files_Trashbin\Trash\ITrashManager;
40+
use OCP\AppFramework\Utility\ITimeFactory;
4041
use OCP\Files\Cache\ICache;
4142
use OCP\Files\Folder;
4243
use OCP\Files\IRootFolder;
4344
use OCP\Files\Node;
4445
use OCP\ILogger;
4546
use OCP\IUserManager;
47+
use OCP\Lock\ILockingProvider;
4648
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
4749

4850
/**
@@ -107,7 +109,7 @@ protected function tearDown(): void {
107109
public function testSingleStorageDeleteFile() {
108110
$this->assertTrue($this->userView->file_exists('test.txt'));
109111
$this->userView->unlink('test.txt');
110-
list($storage,) = $this->userView->resolvePath('test.txt');
112+
[$storage,] = $this->userView->resolvePath('test.txt');
111113
$storage->getScanner()->scan(''); // make sure we check the storage
112114
$this->assertFalse($this->userView->getFileInfo('test.txt'));
113115

@@ -124,7 +126,7 @@ public function testSingleStorageDeleteFile() {
124126
public function testSingleStorageDeleteFolder() {
125127
$this->assertTrue($this->userView->file_exists('folder/inside.txt'));
126128
$this->userView->rmdir('folder');
127-
list($storage,) = $this->userView->resolvePath('folder/inside.txt');
129+
[$storage,] = $this->userView->resolvePath('folder/inside.txt');
128130
$storage->getScanner()->scan(''); // make sure we check the storage
129131
$this->assertFalse($this->userView->getFileInfo('folder'));
130132

@@ -213,7 +215,7 @@ public function testDeleteVersionsOfFile() {
213215
$this->userView->unlink('test.txt');
214216

215217
// rescan trash storage
216-
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
218+
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
217219
$rootStorage->getScanner()->scan('');
218220

219221
// check if versions are in trashbin
@@ -242,7 +244,7 @@ public function testDeleteVersionsOfFolder() {
242244
$this->userView->rmdir('folder');
243245

244246
// rescan trash storage
245-
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
247+
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
246248
$rootStorage->getScanner()->scan('');
247249

248250
// check if versions are in trashbin
@@ -296,7 +298,7 @@ public function testDeleteVersionsOfFileAsRecipient() {
296298
$recipientView->unlink('share/test.txt');
297299

298300
// rescan trash storage for both users
299-
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
301+
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
300302
$rootStorage->getScanner()->scan('');
301303

302304
// check if versions are in trashbin for both users
@@ -350,7 +352,7 @@ public function testDeleteVersionsOfFolderAsRecipient() {
350352
$recipientView->rmdir('share/folder');
351353

352354
// rescan trash storage
353-
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
355+
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
354356
$rootStorage->getScanner()->scan('');
355357

356358
// check if versions are in trashbin for owner
@@ -407,7 +409,7 @@ public function testKeepFileAndVersionsWhenMovingFileBetweenStorages() {
407409
$this->assertTrue($this->userView->file_exists('substorage/test.txt'));
408410

409411
// rescan trash storage
410-
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
412+
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
411413
$rootStorage->getScanner()->scan('');
412414

413415
// versions were moved too
@@ -448,7 +450,7 @@ public function testKeepFileAndVersionsWhenMovingFolderBetweenStorages() {
448450
$this->assertTrue($this->userView->file_exists('substorage/folder/inside.txt'));
449451

450452
// rescan trash storage
451-
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
453+
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
452454
$rootStorage->getScanner()->scan('');
453455

454456
// versions were moved too
@@ -606,4 +608,36 @@ public function testSingleStorageDeleteFileLoggedOut() {
606608
$this->addToAssertionCount(1);
607609
}
608610
}
611+
612+
public function testTrashbinCollision() {
613+
$this->userView->file_put_contents('test.txt', 'foo');
614+
$this->userView->file_put_contents('folder/test.txt', 'bar');
615+
616+
$timeFactory = $this->createMock(ITimeFactory::class);
617+
$timeFactory->method('getTime')
618+
->willReturn(1000);
619+
620+
$lockingProvider = \OC::$server->getLockingProvider();
621+
622+
$this->overwriteService(ITimeFactory::class, $timeFactory);
623+
624+
$this->userView->unlink('test.txt');
625+
626+
$this->assertTrue($this->rootView->file_exists('/' . $this->user . '/files_trashbin/files/test.txt.d1000'));
627+
628+
/** @var \OC\Files\Storage\Storage $trashStorage */
629+
[$trashStorage, $trashInternalPath] = $this->rootView->resolvePath('/' . $this->user . '/files_trashbin/files/test.txt.d1000');
630+
631+
/// simulate a concurrent delete
632+
$trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
633+
634+
$this->userView->unlink('folder/test.txt');
635+
636+
$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
637+
638+
$this->assertTrue($this->rootView->file_exists($this->user . '/files_trashbin/files/test.txt.d1001'));
639+
640+
$this->assertEquals('foo', $this->rootView->file_get_contents($this->user . '/files_trashbin/files/test.txt.d1000'));
641+
$this->assertEquals('bar', $this->rootView->file_get_contents($this->user . '/files_trashbin/files/test.txt.d1001'));
642+
}
609643
}

0 commit comments

Comments
 (0)