Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.94k stars 554 forks source link

Bad leap year calculation in Time::Local #7486

Closed p5pRT closed 15 years ago

p5pRT commented 20 years ago

Migrated from rt.perl.org#31421 (status was 'resolved')

Searchable as RT31421$

p5pRT commented 20 years ago

From nzook@us.ibm.com

Created by nzook@us.ibm.com

Note​: Problem persists in 5.8.0

The Time​::Local module does not identify leap year properly. Demonstrating the bug requires a native 64-bit system\, because the epoch ends before any of the bad century years are reached.

Note​: At this point in the code\, year has been reduced by 1900.

Logic test script​:

nzook : perl -e ' $month = 1 ; for $year ( 1\, 4\, 100\, 200\, 300\, 400) { print "$year\n" unless $month != 1 or $year % 4 or ! ($year % 100) and $year % 400 != 100 ; }' 4 100

==

I have also taken the liberty of adding more robust code to the .*_nocheck functions in the module. My view is that the purpose of these functions is to permit users to modify timestamps without having to worry about range. I therefore force the month in-range & adjust the year.

Nathan Zook nzook@​us.ibm.com nzook@​bga.com

*** Local.pm Thu Mar 13 13​:38​:07 2003 --- Local_new.pm Wed Sep 1 11​:35​:34 2004 *************** *** 86\,92 ****   croak "Month '$month' out of range 0..11" if $month > 11 or $month \< 0;  
  my $md = $MonthDays[$month]; ! ++$md unless $month != 1 or $year % 4 or !($year % 400);  
  croak "Day '$mday' out of range 1..$md" if $mday > $md or $mday \< 1;   croak "Hour '$hour' out of range 0..23" if $hour > 23 or $hour \< 0; --- 86\,92 ----   croak "Month '$month' out of range 0..11" if $month > 11 or $month \< 0;  
  my $md = $MonthDays[$month]; ! ++$md unless $month != 1 or $year % 4 or ! ($year % 100) and $year % 400 != 100 ;  
  croak "Day '$mday' out of range 1..$md" if $mday > $md or $mday \< 1;   croak "Hour '$hour' out of range 0..23" if $hour > 23 or $hour \< 0; *************** *** 110\,117 ****  
 
  sub timegm_nocheck {   local $Options{no_range_check} = 1; ! &timegm;   }  
 
--- 110\,123 ----  
 
  sub timegm_nocheck { + + my ($sec\,$min\,$hour\,$mday\,$month\,$year) = @​_; + my $real_month = $month % 12 ; +   local $Options{no_range_check} = 1; ! ! $year += ($month - $real_month) / 12 ; ! &timegm($sec\,$min\,$hour\,$mday\,$real_month\,$year);   }  
 
*************** *** 137\,144 ****  
 
  sub timelocal_nocheck {   local $Options{no_range_check} = 1; ! &timelocal;   }  
  1; --- 143\,155 ----  
 
  sub timelocal_nocheck { + my ($sec\,$min\,$hour\,$mday\,$month\,$year) = @​_; + my $real_month = $month % 12 ; +   local $Options{no_range_check} = 1; ! ! $year += ($month - $real_month) / 12 ; ! &timelocal($sec\,$min\,$hour\,$mday\,$real_month\,$year);   }  
  1;

