Skip to content

fix(View): safer lock life-cycle handling in basicOperation() + clearer control flow #58947

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

Draft
joshtrichards wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

fix(View): safer lock life-cycle handling in basicOperation() + clearer control flow #58947

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be8efa7
refactor(View): add some useful comments to basicOperation
joshtrichards Mar 13, 2026
c0ed1ab
refactor(View): extract intent into booleans for readability
joshtrichards Mar 13, 2026
35a7cfd
refactor(View): use early returns to reduce nesting
joshtrichards Mar 13, 2026
460c1fb
docs(View): update basicOperation docblock
joshtrichards Mar 13, 2026
6009179
fix(View) avoid unconditional unlock when no lock was acquired
joshtrichards Mar 14, 2026
f1abb9e
chore(View): fixup basicOperation
joshtrichards Mar 15, 2026
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
209 changes: 126 additions & 83 deletions lib/private/Files/View.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1191,109 +1191,152 @@ 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.
*
* 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
* @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)
*
* @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) === '/') ? '/' : '';

// Build and normalize absolute path from the provided relative path.
$absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path));
if (Filesystem::isValidPath($path)
&& !Filesystem::isFileBlacklisted($path)
) {
$path = $this->getRelativePath($absolutePath);
if ($path === null) {
return false;
}

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
// 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);
$isRead = in_array('read', $hooks, true);
$isTouch = in_array('touch', $hooks, true);
$isCreateHook = in_array('create', $hooks, true);
$needsLock = $isWrite || $isDelete || $isRead;

// Convert back (post-normalized) to relative path in the current view context.
$path = $this->getRelativePath($absolutePath);
if ($path === null) {
return false;
}

/** @var null|int $lockState null means no lock currently held */
$lockState = null;
$unlockLater = false; // true only for successful fopen stream wrapper 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;
}

// 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);
if ($run && $storage) {
/** @var Storage $storage */
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
$this->unlockFile($path, ILockingProvider::LOCK_SHARED);
throw $e;
}
}
try {
if (!is_null($extraParam)) {
$result = $storage->$operation($internalPath, $extraParam);
} else {
$result = $storage->$operation($internalPath);
}
} catch (\Exception $e) {
if (in_array('write', $hooks) || in_array('delete', $hooks)) {
$this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE);
} elseif (in_array('read', $hooks)) {
$this->unlockFile($path, ILockingProvider::LOCK_SHARED);
}
throw $e;
}

if ($result !== false && in_array('delete', $hooks)) {
$this->removeUpdate($storage, $internalPath);
}
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);
}
if ($result !== false && in_array('touch', $hooks)) {
$this->writeUpdate($storage, $internalPath, $extraParam, 0);
}
// Guard clause: pre-hooks vetoed or storage unresolved.
/** @var Storage $storage */
if (!$run || !$storage) {
return null;
}

if ((in_array('write', $hooks) || in_array('delete', $hooks)) && ($operation !== 'fopen' || $result === false)) {
$this->changeLock($path, ILockingProvider::LOCK_SHARED);
}
// For mutating operations, upgrade shared lock to exclusive before actual write/delete.
if ($isWrite || $isDelete) {
$this->changeLock($path, ILockingProvider::LOCK_EXCLUSIVE);
$lockState = ILockingProvider::LOCK_EXCLUSIVE;
}

$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
ignore_user_abort(true);
$result = CallbackWrapper::wrap($result, null, null, function () use ($hooks, $path): void {
if (in_array('write', $hooks)) {
$this->unlockFile($path, ILockingProvider::LOCK_EXCLUSIVE);
} elseif (in_array('read', $hooks)) {
$this->unlockFile($path, ILockingProvider::LOCK_SHARED);
}
});
}
// Delegate operation to storage backend, with optional extra parameter.
$result = !is_null($extraParam)
? $storage->$operation($internalPath, $extraParam)
: $storage->$operation($internalPath);

// 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);
}

if ($this->shouldEmitHooks($path) && $result !== false) {
if ($operation !== 'fopen') { //no post hooks for fopen, the file stream is still open
$this->runHooks($hooks, $path, true);
// touch has dedicated bookkeeping behavior.
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 (($isWrite || $isDelete)
&& ($operation !== 'fopen' || $result === false)
&& $lockState === ILockingProvider::LOCK_EXCLUSIVE
) {
$this->changeLock($path, ILockingProvider::LOCK_SHARED);
$lockState = ILockingProvider::LOCK_SHARED;
}

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);
}
}
});
}

if (!$unlockLater
&& (in_array('write', $hooks) || in_array('delete', $hooks) || in_array('read', $hooks))
) {
$this->unlockFile($path, ILockingProvider::LOCK_SHARED);
}
return $result;
} else {
$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);
}

} finally {
// In successful fopen stream path, callback owns unlock responsibility.
if ($unlockLater) {
return null;
}

// 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 null;
}

return $result;
}

/**
* get the path relative to the default root for hook usage
*
Expand Down
Loading