time.asctime segfaults when given a time in the far future (5+ digit year)

>>> import time
>>> time.asctime(time.gmtime(1e12))
Segmentation fault

I get this on Python 2.6, not 2.5.

And it seems to be limited to 64-bit.

(On Mac OS X)

Looks like a case of missing null check.  Patch attached.

New patch with tests and using reentrant functions.

Hi Alexander, can you confirm this bug is MacOs specific? I tried with python2.6 on a Debian sid @64 bit but I can't replicate it. Also, do you see it only on 2.6? if so, I doubt that it will ever be fixed; f.e. on release2.7 branch I have:

Python 2.7.1+ (release27-maint, Dec 31 2010, 20:16:57) 
[GCC 4.4.5] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import time
>>> time.asctime(time.gmtime(1e12))
'Fri Sep 27 01:46:40 33658\n'

It's still a problem on OS X at least and is 64-bit related:

$ arch -i386 /usr/local/bin/python3.2 -c 'import time;print(time.asctime(time.gmtime(1e12)))'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ValueError: timestamp out of range for platform time_t
$ arch -x86_64 /usr/local/bin/python3.2 -c 'import time;print(time.asctime(time.gmtime(1e12)))'
Segmentation fault

Hi Ned, thanks for the fast check! I tried to applied the patch (it failed, so it required a bit of manual editing) but when compiling I got:


/home/morph/python-dev/py3k/Modules/timemodule.c: In function ‘time_asctime’:
/home/morph/python-dev/py3k/Modules/timemodule.c:626: warning: implicit declaration of function ‘PyString_FromStringAndSize’
/home/morph/python-dev/py3k/Modules/timemodule.c:626: warning: return makes pointer from integer without a cast
*** WARNING: renaming "time" since importing it failed: build/lib.linux-x86_64-3.2/time.cpython-32m.so: undefined symbol: PyString_FromStringAndSize

and my knowledge of C ends there :)

Alexander, would you like to revamp your patch? ;)

Updated patch against py3k branch.

Thanks for the py3k patch.  I am also attaching a refreshed patch for current 2.7.  They both fix the segfaults when built and run on OS X 10.6 64-bit.  Since the patches change timemodule to use asctime_r, which AFAICT is not used elsewhere in the standard library, one concern might be if this change introduces a regression on any other platforms, something for the buildbots to test.

The patch is wrong: it hardcodes the number of characters that the time string has, but it can be more than 24 if the year is > 9999.  (Of course, the check for \n currently in the code is wrong too and must be fixed.)

Also, shouldn't the issue be handled as in ctime()?  There is a NULL check there, and by just doing that check we wouldn't depend on asctime_r().

The real problem with years >= 9999 is that it is undefined behaviour anyway (see e.g. http://pubs.opengroup.org/onlinepubs/9699919799/functions/asctime.html: "the behavior is undefined if the above algorithm would attempt to generate more than 26 bytes of output (including the terminating null)").

Sorry, I meant " years > 9999" of course.

Well, then I would have no problem with checking for that condition beforehand and raising ValueError.

On the other hand, it seems that implementations either return a correct string or NULL, so just erroring out in case of NULL would be fine as well.

On Sun, Jan 2, 2011 at 10:52 AM, Georg Brandl <report@bugs.python.org> wrote:
..
> Well, then I would have no problem with checking for that condition beforehand and raising
> ValueError.
>

IIRC, there was a similar bug report about ctime where pre-condition
checking was required because platform ctime would crash for huge
values of time.  I'll try to find the ticket.

> On the other hand, it seems that implementations either return a correct string or NULL,
> so just erroring out in case of NULL would be fine as well.

This is true on the platforms that I have access to: OSX, Linux, and
Solaris.  I think asctime_r is available and behaves this way on
Python supported platforms.  I'll check this in and watch the bots.

On Sun, Jan 2, 2011 at 1:52 PM, Alexander Belopolsky
<report@bugs.python.org> wrote:
..
>> Well, then I would have no problem with checking for that condition beforehand and raising
>> ValueError.
>>
>
> IIRC, there was a similar bug report about ctime where pre-condition
> checking was required because platform ctime would crash for huge
> values of time.  I'll try to find the ticket.

Hmm. My search brought up issue 10563, but the last message on that
issue says that "a change has been recently made to time.asctime() to
reject year > 9999.  See r85137 and issue6608."  I wonder if that
change made this issue moot.

On Sun, Jan 2, 2011 at 1:59 PM, Alexander Belopolsky
<report@bugs.python.org> wrote:
..
> Hmm. My search brought up issue 10563, but the last message on that
> issue says that "a change has been recently made to time.asctime() to
> reject year > 9999.  See r85137 and issue6608."  I wonder if that
> change made this issue moot.

It turns out the check added in r85137 does not cover tm_year even
though CERT recommends it (see msg107605).  These are separate issues
though. I think given where we are in the release cycle, the most
conservative solution would be to simply add a null check as follows.
(I did check that it fixes the crash on OSX.)

===================================================================
--- timemodule.c        (revision 87556)
+++ timemodule.c        (working copy)
@@ -620,6 +620,10 @@
     } else if (!gettmarg(tup, &buf) || !checktm(&buf))
         return NULL;
     p = asctime(&buf);
