• This site uses cookies. By continuing to use this site, you are agreeing to our use of cookies. Learn more.

Fixed modifyThreadUserPostCount is vulnerable to a race condition

Xon

Well-known member
#1
Every so often, the following error is throwing:

Code:
Zend_Db_Statement_Mysqli_Exception: Mysqli statement execute error : Duplicate entry '351160-712' for key 'PRIMARY' - library/Zend/Db/Statement/Mysqli.php:214
#0 /var/www/html/library/Zend/Db/Statement.php(297): Zend_Db_Statement_Mysqli->_execute(Array)
#1 /var/www/html/library/Zend/Db/Adapter/Abstract.php(479): Zend_Db_Statement->execute(Array)
#2 /var/www/html/library/Zend/Db/Adapter/Abstract.php(574): Zend_Db_Adapter_Abstract->query('INSERT INTO `xf...', Array)
#3 /var/www/html/library/XenForo/Model/Thread.php(2053): Zend_Db_Adapter_Abstract->insert('xf_thread_user_...', Array)
#4 /var/www/html/library/XenForo/DataWriter/DiscussionMessage/Post.php(105): XenForo_Model_Thread->modifyThreadUserPostCount(351160, 712, 1)
...
modifyThreadUserPostCount does a select then insert or update. While it runs in a transaction, if there is nothing to select, then the row isn't going to be enrolled into the transaction to that row being blocked. The default transaction level does not block new rows! (SERIALIZABLE isn't usable in most MySQL clustered solutions).

Most of the contents of modifyThreadUserPostCount can be replaced with just:

Code:
$db->query('
insert into xf_thread_user_post (thread_id, user_id, post_count) values
(?, ?, ?)
on duplicate key update  post_count =  post_count + VALUES(post_count)
', array($threadId, $userId, $modifyValue));
Alternatively, updating the existing code so if the insert fails, an update occurs.
 
Last edited:

Xon

Well-known member
#2
I'm fairly sure this is related to an admin on SB (*cough* @Isil`Zha *cough*) who has flood protection disabled, and double-clicking reply to a post or something.
 

Isil`Zha

Active member
#3
hahaha, yeah I did double-click a post accidentally yesterday (and it displayed the thread as though it were in mobile responsive mode on my desktop... and double clicking is exactly what I attributed it to.)
 

Mike

XenForo developer
Staff member
#4
I'm not sure why I wrote the method like this. I think it mostly revolves around returning the new value, which we don't actually use, but since I can't remove that, I'm mostly going to keep it as is and just change to on duplicate key update, though that does mean on this race condition case it could return a slightly incorrect number.

Thanks.