Perl Info ``` Flags: category=library severity=low Site configuration information for perl v5.6.0: Configured by root at Mon Nov 20 11:43:32 CST 2000. Summary of my perl5 (revision 5.0 version 6 subversion 0) configuration: Platform: osname=aix, osvers=5.0.0.0, archname=aix uname='aix shaq 1 5 006044854c00 ' config_args='-de' hint=recommended, useposix=true, d_sigaction=define usethreads=undef use5005threads=undef useithreads=undef usemultiplicity=undef useperlio=undef d_sfio=undef uselargefiles=define use64bitint=undef use64bitall=undef uselongdouble=undef usesocks=undef Compiler: cc='cc', optimize='-O', gccversion= cppflags='-D_ALL_SOURCE -D_ANSI_C_SOURCE -D_POSIX_SOURCE -qmaxmem=16384' ccflags ='-D_ALL_SOURCE -D_ANSI_C_SOURCE -D_POSIX_SOURCE -qmaxmem=16384 -q32 -D_LARGE_FILES -qlonglong' stdchar='unsigned char', d_stdstdio=define, usevfork=false intsize=4, longsize=4, ptrsize=4, doublesize=8 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=8 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=8, usemymalloc=n, prototype=define Linker and Libraries: ld='ld', ldflags ='-b32' libpth=/lib /usr/lib /usr/ccs/lib libs=-lbind -lnsl -lgdbm -ldbm -ldb -ldl -lld -lm -lC -lC_r -lc -lcrypt -lbsd -lPW -liconv libc=/lib/libc.a, so=a, useshrplib=false, libperl=libperl.a Dynamic Linking: dlsrc=dl_aix.xs, dlext=so, d_dlsymun=undef, ccdlflags=' -bE:/usr/opt/perl5/lib/5.6.0/aix/CORE/perl.exp' cccdlflags=' ', lddlflags='-bhalt:4 -bM:SRE -bI:$(PERL_INC)/perl.exp -bE:$(BASEEXT).exp -b noentry -lC -lc ' Locally applied patches: @INC for perl v5.6.0: /dfs/projects/gpu/lab/local/scripts/release /dfs/projects/gpu/lab/local/debug /dfs/projects/gpu/lab/local/LI/release /usr/opt/perl5/lib/5.6.0/aix /usr/opt/perl5/lib/5.6.0 /usr/opt/perl5/lib/site_perl/5.6.0/aix /usr/opt/perl5/lib/site_perl/5.6.0 /usr/opt/perl5/lib/site_perl . Environment for perl v5.6.0: HOME=/.../austin.ibm.com/fs/home/nzook LANG=en_US LANGUAGE (unset) LC__FASTMSG=true LD_LIBRARY_PATH (unset) LIBPATH (unset) LOGDIR (unset) PATH=/bin:/usr/bin:/usr/sbin:/etc:/usr/ucb:/usr/ccs/bin:/usr/afs/bin:/usr/dt/bin:/usr/bin/X11:/.../austin.ibm.com/fs/projects/ppcae/sw/lab/bin:/.../austin.ibm.com/fs/home/nzook/bin:/usr/prod/bin:/usr/contrib/bin PERL5LIB=/dfs/projects/gpu/lab/local/scripts/release:/dfs/projects/gpu/lab/local/debug:/dfs/projects/gpu/lab/local/LI/release PERL_BADLANG (unset) SHELL=/bin/ksh ```
p5pRT commented 20 years ago

From @ysth

On Wed\, Sep 01\, 2004 at 04​:54​:48PM -0000\, Nathan H Zook wrote​:

The Time​::Local module does not identify leap year properly. Demonstrating the bug requires a native 64-bit system\, because the epoch ends before any of the bad century years are reached.

Note​: At this point in the code\, year has been reduced by 1900.

Thanks for catching this; just a couple of comments​:

I have also taken the liberty of adding more robust code to the .*_nocheck functions in the module. My view is that the purpose of these functions is to permit users to modify timestamps without having to worry about range. I therefore force the month in-range & adjust the year.

Can you supply a patch to lib/Time/Local.t that excercises this part? I'm not clear what didn't work before that does with your patch.

sub timegm_nocheck { + + my ($sec\,$min\,$hour\,$mday\,$month\,$year) = @​_; + my $real_month = $month % 12 ;

If $month may be negative\, this won't work everywhere without a "no integer;" around it; use integer invokes C's %-behaviour which isn't guaranteed to work as you would want for negative numbers.

+ local $Options{no_range_check} = 1; ! ! $year += ($month - $real_month) / 12 ; ! &timegm($sec\,$min\,$hour\,$mday\,$real_month\,$year);

You can't adjust the year this way without first doing the adjustments that timegm does when year is in 0..99 or year >= 1000 (see timegm). For instance\, if year is passed as 999 and month as 13\, this should produce February 2900 C.E.\, not February 1000 C.E. If year is 52 and month is 60\, this should be January 2057\, not January 1957. (Apologies for any errors in the above; I didn't actually test it.)

I think the same applies to timelocal (hmm\, that calls timegm; seems like there are range checking problems with that).

p5pRT commented 20 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 20 years ago

From nzook@us.ibm.com

If you tried giving it a month outside 0 .. 11\, it would die.

I have tested the $month % 12 thing\, and I'm pretty sure it works as intended. Is this a portability issue that I just didn't hit?

I didn't think about the 0 ... 99 handling. You're right--we probably want something like​:

$adj_year = $year + ($month - $real_month) / 12 ; $year = abs($year - 49.5) \< 51 ? $adj_year % 100 : $adj_year ;

This still wraps at the extremes\, but if you feed it two-byte data at the extremes\, you deserve what you get...

Likewise for the 3-digit years. These functions are called no_check*\, no no_assert*. I don't feel at all bad about being "wrong" at that end.

Nathan


  Tradition means giving votes to the most obscure of all classes\, our ancestors. It is the democracy of the dead. Tradition refuses to submit to the small and arrogant oligarchy of those who just happen to be walking about. All democrats object to men being disqualified by the accident of their birth; tradition objects to their being disqualified by the accident of their death.

