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:
- 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;
- 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