classification
Title: /tmp does not exist on Android and is used by curses.window.putwin()
Type: security Stage: patch review
Components: Extension Modules Versions: Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Chi Hsuan Yen, christian.heimes, haypo, serhiy.storchaka, twouters, xdegaye
Priority: normal Keywords: patch

Created on 2017-01-06 10:47 by xdegaye, last changed 2017-02-23 11:00 by haypo.

Files
File name Uploaded Description Edit
ncurses.patch xdegaye, 2017-01-12 11:24
mkstemp_curses.patch xdegaye, 2017-01-12 11:32 review
curses_temporaryfile.patch haypo, 2017-02-10 10:54 review
curses_fix_window_class_name.patch haypo, 2017-02-10 10:58 review
Pull Requests
URL Status Linked Edit
PR 52 haypo, 2017-02-12 18:35
PR 53 haypo, 2017-02-12 18:46
PR 235 open christian.heimes, 2017-02-22 10:38
PR 237 closed haypo, 2017-02-22 10:55
Messages (23)
msg284810 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2017-01-06 10:47
======================================================================
ERROR: test_module_funcs (test.test_curses.TestCurses)
Test module-level functions
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/sdcard/org.bitbucket.pyona/lib/python3.7/test/test_curses.py", line 219, in test_module_fun
cs
    self.stdscr.putwin(f)
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/py.curses.putwin.nsIZYY'

----------------------------------------------------------------------
msg284819 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2017-01-06 13:30
I guess replace mkstemp (C function) with tempfile.mkstemp (Python function) can solve the problem.
msg285310 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2017-01-12 11:32
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
msg285320 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-12 13:16
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.
msg285321 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-12 13:16
I change the issue type to security.
msg285322 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-01-12 13:17
Oops, I missed Chi Hsuan Yen's comment:
> I guess replace mkstemp (C function) with tempfile.mkstemp (Python function) can solve the problem.

So yes, I agree :-)
msg287511 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-10 10:54
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.
msg287512 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-10 10:58
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'>:
---
import curses
w = curses.initscr()
curses.endwin()
print(type(w))
---
msg287514 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-10 11:10
Check that pickling/unpickling the windows object doesn't cause crash or undefined behaviour. Currently the pickling fails because the name of the class is not true name.
msg287521 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-10 11:54
Serhiy Storchaka: "Check that pickling/unpickling the windows object doesn't cause crash or undefined behaviour. Currently the pickling fails because the name of the class is not true name."

Ah, good idea.

I tested with curses_fix_window_class_name.patch: window objects cannot be pickled. Hopefully! IMHO it doesn't make sense to serialize such "live" object.

haypo@selma$ cat > x.py
import curses
w = curses.initscr()
curses.endwin()
print(type(w))
^D

haypo@selma$ ./python -i x.py
<class '_curses.window'>
>>> import pickle
>>> pickle.dumps(w)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't pickle _curses.window objects
msg287643 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-12 18:35
I created https://github.com/python/cpython/pull/52 for  curses_fix_window_class_name.patch.
msg287645 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-12 18:46
I created the PR https://github.com/python/cpython/pull/53 for  curses_temporaryfile.patch.
msg288006 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2017-02-17 13:51
Here are test results for PR 53:

shell@ASUS_Z00E_2:/data/local/tmp $ python3.7m -m test -u curses test_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 $ 

I know little about the C API. Just a question: isn't PyImport_ImportModuleNoBlock deprecated?
msg288011 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-17 14:59
Thank you for your check! Good to know that it works on Android.

Chi Hsuan Yen added the comment:
> I know little about the C API. Just a question: isn't PyImport_ImportModuleNoBlock deprecated?

I am not aware of any deprecation:
https://docs.python.org/dev/c-api/import.html#c.PyImport_ImportModuleNoBlock

Oh, it seems like the function is now more an less an alias to
PyImport_ImportModule().

I wasn't aware that the "NoBlock" is no more needed: I copied/pasted
from a different .c file, I don't recall which one. I should updated
other files ;-)
msg288303 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-02-21 14:55
How about https://linux.die.net/man/3/tmpfile instead?

The tmpfile() function opens a unique temporary file in binary read/write (w+b) mode. The file will be automatically deleted when it is closed or the program terminates.
msg288308 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-21 16:03
Is tmpfile() available on Android?
msg288348 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2017-02-22 10:06
> Is tmpfile() available on Android?

Yes. Just tried it with Android NDK r13 with clang and API 21 (Android 5.0). Here's the source code test.c:

