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

os.set_inheritable() fails for O_PATH file descriptors on Linux #86946

Closed
cptpcrd mannequin opened this issue Dec 29, 2020 · 12 comments
Closed

os.set_inheritable() fails for O_PATH file descriptors on Linux #86946

cptpcrd mannequin opened this issue Dec 29, 2020 · 12 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@cptpcrd
Copy link
Mannequin

cptpcrd mannequin commented Dec 29, 2020

BPO 42780
Nosy @vstinner, @benjaminp, @miss-islington, @cptpcrd
PRs
  • bpo-42780: fix set_inheritable() for O_PATH file descriptors on Linux #24172
  • [3.9] bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172) #24269
  • [3.8] bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172) #24270
  • [3.8] bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172) #24277
  • [3.9] bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172) #24278
  • Files
  • set-inheritable-o-path.patch
  • set-inheritable-test.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-01-20.14:13:31.465>
    created_at = <Date 2020-12-29.17:18:38.607>
    labels = ['3.8', 'type-bug', 'library', '3.9', '3.10']
    title = 'os.set_inheritable() fails for O_PATH file descriptors on Linux'
    updated_at = <Date 2021-01-22.00:58:54.375>
    user = 'https://github.com/cptpcrd'

    bugs.python.org fields:

    activity = <Date 2021-01-22.00:58:54.375>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-01-20.14:13:31.465>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2020-12-29.17:18:38.607>
    creator = 'cptpcrd'
    dependencies = []
    files = ['49706', '49721']
    hgrepos = []
    issue_num = 42780
    keywords = ['patch']
    message_count = 12.0
    messages = ['384016', '384029', '384035', '384044', '384396', '385339', '385340', '385405', '385406', '385407', '385414', '385462']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'benjamin.peterson', 'miss-islington', 'cptpcrd']
    pr_nums = ['24172', '24269', '24270', '24277', '24278']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue42780'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @cptpcrd
    Copy link
    Mannequin Author

    cptpcrd mannequin commented Dec 29, 2020

    Note: I filed this bug report after seeing rust-lang/rust#62425 and verifying that it was also reproducible on Python. Credit for discovering the underlying issue should go to Aleksa Sarai, and further discussion can be found there.

    # Background

    Linux has O_PATH file descriptors. These are file descriptors that refer to a specific path, without allowing any other kind of access to the file. They can't be used to read or write data; instead, they're intended to be used for use cases like the *at() functions. In that respect, they have similar semantics to O_SEARCH on other platforms (except that they also work on other file types, not just directories).

    More information on O_PATH file descriptors can be found in open(2) (https://www.man7.org/linux/man-pages/man2/open.2.html), or in the Rust PR linked above.

    # The problem

    As documented in the Rust PR linked above, *no* ioctl() calls will succeed on O_PATH file descriptors (they always fail with EBADF). Since os.set_inheritable() uses ioctl(FIOCLEX)/ioctl(FIONCLEX), it will fail on O_PATH file descriptors.

    This is easy to reproduce:

    >>> import os
    >>> a = os.open("/", os.O_RDONLY)
    >>> b = os.open("/", os.O_PATH)
    >>> os.set_inheritable(a, True)
    >>> os.set_inheritable(b, True)  # Should succeed!
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: [Errno 9] Bad file descriptor
    >>>

    I believe this affects all versions of Python going back to version 3.4 (where os.set_inheritable()/os.get_inheritable() were introduced).

    # Possible fixes

    I see two potential paths for fixing this:

    1. Don't use ioctl(FIOCLEX) at all on Linux.

    This is what Rust did. However, based on bpo-22258 I'm guessing there would be opposition to implementing this strategy in Python, on the grounds that the fcntl() route takes an extra syscall (which is fair).

    1. On Linux, fall back on fcntl() if ioctl(FIOCLEX) fails with EBADF.

    This could be a very simple patch to Python/fileutils.c. I've attached a basic version of said patch (not sure if it matches standard coding conventions).

    Downsides: This would add 2 extra syscalls for O_PATH file descriptors, and 1 extra syscall for actual cases of invalid file descriptors (i.e. EBADF). However, I believe these are edge cases that shouldn't come up frequently.

    @cptpcrd cptpcrd mannequin added 3.7 (EOL) end of life 3.10 only security fixes 3.8 only security fixes 3.9 only security fixes type-bug An unexpected behavior, bug, or error labels Dec 29, 2020
    @izbyshev izbyshev mannequin added stdlib Python modules in the Lib dir and removed 3.7 (EOL) end of life labels Dec 29, 2020
    @benjaminp
    Copy link
    Contributor

    Doing two syscalls does not seem so bad.

    Linux may allow FIOCLEX on O_PATH in the future.

    @benjaminp benjaminp added 3.7 (EOL) end of life labels Dec 29, 2020
    @vstinner
    Copy link
    Member

    1. On Linux, fall back on fcntl() if ioctl(FIOCLEX) fails with EBADF.

    I like this approach!

    @cptpcrd
    Copy link
    Mannequin Author

    cptpcrd mannequin commented Dec 30, 2020

    I like this approach!

    Should I put together unit tests to go with the patch? Maybe test_os.FDInheritanceTests.test_set_inheritable_o_path()?

    @cptpcrd
    Copy link
    Mannequin Author

    cptpcrd mannequin commented Jan 5, 2021

    I've put together some tests (patch attached). Should I PR this to python/cpython?

    @vstinner
    Copy link
    Member

    New changeset 7dc71c4 by cptpcrd in branch 'master':
    bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172)
    7dc71c4

    @vstinner
    Copy link
    Member

    Thanks cptpcrd for your bug report and your great fix! I like your tests ;-)

    @vstinner
    Copy link
    Member

    New changeset 844ec0b by cptpcrd in branch '3.8':
    bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172) (GH-24277)
    844ec0b

    @vstinner
    Copy link
    Member

    New changeset 6893523 by cptpcrd in branch '3.9':
    bpo-42780: Fix set_inheritable() for O_PATH file descriptors on Linux (GH-24172) (GH-24278)
    6893523

    @vstinner
    Copy link
    Member

    Thanks cptpcrd for the bug report, the fix and for the two manual backports ;-)

    Note: Python 3.6 and 3.7 no longer accept bugfixes.
    https://devguide.python.org/#status-of-python-branches

    @vstinner vstinner removed 3.7 (EOL) end of life labels Jan 21, 2021
    @cptpcrd
    Copy link
    Mannequin Author

    cptpcrd mannequin commented Jan 21, 2021

    No problem! I've noticed at least one other (relatively minor) issue in os, so I may be submitting further bug reports.

    I haven't been keeping close track of 3.6/3.7's status, so I added them in without thinking it. Thanks for the reminder.

    @vstinner
    Copy link
    Member

    No problem! I've noticed at least one other (relatively minor) issue in os, so I may be submitting further bug reports.

    Please do. But I suggest to open new issues.

    I haven't been keeping close track of 3.6/3.7's status, so I added them in without thinking it. Thanks for the reminder.

    Don't worry (be happy). Everybody is confused by the Versions field.

    @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.8 only security fixes 3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants