Bug found in datetime for Epoch time = -1

[blais]

BPO	1726687
Nosy	@loewis, @amauryfa, @abalkin, @vstinner
Files	timebug.py: reproduce the bug timebug.c: chech mktime()'s behaviour timebug.patch: the patch that pretends to fix this nasty buggerdibug bug! mktime_fix_and_tests.patch fix_mktime-2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/abalkin'
closed_at = <Date 2011-02-16.18:57:10.410>
created_at = <Date 2007-05-28.02:27:51.000>
labels = ['extension-modules', 'type-bug']
title = 'Bug found in datetime for Epoch time = -1'
updated_at = <Date 2011-02-16.18:57:10.409>
user = 'https://bugs.python.org/blais'

bugs.python.org fields:

activity = <Date 2011-02-16.18:57:10.409>
actor = 'belopolsky'
assignee = 'belopolsky'
closed = True
closed_date = <Date 2011-02-16.18:57:10.410>
closer = 'belopolsky'
components = ['Extension Modules']
creation = <Date 2007-05-28.02:27:51.000>
creator = 'blais'
dependencies = []
files = ['8011', '8012', '8013', '11982', '13378']
hgrepos = []
issue_num = 1726687
keywords = ['patch', 'needs review']
message_count = 20.0
messages = ['52686', '52687', '52688', '52689', '52690', '52691', '75728', '75901', '75918', '80797', '80799', '83838', '83863', '83864', '99528', '99540', '108045', '108054', '125976', '128689']
nosy_count = 5.0
nosy_names = ['loewis', 'amaury.forgeotdarc', 'blais', 'belopolsky', 'vstinner']
pr_nums = []
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1726687'
versions = ['Python 3.1', 'Python 3.2']

[blais]

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.

[blais]

File Added: timebug.py

[blais]

File Added: timebug.c

[blais]

File Added: timebug.patch

[blais]

(Additional note: this bug was found and fixed with Peter Wang from Enthought.)

[loewis]

Reclassifying as a patch.

[vstinner]

The patch is correct. I tried to use errno, but errno is unchanged on
error. Here is a new patch with regression tests.

[vstinner]

Can anyone review the last patch?

[amauryfa]

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

[vstinner]

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).

[vstinner]

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() :-)

[vstinner]

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?

[amauryfa]

Is the "break" intended in the test function? it seems that this will
skip the whole test. Isn't "continue" better?

[vstinner]

@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.

[AlexanderBelopolsky]

Victor,

With bpo-2736, 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.