From 84a91d37143a34797c817e4fcdeecbd26d420e79 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 22 Jul 2026 15:10:34 -0400 Subject: [PATCH 1/2] fix(files): release stream upload lock on storage failure Signed-off-by: Josh --- lib/private/Files/View.php | 106 +++++++++++++++++++------------------ 1 file changed, 55 insertions(+), 51 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index c4162c7df046f..719cbf3216913 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -617,66 +617,70 @@ protected function emit_file_hooks_post(bool $exists, string $path): void { * @throws LockedException */ public function file_put_contents($path, $data) { - if (is_resource($data)) { //not having to deal with streams in file_put_contents makes life easier - $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); - if (Filesystem::isValidPath($path) - && !Filesystem::isFileBlacklisted($path) - ) { - $path = $this->getRelativePath($absolutePath); - if ($path === null) { - throw new InvalidPathException("Path $absolutePath is not in the expected root"); - } + // not having to deal with streams in file_put_contents makes life easier + if (!is_resource($data)) { + $hooks = $this->file_exists($path) ? ['update', 'write'] : ['create', 'write']; + return $this->basicOperation('file_put_contents', $path, $hooks, $data); + } - $this->lockFile($path, ILockingProvider::LOCK_SHARED); + $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); + if (!Filesystem::isValidPath($path) + || Filesystem::isFileBlacklisted($path) + ) { + return false; + } + + $path = $this->getRelativePath($absolutePath); + if ($path === null) { + throw new InvalidPathException("Path $absolutePath is not in the expected root"); + } - $exists = $this->file_exists($path); - if ($this->shouldEmitHooks($path)) { - $run = true; - $this->emit_file_hooks_pre($exists, $path, $run); - if (!$run) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - return false; - } - } + $this->lockFile($path, ILockingProvider::LOCK_SHARED); + $lockType = ILockingProvider::LOCK_SHARED; - try { - $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); - } catch (\Exception $e) { - // Release the shared lock before throwing. - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - throw $e; + try { + $exists = $this->file_exists($path); + if ($this->shouldEmitHooks($path)) { + $run = true; + $this->emit_file_hooks_pre($exists, $path, $run); + if (!$run) { + return false; } + } - /** @var Storage $storage */ - [$storage, $internalPath] = $this->resolvePath($path); - $target = $storage->fopen($internalPath, 'w'); - if ($target) { - $result = stream_copy_to_stream($data, $target); - if ($result !== false) { - $result = true; - } - fclose($target); - fclose($data); + $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); + $lockType = ILockingProvider::LOCK_EXCLUSIVE; - $this->writeUpdate($storage, $internalPath); + /** @var Storage $storage */ + [$storage, $internalPath] = $this->resolvePath($path); + $target = $storage->fopen($internalPath, 'w'); + if (!$target) { + return false; + } - $this->changeLock($path, ILockingProvider::LOCK_SHARED); + try { + $result = stream_copy_to_stream($data, $target); + } finally { + fclose($target); + fclose($data); + } - if ($this->shouldEmitHooks($path) && $result !== false) { - $this->emit_file_hooks_post($exists, $path); - } - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - return $result; - } else { - $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - return false; - } - } else { - return false; + if ($result !== false) { + $result = true; } - } else { - $hooks = $this->file_exists($path) ? ['update', 'write'] : ['create', 'write']; - return $this->basicOperation('file_put_contents', $path, $hooks, $data); + + $this->writeUpdate($storage, $internalPath); + + $this->changeLock($path, ILockingProvider::LOCK_SHARED); + $lockType = ILockingProvider::LOCK_SHARED; + + if ($this->shouldEmitHooks($path) && $result !== false) { + $this->emit_file_hooks_post($exists, $path); + } + + return $result; + } finally { + $this->unlockFile($path, $lockType); } } From f34675836dfe4c8f9caff3d3b1341238453f562c Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 22 Jul 2026 15:14:44 -0400 Subject: [PATCH 2/2] test(files): make sure upload lock released on storage failure Assisted-by: Copilot:GPT-5.6 Signed-off-by: Josh --- tests/lib/Files/ViewTest.php | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index c21bf6fc40c9d..526403a826ef7 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -2017,6 +2017,41 @@ function () use ($view, $path, &$lockTypeDuring) { $this->assertNull($this->getFileLockType($view, $path)); } + public function testLockFilePutContentsWithStreamUnlocksAfterStorageException(): void { + $view = new View('/' . self::$user . '/files/'); + $path = 'failed-stream-upload.txt'; + + /** @var Temporary&MockObject $storage */ + $storage = $this->getMockBuilder(Temporary::class) + ->onlyMethods(['fopen']) + ->getMock(); + + Filesystem::mount($storage, [], self::$user . '/'); + $storage->mkdir('files'); + + $storage->expects($this->once()) + ->method('fopen') + ->willThrowException(new \Exception('Simulated stream upload failure')); + + $source = fopen('php://temp', 'r+'); + + try { + $view->file_put_contents($path, $source); + $this->fail('Expected the storage exception to be rethrown'); + } catch (\Exception $e) { + $this->assertSame('Simulated stream upload failure', $e->getMessage()); + } finally { + if (is_resource($source)) { + fclose($source); + } + } + + $this->assertNull( + $this->getFileLockType($view, $path), + 'The stream-upload lock must be released after a storage exception' + ); + } + /** * Test locks for fopen with fclose at the end */