From be8efa719b5ad7558577a3d38e015028684c465d Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 13 Mar 2026 13:08:09 -0400 Subject: [PATCH 1/6] refactor(View): add some useful comments to basicOperation Signed-off-by: Josh --- lib/private/Files/View.php | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 623c9d66d6e2a..40cda84807dc2 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1202,41 +1202,55 @@ public function free_space($path = '/') { * \OC\Files\Storage\Storage for delegation to a storage backend for execution */ private function basicOperation(string $operation, string $path, array $hooks = [], $extraParam = null) { + // Preserve trailing slash semantics when resolving storage paths. $postFix = (substr($path, -1) === '/') ? '/' : ''; + + // Build and normalize absolute path from the provided relative path. $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); + + // Only proceed for valid, non-blacklisted paths. if (Filesystem::isValidPath($path) && !Filesystem::isFileBlacklisted($path) ) { + // Convert back (post-normalized) to relative path in the current view context. $path = $this->getRelativePath($absolutePath); if ($path === null) { return false; } + // Pre-hook phase: acquire a shared lock so hooks can safely read metadata/content. if (in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks)) { - // always a shared lock during pre-hooks so the hook can read the file $this->lockFile($path, ILockingProvider::LOCK_SHARED); } + // Run pre-hooks; hooks can veto execution by returning false. $run = $this->runHooks($hooks, $path); + + // Resolve absolute path to storage backend + internal path. [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); + /** @var Storage $storage */ if ($run && $storage) { - /** @var Storage $storage */ + + // For mutating operations, upgrade shared lock to exclusive before actual write/delete. if (in_array('write', $hooks) || in_array('delete', $hooks)) { try { $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); } catch (LockedException $e) { - // release the shared lock we acquired before quitting + // If lock upgrade fails, release previously acquired shared lock. $this->unlockFile($path, ILockingProvider::LOCK_SHARED); throw $e; } } + try { + // Delegate operation to storage backend, with optional extra parameter. if (!is_null($extraParam)) { $result = $storage->$operation($internalPath, $extraParam); } else { $result = $storage->$operation($internalPath); } } catch (\Exception $e) { + // On backend error, release whichever lock this branch currently owns. if (in_array('write', $hooks) || in_array('delete', $hooks)) { $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); } elseif (in_array('read', $hooks)) { @@ -1245,18 +1259,25 @@ private function basicOperation(string $operation, string $path, array $hooks = throw $e; } + // Update delete bookkeeping only on successful delete-like operation. if ($result !== false && in_array('delete', $hooks)) { $this->removeUpdate($storage, $internalPath); } + + // Update write bookkeeping for successful write-like operations except stream/touch special cases. if ($result !== false && in_array('write', $hooks, true) && $operation !== 'fopen' && $operation !== 'touch') { $isCreateOperation = $operation === 'mkdir' || ($operation === 'file_put_contents' && in_array('create', $hooks, true)); $sizeDifference = $operation === 'mkdir' ? 0 : $result; $this->writeUpdate($storage, $internalPath, null, $isCreateOperation ? $sizeDifference : null); } + + // touch has dedicated bookkeeping behavior. if ($result !== false && in_array('touch', $hooks)) { $this->writeUpdate($storage, $internalPath, $extraParam, 0); } + // For mutating operations, downgrade the lock from exclusive to shared after the write/delete step. + // Keep it exclusive for successful fopen; it will be unlocked later when the stream closes. if ((in_array('write', $hooks) || in_array('delete', $hooks)) && ($operation !== 'fopen' || $result === false)) { $this->changeLock($path, ILockingProvider::LOCK_SHARED); } @@ -1264,8 +1285,11 @@ private function basicOperation(string $operation, string $path, array $hooks = $unlockLater = false; if ($this->lockingEnabled && $operation === 'fopen' && is_resource($result)) { $unlockLater = true; - // make sure our unlocking callback will still be called if connection is aborted + + // Ensure unlock callback still runs even if client disconnects. ignore_user_abort(true); + + // Defer unlock until stream close. $result = CallbackWrapper::wrap($result, null, null, function () use ($hooks, $path): void { if (in_array('write', $hooks)) { $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); @@ -1275,12 +1299,14 @@ private function basicOperation(string $operation, string $path, array $hooks = }); } + // Emit post-hooks on success, except for fopen (stream still open at this point). if ($this->shouldEmitHooks($path) && $result !== false) { - if ($operation !== 'fopen') { //no post hooks for fopen, the file stream is still open + if ($operation !== 'fopen') { $this->runHooks($hooks, $path, true); } } + // Normal unlock path when lock ownership was not transferred to stream close callback. if (!$unlockLater && (in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks)) ) { @@ -1288,6 +1314,7 @@ private function basicOperation(string $operation, string $path, array $hooks = } return $result; } else { + // If pre-hooks vetoed or no storage resolved, release pre-hook shared lock. $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } } From c0ed1ab40ad98c8ec568e3235f9fc3e760078956 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 13 Mar 2026 13:19:35 -0400 Subject: [PATCH 2/6] refactor(View): extract intent into booleans for readability Signed-off-by: Josh --- lib/private/Files/View.php | 50 +++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 40cda84807dc2..4148d0faa46b0 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1208,6 +1208,14 @@ private function basicOperation(string $operation, string $path, array $hooks = // Build and normalize absolute path from the provided relative path. $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); + // Precompute hook intent flags once for readability and consistency. + $isWrite = in_array('write', $hooks, true); + $isDelete = in_array('delete', $hooks, true); + $isRead = in_array('read', $hooks, true); + $isTouch = in_array('touch', $hooks, true); + $isCreateHook = in_array('create', $hooks, true); + $needsLock = $isWrite || $isDelete || $isRead; + // Only proceed for valid, non-blacklisted paths. if (Filesystem::isValidPath($path) && !Filesystem::isFileBlacklisted($path) @@ -1219,20 +1227,20 @@ private function basicOperation(string $operation, string $path, array $hooks = } // Pre-hook phase: acquire a shared lock so hooks can safely read metadata/content. - if (in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks)) { + if ($needsLock) { $this->lockFile($path, ILockingProvider::LOCK_SHARED); } // Run pre-hooks; hooks can veto execution by returning false. $run = $this->runHooks($hooks, $path); + /** @var Storage $storage */ // Resolve absolute path to storage backend + internal path. [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); - /** @var Storage $storage */ - if ($run && $storage) { + if ($run && $storage) { // For mutating operations, upgrade shared lock to exclusive before actual write/delete. - if (in_array('write', $hooks) || in_array('delete', $hooks)) { + if ($isWrite || $isDelete) { try { $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); } catch (LockedException $e) { @@ -1244,41 +1252,39 @@ private function basicOperation(string $operation, string $path, array $hooks = try { // Delegate operation to storage backend, with optional extra parameter. - if (!is_null($extraParam)) { - $result = $storage->$operation($internalPath, $extraParam); - } else { - $result = $storage->$operation($internalPath); - } + $result = !is_null($extraParam) + ? $storage->$operation($internalPath, $extraParam) + : $storage->$operation($internalPath); } catch (\Exception $e) { // On backend error, release whichever lock this branch currently owns. - if (in_array('write', $hooks) || in_array('delete', $hooks)) { + if ($isWrite || $isDelete) { $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } elseif (in_array('read', $hooks)) { + } elseif ($isRead) { $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } throw $e; } // Update delete bookkeeping only on successful delete-like operation. - if ($result !== false && in_array('delete', $hooks)) { + if ($result !== false && $isDelete) { $this->removeUpdate($storage, $internalPath); } // Update write bookkeeping for successful write-like operations except stream/touch special cases. - if ($result !== false && in_array('write', $hooks, true) && $operation !== 'fopen' && $operation !== 'touch') { - $isCreateOperation = $operation === 'mkdir' || ($operation === 'file_put_contents' && in_array('create', $hooks, true)); + if ($result !== false && $isWrite && $operation !== 'fopen' && $operation !== 'touch') { + $isCreateOperation = $operation === 'mkdir' || ($operation === 'file_put_contents' && $isCreateHook); $sizeDifference = $operation === 'mkdir' ? 0 : $result; $this->writeUpdate($storage, $internalPath, null, $isCreateOperation ? $sizeDifference : null); } // touch has dedicated bookkeeping behavior. - if ($result !== false && in_array('touch', $hooks)) { + if ($result !== false && $isTouch) { $this->writeUpdate($storage, $internalPath, $extraParam, 0); } // For mutating operations, downgrade the lock from exclusive to shared after the write/delete step. // Keep it exclusive for successful fopen; it will be unlocked later when the stream closes. - if ((in_array('write', $hooks) || in_array('delete', $hooks)) && ($operation !== 'fopen' || $result === false)) { + if (($isWrite || $isDelete) && ($operation !== 'fopen' || $result === false)) { $this->changeLock($path, ILockingProvider::LOCK_SHARED); } @@ -1290,10 +1296,10 @@ private function basicOperation(string $operation, string $path, array $hooks = ignore_user_abort(true); // Defer unlock until stream close. - $result = CallbackWrapper::wrap($result, null, null, function () use ($hooks, $path): void { - if (in_array('write', $hooks)) { + $result = CallbackWrapper::wrap($result, null, null, function () use ($isWrite, $isRead, $path): void { + if ($isWrite)) { $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } elseif (in_array('read', $hooks)) { + } elseif ($isRead) { $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } }); @@ -1307,17 +1313,17 @@ private function basicOperation(string $operation, string $path, array $hooks = } // Normal unlock path when lock ownership was not transferred to stream close callback. - if (!$unlockLater - && (in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks)) - ) { + if (!$unlockLater && $needsLock) { $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } + return $result; } else { // If pre-hooks vetoed or no storage resolved, release pre-hook shared lock. $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } } + return null; } From 35a7cfdafef23d338ef672a175c14ab20184f9be Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 13 Mar 2026 13:31:13 -0400 Subject: [PATCH 3/6] refactor(View): use early returns to reduce nesting Signed-off-by: Josh --- lib/private/Files/View.php | 180 ++++++++++++++++++------------------- 1 file changed, 89 insertions(+), 91 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 4148d0faa46b0..5a7d410679da7 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1208,6 +1208,11 @@ private function basicOperation(string $operation, string $path, array $hooks = // Build and normalize absolute path from the provided relative path. $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); + // Guard clause: invalid or blacklisted path. + if (!Filesystem::isValidPath($path) || Filesystem::isFileBlacklisted($path)) { + return null; + } + // Precompute hook intent flags once for readability and consistency. $isWrite = in_array('write', $hooks, true); $isDelete = in_array('delete', $hooks, true); @@ -1216,115 +1221,108 @@ private function basicOperation(string $operation, string $path, array $hooks = $isCreateHook = in_array('create', $hooks, true); $needsLock = $isWrite || $isDelete || $isRead; - // Only proceed for valid, non-blacklisted paths. - if (Filesystem::isValidPath($path) - && !Filesystem::isFileBlacklisted($path) - ) { - // Convert back (post-normalized) to relative path in the current view context. - $path = $this->getRelativePath($absolutePath); - if ($path === null) { - return false; - } + // Convert back (post-normalized) to relative path in the current view context. + $path = $this->getRelativePath($absolutePath); + if ($path === null) { + return false; + } - // Pre-hook phase: acquire a shared lock so hooks can safely read metadata/content. - if ($needsLock) { - $this->lockFile($path, ILockingProvider::LOCK_SHARED); - } + // Pre-hook phase: acquire a shared lock so hooks can safely read metadata/content. + if ($needsLock) { + $this->lockFile($path, ILockingProvider::LOCK_SHARED); + } - // Run pre-hooks; hooks can veto execution by returning false. - $run = $this->runHooks($hooks, $path); + // Run pre-hooks; hooks can veto execution by returning false. + $run = $this->runHooks($hooks, $path); - /** @var Storage $storage */ - // Resolve absolute path to storage backend + internal path. - [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); + /** @var Storage $storage */ + // Resolve absolute path to storage backend + internal path. + [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); - if ($run && $storage) { - // For mutating operations, upgrade shared lock to exclusive before actual write/delete. - if ($isWrite || $isDelete) { - try { - $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); - } catch (LockedException $e) { - // If lock upgrade fails, release previously acquired shared lock. - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - throw $e; - } - } - - try { - // Delegate operation to storage backend, with optional extra parameter. - $result = !is_null($extraParam) - ? $storage->$operation($internalPath, $extraParam) - : $storage->$operation($internalPath); - } catch (\Exception $e) { - // On backend error, release whichever lock this branch currently owns. - if ($isWrite || $isDelete) { - $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } elseif ($isRead) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - } - throw $e; - } + // Guard clause: pre-hooks vetoed or storage unresolved. + if (!$run || !$storage) { + // NOTE: preserves original behavior (unconditional unlock in this branch). + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + return null; + } - // Update delete bookkeeping only on successful delete-like operation. - if ($result !== false && $isDelete) { - $this->removeUpdate($storage, $internalPath); - } + // For mutating operations, upgrade shared lock to exclusive before actual write/delete. + if ($isWrite || $isDelete) { + try { + $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); + } catch (LockedException $e) { + // If lock upgrade fails, release previously acquired shared lock. + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + throw $e; + } + } - // Update write bookkeeping for successful write-like operations except stream/touch special cases. - if ($result !== false && $isWrite && $operation !== 'fopen' && $operation !== 'touch') { - $isCreateOperation = $operation === 'mkdir' || ($operation === 'file_put_contents' && $isCreateHook); - $sizeDifference = $operation === 'mkdir' ? 0 : $result; - $this->writeUpdate($storage, $internalPath, null, $isCreateOperation ? $sizeDifference : null); - } + try { + // Delegate operation to storage backend, with optional extra parameter. + $result = !is_null($extraParam) + ? $storage->$operation($internalPath, $extraParam) + : $storage->$operation($internalPath); + } catch (\Exception $e) { + // On backend error, release whichever lock this branch currently owns. + if ($isWrite || $isDelete) { + $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); + } elseif ($isRead) { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + } + throw $e; + } - // touch has dedicated bookkeeping behavior. - if ($result !== false && $isTouch) { - $this->writeUpdate($storage, $internalPath, $extraParam, 0); - } + // Update delete bookkeeping only on successful delete-like operation. + if ($result !== false && $isDelete) { + $this->removeUpdate($storage, $internalPath); + } - // For mutating operations, downgrade the lock from exclusive to shared after the write/delete step. - // Keep it exclusive for successful fopen; it will be unlocked later when the stream closes. - if (($isWrite || $isDelete) && ($operation !== 'fopen' || $result === false)) { - $this->changeLock($path, ILockingProvider::LOCK_SHARED); - } + // Update write bookkeeping for successful write-like operations except stream/touch special cases. + if ($result !== false && $isWrite && $operation !== 'fopen' && $operation !== 'touch') { + $isCreateOperation = $operation === 'mkdir' || ($operation === 'file_put_contents' && $isCreateHook); + $sizeDifference = $operation === 'mkdir' ? 0 : $result; + $this->writeUpdate($storage, $internalPath, null, $isCreateOperation ? $sizeDifference : null); + } - $unlockLater = false; - if ($this->lockingEnabled && $operation === 'fopen' && is_resource($result)) { - $unlockLater = true; + // touch has dedicated bookkeeping behavior. + if ($result !== false && $isTouch) { + $this->writeUpdate($storage, $internalPath, $extraParam, 0); + } - // Ensure unlock callback still runs even if client disconnects. - ignore_user_abort(true); + // For mutating operations, downgrade the lock from exclusive to shared after the write/delete step. + // Keep it exclusive for successful fopen; it will be unlocked later when the stream closes. + if (($isWrite || $isDelete) && ($operation !== 'fopen' || $result === false)) { + $this->changeLock($path, ILockingProvider::LOCK_SHARED); + } - // Defer unlock until stream close. - $result = CallbackWrapper::wrap($result, null, null, function () use ($isWrite, $isRead, $path): void { - if ($isWrite)) { - $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } elseif ($isRead) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - } - }); - } + $unlockLater = false; + if ($this->lockingEnabled && $operation === 'fopen' && is_resource($result)) { + $unlockLater = true; - // Emit post-hooks on success, except for fopen (stream still open at this point). - if ($this->shouldEmitHooks($path) && $result !== false) { - if ($operation !== 'fopen') { - $this->runHooks($hooks, $path, true); - } - } + // Ensure unlock callback still runs even if client disconnects. + ignore_user_abort(true); - // Normal unlock path when lock ownership was not transferred to stream close callback. - if (!$unlockLater && $needsLock) { + // Defer unlock until stream close. + $result = CallbackWrapper::wrap($result, null, null, function () use ($isWrite, $isRead, $path): void { + if ($isWrite)) { + $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); + } elseif ($isRead) { $this->unlockFile($path, ILockingProvider::LOCK_SHARED); } + }); + } - return $result; - } else { - // If pre-hooks vetoed or no storage resolved, release pre-hook shared lock. - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - } + // Emit post-hooks on success, except for fopen (stream still open at this point). + if ($this->shouldEmitHooks($path) && $result !== false) && $operation !== 'fopen') { + $this->runHooks($hooks, $path, true); } - return null; + // Normal unlock path when lock ownership was not transferred to stream close callback. + if (!$unlockLater && $needsLock) { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + } + + return $result; } /** From 460c1fbf6c7f3c09d1791db9756717d7bc895403 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 13 Mar 2026 13:39:51 -0400 Subject: [PATCH 4/6] docs(View): update basicOperation docblock Signed-off-by: Josh --- lib/private/Files/View.php | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 5a7d410679da7..80329d2d696fe 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1191,17 +1191,31 @@ public function free_space($path = '/') { } /** - * abstraction layer for basic filesystem functions: wrapper for \OC\Files\Storage\Storage + * Execute a low-level filesystem operation on the resolved storage (\OC\Files\Storage\Storage) backend. * - * @param mixed $extraParam (optional) - * @return mixed - * @throws LockedException + * Flow: + * 1) Validate/sanitize the view-relative path. + * 2) Resolve to concrete storage + internal path. + * 3) Run pre-hooks (which may veto execution). + * 4) Acquire/upgrade locks based on hook intent (read/write/delete). + * 5) Delegate the operation to the storage implementation. + * 6) Apply write/delete/touch cache/update bookkeeping. + * 7) Run post-hooks (except for fopen; stream may still be open). + * + * @param non-empty-string $operation Storage method name to call dynamically on the resolved Storage. + * @param string $path View-relative path. + * @param list<'read'|'write'|'delete'|'touch'|'create'|string> $hooks + * Hook tags controlling locking, hook execution, and update behavior. + * @param mixed $extraParam Optional second argument forwarded to the storage operation. + * + * @return mixed Storage operation result. + * - `null` when execution is skipped (e.g. invalid/blacklisted path, hook veto, unresolved storage) + * - otherwise backend-defined return value (often `false` on operation failure) * - * This method takes requests for basic filesystem functions (e.g. reading & writing - * files), processes hooks and proxies, sanitises paths, and finally passes them on to - * \OC\Files\Storage\Storage for delegation to a storage backend for execution + * @throws LockedException If lock acquisition/upgrade fails. + * @throws \Exception Re-thrown from the delegated storage operation. */ - private function basicOperation(string $operation, string $path, array $hooks = [], $extraParam = null) { + private function basicOperation(string $operation, string $path, array $hooks = [], $extraParam = null): mixed { // Preserve trailing slash semantics when resolving storage paths. $postFix = (substr($path, -1) === '/') ? '/' : ''; From 600917905e2b37195ee7178c0fc72b734f621f72 Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 14 Mar 2026 11:51:35 -0400 Subject: [PATCH 5/6] fix(View) avoid unconditional unlock when no lock was acquired use lock-state tracking + centralized finally cleanup Signed-off-by: Josh --- lib/private/Files/View.php | 151 ++++++++++++++++++------------------- 1 file changed, 74 insertions(+), 77 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 80329d2d696fe..40352fcfca552 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1241,104 +1241,101 @@ private function basicOperation(string $operation, string $path, array $hooks = return false; } - // Pre-hook phase: acquire a shared lock so hooks can safely read metadata/content. - if ($needsLock) { - $this->lockFile($path, ILockingProvider::LOCK_SHARED); - } + /** @var null|int $lockState null means no lock currently held */ + $lockState = null; + $unlockLater = false; // true only for successful fopen stream wrapper path - // Run pre-hooks; hooks can veto execution by returning false. - $run = $this->runHooks($hooks, $path); + try { + // Pre-hook phase: acquire a shared lock so hooks can safely read metadata/content. + if ($needsLock) { + $this->lockFile($path, ILockingProvider::LOCK_SHARED); + $lockState = ILockingProvider::LOCK_SHARED; + } - /** @var Storage $storage */ - // Resolve absolute path to storage backend + internal path. - [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); + // Run pre-hooks; hooks can veto execution by returning false. + $run = $this->runHooks($hooks, $path); - // Guard clause: pre-hooks vetoed or storage unresolved. - if (!$run || !$storage) { - // NOTE: preserves original behavior (unconditional unlock in this branch). - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - return null; - } + // Resolve absolute path to storage backend + internal path. + [$storage, $internalPath] = Filesystem::resolvePath($absolutePath . $postFix); - // For mutating operations, upgrade shared lock to exclusive before actual write/delete. - if ($isWrite || $isDelete) { - try { + // Guard clause: pre-hooks vetoed or storage unresolved. + /** @var Storage $storage */ + if (!$run || !$storage) { + return null; + } + + // For mutating operations, upgrade shared lock to exclusive before actual write/delete. + if ($isWrite || $isDelete) { $this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE); - } catch (LockedException $e) { - // If lock upgrade fails, release previously acquired shared lock. - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - throw $e; + $lockState = ILockingProvider::LOCK_EXCLUSIVE; } - } - try { // Delegate operation to storage backend, with optional extra parameter. $result = !is_null($extraParam) ? $storage->$operation($internalPath, $extraParam) : $storage->$operation($internalPath); - } catch (\Exception $e) { - // On backend error, release whichever lock this branch currently owns. - if ($isWrite || $isDelete) { - $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } elseif ($isRead) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - } - throw $e; - } - - // Update delete bookkeeping only on successful delete-like operation. - if ($result !== false && $isDelete) { - $this->removeUpdate($storage, $internalPath); - } - // Update write bookkeeping for successful write-like operations except stream/touch special cases. - if ($result !== false && $isWrite && $operation !== 'fopen' && $operation !== 'touch') { - $isCreateOperation = $operation === 'mkdir' || ($operation === 'file_put_contents' && $isCreateHook); - $sizeDifference = $operation === 'mkdir' ? 0 : $result; - $this->writeUpdate($storage, $internalPath, null, $isCreateOperation ? $sizeDifference : null); - } + // Update delete bookkeeping only on successful delete-like operation. + if ($result !== false && $isDelete) { + $this->removeUpdate($storage, $internalPath); + } - // touch has dedicated bookkeeping behavior. - if ($result !== false && $isTouch) { - $this->writeUpdate($storage, $internalPath, $extraParam, 0); - } + // Update write bookkeeping for successful write-like operations except stream/touch special cases. + if ($result !== false && $isWrite && $operation !== 'fopen' && $operation !== 'touch') { + $isCreateOperation = $operation === 'mkdir' || ($operation === 'file_put_contents' && $isCreateHook); + $sizeDifference = $operation === 'mkdir' ? 0 : $result; + $this->writeUpdate($storage, $internalPath, null, $isCreateOperation ? $sizeDifference : null); + } - // For mutating operations, downgrade the lock from exclusive to shared after the write/delete step. - // Keep it exclusive for successful fopen; it will be unlocked later when the stream closes. - if (($isWrite || $isDelete) && ($operation !== 'fopen' || $result === false)) { - $this->changeLock($path, ILockingProvider::LOCK_SHARED); - } + // touch has dedicated bookkeeping behavior. + if ($result !== false && $isTouch) { + $this->writeUpdate($storage, $internalPath, $extraParam, 0); + } - $unlockLater = false; - if ($this->lockingEnabled && $operation === 'fopen' && is_resource($result)) { - $unlockLater = true; + // For mutating operations, downgrade the lock from exclusive to shared after the write/delete step. + // Keep it exclusive for successful fopen; it will be unlocked later when the stream closes. + if (($isWrite || $isDelete) + && ($operation !== 'fopen' || $result === false) + && $lockState === ILockingProvider::LOCK_EXCLUSIVE + ) { + $this->changeLock($path, ILockingProvider::LOCK_SHARED); + $lockState = ILockingProvider::LOCK_SHARED; + } - // Ensure unlock callback still runs even if client disconnects. - ignore_user_abort(true); + if ($this->lockingEnabled && $operation === 'fopen' && is_resource($result)) { + $unlockLater = true; + // Ensure unlock callback still runs even if client disconnects. + ignore_user_abort(true); - // Defer unlock until stream close. - $result = CallbackWrapper::wrap($result, null, null, function () use ($isWrite, $isRead, $path): void { - if ($isWrite)) { - $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); - } elseif ($isRead) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - } - }); - } + // Defer unlock until stream close. + $result = CallbackWrapper::wrap($result, null, null, function () use ($isWrite, $isRead, $path): void { + if ($isWrite) { + $this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE); + } elseif ($isRead) { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + } + }); + } - // Emit post-hooks on success, except for fopen (stream still open at this point). - if ($this->shouldEmitHooks($path) && $result !== false) && $operation !== 'fopen') { - $this->runHooks($hooks, $path, true); - } + // Emit post-hooks on success, except for fopen (stream still open at this point). + if ($this->shouldEmitHooks($path) && $result !== false && $operation !== 'fopen') { + $this->runHooks($hooks, $path, true); + } + + return $result; + } finally { + // In successful fopen stream path, callback owns unlock responsibility. + if ($unlockLater) { + return; + } - // Normal unlock path when lock ownership was not transferred to stream close callback. - if (!$unlockLater && $needsLock) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + // Normal as well as failsafe unlock path (when lock ownership was not transferred to stream close callback). + if ($lockState !== null) { + $this->unlockFile($path, $lockState); + } } - - return $result; } - + /** * get the path relative to the default root for hook usage * From f1abb9ed38f0f3a13241c3c8fa84f330626bd54e Mon Sep 17 00:00:00 2001 From: Josh Date: Sun, 15 Mar 2026 10:33:17 -0400 Subject: [PATCH 6/6] chore(View): fixup basicOperation Signed-off-by: Josh --- lib/private/Files/View.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 40352fcfca552..a6523fddf6e4e 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1205,7 +1205,7 @@ public function free_space($path = '/') { * @param non-empty-string $operation Storage method name to call dynamically on the resolved Storage. * @param string $path View-relative path. * @param list<'read'|'write'|'delete'|'touch'|'create'|string> $hooks - * Hook tags controlling locking, hook execution, and update behavior. + * Hook tags controlling locking, hook execution, and update behavior. * @param mixed $extraParam Optional second argument forwarded to the storage operation. * * @return mixed Storage operation result. @@ -1321,12 +1321,11 @@ private function basicOperation(string $operation, string $path, array $hooks = if ($this->shouldEmitHooks($path) && $result !== false && $operation !== 'fopen') { $this->runHooks($hooks, $path, true); } - - return $result; + } finally { // In successful fopen stream path, callback owns unlock responsibility. if ($unlockLater) { - return; + return null; } // Normal as well as failsafe unlock path (when lock ownership was not transferred to stream close callback). @@ -1334,6 +1333,8 @@ private function basicOperation(string $operation, string $path, array $hooks = $this->unlockFile($path, $lockState); } } + + return $result; } /**