-- G. K. Chesterton

 
  "Yitzchak
  Scott-Thoennes
  via RT" To   \<perlbug-followup Nathan H Zook/Austin/IBM@​IBMUS
  @​perl.org> cc  
  09/20/2004 06​:33 Subject   PM Re​: [perl #31421] Bad leap year   calculation in Time​::Local
 
  Please respond to
  perlbug-followup
 
 
 

On Wed\, Sep 01\, 2004 at 04​:54​:48PM -0000\, Nathan H Zook wrote​:

The Time​::Local module does not identify leap year properly. Demonstrating the bug requires a native 64-bit system\, because the epoch ends before any of the bad century years are reached.

Note​: At this point in the code\, year has been reduced by 1900.

Thanks for catching this; just a couple of comments​:

I have also taken the liberty of adding more robust code to the .*_nocheck functions in the module. My view is that the purpose of these functions is to permit users to modify timestamps without having to worry about range. I

therefore force the month in-range & adjust the year.

Can you supply a patch to lib/Time/Local.t that excercises this part? I'm not clear what didn't work before that does with your patch.

sub timegm_nocheck { + + my ($sec\,$min\,$hour\,$mday\,$month\,$year) = @​_; + my $real_month = $month % 12 ;

If $month may be negative\, this won't work everywhere without a "no integer;" around it; use integer invokes C's %-behaviour which isn't guaranteed to work as you would want for negative numbers.

+ local $Options{no_range_check} = 1; ! ! $year += ($month - $real_month) / 12 ; ! &timegm($sec\,$min\,$hour\,$mday\,$real_month\,$year);

You can't adjust the year this way without first doing the adjustments that timegm does when year is in 0..99 or year >= 1000 (see timegm). For instance\, if year is passed as 999 and month as 13\, this should produce February 2900 C.E.\, not February 1000 C.E. If year is 52 and month is 60\, this should be January 2057\, not January 1957. (Apologies for any errors in the above; I didn't actually test it.)

I think the same applies to timelocal (hmm\, that calls timegm; seems like there are range checking problems with that).

p5pRT commented 20 years ago

From nzook@us.ibm.com

graycol.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

pic30725.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

ecblank.gif

p5pRT commented 20 years ago

From @ysth

On Tue\, Sep 21\, 2004 at 10​:20​:24AM -0500\, Nathan H Zook \nzook@&#8203;us\.ibm\.com wrote​:

I have tested the $month % 12 thing\, and I'm pretty sure it works as intended. Is this a portability issue that I just didn't hit?

Perhaps; C leaves it ambiguous\, but in practice most platforms look like this​:

$ perl -wle'print -3 % 12; use integer; print -3 % 12; print -3 / 12' 9 -3 0

where you want -3 % 12 to be 9 and -3 / 12 to be -1.

I didn't think about the 0 ... 99 handling. You're right--we probably want something like​:

$adj_year = $year + ($month - $real_month) / 12 ; $year = abs($year - 49.5) \< 51 ? $adj_year % 100 : $adj_year ;

This still wraps at the extremes\, but if you feed it two-byte data at the extremes\, you deserve what you get...

I think it's much more straightforward and free of problems with corner cases to do the adjustment in sub timegm itself\, after it's handled year in 0..99 and year > 999. Something like this (as yet untested; I have 64 bit ints but only 32 bit time_t\, so it will take me a little bit to set up some tests)​:

Inline Patch ```diff --- perl/lib/Time/Local.pm.orig 2004-06-08 11:47:02.000000000 -0700 +++ perl/lib/Time/Local.pm 2004-09-21 10:17:43.903761600 -0700 @@ -102,7 +102,14 @@ $year += ($year > $Breakpoint) ? $Century : $NextCentury; } - unless ($Options{no_range_check}) { + if ($Options{no_range_check}) { + # adjust month to be 0..11 + + no integer; + my $orig_month = $month; + $month %= 12; + $year += ($orig_month - $month) / 12; + } else { if (abs($year) >= 0x7fff) { $year += 1900; croak "Cannot handle date ($sec, $min, $hour, $mday, $month, *$year*)"; ```
p5pRT commented 20 years ago

From nzook@us.ibm.com

My reading of the entrails was that the boundary checking of the core functions was a significant feature. If not\, then we are clearly better to do the adjusting inside.

My time_t is 32 bits as well. ;-)

Nathan


  Tradition means giving votes to the most obscure of all classes\, our ancestors. It is the democracy of the dead. Tradition refuses to submit to the small and arrogant oligarchy of those who just happen to be walking about. All democrats object to men being disqualified by the accident of their birth; tradition objects to their being disqualified by the accident of their death.

