diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index c4162c7df046f..a59297be7dae4 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -955,13 +955,16 @@ public function copy($source, $target, $preserveMtime = false) { } $run = true; - $this->lockFile($target, ILockingProvider::LOCK_SHARED); - $this->lockFile($source, ILockingProvider::LOCK_SHARED); + $targetLocked = $this->lockFile($target, ILockingProvider::LOCK_SHARED); + $sourceLocked = false; $lockTypePath1 = ILockingProvider::LOCK_SHARED; $lockTypePath2 = ILockingProvider::LOCK_SHARED; + $operationException = null; try { + $sourceLocked = $this->lockFile($source, ILockingProvider::LOCK_SHARED); $exists = $this->file_exists($target); + if ($this->shouldEmitHooks($source) && $this->shouldEmitHooks($target)) { \OC_Hook::emit( Filesystem::CLASSNAME, @@ -974,6 +977,7 @@ public function copy($source, $target, $preserveMtime = false) { ); $this->emit_file_hooks_pre($exists, $target, $run); } + if ($run) { $mount1 = $this->getMount($source); $mount2 = $this->getMount($target); @@ -1014,15 +1018,54 @@ public function copy($source, $target, $preserveMtime = false) { $this->emit_file_hooks_post($exists, $target); } } - } catch (\Exception $e) { - $this->unlockFile($target, $lockTypePath2); - $this->unlockFile($source, $lockTypePath1); + } catch (\Throwable $e) { + $operationException = $e; throw $e; - } + } finally { + $cleanupException = null; + + // Preserve the prior target-then-source unlock order, but do not + // leave the source locked if releasing the target itself fails. + try { + if ($targetLocked) { + $this->unlockFile($target, $lockTypePath2); + } + } catch (\Throwable $unlockException) { + if ($operationException !== null) { + $this->logger->error('Failed to release target lock after copy operation', [ + 'app' => 'core', + 'source' => $source, + 'target' => $target, + 'exception' => $unlockException, + ]); + } else { + $cleanupException = $unlockException; + } + } + + try { + if ($sourceLocked) { + $this->unlockFile($source, $lockTypePath1); + } + } catch (\Throwable $unlockException) { + if ($operationException !== null || $cleanupException !== null) { + $this->logger->error('Failed to release source lock after copy operation', [ + 'app' => 'core', + 'source' => $source, + 'target' => $target, + 'exception' => $unlockException, + ]); + } else { + $cleanupException = $unlockException; + } + } - $this->unlockFile($target, $lockTypePath2); - $this->unlockFile($source, $lockTypePath1); + if ($operationException === null && $cleanupException !== null) { + throw $cleanupException; + } + } } + return $result; } diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index c21bf6fc40c9d..de7a9a8fc6c60 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -2315,6 +2315,37 @@ function (): void { } } + public function testCopyUnlocksTargetWhenSourceLockCannotBeAcquired(): void { + $view = new View('/' . self::$user . '/files/'); + $storage = new Temporary([]); + + Filesystem::mount($storage, [], self::$user . '/'); + $storage->mkdir('files'); + + $sourcePath = 'source.txt'; + $targetPath = 'target.txt'; + $view->file_put_contents($sourcePath, 'content'); + + $view->lockFile($sourcePath, ILockingProvider::LOCK_EXCLUSIVE); + + try { + $view->copy($sourcePath, $targetPath); + $this->fail('Expected source-lock acquisition to fail'); + } catch (LockedException) { + $this->assertNull( + $this->getFileLockType($view, $targetPath), + 'The target lock acquired before the source lock must be released' + ); + $this->assertSame( + ILockingProvider::LOCK_EXCLUSIVE, + $this->getFileLockType($view, $sourcePath), + 'The pre-existing source lock must not be released by copy()' + ); + } finally { + $view->unlockFile($sourcePath, ILockingProvider::LOCK_EXCLUSIVE); + } + } + /** * Test rename operation: unlock first path when second path was locked */