From: Bryan Tong Minh Date: Fri, 4 Feb 2011 20:54:11 +0000 (+0000) Subject: (bug 19751) Filesystem is now checked during image undeletion X-Git-Tag: 1.31.0-rc.0~32201 X-Git-Url: https://git.cyclocoop.org/admin/?a=commitdiff_plain;h=4ffb09ba4934873a711bf979d3526a953e03b558;p=lhc%2Fweb%2Fwiklou.git (bug 19751) Filesystem is now checked during image undeletion * FSRepo::storeBatch() now does an sha1 check unless SKIP_VALIDATION flag is set * Introduced Status::$success in addition to Status::$successcount ** FSRepo::storeBatch() now logs success/failure in this variable * LocalFileRestoreBatch now aborts on failure in FSRepo::storeBatch() and cleans up the already copied files ** Introduced FSRepo::cleanupBatch() for this purpose * SpecialUndelete now aborts if LocalFile::restore() gives a fatal --- diff --git a/RELEASE-NOTES b/RELEASE-NOTES index 980ad4f89b..07b19da36d 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -112,6 +112,7 @@ it from source control: http://www.mediawiki.org/wiki/Download_from_SVN * (bug 26809) Uploading files with multiple extensions where one of the extensions is blacklisted now gives the proper extension in the error message. * (bug 26961) Hide anon edits in watchlist preference now actually works. +* (bug 19751) Filesystem is now checked during image undeletion === API changes in 1.18 === * (bug 26339) Throw warning when truncating an overlarge API result diff --git a/includes/Status.php b/includes/Status.php index f049980f64..efd357b583 100644 --- a/includes/Status.php +++ b/includes/Status.php @@ -17,7 +17,9 @@ class Status { var $value; /** Counters for batch operations */ - var $successCount = 0, $failCount = 0; + public $successCount = 0, $failCount = 0; + /** Array to indicate which items of the batch operations failed */ + public $success = array(); /*semi-private*/ var $errors = array(); /*semi-private*/ var $cleanCallback = false; diff --git a/includes/filerepo/FSRepo.php b/includes/filerepo/FSRepo.php index e2251b2bac..a3419e6a15 100644 --- a/includes/filerepo/FSRepo.php +++ b/includes/filerepo/FSRepo.php @@ -146,16 +146,23 @@ class FSRepo extends FileRepo { * same contents as the source */ function storeBatch( $triplets, $flags = 0 ) { + wfDebug( __METHOD__ . ': Storing ' . count( $triplets ) . + " triplets; flags: {$flags}\n" ); + + // Try creating directories if ( !wfMkdirParents( $this->directory ) ) { return $this->newFatal( 'upload_directory_missing', $this->directory ); } if ( !is_writable( $this->directory ) ) { return $this->newFatal( 'upload_directory_read_only', $this->directory ); } + + // Validate each triplet $status = $this->newGood(); foreach ( $triplets as $i => $triplet ) { list( $srcPath, $dstZone, $dstRel ) = $triplet; + // Resolve destination path $root = $this->getZonePath( $dstZone ); if ( !$root ) { throw new MWException( "Invalid zone: $dstZone" ); @@ -166,6 +173,7 @@ class FSRepo extends FileRepo { $dstPath = "$root/$dstRel"; $dstDir = dirname( $dstPath ); + // Create destination directories for this triplet if ( !is_dir( $dstDir ) ) { if ( !wfMkdirParents( $dstDir ) ) { return $this->newFatal( 'directorycreateerror', $dstDir ); @@ -175,6 +183,7 @@ class FSRepo extends FileRepo { } } + // Resolve source if ( self::isVirtualUrl( $srcPath ) ) { $srcPath = $triplets[$i][0] = $this->resolveVirtualUrl( $srcPath ); } @@ -183,6 +192,8 @@ class FSRepo extends FileRepo { $status->fatal( 'filenotfound', $srcPath ); continue; } + + // Check overwriting if ( !( $flags & self::OVERWRITE ) && file_exists( $dstPath ) ) { if ( $flags & self::OVERWRITE_SAME ) { $hashSource = sha1_file( $srcPath ); @@ -196,6 +207,7 @@ class FSRepo extends FileRepo { } } + // Windows does not support moving over existing files, so explicitly delete them $deleteDest = wfIsWindows() && ( $flags & self::OVERWRITE ); // Abort now on failure @@ -203,7 +215,8 @@ class FSRepo extends FileRepo { return $status; } - foreach ( $triplets as $triplet ) { + // Execute the store operation for each triplet + foreach ( $triplets as $i => $triplet ) { list( $srcPath, $dstZone, $dstRel ) = $triplet; $root = $this->getZonePath( $dstZone ); $dstPath = "$root/$dstRel"; @@ -222,6 +235,20 @@ class FSRepo extends FileRepo { $status->error( 'filecopyerror', $srcPath, $dstPath ); $good = false; } + if ( !( $flags & self::SKIP_VALIDATION ) ) { + wfSuppressWarnings(); + $hashSource = sha1_file( $srcPath ); + $hashDest = sha1_file( $dstPath ); + wfRestoreWarnings(); + + if ( $hashDest === false || $hashSource !== $hashDest ) { + wfDebug( __METHOD__ . ': File copy validation failed: ' . + "$srcPath ($hashSource) to $dstPath ($hashDest)\n" ); + + $status->error( 'filecopyerror', $srcPath, $dstPath ); + $good = false; + } + } } if ( $good ) { $this->chmod( $dstPath ); @@ -229,9 +256,28 @@ class FSRepo extends FileRepo { } else { $status->failCount++; } + $status->success[$i] = $good; } return $status; } + + /** + * Deletes a batch of (zone, rel) pairs. It will try to delete each pair, + * but ignores any errors doing so. + * + * @param $pairs array Pair of (zone, rel) pairs to delete + */ + function cleanupBatch( $pairs ) { + foreach ( $pairs as $i => $pair ) { + list( $zone, $rel ) = $pair; + $root = $this->getZonePath( $zone ); + $path = "$root/$rel"; + + wfSuppressWarnings(); + unlink( $path ); + wfRestoreWarnings(); + } + } function append( $srcPath, $toAppendPath, $flags = 0 ) { $status = $this->newGood(); diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php index 386274acb2..e5e9f6b51c 100644 --- a/includes/filerepo/FileRepo.php +++ b/includes/filerepo/FileRepo.php @@ -17,6 +17,7 @@ abstract class FileRepo { const DELETE_SOURCE = 1; const OVERWRITE = 2; const OVERWRITE_SAME = 4; + const SKIP_VALIDATION = 8; var $thumbScriptUrl, $transformVia404; var $descBaseUrl, $scriptDirUrl, $scriptExtension, $articleUrl; diff --git a/includes/filerepo/LocalFile.php b/includes/filerepo/LocalFile.php index 51b5ab8a09..8a61a25d91 100644 --- a/includes/filerepo/LocalFile.php +++ b/includes/filerepo/LocalFile.php @@ -1197,7 +1197,7 @@ class LocalFile extends File { $status = $batch->execute(); - if ( !$status->ok ) { + if ( !$status->isGood() ) { return $status; } @@ -1807,9 +1807,11 @@ class LocalFileRestoreBatch { $storeStatus = $this->file->repo->storeBatch( $storeBatch, FileRepo::OVERWRITE_SAME ); $status->merge( $storeStatus ); - if ( !$status->ok ) { - // Store batch returned a critical error -- this usually means nothing was stored - // Stop now and return an error + if ( !$status->isGood() ) { + // Even if some files could be copied, fail entirely as that is the + // easiest thing to do without data loss + $this->cleanupFailedBatch( $storeStatus, $storeBatch ); + $status->ok = false; $this->file->unlock(); return $status; @@ -1914,6 +1916,17 @@ class LocalFileRestoreBatch { return $status; } + + function cleanupFailedBatch( $storeStatus, $storeBatch ) { + $cleanupBatch = array(); + + foreach ( $storeStatus->success as $i => $success ) { + if ( $success ) { + $cleanupBatch[] = array( $storeBatch[$i][1], $storeBatch[$i][1] ); + } + } + $this->file->repo->cleanupBatch( $cleanupBatch ); + } } # ------------------------------------------------------------------------------ diff --git a/includes/specials/SpecialUndelete.php b/includes/specials/SpecialUndelete.php index e6137251b9..7c5849d94d 100644 --- a/includes/specials/SpecialUndelete.php +++ b/includes/specials/SpecialUndelete.php @@ -336,6 +336,9 @@ class PageArchive { if( $restoreFiles && $this->title->getNamespace() == NS_FILE ) { $img = wfLocalFile( $this->title ); $this->fileStatus = $img->restore( $fileVersions, $unsuppress ); + if ( !$this->fileStatus->isOk() ) { + return false; + } $filesRestored = $this->fileStatus->successCount; } else { $filesRestored = 0;