Skip to content

bpo-31339: Rewrite time.asctime() and time.ctime() #3293

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

Merged
merged 3 commits into from
Sep 5, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 75 additions & 9 deletions Lib/test/test_time.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
import time
import unittest
import sys
import sysconfig


# Max year is only limited by the size of C int.
SIZEOF_INT = sysconfig.get_config_var('SIZEOF_INT') or 4
TIME_MAXYEAR = (1 << 8 * SIZEOF_INT - 1) - 1


class TimeTestCase(unittest.TestCase):
Expand Down Expand Up @@ -45,6 +51,66 @@ def test_strftime(self):
with self.assertRaises(ValueError):
time.strftime('%f')

def _bounds_checking(self, func):
# Make sure that strftime() checks the bounds of the various parts
# of the time tuple (0 is valid for *all* values).

# The year field is tested by other test cases above

# Check month [1, 12] + zero support
func((1900, 0, 1, 0, 0, 0, 0, 1, -1))
func((1900, 12, 1, 0, 0, 0, 0, 1, -1))
self.assertRaises(ValueError, func,
(1900, -1, 1, 0, 0, 0, 0, 1, -1))
self.assertRaises(ValueError, func,
(1900, 13, 1, 0, 0, 0, 0, 1, -1))
# Check day of month [1, 31] + zero support
func((1900, 1, 0, 0, 0, 0, 0, 1, -1))
func((1900, 1, 31, 0, 0, 0, 0, 1, -1))
self.assertRaises(ValueError, func,
(1900, 1, -1, 0, 0, 0, 0, 1, -1))
self.assertRaises(ValueError, func,
(1900, 1, 32, 0, 0, 0, 0, 1, -1))
# Check hour [0, 23]
func((1900, 1, 1, 23, 0, 0, 0, 1, -1))
self.assertRaises(ValueError, func,
(1900, 1, 1, -1, 0, 0, 0, 1, -1))
self.assertRaises(ValueError, func,
(1900, 1, 1, 24, 0, 0, 0, 1, -1))
# Check minute [0, 59]
func((1900, 1, 1, 0, 59, 0, 0, 1, -1))
self.assertRaises(ValueError, func,
(1900, 1, 1, 0, -1, 0, 0, 1, -1))
self.assertRaises(ValueError, func,
(1900, 1, 1, 0, 60, 0, 0, 1, -1))
# Check second [0, 61]
self.assertRaises(ValueError, func,
(1900, 1, 1, 0, 0, -1, 0, 1, -1))
# C99 only requires allowing for one leap second, but Python's docs say
# allow two leap seconds (0..61)
func((1900, 1, 1, 0, 0, 60, 0, 1, -1))
func((1900, 1, 1, 0, 0, 61, 0, 1, -1))
self.assertRaises(ValueError, func,
(1900, 1, 1, 0, 0, 62, 0, 1, -1))
# No check for upper-bound day of week;
# value forced into range by a ``% 7`` calculation.
# Start check at -2 since gettmarg() increments value before taking
# modulo.
self.assertEqual(func((1900, 1, 1, 0, 0, 0, -1, 1, -1)),
func((1900, 1, 1, 0, 0, 0, +6, 1, -1)))
self.assertRaises(ValueError, func,
(1900, 1, 1, 0, 0, 0, -2, 1, -1))
# Check day of the year [1, 366] + zero support
func((1900, 1, 1, 0, 0, 0, 0, 0, -1))
func((1900, 1, 1, 0, 0, 0, 0, 366, -1))
self.assertRaises(ValueError, func,
(1900, 1, 1, 0, 0, 0, 0, -1, -1))
self.assertRaises(ValueError, func,
(1900, 1, 1, 0, 0, 0, 0, 367, -1))

def test_strftime_bounding_check(self):
self._bounds_checking(lambda tup: time.strftime('', tup))

def test_strftime_bounds_checking(self):
# Make sure that strftime() checks the bounds of the various parts
#of the time tuple (0 is valid for *all* values).
Expand Down Expand Up @@ -123,15 +189,15 @@ def test_asctime(self):
time.asctime(time.gmtime(self.t))
self.assertRaises(TypeError, time.asctime, 0)
self.assertRaises(TypeError, time.asctime, ())
# XXX: Posix compiant asctime should refuse to convert
# year > 9999, but Linux implementation does not.
# self.assertRaises(ValueError, time.asctime,
# (12345, 1, 0, 0, 0, 0, 0, 0, 0))
# XXX: For now, just make sure we don't have a crash:
try:
time.asctime((12345, 1, 1, 0, 0, 0, 0, 1, 0))
except ValueError:
pass

