Bug #62864 for Bot-BasicBot-Pluggable: Flood control broken - kicks even if messages are a long way apart

Tue Nov 09 05:00:31 2010 davidp [...] preshweb.co.uk - Ticket created

Subject:

Flood control broken - kicks even if messages are a long way apart

Looks like the anti-flood kicking code is a little broken: * It never forgets timestamps, so a long-running bot on a busy channel will slowly use more and more memory * More importantly, it considers all messages when looking to see if you've sent too many, rather than just messages less than user_flood_seconds ago, leading to things like: 09:45 -!- bigpresh was kicked from #channel by sophie [Stop flooding the channel (7 messages in 35033 seconds).] (That was with the default settings - user_flood_seconds=4 and user_flood_messages=6) I'm happy to supply a patch to fix this if you'll accept it?

Tue Nov 09 14:26:02 2010 diz [...] cpan.org - Correspondence added

On Tue Nov 09 05:00:31 2010, BIGPRESH wrote: Show quoted text

> Looks like the anti-flood kicking code is a little broken: > > * It never forgets timestamps, so a long-running bot on a busy channel > will slowly use more and more memory > > * More importantly, it considers all messages when looking to see if > you've sent too many, rather than just messages less than > user_flood_seconds ago, leading to things like: > 09:45 -!- bigpresh was kicked from #channel by sophie [Stop flooding

the Show quoted text

> channel (7 messages in 35033 seconds).] (That was with the default > settings - user_flood_seconds=4 and user_flood_messages=6) > > I'm happy to supply a patch to fix this if you'll accept it?

patches welcome. patches with tests are even better. :) -mike

Tue Nov 09 14:26:02 2010 The RT System itself - Status changed from 'new' to 'open'

Fri Nov 19 19:51:48 2010 davidp [...] preshweb.co.uk - Correspondence added

On Tue Nov 09 14:26:02 2010, DIZ wrote: Show quoted text

> On Tue Nov 09 05:00:31 2010, BIGPRESH wrote:

> > Looks like the anti-flood kicking code is a little broken:

[...] Show quoted text

> > > > I'm happy to supply a patch to fix this if you'll accept it?

> > patches welcome. patches with tests are even better. :)

Right, finally got a chance to do this. A patch is attached which fixes the behaviour of the flood protection in B::B::P::Module::ChanOp. I don't see any current unit tests which exercise ::ChanOp, and I can't think of a good way to test it effectively either, so no test added. I have tested its behaviour though, and it now works as you'd expect it to. I have a few other improvements I'd like to contribute too; is Bot::BasicBot::Pluggable still developed on GitHub? I have a fork of Mario's repo, and contributed some changes before by sending pull requests, but you appear to be a new maintainer of this distribution, and I don't know if mdom's repository is to be considered the canonical repo any more? Cheers Dave P (bigpresh)

Subject:

fix-flood-protection.diff

diff --git a/lib/Bot/BasicBot/Pluggable/Module/ChanOp.pm b/lib/Bot/BasicBot/Pluggable/Module/ChanOp.pm index 2be6c18..1f960dd 100644 --- a/lib/Bot/BasicBot/Pluggable/Module/ChanOp.pm +++ b/lib/Bot/BasicBot/Pluggable/Module/ChanOp.pm @@ -54,17 +54,20 @@ sub seen { return if !$self->get('user_flood_control'); return if !$self->isop($channel); - push @{ $self->{data}->{$channel}->{$who} }, time; - my @timestamps = @{ $self->{data}->{$channel}->{$who} }; - if ( @timestamps > $self->get('user_flood_messages') ) { - my ( $min, $max ) = ( sort { $a <=> $b } @timestamps )[ 0, -1 ]; + my $threshold_timestamp = time - $self->get('user_flood_seconds'); + my $timestamps = $self->{data}->{$channel}->{$who} = [ + grep { $_ > $threshold_timestamp } + @{ $self->{data}->{$channel}->{$who} }, + time, + ]; + warn "Timestamps for $who are " . join ',', @$timestamps; + if ( @$timestamps > $self->get('user_flood_messages') ) { + my ( $min, $max ) = ( sort { $a <=> $b } @$timestamps )[ 0, -1 ]; my $seconds = $max - $min; - if ( $seconds > $self->get('user_flood_seconds') ) { - $self->kick( $channel, $who, - "Stop flooding the channel (" - . @timestamps - . " messages in $seconds seconds)." ); - } + $self->kick( $channel, $who, + "Stop flooding the channel (" + . @$timestamps + . " messages in $seconds seconds)." ); delete $self->{data}->{$channel}->{$who}; } } @@ -177,4 +180,4 @@ C<user_flood_seconds>. Defaults to 6. Mario Domgoergen <mdom@cpan.org> This program is free software; you can redistribute it -and/or modify it under the same terms as Perl itself. \ No newline at end of file +and/or modify it under the same terms as Perl itself.

Thu Dec 02 18:28:01 2010 diz [...] cpan.org - Correspondence added

On Fri Nov 19 19:51:48 2010, BIGPRESH wrote: Show quoted text

> On Tue Nov 09 14:26:02 2010, DIZ wrote:

> > On Tue Nov 09 05:00:31 2010, BIGPRESH wrote:

> > > Looks like the anti-flood kicking code is a little broken:

> [...]

> > > > > > I'm happy to supply a patch to fix this if you'll accept it?

> > > > patches welcome. patches with tests are even better. :)

> > Right, finally got a chance to do this. A patch is attached which

fixes Show quoted text

> the behaviour of the flood protection in B::B::P::Module::ChanOp. > > I don't see any current unit tests which exercise ::ChanOp, and I

can't Show quoted text

> think of a good way to test it effectively either, so no test added.

I Show quoted text

> have tested its behaviour though, and it now works as you'd expect it > to. > > I have a few other improvements I'd like to contribute too; is > Bot::BasicBot::Pluggable still developed on GitHub? I have a fork of > Mario's repo, and contributed some changes before by sending pull > requests, but you appear to be a new maintainer of this distribution, > and I don't know if mdom's repository is to be considered the

canonical Show quoted text

> repo any more?

i forked mdom's repo when i took over maintenance: https://github.com/tripside/bot-basicbot-pluggable -mike

Fri Dec 03 20:08:50 2010 davidp [...] preshweb.co.uk - Correspondence added

On Thu Dec 02 18:28:01 2010, DIZ wrote: Show quoted text

> i forked mdom's repo when i took over maintenance: > > https://github.com/tripside/bot-basicbot-pluggable

Ah, perfect, thanks. I've forked your repo, and submitted a pull request with the appropriate commits, so I'll close this ticket for you.

Fri Dec 03 20:08:50 2010 davidp [...] preshweb.co.uk - Status changed from 'open' to 'resolved'

Tue Dec 07 01:18:58 2010 diz [...] cpan.org - Correspondence added

On Fri Dec 03 20:08:50 2010, BIGPRESH wrote: Show quoted text

> On Thu Dec 02 18:28:01 2010, DIZ wrote:

> > i forked mdom's repo when i took over maintenance: > > > > https://github.com/tripside/bot-basicbot-pluggable

> > Ah, perfect, thanks. > > I've forked your repo, and submitted a pull request with the

appropriate Show quoted text

> commits, so I'll close this ticket for you.

i've uploaded 0.91 to CPAN. although RT is a pile of crap, i think it's in the best interests of the community to leave the ticket open until a release has been made that fixes the issue. i do love this module, so thank you for the support! -mike

Tue Dec 07 01:18:59 2010 The RT System itself - Status changed from 'resolved' to 'open'

Tue Dec 07 01:19:40 2010 diz [...] cpan.org - Correspondence added

uhm, no, it's fixed, really. /me kicks RT

Tue Dec 07 01:19:41 2010 diz [...] cpan.org - Status changed from 'open' to 'resolved'