Skip to content
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

Closed
xdegaye mannequin opened this issue Jan 6, 2017 · 30 comments
Closed

/tmp does not exist on Android and is used by curses.window.putwin() #73362

xdegaye mannequin opened this issue Jan 6, 2017 · 30 comments
Labels
3.7 extension-modules C modules in the Modules dir type-security A security issue

Comments

@xdegaye
Copy link
Mannequin

xdegaye mannequin commented Jan 6, 2017

BPO 29176
Nosy @Yhg1s, @vstinner, @tiran, @xdegaye, @serhiy-storchaka, @yan12125
PRs
  • bpo-29176: Fix name of the _curses.window class #52
  • curses: use tempfile.TemporaryFile for get/putwin #53
  • bpo-29176 Use tmpfile() in curses module #235
  • bpo-29176: curses: use tmpfile() for getwin/putwin #237
  • [3.6] bpo-29176: Fix name of the _curses.window class (GH-52) #532
  • Files
  • ncurses.patch
  • mkstemp_curses.patch
  • curses_temporaryfile.patch
  • curses_fix_window_class_name.patch
  • 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']

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented Jan 6, 2017

    ======================================================================
    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'

    @xdegaye xdegaye mannequin added the type-bug An unexpected behavior, bug, or error label Jan 6, 2017
    @xdegaye xdegaye mannequin self-assigned this Jan 6, 2017
    @xdegaye xdegaye mannequin added 3.7 extension-modules C modules in the Modules dir labels Jan 6, 2017
    @yan12125
    Copy link
    Mannequin

    yan12125 mannequin commented Jan 6, 2017

    I guess replace mkstemp (C function) with tempfile.mkstemp (Python function) can solve the problem.

    @xdegaye
    Copy link
    Mannequin Author

    xdegaye mannequin commented 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
    Copy link
    Member

    vstinner commented 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
    Copy link
    Member

    vstinner commented Jan 12, 2017

    I change the issue type to security.

    @vstinner vstinner added type-security A security issue and removed type-bug An unexpected behavior, bug, or error labels Jan 12, 2017
    @vstinner
    Copy link
    Member

    vstinner commented Jan 12, 2017

    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 :-)

    @xdegaye xdegaye mannequin removed their assignment Feb 3, 2017
    @vstinner
    Copy link
    Member

    vstinner commented 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
    Copy link
    Member

    vstinner commented 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'>:
    ---

    import curses
    w = curses.initscr()
    curses.endwin()
    print(type(w))

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Feb 10, 2017

    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.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 10, 2017

    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

    @vstinner
    Copy link
    Member

    vstinner commented Feb 12, 2017

    I created #52 for curses_fix_window_class_name.patch.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 12, 2017

    I created the PR #53 for curses_temporaryfile.patch.

    @yan12125
    Copy link
    Mannequin

    yan12125 mannequin commented Feb 17, 2017

    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?

    @vstinner
    Copy link
    Member

    vstinner commented Feb 17, 2017

    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 ;-)

    @tiran
    Copy link
    Member

    tiran commented Feb 21, 2017

    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.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 21, 2017

    Is tmpfile() available on Android?

    @yan12125
    Copy link
    Mannequin

    yan12125 mannequin commented Feb 22, 2017

    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

    @tiran
    Copy link
    Member

    tiran commented Feb 22, 2017

    Thanks Chi Hsuan Yen,

    I have created a new PR that replaces mkstemp() with tmpfile().

    @vstinner
    Copy link
    Member

    vstinner commented 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
    Copy link
    Member

    serhiy-storchaka commented Feb 22, 2017

    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?

    @tiran
    Copy link
    Member

    tiran commented 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
    Copy link
    Member

    serhiy-storchaka commented 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
    Copy link
    Member

    vstinner commented Feb 23, 2017

    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

    @vstinner
    Copy link
    Member

    vstinner commented 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
    Copy link
    Member

    vstinner commented 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
    Copy link
    Mannequin

    yan12125 mannequin commented 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
    Copy link
    Member

    vstinner commented 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
    Copy link
    Member

    vstinner commented Mar 24, 2017

    New changeset 2b221b7 by Victor Stinner (Christian Heimes) in branch 'master':
    bpo-29176 Use tmpfile() in curses module (#235)
    2b221b7

    @vstinner
    Copy link
    Member

    vstinner commented Mar 25, 2017

    New changeset 61e2bc7 by Victor Stinner in branch 'master':
    bpo-29176: Fix name of the _curses.window class (#52)
    61e2bc7

    @vstinner
    Copy link
    Member

    vstinner commented 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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 extension-modules C modules in the Modules dir type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants