Last modified: 2012-04-16 09:15:35 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T30611, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 28611 - SqlBagOStuff::incr() vulnerable to race conditions, dies with query error
SqlBagOStuff::incr() vulnerable to race conditions, dies with query error
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
General/Unknown (Other open bugs)
unspecified
All All
: Low critical (vote)
: ---
Assigned To: Tim Starling
:
Depends on:
Blocks: 26676
  Show dependency treegraph
 
Reported: 2011-04-19 16:15 UTC by Roan Kattouw
Modified: 2012-04-16 09:15 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Roan Kattouw 2011-04-19 16:15:34 UTC
SqlBagOStuff uses a SELECT FOR UPDATE followed by a DELETE followed by an INSERT. This seems to be deliberate to ensure atomicity, see r55079. However, the following race condition is possible:

* Process 1 calls incr()
* Process 2 calls incr()
* Process 1 does the SELECT FOR UPDATE
* Process 2 does the SELECT FOR UPDATE
* Process 1 DELETEs the row
* Process 2 tries to DELETE the row. The DELETE fails silently (0 rows affected)
* Process 1 INSERTs the new row
* Process 2 tries to INSERT the new row but dies with a duplicate key error

Because incr() bails if the SELECT FOR UPDATE doesn't return a row, this is a somewhat unlikely race condition, but I've seen it happen several times on my test install when hard refreshing, because each load.php instance calls incr( 'requests-with-session' ) or something similar.

A similar bug in SqlBagOStuff::set() used to exist, see bug 24425 and r70203.

(Note that UPDATE with value=value+1 can't be used because the value is a serialized blob rather than a proper integer.)

This is a really nasty bug that'll cause intermittent RL bugs on SQL-cache-based installs, so I'm making this a blocker for the 1.17 release.
Comment 1 Platonides 2011-04-19 18:00:45 UTC
Did it happen to you when using InnoDB? It's wrapped in a transaction. 

The problem is there for MyISAM, though. I commited a fix in r86418 which should fix it.
Comment 2 Max Semenik 2011-04-19 19:18:05 UTC
We could also have a special case for ints and store them not serialized. It will also work faster this way (UPDATE objectcache SET value=value+1 WHERE keyname='foo').
Comment 3 Tim Starling 2011-04-19 23:29:44 UTC
Is there a caller for incr() in a default installation? It used to be that incr() was only called on $wgMemc, and nobody ever sets $wgMainCacheType to CACHE_DB.
Comment 4 Brion Vibber 2011-04-19 23:46:31 UTC
The only callers I can find in trunk core or extensions are using $wgMemc or the explicit calls in mctest.php which is memcache-specific.

A special case with UPDATE probably wouldn't be too hard to do as suggested and might simplify the code a bit -- but then again, it'd need to check for expirations (if the old entry expired, then we should be incrementing/decrementing from 0 as if it were brand new) and needs to return the new value, so would still need to go back and read.
Comment 5 Tim Starling 2011-04-20 02:42:52 UTC
It wouldn't be too hard, no, I was just wondering about the priority and severity.
Comment 6 Mark A. Hershberger 2011-04-20 03:12:28 UTC
I'll defer to your judgment on the priority for possible race conditions.  Please set this one to what you feel is appropriate.
Comment 7 Roan Kattouw 2011-04-20 09:38:23 UTC
(In reply to comment #3)
> Is there a caller for incr() in a default installation? It used to be that
> incr() was only called on $wgMemc, and nobody ever sets $wgMainCacheType to
> CACHE_DB.
I have MainCacheType set to CACHE_DB, maybe that's why. I didn't realize this wasn't the default, and you're right that other caching backends don't have this problem, so I guess that lowers the severity.
Comment 8 Roan Kattouw 2011-04-21 13:55:16 UTC
(In reply to comment #1)
> Did it happen to you when using InnoDB? It's wrapped in a transaction. 
> 
Yes, it seems I have a MyISAM database. No idea why.

> The problem is there for MyISAM, though. I commited a fix in r86418 which
> should fix it.
Yup, that fixes it. Running while(true) $wgMemc->incr('foo'); concurrently in two eval.php instances used to hit the bug immediately but runs fine now.
Comment 9 Max Semenik 2011-04-23 07:16:33 UTC
Filed bug 28669 regading MyISAM in general.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links