# Max year is only limited by the size of C int.
asc = time.asctime((TIME_MAXYEAR, 6, 1) + (0,) * 6)
self.assertEqual(asc[-len(str(TIME_MAXYEAR)):], str(TIME_MAXYEAR))
self.assertRaises(OverflowError, time.asctime,
(TIME_MAXYEAR + 1,) + (0,) * 8)
self.assertRaises(TypeError, time.asctime, 0)
self.assertRaises(TypeError, time.asctime, ())
self.assertRaises(TypeError, time.asctime, (0,) * 10)

@unittest.skipIf(not hasattr(time, "tzset"),
"time module has no attribute tzset")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Rewrite time.asctime() and time.ctime(). Backport and adapt the _asctime()
function from the master branch to not depend on the implementation of
asctime() and ctime() from the external C library. This change fixes a bug
when Python is run using the musl C library.
134 changes: 116 additions & 18 deletions Modules/timemodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,76 @@ gettmarg(PyObject *args, struct tm *p)
return 1;
}

/* Check values of the struct tm fields before it is passed to strftime() and
* asctime(). Return 1 if all values are valid, otherwise set an exception
* and returns 0.
*/
static int
checktm(struct tm* buf)
{
/* Checks added to make sure strftime() and asctime() does not crash Python by
indexing blindly into some array for a textual representation
by some bad index (fixes bug #897625 and #6608).

Also support values of zero from Python code for arguments in which
that is out of range by forcing that value to the lowest value that
is valid (fixed bug #1520914).

Valid ranges based on what is allowed in struct tm:

- tm_year: [0, max(int)] (1)
- tm_mon: [0, 11] (2)
- tm_mday: [1, 31]
- tm_hour: [0, 23]
- tm_min: [0, 59]
- tm_sec: [0, 60]
- tm_wday: [0, 6] (1)
- tm_yday: [0, 365] (2)
- tm_isdst: [-max(int), max(int)]

(1) gettmarg() handles bounds-checking.
(2) Python's acceptable range is one greater than the range in C,
thus need to check against automatic decrement by gettmarg().
*/
if (buf->tm_mon == -1)
buf->tm_mon = 0;
else if (buf->tm_mon < 0 || buf->tm_mon > 11) {
PyErr_SetString(PyExc_ValueError, "month out of range");
return 0;
}
if (buf->tm_mday == 0)
buf->tm_mday = 1;
else if (buf->tm_mday < 0 || buf->tm_mday > 31) {
PyErr_SetString(PyExc_ValueError, "day of month out of range");
return 0;
}
if (buf->tm_hour < 0 || buf->tm_hour > 23) {
PyErr_SetString(PyExc_ValueError, "hour out of range");
return 0;
}
if (buf->tm_min < 0 || buf->tm_min > 59) {
PyErr_SetString(PyExc_ValueError, "minute out of range");
return 0;
}
if (buf->tm_sec < 0 || buf->tm_sec > 61) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that leap seconds are limited to maximum 2 seconds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO supporting tm_sec=61 is already crazy. I expect seconds to be in the inclusive range 0..60. Linux asctime() and mktime() documentation say:

int tm_sec;    /* Seconds (0-60) */

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect a minimum of one leap second per day. Currently, it seems to be limited to 1 leap second per UTC month. In practice, it's one leap second every 18 months.
https://en.wikipedia.org/wiki/Leap_second#Insertion_of_leap_seconds

Note: I copied the code from master without reading it :-)

PyErr_SetString(PyExc_ValueError, "seconds out of range");
return 0;
}
/* tm_wday does not need checking of its upper-bound since taking
``% 7`` in gettmarg() automatically restricts the range. */
if (buf->tm_wday < 0) {
PyErr_SetString(PyExc_ValueError, "day of week out of range");
return 0;
}
if (buf->tm_yday == -1)
buf->tm_yday = 0;
else if (buf->tm_yday < 0 || buf->tm_yday > 365) {
PyErr_SetString(PyExc_ValueError, "day of year out of range");
return 0;
}
return 1;
}

