Won't fix Race conditional allows warnings to be issued to content twice

[Xon]

Affected version

2.1.3

There is a race condition on issuing warnings; where two xf_warning records can be entered for the single piece of content. The overall design assumes one one entry per content will exists, except for the user content type

 

[Kier]

Given that warnings are moderator actions, the chances of a race condition occurring are vanishingly small and not reeeeeally worth handling, unless you've spotted an interaction we haven't considered?

 

[Xon]

It has been ages since I investigated this, I believe the most often way this would be triggered is actually a double-submit while the server is being busy resulting in the two requests running at the same time.

Honestly a flood-check with 1-5 second timer is probably enough to prevent a reoccurrence.

 

[Kier]

Is it really worth bringing up the flood check system for this though?

 

[Kier]

The only way I've been able to reproduce this was to have two open windows ready to submit the warning and sleep(5) in the PHP to allow me time to submit them both. The submitPending check in the JS takes care of the single window case, so I think I'm going mark this one as 'Won't fix' as it's a real edge case and probably not worth the time required to do anything about it.

 

[Xon]

This was triggered when the database VM was having high CPU steal from what I can vaguely remember.

 

[PaulB]

@Kier In theory, this can be trivially resolved by adding the following to the Warning entity:

protected function _saveToSource()
{
   $this->getContent();
   
   return parent::_saveToSource();
}

 

[PaulB]

Actually, that's not quite right. You'll need to get the warning_id from the content entity--assuming it has one--and ensure that it's zero if it's an insert. Calling getContent should ensure that a read lock is taken out on the entity within the transaction.

This could also be resolved by adding a unique constraint on xf_warning for (content_type, content_id). content_type and content_id would need to be made nullable, and warnings associated with users--a bit of a redundant concept--would need to have (null, null) for those columns instead of ('user', user_id). That would be a breaking change, though.