+    if (p == NULL) {
+        PyErr_SetString(PyExc_ValueError, "invalid time");
+        return NULL;
+    }
     if (p[24] == '\n')
         p[24] = '\0';
     return PyUnicode_FromString(p);

Committed in revision 87648.

Sasha, commit is not working. It doesn't pass test on Ubuntu and returns the string with a trailing \n. Seems like that hunk of code is misplaced.

> Sasha, commit is not working.

I suppose that the fix for the segfault is correct. The problem on Linux is the new test: asc

>>> import time; time.asctime((12345, 1, 0, 0, 0, 0, 0, 0, 0))
'Mon Jan  1 00:00:00 12345\n'

asctime() of the GNU libc doesn't fail. The test should maybe just calls the function without checking the result and ignores the exception.

Tests fixed to ignore ValueError in r87656.

Both asctime() and ctime() fixed to remove newline no matter how many digits the year has in r87657.  I also took the liberty of making the error messages consistent.

On Sun, Jan 2, 2011 at 5:35 PM, Georg Brandl <report@bugs.python.org> wrote:
..
> Both asctime() and ctime() fixed to remove newline no matter how many
> digits the year has in r87657.  I also took the liberty of making the error
> messages consistent.

Georg,

I disagree with your solution.  According to relevant standards,
asctime is undefined for year > 9999.  A compliant implementation can
do anything in this case including not null-terminating the internal
buffer.  With your change, time.strftime  will happily replace the
first unrelated '\n' with '\0' that it will find beyond the internal
buffer.

I was considering raising an ValueError if '\n' is not found at 24th
position, but this (or a precondition check solution) should wait
until 3.3.

In that case however, it's equally unsafe to not replace a \n, but still use PyUnicode_FromString() without a size given -- you will read from random memory.

Since all implementations we have or can test have a defined behavior in one way or the other, I think an example of an implementation that exhibits such undefined behavior is required first.

On Sun, Jan 2, 2011 at 6:01 PM, Georg Brandl <report@bugs.python.org> wrote:
..
>
> Since all implementations we have or can test have a defined behavior in one way or the other,
> I think an example of an implementation that exhibits such undefined behavior is required first.

No.  A CERT recommendation on how to write secure and portable code
should be enough.  See msg107605.

All right, then I wonder why your checktm() doesn't check the tm_year?

(What I mean is that overwriting \n or not, the code is unsafe, so the check must be done beforehand.  Why should that be left to 3.3?)

On Sun, Jan 2, 2011 at 6:10 PM, Georg Brandl <report@bugs.python.org> wrote:
..
> All right, then I wonder why your checktm() doesn't check the tm_year?

It is not mine.  I thought it did.  I might have missed that when I
reviewed the patch or there was a reason for that at the time.  Note
that there is a comment that says:

"""
tm_year: [0, max(int)] (1)
..
(1) gettmarg() handles bounds-checking.
"""

If you are ok with introducing stricter bounds checking in beta, I'll
try to get to the bottom of it shortly.

On Sun, Jan 2, 2011 at 6:17 PM, Georg Brandl <report@bugs.python.org> wrote:
..
> (What I mean is that overwriting \n or not, the code is unsafe, so the check must be
> done beforehand.  Why should that be left to 3.3?)

Reading beyond a buffer is somewhat safer than writing, but I agree
that checks must be done before calling asctime/ctime.   I thought it
would have to wait because it is a feature.  Some Linux users may
expect year 10000 to work.  (Maybe as a sentinel value somewhere.)
But you are the RM, so it is your call.