#ifdef HAVE_STRFTIME
static PyObject *
time_strftime(PyObject *self, PyObject *args)
Expand All @@ -407,8 +477,10 @@ time_strftime(PyObject *self, PyObject *args)
if (tup == NULL) {
time_t tt = time(NULL);
buf = *localtime(&tt);
} else if (!gettmarg(tup, &buf))
} else if (!gettmarg(tup, &buf)
|| !checktm(&buf)) {
return NULL;
}

/* Checks added to make sure strftime() does not crash Python by
indexing blindly into some array for a textual representation
Expand Down Expand Up @@ -558,27 +630,51 @@ Parse a string to a time tuple according to a format specification.\n\
See the library reference manual for formatting codes (same as strftime()).");


static PyObject *
_asctime(struct tm *timeptr)
{
/* Inspired by Open Group reference implementation available at
* http://pubs.opengroup.org/onlinepubs/009695399/functions/asctime.html */
static const char wday_name[7][4] = {
"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"
};
static const char mon_name[12][4] = {
"Jan", "Feb", "Mar", "Apr", "May", "Jun",
"Jul", "Aug", "Sep", "Oct", "Nov", "Dec"
};
PyObject *unicode, *str;
/* PyString_FromString() cannot be used because it doesn't support %3d */
unicode = PyUnicode_FromFormat(
"%s %s%3d %.2d:%.2d:%.2d %d",
wday_name[timeptr->tm_wday],
mon_name[timeptr->tm_mon],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function should verify that tm_wday and tm_mon are not under- or overflowing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such check is done in the caller:

>>> time.asctime((2017, 13, 4, 22, 44, 18, 0, 247, 1))
ValueError: month out of range

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, sorry, I was checking Python 3.

You are right that my PR is wrong:

>>> time.asctime((2017, 13, 4, 22, 44, 18, 0, 247, 1))
'Mon ???  4 22:44:18 2017'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's now fixed:

>>> time.asctime((2017, 13, 4, 22, 44, 18, 0, 247, 1))
ValueError: month out of range

timeptr->tm_mday, timeptr->tm_hour,
timeptr->tm_min, timeptr->tm_sec,
1900 + timeptr->tm_year);
if (unicode == NULL) {
return NULL;
}

str = PyUnicode_AsASCIIString(unicode);
Py_DECREF(unicode);
return str;
}

static PyObject *
time_asctime(PyObject *self, PyObject *args)
{
PyObject *tup = NULL;
struct tm buf;
char *p;
if (!PyArg_UnpackTuple(args, "asctime", 0, 1, &tup))
return NULL;
if (tup == NULL) {
time_t tt = time(NULL);
buf = *localtime(&tt);
} else if (!gettmarg(tup, &buf))
return NULL;
p = asctime(&buf);
if (p == NULL) {
PyErr_SetString(PyExc_ValueError, "invalid time");
} else if (!gettmarg(tup, &buf)
|| !checktm(&buf)) {
return NULL;
}
if (p[24] == '\n')
p[24] = '\0';
return PyString_FromString(p);
return _asctime(&buf);
}

PyDoc_STRVAR(asctime_doc,
Expand All @@ -593,7 +689,7 @@ time_ctime(PyObject *self, PyObject *args)
{
PyObject *ot = NULL;
time_t tt;
char *p;
struct tm *buf;

if (!PyArg_UnpackTuple(args, "ctime", 0, 1, &ot))
return NULL;
Expand All @@ -607,14 +703,16 @@ time_ctime(PyObject *self, PyObject *args)
if (tt == (time_t)-1 && PyErr_Occurred())
return NULL;
}
p = ctime(&tt);
if (p == NULL) {
PyErr_SetString(PyExc_ValueError, "unconvertible time");
return NULL;
buf = localtime(&tt);
if (buf == NULL) {
#ifdef EINVAL
if (errno == 0) {
errno = EINVAL;
}
#endif
return PyErr_SetFromErrno(PyExc_ValueError);
}
if (p[24] == '\n')
p[24] = '\0';
return PyString_FromString(p);
return _asctime(buf);
}

PyDoc_STRVAR(ctime_doc,
Expand Down