-- G. K. Chesterton

 
  "Yitzchak
  Scott-Thoennes
  via RT" To   \<perlbug-followup Nathan H Zook/Austin/IBM@​IBMUS
  @​perl.org> cc  
  09/21/2004 12​:29 Subject   PM Re​: [perl #31421] Bad leap year   calculation in Time​::Local
 
  Please respond to
  perlbug-followup
 
 
 

On Tue\, Sep 21\, 2004 at 10​:20​:24AM -0500\, Nathan H Zook \nzook@&#8203;us\.ibm\.com wrote​:

I have tested the $month % 12 thing\, and I'm pretty sure it works as intended. Is this a portability issue that I just didn't hit?

Perhaps; C leaves it ambiguous\, but in practice most platforms look like this​:

$ perl -wle'print -3 % 12; use integer; print -3 % 12; print -3 / 12' 9 -3 0

where you want -3 % 12 to be 9 and -3 / 12 to be -1.

I didn't think about the 0 ... 99 handling. You're right--we probably want something like​:

$adj_year = $year + ($month - $real_month) / 12 ; $year = abs($year - 49.5) \< 51 ? $adj_year % 100 : $adj_year ;

This still wraps at the extremes\, but if you feed it two-byte data at the extremes\, you deserve what you get...

I think it's much more straightforward and free of problems with corner cases to do the adjustment in sub timegm itself\, after it's handled year in 0..99 and year > 999. Something like this (as yet untested; I have 64 bit ints but only 32 bit time_t\, so it will take me a little bit to set up some tests)​:

--- perl/lib/Time/Local.pm.orig 2004-06-08 11​:47​:02.000000000 -0700 +++ perl/lib/Time/Local.pm 2004-09-21 10​:17​:43.903761600 -0700 @​@​ -102\,7 +102\,14 @​@​   $year += ($year > $Breakpoint) ? $Century : $NextCentury;   }

