New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug found in datetime for Epoch time = -1 #45007
Comments
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:
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:
P.S. I hit this bug in a graphics application while zooming in/out of a viewer rendering time-based data. Sheer luck. |
File Added: timebug.py |
File Added: timebug.c |
File Added: timebug.patch |
(Additional note: this bug was found and fixed with Peter Wang from Enthought.) |
Reclassifying as a patch. |
The patch is correct. I tried to use errno, but errno is unchanged on |
Can anyone review the last patch? |
on Windows (with Visual Studio), mktime() also sets tm_wday only if But negative time_t are still not allowed by the Microsoft CRT, the OTOH, the docs of the time module explicitly says that dates before the |
Linux mktime() supports any timestamp from 1901..2038. Should we limit |
My test included in mktime_fix_and_tests.patch has a problem: the |
New version of my fix:
Is it now ok for everyone? |
Is the "break" intended in the test function? it seems that this will |
@Amaury: You wrote: << But negative time_t are still not allowed by the Microsoft CRT, So I choosed to skip mktime(-1) test if mktime(-2) fails. I don't have Windows to test my patch nor current behaviour. |
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. |
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 bpo-2736 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.) |
Is this important enough to try to get in 2.7 before rc2? Victor? |
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)? |
Committed in revision 87919. If bots are happy about the unit test, this should be backported to 3.1 and 2.7. |
Backported in r88425 (3.1) and r88427 (2.7). |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: