1. 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

Discussion in 'Resolved Bug Reports' started by Xon, Aug 9, 2015.

  1. Xon

    Xon Well-Known Member

    Every so often, the following error is throwing:

    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:

    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: Aug 9, 2015
    Liam W likes this.
  2. Xon

    Xon Well-Known Member

    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.
  3. Isil`Zha

    Isil`Zha Active Member

    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.)
  4. Mike

    Mike XenForo Developer Staff Member

    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.

    Xon likes this.

Share This Page