- unless ($Options{no_range_check}) { + if ($Options{no_range_check}) { + # adjust month to be 0..11 + + no integer; + my $orig_month = $month; + $month %= 12; + $year += ($orig_month - $month) / 12; + } else {   if (abs($year) >= 0x7fff) {   $year += 1900;   croak "Cannot handle date ($sec\, $min\, $hour\, $mday\, $month\, *$year*)";

p5pRT commented 20 years ago

From nzook@us.ibm.com

graycol.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

pic16438.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

ecblank.gif

p5pRT commented 20 years ago

From @ysth

On Tue\, Sep 21\, 2004 at 01​:37​:13PM -0500\, Nathan H Zook wrote​:

My reading of the entrails was that the boundary checking of the core functions was a significant feature. If not\, then we are clearly better to do the adjusting inside.

I'm not sure I'm understanding you; I was proposing changing sub timegm only in the case where it is called from timegm_nocheck.

p5pRT commented 20 years ago

From @autarch

On Mon\, 20 Sep 2004\, Yitzchak Scott-Thoennes wrote​:

sub timegm_nocheck { + + my ($sec\,$min\,$hour\,$mday\,$month\,$year) = @​_; + my $real_month = $month % 12 ;

If $month may be negative\, this won't work everywhere without a "no integer;" around it; use integer invokes C's %-behaviour which isn't guaranteed to work as you would want for negative numbers.

Isn't this the whole reason _not_ to use timegm_nocheck?

! $year += ($month - $real_month) / 12 ; ! &timegm($sec\,$min\,$hour\,$mday\,$real_month\,$year);

You can't adjust the year this way without first doing the adjustments that timegm does when year is in 0..99 or year >= 1000 (see timegm). For instance\, if year is passed as 999 and month as 13\, this should produce February 2900 C.E.\, not February 1000 C.E. If year is 52 and month is 60\, this should be January 2057\, not January 1957. (Apologies for any errors in the above; I didn't actually test it.)

Again\, I'm not sure this is worthwhile. The docs says this about calling the nocheck versions with params outside the valid ranges​:

  Your mileage may vary when trying these with minutes and hours\,   and it doesn't work at all for months.

-dave

/*=========================== VegGuide.Org Your guide to all that's veg. ===========================*/

p5pRT commented 20 years ago

From @autarch

On Mon\, 20 Sep 2004\, Yitzchak Scott-Thoennes wrote​:

I have also taken the liberty of adding more robust code to the .*_nocheck functions in the module. My view is that the purpose of these functions is to permit users to modify timestamps without having to worry about range. I therefore force the month in-range & adjust the year.

Can you supply a patch to lib/Time/Local.t that excercises this part? I'm not clear what didn't work before that does with your patch.

It needs tests that _won't_ blow up when time_t is 32 bits. I don't see anything in config to check for this though.

-dave

/*=========================== VegGuide.Org Your guide to all that's veg. ===========================*/

p5pRT commented 20 years ago

From nzook@us.ibm.com

If I want the time stamp from last month\, or 50 days ago\, the naive way to do it yields negative numbers. It seems very much to me that we want/need to support these values.

Nathan


  Tradition means giving votes to the most obscure of all classes\, our ancestors. It is the democracy of the dead. Tradition refuses to submit to the small and arrogant oligarchy of those who just happen to be walking about. All democrats object to men being disqualified by the accident of their birth; tradition objects to their being disqualified by the accident of their death.

-- G. K. Chesterton

 
  "Dave Rolsky via
  RT"
  \<perlbug-followup To   @​perl.org> Nathan H Zook/Austin/IBM@​IBMUS
  cc   09/22/2004 10​:54
  PM Subject   Re​: [perl #31421] Bad leap year   calculation in Time​::Local
  Please respond to
  perlbug-followup
 
 
 
 

On Mon\, 20 Sep 2004\, Yitzchak Scott-Thoennes wrote​:

sub timegm_nocheck { + + my ($sec\,$min\,$hour\,$mday\,$month\,$year) = @​_; + my $real_month = $month % 12 ;

If $month may be negative\, this won't work everywhere without a "no integer;" around it; use integer invokes C's %-behaviour which isn't guaranteed to work as you would want for negative numbers.

Isn't this the whole reason _not_ to use timegm_nocheck?

! $year += ($month - $real_month) / 12 ; ! &timegm($sec\,$min\,$hour\,$mday\,$real_month\,$year);

You can't adjust the year this way without first doing the adjustments that timegm does when year is in 0..99 or year >= 1000 (see timegm). For instance\, if year is passed as 999 and month as 13\, this should produce February 2900 C.E.\, not February 1000 C.E. If year is 52 and month is 60\, this should be January 2057\, not January 1957. (Apologies for any errors in the above; I didn't actually test it.)

Again\, I'm not sure this is worthwhile. The docs says this about calling the nocheck versions with params outside the valid ranges​:

  Your mileage may vary when trying these with minutes and hours\,   and it doesn't work at all for months.

-dave

/*=========================== VegGuide.Org Your guide to all that's veg. ===========================*/

p5pRT commented 20 years ago

From nzook@us.ibm.com

graycol.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

pic25449.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

ecblank.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

You can't get to 2100 with a 32-bit time_t in the current era. Blowup is guaranteed.

Nathan


  Tradition means giving votes to the most obscure of all classes\, our ancestors. It is the democracy of the dead. Tradition refuses to submit to the small and arrogant oligarchy of those who just happen to be walking about. All democrats object to men being disqualified by the accident of their birth; tradition objects to their being disqualified by the accident of their death.

-- G. K. Chesterton

 
  "Dave Rolsky via
  RT"
  \<perlbug-followup To   @​perl.org> Nathan H Zook/Austin/IBM@​IBMUS
  cc   09/22/2004 10​:56
  PM Subject   Re​: [perl #31421] Bad leap year   calculation in Time​::Local
  Please respond to
  perlbug-followup
 
 
 
 

On Mon\, 20 Sep 2004\, Yitzchak Scott-Thoennes wrote​:

I have also taken the liberty of adding more robust code to the .*_nocheck functions in the module. My view is that the purpose of these functions is to permit users to modify timestamps without having to worry about range. I therefore force the month in-range & adjust the year.

Can you supply a patch to lib/Time/Local.t that excercises this part? I'm not clear what didn't work before that does with your patch.

It needs tests that _won't_ blow up when time_t is 32 bits. I don't see anything in config to check for this though.

-dave

/*=========================== VegGuide.Org Your guide to all that's veg. ===========================*/

p5pRT commented 20 years ago

From nzook@us.ibm.com

graycol.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

pic07293.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

ecblank.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

Oh. Okay.

Nathan


  Tradition means giving votes to the most obscure of all classes\, our ancestors. It is the democracy of the dead. Tradition refuses to submit to the small and arrogant oligarchy of those who just happen to be walking about. All democrats object to men being disqualified by the accident of their birth; tradition objects to their being disqualified by the accident of their death.

-- G. K. Chesterton

 
  "Yitzchak
  Scott-Thoennes
  via RT" To   \<perlbug-followup Nathan H Zook/Austin/IBM@​IBMUS
  @​perl.org> cc  
  09/22/2004 07​:49 Subject   AM Re​: [perl #31421] Bad leap year   calculation in Time​::Local
 
  Please respond to
  perlbug-followup
 
 
 

On Tue\, Sep 21\, 2004 at 01​:37​:13PM -0500\, Nathan H Zook wrote​:

My reading of the entrails was that the boundary checking of the core functions was a significant feature. If not\, then we are clearly better to do the adjusting inside.

I'm not sure I'm understanding you; I was proposing changing sub timegm only in the case where it is called from timegm_nocheck.

p5pRT commented 20 years ago

From nzook@us.ibm.com

graycol.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

pic28635.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

ecblank.gif

p5pRT commented 20 years ago

From @ysth

On Wed\, Sep 22\, 2004 at 10​:54​:30PM -0500\, Dave Rolsky wrote​:

On Mon\, 20 Sep 2004\, Yitzchak Scott-Thoennes wrote​:

sub timegm_nocheck { + + my ($sec\,$min\,$hour\,$mday\,$month\,$year) = @​_; + my $real_month = $month % 12 ;

If $month may be negative\, this won't work everywhere without a "no integer;" around it; use integer invokes C's %-behaviour which isn't guaranteed to work as you would want for negative numbers.

Isn't this the whole reason _not_ to use timegm_nocheck?

I don't follow that. Can you explain what you mean?

! $year += ($month - $real_month) / 12 ; ! &timegm($sec\,$min\,$hour\,$mday\,$real_month\,$year);

You can't adjust the year this way without first doing the adjustments that timegm does when year is in 0..99 or year >= 1000 (see timegm). For instance\, if year is passed as 999 and month as 13\, this should produce February 2900 C.E.\, not February 1000 C.E. If year is 52 and month is 60\, this should be January 2057\, not January 1957. (Apologies for any errors in the above; I didn't actually test it.)

Again\, I'm not sure this is worthwhile. The docs says this about calling the nocheck versions with params outside the valid ranges​:

Your mileage may vary when trying these with minutes and hours\, and it doesn't work at all for months.

That is an especially good reason to make sure if month-munging is introduced that it works sanely (for some value of sane) and has tests.

On Wed\, Sep 22\, 2004 at 10​:56​:32PM -0500\, Dave Rolsky wrote​:

It needs tests that _won't_ blow up when time_t is 32 bits. I don't see anything in config to check for this though.

32-bit time_t is only needed to test via gmtime(timegm( ....)). If testing the return of timegm directly\, $Config{ivsize} could be used\, or keep the test results within reasonable NV range.

p5pRT commented 20 years ago

From @autarch

On Thu\, 23 Sep 2004\, Yitzchak Scott-Thoennes wrote​:

Isn't this the whole reason _not_ to use timegm_nocheck?

I don't follow that. Can you explain what you mean?

If you want your parameters validated\, you shouldn't be calling the _nocheck versions of these subs.

Again\, I'm not sure this is worthwhile. The docs says this about calling the nocheck versions with params outside the valid ranges​:

Your mileage may vary when trying these with minutes and hours\, and it doesn't work at all for months.

That is an especially good reason to make sure if month-munging is introduced that it works sanely (for some value of sane) and has tests.

Indeed\, along with a doc patch.

It needs tests that _won't_ blow up when time_t is 32 bits. I don't see anything in config to check for this though.

32-bit time_t is only needed to test via gmtime(timegm( ....)). If testing the return of timegm directly\, $Config{ivsize} could be used\, or keep the test results within reasonable NV range.

Ah\, good point.

-dave

/*=========================== VegGuide.Org Your guide to all that's veg. ===========================*/

p5pRT commented 20 years ago

From @autarch

On Thu\, 23 Sep 2004\, Nathan H Zook wrote​:

If I want the time stamp from last month\, or 50 days ago\, the naive way to do it yields negative numbers. It seems very much to me that we want/need to support these values.

Time​::Local is not really designed to be a super-duper date math module. I'd recommend DateTime or Date​::Calc or basically anything else ;)

-dave

/*=========================== VegGuide.Org Your guide to all that's veg. ===========================*/

p5pRT commented 20 years ago

From @ysth

On Mon\, 20 Sep 2004\, Yitzchak Scott-Thoennes wrote​:

sub timegm_nocheck { + + my ($sec\,$min\,$hour\,$mday\,$month\,$year) = @​_; + my $real_month = $month % 12 ;

If $month may be negative\, this won't work everywhere without a "no integer;" around it; use integer invokes C's %-behaviour which isn't guaranteed to work as you would want for negative numbers.

On Thu\, Sep 23\, 2004 at 05​:20​:22PM -0500\, Dave Rolsky \autarch@&#8203;urth\.org wrote​:

On Thu\, 23 Sep 2004\, Yitzchak Scott-Thoennes wrote​:

Isn't this the whole reason _not_ to use timegm_nocheck?

I don't follow that. Can you explain what you mean?

If you want your parameters validated\, you shouldn't be calling the _nocheck versions of these subs.

This isn't about validation. According to my understanding of the proposed patch\, the goal is to be able to take an actual year/month/day\, subtract or add an arbitrary number to the month\, and pass it to timegm. So the patch is making month be in the range 0..11 and adjusting the year. My comment was that because Time​::Local does "use integer;"\, a negative month % 12 will be either 0..11 or -11..0 depending on the platform/C compiler being used.

p5pRT commented 20 years ago

From @autarch

On Thu\, 23 Sep 2004\, Yitzchak Scott-Thoennes wrote​:

If you want your parameters validated\, you shouldn't be calling the _nocheck versions of these subs.

This isn't about validation. According to my understanding of the proposed patch\, the goal is to be able to take an actual year/month/day\, subtract or add an arbitrary number to the month\, and pass it to timegm. So the patch is making month be in the range 0..11 and adjusting the year. My comment was that because Time​::Local does "use integer;"\, a negative month % 12 will be either 0..11 or -11..0 depending on the platform/C compiler being used.

I understand what the OP wants. My point was that​:

- months outside 0..11 are documented to not work. With the regular versions it'll die\, with the _nocheck subs the results are undefined.

- Time​::Local is a really poor tool for try to date math with.

- I'm not too keen on improving it for date math\, since it will still be a really poor tool for date math even with a patch to handle months outside 0..11.

-dave

/*=========================== VegGuide.Org Your guide to all that's veg. ===========================*/

p5pRT commented 20 years ago

From @gbarr

On 24 Sep 2004\, at 07​:01\, Dave Rolsky wrote​:

I understand what the OP wants. My point was that​:

- months outside 0..11 are documented to not work. With the regular versions it'll die\, with the _nocheck subs the results are undefined.

I agree. Time​::Local was designed to be the opposite of localtime/gmtime.

- Time​::Local is a really poor tool for try to date math with.

- I'm not too keen on improving it for date math\, since it will still be a really poor tool for date math even with a patch to handle months outside 0..11.

I was never intended to do any sort of date math and I don't think it should. As you have pointed out there are other\, better\, modules on CPAN for doing date math.

Graham.

p5pRT commented 20 years ago

From @ysth

On Fri\, Sep 24\, 2004 at 07​:33​:57AM +0100\, Graham Barr wrote​:

On 24 Sep 2004\, at 07​:01\, Dave Rolsky wrote​:

I understand what the OP wants. My point was that​:

- months outside 0..11 are documented to not work. With the regular versions it'll die\, with the _nocheck subs the results are undefined.

I agree. Time​::Local was designed to be the opposite of localtime/gmtime.

- Time​::Local is a really poor tool for try to date math with.

- I'm not too keen on improving it for date math\, since it will still be a really poor tool for date math even with a patch to handle months outside 0..11.

I was never intended to do any sort of date math and I don't think it should. As you have pointed out there are other\, better\, modules on CPAN for doing date math.

If the _nocheck subs didn't exist\, I would agree with you. If someone were proposing adding them\, I'd object and point to the CPAN date modules. But in fact they do exist\, and do allow simple date math of adding seconds or days to a given time\, and I see no reason why they shouldn't be made more consistent in what they allow\, if a patch with doc and tests were proposed.

However\, I don't have such a patch\, and may or may not ever have such a patch\, so I'm going to bow out of this conversation now.

p5pRT commented 20 years ago

From @gbarr

On 24 Sep 2004\, at 08​:48\, Yitzchak Scott-Thoennes wrote​:

I was never intended to do any sort of date math and I don't think it should. As you have pointed out there are other\, better\, modules on CPAN for doing date math.

If the _nocheck subs didn't exist\, I would agree with you. If someone were proposing adding them\, I'd object and point to the CPAN date modules.

But date math is not the reason the _nocheck subs were added. Maybe they can do simple date math as a side effect\, but the reason for adding them was performance. That is if you think you have valid input data you can skip the checking that the normal timegm does. Any result from *_nocheck resulting from data outside the normal range is unspecified.

But in fact they do exist\, and do allow simple date math of adding seconds or days to a given time\, and I see no reason why they shouldn't be made more consistent in what they allow\, if a patch with doc and tests were proposed.

Because it goes against why they were added in the first place\, performance

However\, I don't have such a patch\, and may or may not ever have such a patch\, so I'm going to bow out of this conversation now.

Graham.

p5pRT commented 20 years ago

From nzook@us.ibm.com

I grok "patch with doc". I don't grok "and tests". What is it that you (Yitzchak) need here?

I can accept that the reason that the *_nocheck versions were written were for performance. HOWEVER...

1) They do more than that. The permit naive date manipulations to "just work"--so long as the months stay cool. 2) The documents encourage\, by way of examples\, the use of these functions to do naive date manipulation. While the camel book does point towards CPAN\, the docs do not. 3) The proposed patch does not significantly affect performance. There is no conditional code\, which means that there are no branches to mispredict. Assuming that the perl compiler is smart enough\, on many systems\, the total cost is that of an integer divide. If not\, it is divide\, mul\, sub. 4) It is part of the standard distribution. I know that CPAN is considered by "everyone" to be effectively part of the standard distribution\, but a corporate user who is NOT a geek (or who has really stupid management) might well find this module far more accessible than CPAN.

Nathan


  Tradition means giving votes to the most obscure of all classes\, our ancestors. It is the democracy of the dead. Tradition refuses to submit to the small and arrogant oligarchy of those who just happen to be walking about. All democrats object to men being disqualified by the accident of their birth; tradition objects to their being disqualified by the accident of their death.

-- G. K. Chesterton

 
  "Graham Barr via
  RT"
  \<perlbug-followup To   @​perl.org> Nathan H Zook/Austin/IBM@​IBMUS
  cc   09/24/2004 05​:33
  AM Subject   Re​: [perl #31421] Bad leap year   calculation in Time​::Local
  Please respond to
  perlbug-followup
 
 
 
 

On 24 Sep 2004\, at 08​:48\, Yitzchak Scott-Thoennes wrote​:

I was never intended to do any sort of date math and I don't think it should. As you have pointed out there are other\, better\, modules on CPAN for doing date math.

If the _nocheck subs didn't exist\, I would agree with you. If someone were proposing adding them\, I'd object and point to the CPAN date modules.

But date math is not the reason the _nocheck subs were added. Maybe they can do simple date math as a side effect\, but the reason for adding them was performance. That is if you think you have valid input data you can skip the checking that the normal timegm does. Any result from *_nocheck resulting from data outside the normal range is unspecified.

But in fact they do exist\, and do allow simple date math of adding seconds or days to a given time\, and I see no reason why they shouldn't be made more consistent in what they allow\, if a patch with doc and tests were proposed.

Because it goes against why they were added in the first place\, performance

However\, I don't have such a patch\, and may or may not ever have such a patch\, so I'm going to bow out of this conversation now.

Graham.

p5pRT commented 20 years ago

From nzook@us.ibm.com

graycol.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

pic14412.gif

p5pRT commented 20 years ago

From nzook@us.ibm.com

ecblank.gif

p5pRT commented 20 years ago

From @autarch

On Fri\, 24 Sep 2004\, Nathan H Zook wrote​:

I grok "patch with doc". I don't grok "and tests". What is it that you (Yitzchak) need here?

Patch the t/Local.t file.

Also please send a unified diff (diff -u).

I can accept that the reason that the *_nocheck versions were written were for performance. HOWEVER...

1) They do more than that. The permit naive date manipulations to "just work"--so long as the months stay cool. 2) The documents encourage\, by way of examples\, the use of these functions to do naive date manipulation. While the camel book does point towards CPAN\, the docs do not.

This is a problem of the docs more than the code\, I'd say. But regardless\, I don't really want to encourage _more_ use of Time​::Local this way.

3) The proposed patch does not significantly affect performance. There is no conditional code\, which means that there are no branches to mispredict. Assuming that the perl compiler is smart enough\, on many systems\, the total cost is that of an integer divide. If not\, it is divide\, mul\, sub.

It complicates the code for the maintainer (me and p5p).

4) It is part of the standard distribution. I know that CPAN is considered by "everyone" to be effectively part of the standard distribution\, but a corporate user who is NOT a geek (or who has really stupid management) might well find this module far more accessible than CPAN.

The standard distro does not provide sufficient tools to do real date math in a convenient way\, and patching Time​::Local this way won't really change that.

-dave

/*=========================== VegGuide.Org Your guide to all that's veg. ===========================*/

p5pRT commented 15 years ago

From @smpeters

On Wed Sep 01 09​:54​:48 2004\, nzook@​us.ibm.com wrote​:

This is a bug report for perl from nzook@​us.ibm.com\, generated with the help of perlbug 1.28 running under perl v5.6.0.

----------------------------------------------------------------- [Please enter your report here]

Note​: Problem persists in 5.8.0

The Time​::Local module does not identify leap year properly. Demonstrating the bug requires a native 64-bit system\, because the epoch ends before any of the bad century years are reached.

I believe all these changes have been made to dealing with 64-bit dates in a portable way so that years past 2038 are identified correctly.

Steve Peters

p5pRT commented 15 years ago

@smpeters - Status changed from 'open' to 'resolved'