Last modified: 2013-10-29 18:35:15 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 T22595, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 20595 - Rate limiter blocks never expires when not using memcached
Rate limiter blocks never expires when not using memcached
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
User blocking (Other open bugs)
1.16.x
All All
: Low major (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-11 15:21 UTC by Juliano F. Ravasi
Modified: 2013-10-29 18:35 UTC (History)
4 users (show)

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


Attachments

Description Juliano F. Ravasi 2009-09-11 15:21:35 UTC
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?
Comment 1 Aryeh Gregor (not reading bugmail, please e-mail directly) 2009-12-09 19:57:52 UTC
r57659 fixed one thing mentioned here (relocating the incr() call).
Comment 2 Vitaliy Filippov 2012-05-25 10:28:22 UTC
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.
Comment 3 Vitaliy Filippov 2012-05-25 10:47:47 UTC
https://gerrit.wikimedia.org/r/8893
Comment 4 Nemo 2012-11-06 11:42:27 UTC
(In reply to comment #3)
> https://gerrit.wikimedia.org/r/8893

Commit was merged: is the bug fixed?
Comment 5 Juliano F. Ravasi 2012-11-07 11:33:33 UTC
From a very quick test, it seems to be fixed.

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


Navigation
Links