You cannot have both: a safe implementation and the correct behavior with glibc (not Linux!) -- except if you start special-casing.  Not sure that's worth it.

Note that time.asctime() is documented in time.rst to return a 24-character string, so what it currently returns with glibc is violating our docs as well.

On Sun, Jan 2, 2011 at 6:29 PM, Georg Brandl <report@bugs.python.org> wrote:
..
> You cannot have both: a safe implementation and the correct behavior with glibc
> (not Linux!) -- except if you start special-casing.  Not sure that's worth it.
>
That's the reason why this and the related ctime issue were lingering
for so long.

My plan was to pick the low-hanging fruit (the null check) for 3.3 and
leave proper bounds checking and possibly switch to reentrant APIs for
the next release.   There is a long tradition in keeping OS functions'
wrappers thin with an expectation that application programmers will
know the limitations/quirks of their target OSes.  Given that datetime
module does not have these issues, I don't see this as "must fix."

..
> My plan was to pick the low-hanging fruit (the null check) for 3.3 and

s/3.3/3.2/

> There is a long tradition in keeping OS functions'
> wrappers thin with an expectation that application programmers will
> know the limitations/quirks of their target OSes.

Sorry, but that does not apply if we trigger undefined behavior which
is inherently unsafe, as you rightly insist.

I don't see the range checking as particularly challenging; I'm sure you can get it done in time for 3.2.

On Sun, Jan 2, 2011 at 6:46 PM, Georg Brandl <report@bugs.python.org> wrote:
..
> I don't see the range checking as particularly challenging; I'm sure you can get it done in time for 3.2.

Will do.

I'm not sure that whether it's related to the current issue, but asctime doesn't seem to accept years < 1900. Which might be fair enough, has this been documented.

It's documented under "Year 2000 (Y2K) issues": http://docs.python.org/library/time.html#time-y2kissues

Backported in r87664 (2.6), r87663 (2.7) and r87662 (3.1).

Reopening to implement bounds checking for 3.2.  Note that for time.asctime, checking the year range is trivial, but for time.ctime it is not because year is not computed in python code.  One solution may be to check bounds of the time_t timestamp, but those depend on the timezone.  Maybe it is easiest to simply call mktime(), check year and call asctime() bypassing OS ctime completely.

yes, sorry. what I meant to say is that fixing only upper bound for the year (according to CERT recommendation cited above) and leaving the lower bound in its current state is somewhat unsatisfactory.

> http://docs.python.org/library/time.html#time-y2kissues
> "Values 100–1899 are always illegal."

Why are these values illegal? The GNU libc accepts year in [1900-2^31; 2^31-1] (tm_year in [-2147483648; 2147481747]). If time.accept2dyear=False, we should at least accept years in [1; 9999]. The system libc would raise an error (return NULL) if it doesn't know how to format years older than 1900.

test_time fails with an (C) assertion error on Windows: see issue #10814.

Attached patch, issue8013-pre-check.diff, checks for year range before calling system asctime and replaces a call to ctime with a asctime(localtime(..)).

All tests pass and all works as documented with the latest patch on Ubuntu (glibc 2.11).

Following Guido's response [1] to my inquiry on python-dev, I am attaching a new patch that makes time.asctime and time.ctime produce longer than 24-character strings for large years. 

[1] http://mail.python.org/pipermail/python-dev/2011-January/107187.html

Committed in r87736.  I opened a separate issue #10827 to address the lower bound.

Thanks!

[Strictly speaking, this is actually issue #10563, but that was marked superseded by the changes for this issue, hence the comment here.]

The 5-digit year still displays an extra newline in Python 2.7.11 (there's extra whitespace on OSX as well, but that seems to due to the fact that ctime() returns 30 bytes on OSX instead of the documented 26):

{~}$ /usr/local/bin/python
Python 2.7.11 (default, Jan 22 2016, 08:29:18)
[GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import time
>>> time.ctime(253402300799)
'Fri Dec 31 23:59:59 9999'
>>> time.ctime(253402300800)
'Sat Jan  1 00:00:00     10000\n'

...and, as far as I can see, the patch hasn't been applied to py2.7 yet (https://hg.python.org/cpython/file/2.7/Modules/timemodule.c#l579)


Please let me know if I am missing something obvious (I'm pretty new to the bugtracker), or if this issue is planned not to be fixed for versions < 3.0. 

If not, should this issue be reopened, or the py27-specific problem be migrated to a new issue?