#include <stdio.h>

int main()
{
    FILE *fp = NULL;
    char c = '\0';

    fp = tmpfile();
    fprintf(fp, "123\n");
    scanf("%c", &c);
    fclose(fp);
    return 0;
}

Compilation command:

/opt/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/clang -target aarch64-none-android-linux -gcc-toolchain /opt/android-ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64 --sysroot /opt/android-ndk/platforms/android-21/arch-arm64/usr -pie test.c

(didn't test gcc as it's declared as 'deprecated' in NDK)

Before scanf() returns, the temporary file is already deleted:

shell@ASUS_Z00E_2:/ $ ls -l /proc/19300/fd                                     
lrwx------ shell    shell             2017-02-22 15:21 0 -> /dev/pts/0
lrwx------ shell    shell             2017-02-22 15:21 1 -> /dev/pts/0
lrwx------ shell    shell             2017-02-22 15:21 2 -> /dev/pts/0
lrwx------ shell    shell             2017-02-22 15:20 3 -> /data/local/tmp/tmp.VguNf9sqcW (deleted)
lr-x------ shell    shell             2017-02-22 15:21 9 -> /dev/__properties__

For interested, here's its implementation: https://android.googlesource.com/platform/bionic/+/android-5.0.0_r1/libc/bionic/tmpfile.cpp
msg288350 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-02-22 10:53
Thanks Chi Hsuan Yen,

I have created a new PR that replaces mkstemp() with tmpfile().
msg288352 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-22 10:59
I created https://github.com/python/cpython/pull/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
msg288359 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-22 14:37
The manpage of tmpfile contains the note:

       POSIX.1-2001 specifies: an error message may be written to stdout if the stream cannot be opened.

Did you tested tmpfile() with readonly temporary files directory or overfull file system?
msg288384 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-02-22 21:09
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.
msg288440 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-23 10:47
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.
msg288441 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-23 11:00
> POSIX.1-2001 specifies: an error message may be written to stdout if the stream cannot be opened.

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
History
Date User Action Args
2017-02-23 11:00:20hayposetmessages: + msg288441
2017-02-23 10:47:14serhiy.storchakasetmessages: + msg288440
2017-02-22 21:09:08christian.heimessetmessages: + msg288384
2017-02-22 14:37:18serhiy.storchakasetmessages: + msg288359
2017-02-22 10:59:52hayposetmessages: + msg288352
2017-02-22 10:55:18hayposetpull_requests: + pull_request198
2017-02-22 10:53:46christian.heimessetmessages: + msg288350
2017-02-22 10:38:21christian.heimessetpull_requests: + pull_request196
2017-02-22 10:06:02Chi Hsuan Yensetmessages: + msg288348
2017-02-21 16:03:53hayposetmessages: + msg288308
2017-02-21 14:55:09christian.heimessetnosy: + christian.heimes
messages: + msg288303
2017-02-17 14:59:55hayposetmessages: + msg288011
2017-02-17 13:51:33Chi Hsuan Yensetmessages: + msg288006
2017-02-12 18:46:31hayposetmessages: + msg287645
pull_requests: + pull_request48
2017-02-12 18:35:44hayposetmessages: + msg287643
pull_requests: + pull_request47
2017-02-10 11:54:48hayposetmessages: + msg287521
2017-02-10 11:10:36serhiy.storchakasetmessages: + msg287514
2017-02-10 10:58:13hayposetfiles: + curses_fix_window_class_name.patch

messages: + msg287512
2017-02-10 10:54:17hayposetfiles: + curses_temporaryfile.patch

messages: + msg287511
2017-02-03 18:45:57xdegayesetassignee: xdegaye ->
2017-01-12 13:17:38hayposetmessages: + msg285322
2017-01-12 13:16:46hayposettype: behavior -> security
messages: + msg285321
2017-01-12 13:16:23hayposetmessages: + msg285320
2017-01-12 11:44:18serhiy.storchakasetnosy: + haypo, serhiy.storchaka

stage: needs patch -> patch review
2017-01-12 11:32:21xdegayesetfiles: + mkstemp_curses.patch

messages: + msg285310
2017-01-12 11:24:35xdegayesetfiles: + ncurses.patch
2017-01-06 21:30:19xdegayelinkissue26865 dependencies
2017-01-06 13:30:44Chi Hsuan Yensetnosy: + Chi Hsuan Yen
messages: + msg284819
2017-01-06 10:47:00xdegayecreate