(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 980ad4f..07b19da 100644 (file)
--- 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 f049980..efd357b 100644 (file)
--- 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 e2251b2..a3419e6 100644 (file)
--- 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 386274a..e5e9f6b 100644 (file)
--- 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 51b5ab8..8a61a25 100644 (file)
--- 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 e613725..7c5849d 100644 (file)
--- 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;