Do not take X locks in read-only methods of the MFA Setup class

This was a fun one. Apparently even a query locking exactly a single row by
leveraging an UNIQUE KEY outside of a transaction can deadlock with another
transaction.

Sending two requests to regenerate MFA backup codes at the same time caused the
following to happen within the database:

T1: Within a transaction locks the Setup by using Setup::lock(), putting an X
    onto the PRIMARY KEY.
T2: Outside of a transaction reads a Setup using Setup::find(), hitting exactly
    a single entry within the `userID` UNIQUE KEY (i.e. a specific
    `(userID, objectTypeID)` tuple).
    -> This puts an X onto the `userID` UNIQUE KEY.
    -> This wants to also put an X onto the corresponding PRIMARY KEY.
    -> The PRIMARY KEY is already locked by T1.
    -> This query (and thus this transaction) waits within this specific query
       for the lock to be granted.
T1: Within the transaction calls Setup::find() with the same parameters as T2.
    -> This needs to put the X onto the same row in the `userID` UNIQUE KEY.
    -> This row is locked by T2.
    -> This transaction needs to wait for that lock to be granted.

Now T1 needs to wait for T2 which already waits for T1 and we're experiencing a
deadlock.

Fix this issue by not taking an X within Setup::find() (i.e. removing the `FOR
UPDATE`). I've verified that nothing calls Setup::find() within a transaction
(without locking the Setup by a different means). Thus this change does not
result in a difference with regard to lock safety. Everything that needs to
modify a Setup already calls Setup::lock() which locks the PRIMARY KEY only.

Fixes #3874

diff --git a/wcfsetup/install/files/lib/system/user/multifactor/Setup.class.php b/wcfsetup/install/files/lib/system/user/multifactor/Setup.class.php
index 1fd920740c8f01dbea82a67c3722db09340ad83f..e15c42585e82f36440bd8492283a711b88540665 100644 (file)
--- a/wcfsetup/install/files/lib/system/user/multifactor/Setup.class.php
+++ b/wcfsetup/install/files/lib/system/user/multifactor/Setup.class.php
@@ -115,8 +115,7 @@ final class Setup implements IIDObject {
                $sql = "SELECT  *
                        FROM    wcf".WCF_N."_user_multifactor
                        WHERE   userID = ?
-                               AND objectTypeID = ?
-                       FOR UPDATE";
+                               AND objectTypeID = ?";
                $statement = WCF::getDB()->prepareStatement($sql);
                $statement->execute([
                        $user->userID,
@@ -139,8 +138,7 @@ final class Setup implements IIDObject {
        public static function getAllForUser(User $user): array {
                $sql = "SELECT  *
                        FROM    wcf".WCF_N."_user_multifactor
-                       WHERE   userID = ?
-                       FOR UPDATE";
+                       WHERE   userID = ?";
                $statement = WCF::getDB()->prepareStatement($sql);
                $statement->execute([$user->userID]);