classification
Title: Bug found in datetime for Epoch time = -1
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.1, Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: belopolsky Nosy List: amaury.forgeotdarc, belopolsky, blais, loewis, vstinner
Priority: low Keywords: needs review, patch

Created on 2007-05-28 02:27 by blais, last changed 2011-02-16 18:57 by belopolsky. This issue is now closed.

Files
File name Uploaded Description Edit
timebug.py blais, 2007-05-28 02:28 reproduce the bug
timebug.c blais, 2007-05-28 02:29 chech mktime()'s behaviour
timebug.patch blais, 2007-05-28 02:30 the patch that pretends to fix this nasty buggerdibug bug!
mktime_fix_and_tests.patch vstinner, 2008-11-11 04:06
fix_mktime-2.patch vstinner, 2009-03-20 01:02
Messages (20)
msg52686 - (view) Author: Martin Blais (blais) * (Python committer) Date: 2007-05-28 02:27
There is a bug in datetime.fromtimestamp(), whereby if it is called with -1, it fails with "mktime argument out of range" when it should not (see attached test program to reproduce the problem).

The bug is that the way that mktime() signals an error code is subtle and error-prone: you need to set a sentinel in the tm's wday or yday and not only check the return value of mktime, but also check if those values have been modified; it figures: -1 is a valid value in the return domain of mktime() and is not a sufficient condition for signaling an error.

Here is the relevant excerpt from the Linux man page:


       The mktime() function converts a broken-down time structure,  expressed
       as  local  time, to calendar time representation.  The function ignores
       the specified contents of the structure members tm_wday and tm_yday and
       recomputes  them  from  the  other  information in the broken-down time
       structure.  If structure members are outside their legal interval, they
       will  be normalized (so that, e.g., 40 October is changed into 9 Novem-
       ber).  Calling mktime() also sets the  external  variable  tzname  with
       information  about the current time zone.  If the specified broken-down
       time cannot be represented as calendar time (seconds since the  epoch),
       mktime() returns a value of (time_t)(-1) and does not alter the tm_wday
       and tm_yday members of the broken-down time structure.


This was found under Linux, I do not know if this bug also occurs on Windows or the Mac.

I attached a couple of files:

- timebug.py: reproduce the bug
- timebug.c: tests that mktime()'s behaviour is as wicked as expected
- timebug.patch: the fix to the datetime module.



P.S. I hit this bug in a graphics application while zooming in/out of a viewer rendering time-based data. Sheer luck.


msg52687 - (view) Author: Martin Blais (blais) * (Python committer) Date: 2007-05-28 02:28
File Added: timebug.py
msg52688 - (view) Author: Martin Blais (blais) * (Python committer) Date: 2007-05-28 02:29
File Added: timebug.c
msg52689 - (view) Author: Martin Blais (blais) * (Python committer) Date: 2007-05-28 02:30
File Added: timebug.patch
msg52690 - (view) Author: Martin Blais (blais) * (Python committer) Date: 2007-05-28 02:38
(Additional note: this bug was found and fixed with Peter Wang from Enthought.)
msg52691 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2007-05-29 05:09
Reclassifying as a patch.
msg75728 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2008-11-11 04:06
The patch is correct. I tried to use errno, but errno is unchanged on 
error. Here is a new patch with regression tests.
msg75901 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2008-11-15 00:43
Can anyone review the last patch?
msg75918 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-11-15 22:33
on Windows (with Visual Studio), mktime() also sets tm_wday only if 
successful.

But negative time_t are still not allowed by the Microsoft CRT, the 
tests fail.
There are workaround to this - for example python could use techniques 
similar to http://robertinventor.com/software/t64/
 
OTOH, the docs of the time module explicitly says that dates before the 
Epoch are not handled. Do you want to change this? in other words: is 
this a bug or a feature request?
http://docs.python.org/library/time.html
msg80797 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2009-01-29 23:48
> But negative time_t are still not allowed by the Microsoft CRT, 
> the tests fail.
> (...)
> is this a bug or a feature request?

Linux mktime() supports any timestamp from 1901..2038. Should we limit 
the timestamp to 1970 just because of Microsoft? Test tm_wday fixes a 
bug on Linux and doesn't change the behaviour on Windows. So the 
problem is just the unit test: the test should be different on Windows 
(make sure that -1 raises an error).
msg80799 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2009-01-30 00:05
My test included in mktime_fix_and_tests.patch has a problem: the 
timezone is constant and it's mine (GMT+1). I don't know how to write 
a generic test working on any time zone. I can't use 
datetime.fromtimestamp() because datetime.fromtimestamp() uses 
time.mktime() :-)
msg83838 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2009-03-20 01:02
New version of my fix:
 - the test doesn't depend on _my_ local anymore: it uses localtime() 
