Bug #82115 for Algorithm-Cron: Infinite loop when unexpected cron string comes in

Wed Dec 19 15:47:22 2012 triddle [...] cpan.org - Ticket created

Subject:

Infinite loop when unexpected cron string comes in

Hi, I've put Algorithm::Cron to use quite nicely generating System::Introspector reports of the cron invocations that will happen for the current week across an entire set of servers. A::C works fantastically well except when it gets unexpected input there's a case where it'll go into an infinite loop. One of those cases is Vixie cron special values such as @daily or @reboot. Some sample code and the output are below. Making warnings fatal is a fairly trivial way to at least stop the infinite loop which I as the caller of next_time() don't seem to be able to break out of. Then an end user could just wrap the invocations with eval and gracefully handle the cases where one cron line has something Algorithm:Cron can't deal with but not the others. Cheers and thanks for those module! Tyler Riddle use strictures; use Algorithm::Cron; #also @reboot causes this issue my $cron = Algorithm::Cron->new(crontab => '@daily', base => 'utc'); my $time = time(); while(1) { $time = $cron->next_time($time); warn $time; } Causes infinite loop while perl generates warnings: Use of uninitialized value $spec in string eq at lib//Algorithm/Cron.pm line 156. Use of uninitialized value $spec in split at lib//Algorithm/Cron.pm line 159. Use of uninitialized value $spec in string eq at lib//Algorithm/Cron.pm line 156. Use of uninitialized value $spec in split at lib//Algorithm/Cron.pm line 159. Use of uninitialized value $spec in string eq at lib//Algorithm/Cron.pm line 156. Use of uninitialized value $spec in split at lib//Algorithm/Cron.pm line 159. Use of uninitialized value $spec in string eq at lib//Algorithm/Cron.pm line 156. Use of uninitialized value $spec in split at lib//Algorithm/Cron.pm line 159. Use of uninitialized value in subroutine entry at lib//Algorithm/Cron.pm line 197. Use of uninitialized value in subroutine entry at lib//Algorithm/Cron.pm line 197. Use of uninitialized value in subroutine entry at lib//Algorithm/Cron.pm line 197. Use of uninitialized value in subroutine entry at lib//Algorithm/Cron.pm line 197. Use of uninitialized value in subroutine entry at lib//Algorithm/Cron.pm line 197. Use of uninitialized value in subroutine entry at lib//Algorithm/Cron.pm line 197. Use of uninitialized value in subroutine entry at lib//Algorithm/Cron.pm line 197.

Thu Dec 27 11:01:49 2012 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Show quoted text

> I've put Algorithm::Cron to use quite nicely generating > System::Introspector reports of the cron invocations that will happen > for the current week across an entire set of servers. A::C works > fantastically well except when it gets unexpected input there's a case > where it'll go into an infinite loop. One of those cases is Vixie cron > special values such as @daily or @reboot. Some sample code and the > output are below. > > Making warnings fatal is a fairly trivial way to at least stop the > infinite loop which I as the caller of next_time() don't seem to be able > to break out of. Then an end user could just wrap the invocations with > eval and gracefully handle the cases where one cron line has something > Algorithm:Cron can't deal with but not the others.

Ah yes. A little more sanity-checking of arguments would be good here. Have now added some. Will be in next version. -- Paul Evans

Subject:

rt82115.patch

=== modified file 'Build.PL' --- Build.PL 2012-09-07 21:10:39 +0000 +++ Build.PL 2012-12-27 15:55:59 +0000 @@ -9,6 +9,7 @@ 'Time::timegm' => 0, }, build_requires => { + 'Test::Fatal' => 0, 'Test::More' => 0, }, auto_configure_requires => 0, # Don't add M::B to configure_requires === modified file 'lib/Algorithm/Cron.pm' --- lib/Algorithm/Cron.pm 2012-11-27 16:13:43 +0000 +++ lib/Algorithm/Cron.pm 2012-12-27 16:00:01 +0000 @@ -178,6 +178,9 @@ $end = 7 if defined $end and $end == 0 and $val > 0; } + $val =~ m/^\d+$/ or croak "$val is unrecognised for $kind"; + $end =~ m/^\d+$/ or croak "$end is unrecognised for $kind" if defined $end; + push @vals, $val; push @vals, $val while defined $end and ( $val += $step ) <= $end; @@ -247,6 +250,9 @@ if( exists $params{crontab} ) { my $crontab = delete $params{crontab}; my @fields = split m/\s+/, $crontab; + @fields >= 5 or croak "Expected at least 5 crontab fields"; + @fields <= 6 or croak "Expected no more than 6 crontab fields"; + @fields = ( "0", @fields ) if @fields < 6; @params{ @FIELDS_CTOR } = @fields; } === modified file 't/02cron.t' --- t/02cron.t 2012-09-08 02:57:31 +0000 +++ t/02cron.t 2012-12-27 15:59:03 +0000 @@ -3,7 +3,8 @@ use strict; use warnings; -use Test::More tests => 25; +use Test::More tests => 27; +use Test::Fatal qw( dies_ok ); use Algorithm::Cron; @@ -74,3 +75,9 @@ is_deeply( [ $cron->mon ], [ 1 ], '$cron->mon for named' ); is_deeply( [ $cron->wday ], [], '$cron->wday for named' ); } + +dies_ok { Algorithm::Cron->new( crontab => '@hourly', base => 'utc' ) } + "crontab => '\@hourly' dies"; + +dies_ok { Algorithm::Cron->new( crontab => 'one * * * *', base => 'utc' ) } + 'Unrecognised number dies';

Thu Dec 27 11:01:50 2012 The RT System itself - Status changed from 'new' to 'open'

Thu Dec 27 11:01:51 2012 leonerd-cpan [...] leonerd.org.uk - Status changed from 'open' to 'patched'

Wed Jan 02 13:47:30 2013 triddle [...] cpan.org - Correspondence added

I installed 0.06 and it's working great. Cheers! Tyler On Thu Dec 27 11:01:49 2012, PEVANS wrote: Show quoted text

> > I've put Algorithm::Cron to use quite nicely generating > > System::Introspector reports of the cron invocations that will happen > > for the current week across an entire set of servers. A::C works > > fantastically well except when it gets unexpected input there's a case > > where it'll go into an infinite loop. One of those cases is Vixie cron > > special values such as @daily or @reboot. Some sample code and the > > output are below. > > > > Making warnings fatal is a fairly trivial way to at least stop the > > infinite loop which I as the caller of next_time() don't seem to be able > > to break out of. Then an end user could just wrap the invocations with > > eval and gracefully handle the cases where one cron line has something > > Algorithm:Cron can't deal with but not the others.

> > Ah yes. A little more sanity-checking of arguments would be good here. > Have now added some. Will be in next version.

Wed Jan 02 13:47:31 2013 The RT System itself - Status changed from 'patched' to 'open'

Sun Jan 06 20:44:37 2013 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Fixed in 0.06. -- Paul Evans

Sun Jan 06 20:44:37 2013 leonerd-cpan [...] leonerd.org.uk - Status changed from 'open' to 'resolved'

Sun Jan 06 20:44:44 2013 leonerd-cpan [...] leonerd.org.uk - Fixed in 0.06 added