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
/tmp does not exist on Android and is used by curses.window.putwin() #73362
Comments
====================================================================== 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' |
I guess replace mkstemp (C function) with tempfile.mkstemp (Python function) can solve the problem. |
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: |
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. |
I change the issue type to security. |
Oops, I missed Chi Hsuan Yen's comment:
So yes, I agree :-) |
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. |
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)) |
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. |
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 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 |
I created #52 for curses_fix_window_class_name.patch. |
I created the PR #53 for curses_temporaryfile.patch. |
Here are test results for PR 53: shell@ASUS_Z00E_2:/data/local/tmp $ python3.7m -m test -u curses test_curses Total duration: 1 sec I know little about the C API. Just a question: isn't PyImport_ImportModuleNoBlock deprecated? |
Thank you for your check! Good to know that it works on Android. Chi Hsuan Yen added the comment:
I am not aware of any deprecation: Oh, it seems like the function is now more an less an alias to I wasn't aware that the "NoBlock" is no more needed: I copied/pasted |
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. |
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 For interested, here's its implementation: https://android.googlesource.com/platform/bionic/+/android-5.0.0_r1/libc/bionic/tmpfile.cpp |
Thanks Chi Hsuan Yen, I have created a new PR that replaces mkstemp() with tmpfile(). |
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 |
The manpage of tmpfile contains the note:
Did you tested tmpfile() with readonly temporary files directory or overfull file system? |
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. |
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. |
At least, I don't see such message in the Android implementation: |
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 :-) |
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. |
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 Total duration: 1 sec 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 |
Chi Hsuan Yen confirmed that "python3.7m -m test test_curses -u curses" now pass on Android, so I close the issue. Thanks. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: