From 2b65587e4d92e7f27661e8821b14f74ade939cfa Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Mon, 7 Apr 2025 17:36:35 +1000 Subject: block: Fix DBS::acquireTarget() race using GET_LOCK() A crude solution for the acquireTarget() race condition. Use SQL GET_LOCK() to lock the target from the acquireTarget() call until the transaction is committed. Add FOR UPDATE to the acquireTarget() SELECT, otherwise it just sees the snapshot version of the row and inserts a new row anyway. Add a test which reliably failed prior to the change. Reword the ipb-block-not-found message. This is normal for simultaneous blocks of the same target. Don't contact us. In the API, remap it to "alreadyblocked". Bug: T389028 Change-Id: I1fa35bf08d456a93930194786f77df389217ba61 --- includes/api/ApiMessageTrait.php | 1 + includes/block/DatabaseBlockStore.php | 13 +++++++++++++ languages/i18n/en.json | 2 +- tests/api-testing/action/Block.js | 29 +++++++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 tests/api-testing/action/Block.js diff --git a/includes/api/ApiMessageTrait.php b/includes/api/ApiMessageTrait.php index 8f796101ce2b..9c2bd7575117 100644 --- a/includes/api/ApiMessageTrait.php +++ b/includes/api/ApiMessageTrait.php @@ -67,6 +67,7 @@ trait ApiMessageTrait { 'importuploaderrorpartial' => 'partialupload', 'importuploaderrorsize' => 'filetoobig', 'importuploaderrortemp' => 'notempdir', + 'ipb-block-not-found' => 'alreadyblocked', 'ipb_already_blocked' => 'alreadyblocked', 'ipb_blocked_as_range' => 'blockedasrange', 'ipb_cant_unblock' => 'cantunblock', diff --git a/includes/block/DatabaseBlockStore.php b/includes/block/DatabaseBlockStore.php index c33157b58157..20f662e044d5 100644 --- a/includes/block/DatabaseBlockStore.php +++ b/includes/block/DatabaseBlockStore.php @@ -1044,6 +1044,7 @@ class DatabaseBlockStore { $targetUserName = (string)$target; $targetUserId = $target->getUserIdentity()->getId( $this->wikiId ); $targetConds = [ 'bt_user' => $targetUserId ]; + $targetLockKey = $dbw->getDomainID() . ':block:u:' . $targetUserId; } else { $targetAddress = (string)$target; $targetUserName = null; @@ -1052,6 +1053,8 @@ class DatabaseBlockStore { 'bt_address' => $targetAddress, 'bt_auto' => $isAuto, ]; + $targetLockKey = $dbw->getDomainID() . ':block:' . + ( $isAuto ? 'a' : 'i' ) . ':' . $targetAddress; } $condsWithCount = $targetConds; @@ -1059,6 +1062,15 @@ class DatabaseBlockStore { $condsWithCount['bt_count'] = $expectedTargetCount; } + $dbw->lock( $targetLockKey, __METHOD__ ); + $func = __METHOD__; + $dbw->onTransactionCommitOrIdle( + static function () use ( $dbw, $targetLockKey, $func ) { + $dbw->unlock( $targetLockKey, "$func.closure" ); + }, + __METHOD__ + ); + // This query locks the index gap when the target doesn't exist yet, // so there is a risk of throttling adjacent block insertions, // especially on small wikis which have larger gaps. If this proves to @@ -1076,6 +1088,7 @@ class DatabaseBlockStore { ->select( [ 'bt_id', 'bt_count' ] ) ->from( 'block_target' ) ->where( $targetConds ) + ->forUpdate() ->caller( __METHOD__ ) ->fetchResultSet(); if ( $res->numRows() > 1 ) { diff --git a/languages/i18n/en.json b/languages/i18n/en.json index a2f807404712..2393d99a6656 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -2621,7 +2621,7 @@ "blockipsuccesssub": "Block succeeded", "blockipsuccesstext": "[[Special:Contributions/$1|$1]] has been blocked.
\nSee the [[Special:BlockList|block list]] to review blocks.", "ipb-empty-block": "The block submitted has no restrictions enabled.", - "ipb-block-not-found": "The block could not be made, but no existing block was found for \"$1\". If this problem persists, please [https://www.mediawiki.org/wiki/Special:MyLanguage/Help_talk:Blocking_users report it].", + "ipb-block-not-found": "The block could not be made, probably because another administrator tried to block this user at the same time. Check the block status and try again.", "ipb-blockingself": "You are about to block yourself! Are you sure you want to do that?", "ipb-confirmhideuser": "You are about to block a user with \"hide user\" enabled. This will suppress the user's name in all lists and log entries. Are you sure you want to do that?", "ipb-confirmaction": "If you are sure you really want to do it, please check the \"{{int:ipb-confirm}}\" field at the bottom.", diff --git a/tests/api-testing/action/Block.js b/tests/api-testing/action/Block.js new file mode 100644 index 000000000000..8053137494b5 --- /dev/null +++ b/tests/api-testing/action/Block.js @@ -0,0 +1,29 @@ +'use strict'; + +const { action, assert } = require( 'api-testing' ); + +describe( 'Block', () => { + const ip = '::' + Math.floor( Math.random() * 65534 ).toString( 16 ); + it( 'should not allow multiblocks without newblock (T389028)', async () => { + const mindy = await action.mindy(); + const token = await mindy.token(); + const promises = [ + mindy.request( { action: 'block', user: ip, token: token }, 'POST' ), + mindy.request( { action: 'block', user: ip, token: token }, 'POST' ) + ]; + const res = await Promise.all( promises ); + assert.lengthOf( res, 2 ); + assert.equal( res[ 0 ].status, 200 ); + assert.equal( res[ 1 ].status, 200 ); + + const goodIndex = 'block' in res[ 0 ].body ? 0 : 1; + const goodBody = res[ goodIndex ].body; + const badBody = res[ +!goodIndex ].body; + + assert.property( goodBody, 'block' ); + assert.isOk( goodBody.block.id ); + + assert.property( badBody, 'error' ); + assert.equal( badBody.error.code, 'alreadyblocked' ); + } ); +} ); -- cgit v1.2.3