classification
Title: Using time.asctime() with an array with negative tm_hour causes Python Crash.
Type: crash Stage: resolved
Components: Extension Modules, Windows Versions: Python 2.7
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: benjamin.peterson Nosy List: Viktor.Chynarov, belopolsky, benjamin.peterson, eryksun, kushal.das, lemburg, paul.moore, r.david.murray, serhiy.storchaka, steve.dower, tim.golden, vstinner, zach.ware
Priority: critical Keywords: patch

Created on 2012-10-04 22:35 by Viktor.Chynarov, last changed 2017-09-05 23:46 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
asctime.patch serhiy.storchaka, 2012-11-18 19:45 review
asctime_s.patch serhiy.storchaka, 2013-02-05 12:30 review
asctime_s_2.patch serhiy.storchaka, 2013-02-05 14:33 review
Messages (26)
msg172005 - (view) Author: Viktor Chynarov (Viktor.Chynarov) Date: 2012-10-04 22:35
If a that has a negative tm_hour is passed as an argument to time.asctime(), Python crashes. 

>>> initial_struct_time = [tm for tm in time.localtime()]
>>> initial_struct_time[3] = -1
>>> faulty_time = time.asctime(initial_struct_time)
msg172008 - (view) Author: Viktor Chynarov (Viktor.Chynarov) Date: 2012-10-04 22:36
If an array created from a struct_time that has a negative tm_hour is passed as an argument to time.asctime(), Python crashes. 

>>> initial_struct_time = [tm for tm in time.localtime()]
>>> initial_struct_time[3] = -1
>>> faulty_time = time.asctime(initial_struct_time)
msg172016 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-04 22:52
I can't reproduce this.  What version of 2.7?  (This might have been fixed by the issue 8013 fix).
msg172048 - (view) Author: Kushal Das (kushal.das) * (Python committer) Date: 2012-10-05 04:43
Can not reproduce this on 2.7.3 on Fedora 17, x86_64.
msg172098 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2012-10-05 16:48
I can reproduce this on Windows Vista running 2.7.3.  With 3.3.0 I get
>>> faulty_time = time.asctime(initial_struct_time)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Tuple or struct_time argument required
>>>
msg172108 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-05 18:47
Viktor, what happen when a large positive number (>=100) used as tm_hour?
msg172142 - (view) Author: Viktor Chynarov (Viktor.Chynarov) Date: 2012-10-05 20:58
Serhiy, when I use a large number, Python also crashes.

>>> initial_struct_time = [tm for tm in time.localtime()]
>>> initial_struct_time[3] = 101
>>> faulty_time = time.asctime(initial_struct_time)

The above code leads to crash.

My Python version information and computer architecture are shown below:
Python 2.7.3 (default, Apr 10 2012, 23:24:47) [MSC v.1500 64 bit (AMD64)] on win32
msg172145 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-05 21:10
Yes, it looks as issue8013. Fix was not applied to 2.7.
msg172146 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-05 21:10
This sounds like Issue 10814, but that was supposedly fixed.
msg172147 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-10-05 21:12
Ah, looks like Serhiy found the correct issue.
msg172150 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-05 21:26
No, it's you found (msg172016) the correct issue. ;)
msg173360 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-19 21:50
Alexander, looks as you changes in r87736 was not backported to 2.7.
msg175470 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-12 20:37
Alexander, do you want to backport r87736 to 2.7?
msg175911 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-18 19:45
Here is a patch which includes backported issue6608 and issue8013.  This should fix this crash.
msg175920 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2012-11-18 20:25
I recall a discussion in which it was argued that "look before you leap"-style argument checking that we implemented in py3k was a feature and backporting it to 2.x could potentially break code running on platforms with promiscuous (and possibly buggy) asctime().

I am +0 on back-porting, but would like someone else to make the call.
msg180764 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-27 12:38
Any news?
msg181401 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2013-02-05 00:35
Benjamin,

I am assigning this to you because 2.7.4 release is probably the last chance to do something about this behavior in 2.7 series.

I am tentatively resolving this as "won't fix."  In 3.x, we decided that well defined behavior is more important than bug compatibility on broken platforms.  For 2.x, however, the priorities are different.  In this particular case, it is very easy to work around platform bug, but if we add a bound check, we may break code that works.

For example, on a recent Mac OS X release and preloaded Python 2.7, I get:

