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

Description

Author: bugs

Description:
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:
https://bugzilla.wikimedia.org/show_bug.cgi?id=56069

bzimport added a subscriber: Unknown Object (MLST).
bzimport set Reference to bz20595.
bzimport created this task.Via LegacySep 11 2009, 3:21 PM
bzimport added a comment.Via ConduitDec 9 2009, 7:57 PM

ayg wrote:

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

VitaliyFilippov added a comment.Via ConduitMay 25 2012, 10:28 AM

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.

Nemo_bis added a comment.Via ConduitNov 6 2012, 11:42 AM

(In reply to comment #3)

https://gerrit.wikimedia.org/r/8893

Commit was merged: is the bug fixed?

bzimport added a comment.Via ConduitNov 7 2012, 11:33 AM

bugs wrote:

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

Add Comment