Repository navigation
/tmp does not exist on Android and is used by curses.window.putwin() #73362
Description
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 = None
closed_at = <Date 2017-03-27.11:15:29.155>
created_at = <Date 2017-01-06.10:47:00.717>
labels = ['type-security', 'extension-modules', '3.7']
title = '/tmp does not exist on Android and is used by curses.window.putwin()'
updated_at = <Date 2017-03-27.11:15:29.153>
user = 'https://github.com/xdegaye'bugs.python.org fields:
activity = <Date 2017-03-27.11:15:29.153>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-03-27.11:15:29.155>
closer = 'vstinner'
components = ['Extension Modules']
creation = <Date 2017-01-06.10:47:00.717>
creator = 'xdegaye'
dependencies = []
files = ['46266', '46267', '46622', '46623']
hgrepos = []
issue_num = 29176
keywords = ['patch']
message_count = 30.0
messages = ['284810', '284819', '285310', '285320', '285321', '285322', '287511', '287512', '287514', '287521', '287643', '287645', '288006', '288011', '288303', '288308', '288348', '288350', '288352', '288359', '288384', '288440', '288441', '288794', '288795', '288989', '290259', '290349', '290440', '290585']
nosy_count = 6.0
nosy_names = ['twouters', 'vstinner', 'christian.heimes', 'xdegaye', 'serhiy.storchaka', 'yan12125']
pr_nums = ['52', '53', '235', '237', '532']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue29176'
versions = ['Python 3.6', 'Python 3.7']
Activity
xdegaye commentedon Jan 6, 2017
======================================================================
ERROR: test_module_funcs (test.test_curses.TestCurses)
Test module-level functions
----------------------------------------------------------------------
yan12125 commentedon Jan 6, 2017
I guess replace mkstemp (C function) with tempfile.mkstemp (Python function) can solve the problem.
xdegaye commentedon Jan 12, 2017
The test suite runner in the test.libregrtest.main module calls tempfile.gettempdir() and mkstemp_curses.patch uses the same logic to fix the problem on Android where the tests that I run use TMPDIR=/data/local/tmp on Android API 24.
As a side note, when the curses library is ncurses-6.0, the library must be patched in ncurses/base/lib_screen.c with the attached ncurses.patch, see:
http://lists.gnu.org/archive/html/bug-ncurses/2016-05/msg00000.html
https://bugs.python.org/issue27323
vstinner commentedon Jan 12, 2017
The putwin() function uses mkstemp() with the path template /tmp/py.curses.getwin.XXXXXX.
I would prefer to use the Python function tempfile.mkstemp(). This function has a more portable behaviour. For example, Python is able to atomatically make the file descriptor non-inherirable if the OS supports this feature.
I suggest to expect a file descriptor in the C putwin() and write a Python putwin() which expects a file and pass file.fileno() if available, or use tempfile.TemporaryFile().
tempfile.TemporaryFile is even more secure because the file is not accessible from the regular file system on most platforms. This function is able to use the secure Linux O_TMPFILE flag and the O_TEMPORARY flag on Windows.
vstinner commentedon Jan 12, 2017
I change the issue type to security.
vstinner commentedon Jan 12, 2017
Oops, I missed Chi Hsuan Yen's comment:
So yes, I agree :-)
vstinner commentedon Feb 10, 2017
curses_temporaryfile.patch: Call tempfile.TemporaryFile in getwin() and putwin() to create the temporary file in a more portable and safer manner.
The curses library requires a FILE* object, but TemporaryFile.close() must be called to remove the temporary file. So I chose to duplicate the file descriptor, so we can close fclose() and tmpfile.close().
I chose to modify the C rather rather than trying to implement a window class in Python, because the C window class is part of a "PyCurses_API", and many C functions instanciate the C class.
vstinner commentedon Feb 10, 2017
curses_fix_window_class_name.patch: Fix the name of the C window class: "_curses.window", not "_curses.curses window" (with a space in the class name) !?
The following example current displays <class '_curses.curses window'>:
---
9 remaining items
tiran commentedon Feb 22, 2017
Thanks Chi Hsuan Yen,
I have created a new PR that replaces mkstemp() with tmpfile().
vstinner commentedon Feb 22, 2017
I created #237 using tmpfile(). The patch is simpler than my previous PR using Python tempfile.
Oh... I didn't see that Christian wrote almost the same change 20 min before me :-D
serhiy-storchaka commentedon Feb 22, 2017
The manpage of tmpfile contains the note:
Did you tested tmpfile() with readonly temporary files directory or overfull file system?
tiran commentedon Feb 22, 2017
I think we can live with the fact some tmpfile() might write an error message if the system is broken. If user's tmp is not writeable, the user has more serious issues than a write to stdout inside curses module.
serhiy-storchaka commentedon Feb 23, 2017
I hope that this never happen in real world, but in theory this change can introduce regression. That is why I think it should be documented in Misc/NEWS.
vstinner commentedon Feb 23, 2017
At least, I don't see such message in the Android implementation:
https://android.googlesource.com/platform/bionic/+/android-5.0.0_r1/libc/bionic/tmpfile.cpp
vstinner commentedon Mar 2, 2017
Xavier de Gaye, Chi Hsuan Yen: I just merged Christian Heimes's PR. So the function should now work on Android. Can you double check that it works? Is it worth it to backport the change to Python 3.6? Do you plan to try to have a full Android support in Python 3.6? Or do you target Python 3.7?
I don't think that the backport is worth it, but the patch is simple enough, so it's not a big deal if it makes your life simpler :-)
vstinner commentedon Mar 2, 2017
me: "I change the issue type to security"
I'm not sure anymore that the change is really related to security. I don't think that replacing mkstemp("/tmp/py.curses.putwin.XXXXXX") with tmpfile() has an impact on security. So I don't think that it's worth it to backport to change to Python 2.7 and 3.3-3.5.
yan12125 commentedon Mar 4, 2017
I've just tried with the latest git-master. With NDK r13b and my build scripts, [1] test_curses passed.
shell@ASUS_Z00E_2:/data/local/tmp $ python3.7m -m test test_curses -u curses
Run tests sequentially
0:00:00 [1/1] test_curses
1 test OK.
Total duration: 1 sec
Tests result: SUCCESS
abcshell@ASUS_Z00E_2:/data/local/tmp $
By the way, with NDK r14 and unified headers, [2] the test also passed. Note that this configuration is not officially supported by CPython yet. (bpo-29040)
[1] https://github.com/yan12125/python3-android/tree/ndk-r13
[2] https://github.com/yan12125/python3-android/tree/master
vstinner commentedon Mar 24, 2017
New changeset 7253ade by Victor Stinner (Mariatta) in branch '3.6':
bpo-29176: Fix name of the _curses.window class (#52) (#532)
7253ade
vstinner commentedon Mar 24, 2017
New changeset 2b221b7 by Victor Stinner (Christian Heimes) in branch 'master':
bpo-29176 Use tmpfile() in curses module (#235)
2b221b7
vstinner commentedon Mar 25, 2017
New changeset 61e2bc7 by Victor Stinner in branch 'master':
bpo-29176: Fix name of the _curses.window class (#52)
61e2bc7
vstinner commentedon Mar 27, 2017
Chi Hsuan Yen confirmed that "python3.7m -m test test_curses -u curses" now pass on Android, so I close the issue. Thanks.