Merge "(bug 16020) Fix race condition in User::addToDatabase()"

diff --git a/includes/Status.php b/includes/Status.php
index 10dfb51..147b0df 100644 (file)
--- a/includes/Status.php
+++ b/includes/Status.php
@@ -225,6 +225,15 @@ class Status {
                }
        }
 
+       /**
+        * Get the error message as HTML. This is done by parsing the wikitext error 
+        * message.
+        */
+       public function getHTML( $shortContext = false, $longContext = false ) {
+               $text = $this->getWikiText( $shortContext, $longContext );
+               return MessageCache::singleton()->transform( $text, true );
+       }
+
        /**
         * Return an array with the wikitext for each item in the array.
         * @param $errors Array

diff --git a/includes/User.php b/includes/User.php
index e3d22b0..2afe326 100644 (file)
--- a/includes/User.php
+++ b/includes/User.php
@@ -3008,7 +3008,29 @@ class User {
        }
 
        /**
-        * Add this existing user object to the database
+        * Add this existing user object to the database. If the user already 
+        * exists, a fatal status object is returned, and the user object is 
+        * initialised with the data from the database.
+        *
+        * Previously, this function generated a DB error due to a key conflict
+        * if the user already existed. Many extension callers use this function
+        * in code along the lines of:
+        *
+        *   $user = User::newFromName( $name );
+        *   if ( !$user->isLoggedIn() ) {
+        *       $user->addToDatabase();
+        *   }
+        *   // do something with $user...
+        *
+        * However, this was vulnerable to a race condition (bug 16020). By 
+        * initialising the user object if the user exists, we aim to support this
+        * calling sequence as far as possible.
+        *
+        * Note that if the user exists, this function will acquire a write lock,
+        * so it is still advisable to make the call conditional on isLoggedIn(), 
+        * and to commit the transaction after calling.
+        *
+        * @return Status
         */
        public function addToDatabase() {
                $this->load();
@@ -3031,14 +3053,31 @@ class User {
                                'user_registration' => $dbw->timestamp( $this->mRegistration ),
                                'user_editcount' => 0,
                                'user_touched' => $dbw->timestamp( $this->mTouched ),
-                       ), __METHOD__
+                       ), __METHOD__,
+                       array( 'IGNORE' )
                );
+               if ( !$dbw->affectedRows() ) {
+                       $this->mId = $dbw->selectField( 'user', 'user_id', 
+                               array( 'user_name' => $this->mName ), __METHOD__ );
+                       $loaded = false;
+                       if ( $this->mId ) {
+                               if ( $this->loadFromDatabase() ) {
+                                       $loaded = true;
+                               }
+                       }
+                       if ( !$loaded ) {
+                               throw new MWException( __METHOD__. ": hit a key conflict attempting " .
+                                       "to insert a user row, but then it doesn't exist when we select it!" );
+                       }
+                       return Status::newFatal( 'userexists' );
+               }
                $this->mId = $dbw->insertId();
 
                // Clear instance cache other than user table data, which is already accurate
                $this->clearInstanceCache();
 
                $this->saveOptions();
+               return Status::newGood();
        }
 
        /**

diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php
index 0e0b712..f80e7da 100644 (file)
--- a/includes/specials/SpecialUserlogin.php
+++ b/includes/specials/SpecialUserlogin.php
@@ -443,7 +443,13 @@ class LoginForm extends SpecialPage {
                }
 
                self::clearCreateaccountToken();
-               return $this->initUser( $u, false );
+
+               $status = $this->initUser( $u, false );
+               if ( !$status->isOK() ) {
+                       $this->mainLoginForm( $status->getHTML() );
+                       return false;
+               }
+               return $status->value;
        }
 
        /**
@@ -452,13 +458,16 @@ class LoginForm extends SpecialPage {
         *
         * @param $u User object.
         * @param $autocreate boolean -- true if this is an autocreation via auth plugin
-        * @return User object.
+        * @return Status object, with the User object in the value member on success
         * @private
         */
        function initUser( $u, $autocreate ) {
                global $wgAuth;
 
-               $u->addToDatabase();
+               $status = $u->addToDatabase();
+               if ( !$status->isOK() ) {
+                       return $status;
+               }
 
                if ( $wgAuth->allowPasswordChange() ) {
                        $u->setPassword( $this->mPassword );
@@ -484,7 +493,7 @@ class LoginForm extends SpecialPage {
                # Update user count
                DeferredUpdates::addUpdate( new SiteStatsUpdate( 0, 0, 0, 0, 1 ) );
 
-               return $u;
+               return Status::newGood( $u );
        }
 
        /**
@@ -730,7 +739,14 @@ class LoginForm extends SpecialPage {
                }
 
                wfDebug( __METHOD__ . ": creating account\n" );
-               $this->initUser( $user, true );
+               $status = $this->initUser( $user, true );
+
+               if ( !$status->isOK() ) {
+                       $errors = $status->getErrorsByType( 'error' );
+                       $this->mAbortLoginErrorMsg = $errors[0]['message'];
+                       return self::ABORTED;
+               }
+
                return self::SUCCESS;
        }