Python 2.7.2 (default, Jun 20 2012, 16:23:33)
[GCC 4.2.1 Compatible Apple Clang 4.0 (tags/Apple/clang-418.0.60)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import time
>>> time.asctime((2011,1,1,-1,0,0,0,0,0))
'Mon Jan  1 -01:00:00 2011\n'

This behavior is not "obviously wrong."

Please close this or make it a release blocker.  I don't think there is any value in letting this linger past 2.7.4 release.
msg181412 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-02-05 03:41
This is long behavior standing, which we can leave in 2.x.
msg181429 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-05 11:41
> 'Mon Jan  1 -01:00:00 2011\n'

This is obviously wrong because trailing '\n' was not dropped.

The issue is not about what is more wrong. The issue is about Python crash. At least we should add a warning in the documentation that incorrect data may crash Python on some platforms.
msg181430 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-05 12:27
A new patch uses asctime_s() on Windows (this crash was observed only on Windows). This should prevent a crash. In additional it drops '\n' from a result even if the result is longer than usually (this happened on Linux when an invalid time is used).

Please run test_time on Windows.
msg181432 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-02-05 13:13
asctime_s.patch: you should catch ValueError on each call to
self.assertNotIn(). You can use PyString_FromStringAndSize() instead
of writing the NUL character, it may be safer (to not modify the
output of asctime).

2013/2/5 Serhiy Storchaka <report@bugs.python.org>:
>
> Changes by Serhiy Storchaka <storchaka@gmail.com>:
>
>
> Added file: http://bugs.python.org/file28958/asctime_s.patch
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue16137>
> _______________________________________
msg181439 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-05 14:33
Thank you for comments, Victor. Here is an updated patch.
msg269294 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-26 16:14
Benjamin, there is a crash, and there is a simple patch that presumably fixes a crash. I think fixing crashes has high priority. The patch is tested on Linux and needs to be tested on Windows.
msg269305 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-06-26 19:37
To resolve the crash on Windows in 2.7 requires backporting checktm(). Using asctime_s doesn't solve the problem. The CRT still calls the default invalid parameter handler, which kills the process -- as shown by the following ctypes example:

    from ctypes import *
    libc = CDLL('msvcr90')

    class tm(Structure):
        _fields_ = (('tm_sec', c_int),
                    ('tm_min', c_int),
                    ('tm_hour', c_int),
                    ('tm_mday', c_int),
                    ('tm_mon', c_int),
                    ('tm_year', c_int),
                    ('tm_wday', c_int),
                    ('tm_yday', c_int),
                    ('tm_isdst', c_int))

    libc._localtime32.restype = POINTER(tm)
    libc.asctime_s.restype = c_char_p

    t = c_int()
    libc._time32(byref(t))
    lt = libc._localtime32(byref(t))
    sbuf = (c_char * 100)()

    >>> libc.asctime_s(sbuf, sizeof(sbuf), lt)
    >>> sbuf.value
    'Sun Jun 26 19:22:47 2016\n'

    >>> lt[0].tm_hour = -1
    >>> libc.asctime_s(sbuf, sizeof(sbuf), lt)

    Breakpoint 0 hit
    MSVCR90!_invoke_watson:
    00000000`6b8950e4 4053            push    rbx
    0:000> k5
    Child-SP          RetAddr           Call Site
    00000000`005ef628 00000000`6b8952d4 MSVCR90!_invoke_watson
    00000000`005ef630 00000000`6b859830 MSVCR90!_invalid_parameter+0x70
    00000000`005ef670 00000000`1d1aff53 MSVCR90!asctime_s+0x2fc
    00000000`005ef6b0 00000000`1d1ae7fc _ctypes!ffi_call_AMD64+0x77
    00000000`005ef700 00000000`1d1aa4c6 _ctypes!ffi_call+0x8c
msg269579 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-30 12:25
Thank you Eryk.

Sad, a simple solution doesn't help. We have returned to the old decision -- won't fix.
msg301406 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-05 23:46
This issue should be fixed by my commit eeadf5fc231163ec97a8010754d9c995c7c14876.
History
Date User Action Args
2017-09-05 23:46:14vstinnersetmessages: + msg301406
2016-06-30 12:25:30serhiy.storchakasetstatus: open -> closed

messages: + msg269579
stage: patch review -> resolved
2016-06-26 19:53:29BreamoreBoysetnosy: - BreamoreBoy
2016-06-26 19:37:05eryksunsetnosy: + eryksun
messages: + msg269305
2016-06-26 16:14:55serhiy.storchakasetstatus: closed -> open

nosy: + paul.moore, tim.golden, zach.ware, steve.dower
messages: + msg269294

components: + Windows
2013-02-05 14:33:29serhiy.storchakasetfiles: + asctime_s_2.patch

messages: + msg181439
2013-02-05 13:13:44vstinnersetmessages: + msg181432
2013-02-05 12:30:25serhiy.storchakasetfiles: + asctime_s.patch
2013-02-05 12:27:05serhiy.storchakasetmessages: + msg181430
2013-02-05 11:41:32serhiy.storchakasetmessages: + msg181429
2013-02-05 03:41:03benjamin.petersonsetstatus: pending -> closed

messages: + msg181412
2013-02-05 00:35:26belopolskysetstatus: open -> pending

nosy: + benjamin.peterson
messages: + msg181401

assignee: benjamin.peterson
resolution: wont fix
2013-01-27 12:38:03serhiy.storchakasetpriority: normal -> critical

messages: + msg180764
2012-11-18 20:25:47belopolskysetnosy: + lemburg, vstinner
messages: + msg175920
2012-11-18 19:45:16serhiy.storchakasetfiles: + asctime.patch
keywords: + patch
messages: + msg175911

stage: needs patch -> patch review
2012-11-12 20:37:13serhiy.storchakasetmessages: + msg175470
2012-10-19 21:50:00serhiy.storchakasetnosy: + belopolsky
messages: + msg173360

components: + Extension Modules, - Library (Lib)
stage: needs patch
2012-10-05 21:26:51serhiy.storchakasetmessages: + msg172150
2012-10-05 21:12:00r.david.murraysetmessages: + msg172147
2012-10-05 21:10:55r.david.murraysetmessages: + msg172146
2012-10-05 21:10:15serhiy.storchakasetmessages: + msg172145
2012-10-05 20:58:01Viktor.Chynarovsetmessages: + msg172142
2012-10-05 18:47:03serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg172108
2012-10-05 16:48:34BreamoreBoysetnosy: + BreamoreBoy
messages: + msg172098
2012-10-05 04:43:10kushal.dassetnosy: + kushal.das
messages: + msg172048
2012-10-04 22:52:39r.david.murraysetnosy: + r.david.murray
messages: + msg172016
2012-10-04 22:36:51Viktor.Chynarovsetmessages: + msg172008
2012-10-04 22:35:43Viktor.Chynarovcreate