to get the time tuple in the host local
 - ignore the test if mktime(-2) raise an OverflowError: avoid the 
test on Windows

Is it now ok for everyone?
msg83863 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-03-20 14:38
Is the "break" intended in the test function? it seems that this will
skip the whole test. Isn't "continue" better?
msg83864 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2009-03-20 15:20
@Amaury: You wrote:

 << But negative time_t are still not allowed by the Microsoft CRT, 
the tests fail.>>

So I choosed to skip mktime(-1) test if mktime(-2) fails.

I don't have Windows to test my patch nor current behaviour.
msg99528 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-02-18 21:22
Victor,

With issue2736, you included a variant of this patch where you use -1 as a sentinel value for tm_wday.  I think this is a better choice. (The answer to the ultimate question of life is not universally known.:-)

On a more serious note, is checking for the -1 return value necessary?  I would think a check for changed tm_wday would be enough.

Finally, a coding style nitpick: don't use superfluous parenthesis in logical expressions.
msg99540 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-02-18 22:28
I wonder: with year bounds being checked in gettmarg() and mktime accepting arbitrary values for the rest of the tm structure members (at least it appears to on my Mac), is it possible trigger "mktime argument out of range"?

If it is possible, then a unit test should be added for such case.  Note that the issue2736 patch contains a typo that assures that overflow is never reported, but the unit test presented here does not catch that bug:

"""
+	buf.tm_wday = -1;
 	tt = mktime(&buf);
-	if (tt == (time_t)(-1)) {
+	if (tt == (time_t)(-1) && buf.tm_wday == 1) {
 		PyErr_SetString(PyExc_OverflowError,
 				"mktime argument out of range");
"""
(Note missing '-' in buf.tm_wday == 1 check. See issue2736.)
msg108045 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-17 18:47
Is this important enough to try to get in 2.7 before rc2?  Victor?
msg108054 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2010-06-17 19:44
> Is this important enough to try to get in 2.7 before rc2?

I prefer to not include this patch in 2.7. I don't think that many people have this problem and it can be fixed later. It's too late for 2.7. Should it be fixed in 2.7.1 or only in 3.2 (and maybe in 3.1)?
msg125976 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2011-01-11 01:24
Committed in revision 87919.  If bots are happy about the unit test, this should be backported to 3.1 and 2.7.
msg128689 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2011-02-16 18:57
Backported in r88425 (3.1) and r88427 (2.7).
History
Date User Action Args
2011-02-16 18:57:10belopolskysetstatus: open -> closed
nosy: loewis, amaury.forgeotdarc, blais, belopolsky, vstinner
messages: + msg128689
2011-01-11 01:24:46belopolskysetnosy: loewis, amaury.forgeotdarc, blais, belopolsky, vstinner
messages: + msg125976
resolution: fixed
stage: test needed -> resolved
2010-06-17 19:47:26belopolskysetpriority: normal -> low
stage: patch review -> test needed
versions: + Python 3.1, - Python 2.7
2010-06-17 19:44:16vstinnersetmessages: + msg108054
2010-06-17 18:47:16belopolskysetversions: + Python 3.2, - Python 2.5, Python 3.1
nosy: loewis, amaury.forgeotdarc, blais, belopolsky, vstinner
messages: + msg108045

components: + Extension Modules, - None
type: behavior
2010-06-06 01:40:18belopolskysetassignee: belopolsky
nosy: + belopolsky, - Alexander.Belopolsky
2010-02-18 22:28:55Alexander.Belopolskysetmessages: + msg99540
2010-02-18 21:22:08Alexander.Belopolskysetnosy: + Alexander.Belopolsky
messages: + msg99528
2009-03-20 15:20:31vstinnersetmessages: + msg83864
2009-03-20 14:38:22amaury.forgeotdarcsetmessages: + msg83863
2009-03-20 01:02:24vstinnersetfiles: + fix_mktime-2.patch

messages: + msg83838
2009-01-30 00:05:28vstinnersetmessages: + msg80799
2009-01-29 23:48:38vstinnersetmessages: + msg80797
2008-11-15 22:33:47amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg75918
2008-11-15 00:43:55vstinnersetkeywords: + needs review
messages: + msg75901
stage: patch review
2008-11-11 04:07:00vstinnersetfiles: + mktime_fix_and_tests.patch
nosy: + vstinner
messages: + msg75728
versions: + Python 3.1, Python 2.7
2007-05-28 02:27:51blaiscreate