Rate limiter blocks never expires when not using memcached
Closed, ResolvedPublic


Author: bugs

Setting, for example:

$wgRateLimits['edit']['ip'] = array( 10, 600 );

Anonymous users are blocked after 10 hits, but the block never expires, even hours and hours later. This only happens when using any memory cache other than memcached (for example: eAccelerator, Turck MMCache, etc.). Memcached presents the correct expected behavior.

The problem seems to be in the implementation of incr() in BagOStuff class. The increment operation is implemented as:

if( ( $n = $this->get( $key ) ) !== false ) {

		$n += $value;
		$this->set( $key, $n ); // exptime?


Note that when the new value is set, no expire time is passed, and this makes the new replaced key to last forever in the cache. Whoever implemented it already noticed this case and left that comment in the code (it should have also noted that it is a bug).

Also, related: In User::pingLimiter(), the limiter is implemented as:

if( $count ) {

		/* ...code to check the limit... */

} else {

		wfDebug( __METHOD__.": adding record for $key $summary\n" );
		$wgMemc->add( $key, 1, intval( $period ) );

$wgMemc->incr( $key );

The key is added with the correct expire time, but right after that it is incremented using incr(), which kills the expire time. In fact, in this code, if $count==0, the user starts with 2 hits instead of one (another bug). Probably the incr() call should be inside the 'if' clause.

What would be the correct fix for this bug? I think of two possibilities:

  1. Modify BagOStuff to store an aggregate like array( $value, $exptime ) in order to be able to have access to the original exptime and properly reset it when incrementing/decrementing the value;
  2. Or deprecate incr() and decr(), consider that they can't be reliably implemented out of memcached and reformat User::pingLimiter() in order to do its own expiry control, independent of the cache TTL?

Version: 1.16.x
Severity: major
See Also:


bzimport set Reference to bz20595.
bzimport added a subscriber: Unknown Object (MLST).
bzimport created this task.Sep 11 2009, 3:21 PM

ayg wrote:

r57659 fixed one thing mentioned here (relocating the incr() call).

There is another example - 5 minute limit for "too many incorrect password attempts" never expires!

The problem is that default BagOStuff::incr() implementation doesn't preserve TTL of cache keys.
SQL and memcache are OK, as they are implementing incr() themselves.
It's easy to fix for APC and XCache as there are underlying apc_inc() and xcache_inc() methods.
eAccelerator didn't have increment method in previous versions... And now it doesn't have variable cache at all :-D also it doesn't work with PHP 5.4, so I think it doesn't matter if it will work.
So, only DBA and Ehcache are left.
DBA stores TTL with the key, so it's also easy to fix incr() for it.
Don't know what Ehcache and its API is slightly more complex to check for increment quickly.

I'll probably make and push the fix for APC, XCache and DBA to gerrit.

(In reply to comment #3)


Commit was merged: is the bug fixed?

bugs wrote:

From a very quick test, it seems to be fixed.

Add Comment