From a9f02d402240c6236b94c5711080aa5d7365c8c6 Mon Sep 17 00:00:00 2001
From: Tim Starling <tstarling@wikimedia.org>
Date: Tue, 9 Oct 2012 09:45:03 +1100
Subject: [PATCH] (bug 16020) Fix race condition in User::addToDatabase()

Fix the DB error which comes from User::addToDatabase() if it is called
when the user already exists. This is the most common DB error we log at
WMF in normal operation, perhaps because of double clicks on the "create
account" button, or perhaps due to CentralAuth autocreation when
multiple pages on another wiki are opened in the browser simultaneously,
as the bug reporter suggests.

See the doc comment for the interface rationale. Patched
Special:Userlogin to be aware of the new return value. Most extension
callers will continue to work, I will patch a couple that need it in
subsequent commits.

Change-Id: I1f6ef5e6319bfe692fb82a3fa50dc66c9fde8f15
---
 includes/Status.php                    |  9 ++++++
 includes/User.php                      | 43 ++++++++++++++++++++++++--
 includes/specials/SpecialUserlogin.php | 26 +++++++++++++---
 3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/includes/Status.php b/includes/Status.php
index 10dfb516b8..147b0df6e0 100644
--- 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 4ab90ed399..a786284964 100644
--- 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 b7d01c8114..64bd7231ab 100644
--- a/includes/specials/SpecialUserlogin.php
+++ b/includes/specials/SpecialUserlogin.php
@@ -442,7 +442,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;
 	}
 
 	/**
@@ -451,13 +457,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 );
@@ -483,7 +492,7 @@ class LoginForm extends SpecialPage {
 		# Update user count
 		DeferredUpdates::addUpdate( new SiteStatsUpdate( 0, 0, 0, 0, 1 ) );
 
-		return $u;
+		return Status::newGood( $u );
 	}
 
 	/**
@@ -729,7 +738,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;
 	}
 
-- 
2.20.1