From e2e0c0d9b7cc2249935dbc01157a437f8612b80c Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 19 Jan 2012 23:18:03 +0000 Subject: [PATCH] * Follow-up r109009: Check that paths are usable in FileOp::doPrecheck(). Also lock parent directories to avoid prepare()/clean() race conditions for FS backends. * Fixed bogus $params var in logException() call in SwiftFileBackend. * Added 'latest' param to FileBackendMultliWrite::consistencyCheck(). * Dummy-proof FileBackend::getFileStat() w.r.t the 'latest' param and removed related FileOp::allowStaleReads() comment. * Tweaked backend-fail-batchsize message from r109469. --- includes/filerepo/backend/FSFileBackend.php | 21 +++++ includes/filerepo/backend/FileBackend.php | 31 ++++++- .../backend/FileBackendMultiWrite.php | 5 +- includes/filerepo/backend/FileOp.php | 85 ++++++++++--------- .../filerepo/backend/SwiftFileBackend.php | 23 ++++- 5 files changed, 118 insertions(+), 47 deletions(-) diff --git a/includes/filerepo/backend/FSFileBackend.php b/includes/filerepo/backend/FSFileBackend.php index dc81c48f1c..8aaba700a2 100644 --- a/includes/filerepo/backend/FSFileBackend.php +++ b/includes/filerepo/backend/FSFileBackend.php @@ -101,6 +101,27 @@ class FSFileBackend extends FileBackend { return $fsPath; } + /** + * @see FileBackend::isPathUsableInternal() + */ + public function isPathUsableInternal( $storagePath ) { + $fsPath = $this->resolveToFSPath( $storagePath ); + if ( $fsPath === null ) { + return false; // invalid + } + $parentDir = dirname( $fsPath ); + + wfSuppressWarnings(); + if ( file_exists( $fsPath ) ) { + $ok = is_file( $fsPath ) && is_writable( $fsPath ); + } else { + $ok = is_dir( $parentDir ) && is_writable( $parentDir ); + } + wfRestoreWarnings(); + + return $ok; + } + /** * @see FileBackend::doStoreInternal() */ diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php index 636b054518..baa18c1485 100644 --- a/includes/filerepo/backend/FileBackend.php +++ b/includes/filerepo/backend/FileBackend.php @@ -419,6 +419,7 @@ abstract class FileBackendBase { * Otherwise, the result is an associative array that includes: * mtime : the last-modified timestamp (TS_MW) * size : the file size (bytes) + * Additional values may be included for internal use only. * * $params include: * src : source storage path @@ -577,11 +578,11 @@ abstract class FileBackendBase { * This class defines the methods as abstract that subclasses must implement. * Callers outside of FileBackend and its helper classes, such as FileOp, * should only call functions that are present in FileBackendBase. - * + * * The FileBackendBase operations are implemented using primitive functions * such as storeInternal(), copyInternal(), deleteInternal() and the like. * This class is also responsible for path resolution and sanitization. - * + * * @ingroup FileBackend * @since 1.19 */ @@ -605,6 +606,16 @@ abstract class FileBackend extends FileBackendBase { return $this->maxFileSize; } + /** + * Check if a file can be created at a given storage path. + * FS backends should check if the parent directory exists and the file is writable. + * Backends using key/value stores should check if the container exists. + * + * @param $storagePath string + * @return bool + */ + abstract public function isPathUsableInternal( $storagePath ); + /** * Create a file in the backend with the given contents. * Do not call this function from places outside FileBackend and FileOp. @@ -878,6 +889,12 @@ abstract class FileBackend extends FileBackendBase { $status->fatal( 'backend-fail-invalidpath', $params['dir'] ); return $status; // invalid storage path } + // Attempt to lock this directory... + $filesLockEx = array( $params['dir'] ); + $scopedLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status ); + if ( !$status->isOK() ) { + return $status; // abort + } if ( $shard !== null ) { // confined to a single container/shard $status->merge( $this->doCleanInternal( $fullCont, $dir, $params ) ); } else { // directory is on several shards @@ -937,13 +954,19 @@ abstract class FileBackend extends FileBackendBase { */ final public function getFileStat( array $params ) { $path = $params['src']; + $latest = !empty( $params['latest'] ); if ( isset( $this->cache[$path]['stat'] ) ) { - return $this->cache[$path]['stat']; + // If we want the latest data, check that this cached + // value was in fact fetched with the latest available data. + if ( !$latest || $this->cache[$path]['stat']['latest'] ) { + return $this->cache[$path]['stat']; + } } $stat = $this->doGetFileStat( $params ); if ( is_array( $stat ) ) { // don't cache negatives $this->trimCache(); // limit memory $this->cache[$path]['stat'] = $stat; + $this->cache[$path]['stat']['latest'] = $latest; } return $stat; } @@ -1161,6 +1184,8 @@ abstract class FileBackend extends FileBackendBase { } // Optimization: if doing an EX lock anyway, don't also set an SH one $filesLockSh = array_diff( $filesLockSh, $filesLockEx ); + // Get a shared lock on the parent directory of each path changed + $filesLockSh = array_merge( $filesLockSh, array_map( 'dirname', $filesLockEx ) ); // Try to lock those files for the scope of this function... $scopeLockS = $this->getScopedFileLocks( $filesLockSh, LockManager::LOCK_UW, $status ); $scopeLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status ); diff --git a/includes/filerepo/backend/FileBackendMultiWrite.php b/includes/filerepo/backend/FileBackendMultiWrite.php index 8685774215..44a07bef6a 100644 --- a/includes/filerepo/backend/FileBackendMultiWrite.php +++ b/includes/filerepo/backend/FileBackendMultiWrite.php @@ -96,6 +96,9 @@ class FileBackendMultiWrite extends FileBackendBase { if ( empty( $opts['nonLocking'] ) ) { $filesLockSh = array_diff( $filesRead, $filesChanged ); // optimization $filesLockEx = $filesChanged; + // Get a shared lock on the parent directory of each path changed + $filesLockSh = array_merge( $filesLockSh, array_map( 'dirname', $filesLockEx ) ); + // Try to lock those files for the scope of this function... $scopeLockS = $this->getScopedFileLocks( $filesLockSh, LockManager::LOCK_UW, $status ); $scopeLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status ); if ( !$status->isOK() ) { @@ -156,7 +159,7 @@ class FileBackendMultiWrite extends FileBackendBase { $mBackend = $this->backends[$this->masterIndex]; foreach ( array_unique( $paths ) as $path ) { - $params = array( 'src' => $path ); + $params = array( 'src' => $path, 'latest' => true ); // Stat the file on the 'master' backend $mStat = $mBackend->getFileStat( $this->substOpPaths( $params, $mBackend ) ); // Check of all clone backends agree with the master... diff --git a/includes/filerepo/backend/FileOp.php b/includes/filerepo/backend/FileOp.php index 86f2a942f0..c325190ef5 100644 --- a/includes/filerepo/backend/FileOp.php +++ b/includes/filerepo/backend/FileOp.php @@ -55,11 +55,7 @@ abstract class FileOp { } /** - * Allow stale data for file reads and existence checks. - * - * Note that we don't want to mix stale and non-stale reads - * because stat calls are cached: if we read X without 'latest' - * and then read it with 'latest', the data may still be stale. + * Allow stale data for file reads and existence checks * * @return void */ @@ -72,11 +68,11 @@ abstract class FileOp { * Callers are responsible for handling file locking. * * $opts is an array of options, including: - * 'force' : Errors that would normally cause a rollback do not. - * The remaining operations are still attempted if any fail. - * 'allowStale' : Don't require the latest available data. - * This can increase performance for non-critical writes. - * This has no effect unless the 'force' flag is set. + * 'force' : Errors that would normally cause a rollback do not. + * The remaining operations are still attempted if any fail. + * 'allowStale' : Don't require the latest available data. + * This can increase performance for non-critical writes. + * This has no effect unless the 'force' flag is set. * * @param $performOps Array List of FileOp operations * @param $opts Array Batch operation options @@ -155,7 +151,7 @@ abstract class FileOp { /** * Check if this operation failed precheck() or attempt() * - * @return type + * @return bool */ final public function failed() { return $this->failed; @@ -396,20 +392,22 @@ class StoreFileOp extends FileOp { if ( !is_file( $this->params['src'] ) ) { $status->fatal( 'backend-fail-notexists', $this->params['src'] ); return $status; - } // Check if the source file is too big - if ( filesize( $this->params['src'] ) > $this->backend->maxFileSizeInternal() ) { - $status->fatal( 'backend-fail-store', $this->params['dst'] ); + } elseif ( filesize( $this->params['src'] ) > $this->backend->maxFileSizeInternal() ) { + $status->fatal( 'backend-fail-store', $this->params['src'], $this->params['dst'] ); + return $status; + // Check if a file can be placed at the destination + } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { + $status->fatal( 'backend-fail-store', $this->params['src'], $this->params['dst'] ); return $status; } // Check if destination file exists $status->merge( $this->precheckDestExistence( $predicates ) ); - if ( !$status->isOK() ) { - return $status; + if ( $status->isOK() ) { + // Update file existence predicates + $predicates['exists'][$this->params['dst']] = true; + $predicates['sha1'][$this->params['dst']] = $this->sourceSha1; } - // Update file existence predicates - $predicates['exists'][$this->params['dst']] = true; - $predicates['sha1'][$this->params['dst']] = $this->sourceSha1; return $status; // safe to call attempt() } @@ -456,15 +454,18 @@ class CreateFileOp extends FileOp { if ( strlen( $this->params['content'] ) > $this->backend->maxFileSizeInternal() ) { $status->fatal( 'backend-fail-create', $this->params['dst'] ); return $status; + // Check if a file can be placed at the destination + } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { + $status->fatal( 'backend-fail-create', $this->params['dst'] ); + return $status; } // Check if destination file exists $status->merge( $this->precheckDestExistence( $predicates ) ); - if ( !$status->isOK() ) { - return $status; + if ( $status->isOK() ) { + // Update file existence predicates + $predicates['exists'][$this->params['dst']] = true; + $predicates['sha1'][$this->params['dst']] = $this->sourceSha1; } - // Update file existence predicates - $predicates['exists'][$this->params['dst']] = true; - $predicates['sha1'][$this->params['dst']] = $this->sourceSha1; return $status; // safe to call attempt() } @@ -505,15 +506,18 @@ class CopyFileOp extends FileOp { if ( !$this->fileExists( $this->params['src'], $predicates ) ) { $status->fatal( 'backend-fail-notexists', $this->params['src'] ); return $status; + // Check if a file can be placed at the destination + } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { + $status->fatal( 'backend-fail-copy', $this->params['src'], $this->params['dst'] ); + return $status; } // Check if destination file exists $status->merge( $this->precheckDestExistence( $predicates ) ); - if ( !$status->isOK() ) { - return $status; + if ( $status->isOK() ) { + // Update file existence predicates + $predicates['exists'][$this->params['dst']] = true; + $predicates['sha1'][$this->params['dst']] = $this->sourceSha1; } - // Update file existence predicates - $predicates['exists'][$this->params['dst']] = true; - $predicates['sha1'][$this->params['dst']] = $this->sourceSha1; return $status; // safe to call attempt() } @@ -557,17 +561,20 @@ class MoveFileOp extends FileOp { if ( !$this->fileExists( $this->params['src'], $predicates ) ) { $status->fatal( 'backend-fail-notexists', $this->params['src'] ); return $status; + // Check if a file can be placed at the destination + } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) { + $status->fatal( 'backend-fail-move', $this->params['src'], $this->params['dst'] ); + return $status; } // Check if destination file exists $status->merge( $this->precheckDestExistence( $predicates ) ); - if ( !$status->isOK() ) { - return $status; + if ( $status->isOK() ) { + // Update file existence predicates + $predicates['exists'][$this->params['src']] = false; + $predicates['sha1'][$this->params['src']] = false; + $predicates['exists'][$this->params['dst']] = true; + $predicates['sha1'][$this->params['dst']] = $this->sourceSha1; } - // Update file existence predicates - $predicates['exists'][$this->params['src']] = false; - $predicates['sha1'][$this->params['src']] = false; - $predicates['exists'][$this->params['dst']] = true; - $predicates['sha1'][$this->params['dst']] = $this->sourceSha1; return $status; // safe to call attempt() } @@ -582,9 +589,6 @@ class MoveFileOp extends FileOp { // Just delete source as the destination needs no changes $params = array( 'src' => $this->params['src'] ); $status->merge( $this->backend->deleteInternal( $params ) ); - if ( !$status->isOK() ) { - return $status; - } } } return $status; @@ -633,9 +637,6 @@ class DeleteFileOp extends FileOp { if ( $this->needsDelete ) { // Delete the source file $status->merge( $this->backend->deleteInternal( $this->params ) ); - if ( !$status->isOK() ) { - return $status; - } } return $status; } diff --git a/includes/filerepo/backend/SwiftFileBackend.php b/includes/filerepo/backend/SwiftFileBackend.php index a2d9a38e64..29c0fa8c62 100644 --- a/includes/filerepo/backend/SwiftFileBackend.php +++ b/includes/filerepo/backend/SwiftFileBackend.php @@ -72,6 +72,27 @@ class SwiftFileBackend extends FileBackend { return $relStoragePath; } + /** + * @see FileBackend::isPathUsableInternal() + */ + public function isPathUsableInternal( $storagePath ) { + list( $container, $rel ) = $this->resolveStoragePathReal( $storagePath ); + if ( $rel === null ) { + return false; // invalid + } + + try { + $this->getContainer( $container ); + return true; // container exists + } catch ( NoSuchContainerException $e ) { + } catch ( InvalidResponseException $e ) { + } catch ( Exception $e ) { // some other exception? + $this->logException( $e, __METHOD__, array( 'path' => $storagePath ) ); + } + + return false; + } + /** * @see FileBackend::doCopyInternal() */ @@ -492,7 +513,7 @@ class SwiftFileBackend extends FileBackend { } catch ( NoSuchObjectException $e ) { } catch ( InvalidResponseException $e ) { } catch ( Exception $e ) { // some other exception? - $this->logException( $e, __METHOD__, $params ); + $this->logException( $e, __METHOD__, array( 'cont' => $fullCont, 'dir' => $dir ) ); } return $files; -- 2.20.1