Navigation Menu

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

Opening a file holds the GIL when it calls "isatty()" #88385

Closed
smurfix mannequin opened this issue May 23, 2021 · 12 comments
Closed

Opening a file holds the GIL when it calls "isatty()" #88385

smurfix mannequin opened this issue May 23, 2021 · 12 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@smurfix
Copy link
Mannequin

smurfix mannequin commented May 23, 2021

BPO 44219
Nosy @vstinner, @smurfix, @ambv, @vxgmichel, @miss-islington
PRs
  • bpo-44219: Release the GIL during isatty syscalls #28250
  • [3.10] bpo-44219: Release the GIL during isatty syscalls (GH-28250) #28255
  • [3.9] bpo-44219: Release the GIL during isatty syscalls (GH-28250) #28256
  • bpo-44219: Mention GH-28250 is a deadlock bugfix #28261
  • [3.10] bpo-44219: Mention GH-28250 is a deadlock bugfix (GH-28261) #28274
  • [3.9] bpo-44219: Mention GH-28250 is a deadlock bugfix (GH-28261) #28275
  • Files
  • bpo-44219.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 2021-09-09.16:42:10.377>
    created_at = <Date 2021-05-23.17:18:13.788>
    labels = ['interpreter-core', '3.10', '3.9', 'type-crash', '3.11']
    title = 'Opening a file holds the GIL when it calls "isatty()"'
    updated_at = <Date 2021-09-10.16:22:31.565>
    user = 'https://github.com/smurfix'

    bugs.python.org fields:

    activity = <Date 2021-09-10.16:22:31.565>
    actor = 'lukasz.langa'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-09-09.16:42:10.377>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2021-05-23.17:18:13.788>
    creator = 'smurfix'
    dependencies = []
    files = ['50270']
    hgrepos = []
    issue_num = 44219
    keywords = ['patch']
    message_count = 12.0
    messages = ['394208', '401261', '401402', '401450', '401458', '401470', '401478', '401495', '401497', '401584', '401588', '401591']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'smurfix', 'lukasz.langa', 'vxgmichel', 'miss-islington']
    pr_nums = ['28250', '28255', '28256', '28261', '28274', '28275']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue44219'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @smurfix
    Copy link
    Mannequin Author

    smurfix mannequin commented May 23, 2021

    Opening a file calls isatty which calls an ioctl with the GIL held.

    GDB:

    #0  __GI___tcgetattr (fd=18, termios_p=termios_p@entry=0x7f618a5df920)
        at ../sysdeps/unix/sysv/linux/tcgetattr.c:38
    #1  0x00007f618bd1ca0c in __isatty (fd=<optimized out>) at ../sysdeps/posix/isatty.c:27
    #2  0x000000000062b746 in _Py_device_encoding (fd=<optimized out>) at ../Python/fileutils.c:62
    #3  0x000000000060bf90 in _io_TextIOWrapper___init___impl (write_through=0, line_buffering=0, 
        newline=0x0, errors='strict', encoding=<optimized out>, 
        buffer=<_io.BufferedWriter at remote 0x7f618986aeb0>, self=0x7f618985a860)
        at ../Modules/_io/textio.c:1149
    ...
    

    Please don't do that.

    In my case, the file in question is implemented as a FUSE mount which is served by the same process (different thread of course). Thus holding the GIL at this point causes a rather interesting deadlock.

    Tested with 3.9.

    @smurfix smurfix mannequin added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels May 23, 2021
    @vxgmichel
    Copy link
    Mannequin

    vxgmichel mannequin commented Sep 7, 2021

    My team ran into this issue while developing a fuse application too.

    In an effort to help this issue move forward, I tried to list all occurrences of the isatty C function in the cpython code base. I found 14 of them.

    9 of them are directly related to stdin/stdout/stderr, so it's probably not crucial to release the GIL for those occurrences:

    • main.c:stdin_is_interactive
    • main.c:pymain_import_readline
    • readline.c:setup_readline
    • bltinmodule.c:builtin_input_impl (x2)
    • frozenmain.c:Py_FrozenMain
    • pylifecycle.c:Py_FdIsInteractive (x2)
    • fileobject.c:stdprinter_isatty (GIL is actually released for this one)

    Out of the remaining 4, only 1 releases the GIL:

    • fileio.c:_io_FileIO_isatty_impl: used for FileIO.isatty

    Which gives 3 occurrences of non-stdstream specific usage of isatty that do not release the GIL:

    • posixmodule.c:os_isatty_impl: used by os.isatty
    • fileutils.c:_Py_device_encoding: used TextIOWrapper.__init__
    • fileutils.c:_Py_write_impl: windows specific, issue bpo-11395

    The first one is used by os.isatty which means this call can also deadlock. I did manage to reproduce it with a simple fuse loopback file system: https://github.com/fusepy/fusepy/blob/master/examples/loopback.py

    The second one is the one found by @smurfix and gets triggered when io.open() is used in text mode.

    The third one only triggers on windows when writing more than 32767 bytes to a file descriptor. A comment points to issue bpo-11395 (https://bugs.python.org/issue11395). Also, it seems from the function signature that this function might be called with or without the GIL held, which might cause the fix to be a bit more complicated than the first two use cases.

    I hope this helps.

    @vxgmichel
    Copy link
    Mannequin

    vxgmichel mannequin commented Sep 8, 2021

    Here's a possible patch that fixes the 3 unprotected calls to isatty mentioned above. It successfully passes the test suite. I can submit a PR with this patch if necessary.

    @smurfix
    Copy link
    Mannequin Author

    smurfix mannequin commented Sep 9, 2021

    Please do. However, I do think that changing the stdstream related ioctl calls also is a good idea, if only for code regularity/completeness sake.

    (Besides, nothing prevents somebody from starting a FUSE file system and then redirecting stdout to it …)

    @vxgmichel
    Copy link
    Mannequin

    vxgmichel mannequin commented Sep 9, 2021

    There are a couple of reasons why I did not make changes to the stdstream related functions.

    The first one is that a PR with many changes is less likely to get reviewed and merged than a PR with fewer changes. The second one is that it's hard for me to make sure that those functions are always called with the GIL already held. Maybe some of them never hold the GIL in the first place, and I'm not familiar enough with the code base to tell.

    So in the end it will probably be up to the coredev reviewing the PR, but better start small.

    Besides, nothing prevents somebody from starting a FUSE file system and then redirecting stdout to it …

    I ran some checks and python -c 'input()' < my-fuse-mountpoint/data_in does indeed trigger an ioctl call to the corresponding fuse file system. But how would my-fuse-mountpoint/data_in be the stdin for the process that itself starts the fuse filesystem? I don't see how to generate this deadlock, apart from setting up interprocess communication between the fuse process and the my-fuse-mountpoint/data_in-as-stdin process.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 9, 2021

    New changeset 06148b1 by Vincent Michel in branch 'main':
    bpo-44219: Release the GIL during isatty syscalls (GH-28250)
    06148b1

    @miss-islington
    Copy link
    Contributor

    New changeset 5c65834 by Miss Islington (bot) in branch '3.9':
    bpo-44219: Release the GIL during isatty syscalls (GH-28250)
    5c65834

    @ambv
    Copy link
    Contributor

    ambv commented Sep 9, 2021

    New changeset 23c4677 by Miss Islington (bot) in branch '3.10':
    bpo-44219: Release the GIL during isatty syscalls (GH-28250) (GH-28255)
    23c4677

    @vstinner
    Copy link
    Member

    vstinner commented Sep 9, 2021

    In my case, the file in question is implemented as a FUSE mount which is served by the same process (different thread of course). Thus holding the GIL at this point causes a rather interesting deadlock.

    Since the change fixes a deadlock, I agree to backport it to 3.9 and 3.10.

    Thanks for the fix!

    @ambv
    Copy link
    Contributor

    ambv commented Sep 10, 2021

    New changeset 296b710 by Łukasz Langa in branch 'main':
    bpo-44219: Mention #72437 is a deadlock bugfix (GH-28261)
    296b710

    @ambv
    Copy link
    Contributor

    ambv commented Sep 10, 2021

    New changeset 60ddf49 by Miss Islington (bot) in branch '3.10':
    bpo-44219: Mention #72437 is a deadlock bugfix (GH-28261) (GH-28274)
    60ddf49

    @ambv
    Copy link
    Contributor

    ambv commented Sep 10, 2021

    New changeset 314de53 by Miss Islington (bot) in branch '3.9':
    bpo-44219: Mention #72437 is a deadlock bugfix (GH-28261) (GH-28275)
    314de53

    @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.9 only security fixes 3.10 only security fixes 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants