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

Disallow fork in a subinterpreter broke subprocesses in mod_wsgi daemon mode #82132

Closed
tiran opened this issue Aug 26, 2019 · 15 comments
Closed
Assignees
Labels
3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@tiran
Copy link
Member

tiran commented Aug 26, 2019

BPO 37951
Nosy @gpshead, @vstinner, @tiran, @ambv, @ericsnowcurrently, @AdamWill, @miss-islington
PRs
  • bpo-37951: Lift subprocess's fork() restriction (GH-15544) #15544
  • [3.8] bpo-37951: Lift subprocess's fork() restriction (GH-15544) #15554
  • 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 = 'https://github.com/ericsnowcurrently'
    closed_at = <Date 2020-03-09.11:20:17.392>
    created_at = <Date 2019-08-26.08:42:32.656>
    labels = ['extension-modules', 'interpreter-core', 'type-bug', '3.8', '3.9']
    title = 'Disallow fork in a subinterpreter broke subprocesses in mod_wsgi daemon mode'
    updated_at = <Date 2020-03-09.11:20:17.391>
    user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2020-03-09.11:20:17.391>
    actor = 'vstinner'
    assignee = 'eric.snow'
    closed = True
    closed_date = <Date 2020-03-09.11:20:17.392>
    closer = 'vstinner'
    components = ['Extension Modules', 'Interpreter Core']
    creation = <Date 2019-08-26.08:42:32.656>
    creator = 'christian.heimes'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37951
    keywords = ['patch', '3.8regression']
    message_count = 15.0
    messages = ['350511', '350522', '350526', '350544', '350550', '350615', '350646', '350647', '350648', '354126', '354130', '354131', '354135', '354136', '363718']
    nosy_count = 7.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'christian.heimes', 'lukasz.langa', 'eric.snow', 'adamwill', 'miss-islington']
    pr_nums = ['15544', '15554']
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue37951'
    versions = ['Python 3.8', 'Python 3.9']

    @tiran
    Copy link
    Member Author

    tiran commented Aug 26, 2019

    BPO https://bugs.python.org/issue34651 disabled fork in subinterpreters. The patch also disabled fork() in _posixsubprocess.fork_exec(). This broke the ability to spawn subprocesses in mod_wsgi daemons, which use subinterpreters. Any attempt to spawn (fork + exec) a subprocess fails with "RuntimeError: fork not supported for subinterpreters":

    ...
    File "/usr/lib64/python3.8/subprocess.py", line 829, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
    File "/usr/lib64/python3.8/subprocess.py", line 1608, in _execute_child
    self.pid = _posixsubprocess.fork_exec(
    RuntimeError: fork not supported for subinterpreters

    Also see https://bugzilla.redhat.com/show_bug.cgi?id=1745450

    @tiran tiran added 3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 26, 2019
    @vstinner
    Copy link
    Member

    subprocess still work in subinterpreters in Python 3.8 if posix_spawn() can be used, but posix_spawn() is only used under some conditions:
    https://docs.python.org/dev/whatsnew/3.8.html#optimizations

    "The subprocess module can now use the os.posix_spawn() function in some cases for better performance. Currently, it is only used on macOS and Linux (using glibc 2.24 or newer) if all these conditions are met:

    • close_fds is false;
    • preexec_fn, pass_fds, cwd and start_new_session parameters are not set;
    • the executable path contains a directory."

    --

    It seems like FreeIPA uses ctypes and ctypes calls subprocess.Popen(['/sbin/ldconfig', '-p'], ...) to locale libcrypto.

    I see different options:

    • modify FreeIPA / ctypes to ensure that posix_spawn() can be used
    • avoid subinterpreters to deploy FreeIPA
    • revert the change to allow again fork in subprocesses: see bpo-34651 for the rationale why it was denied

    I understand that FreeIPA is run as WSGI using mod_wsgi in Apache.

    @tiran
    Copy link
    Member Author

    tiran commented Aug 26, 2019

    It's a bit more complicated. FreeIPA uses cryptography, which uses asn1crypto, which uses ctypes, which is broken in mod_wsgi due to bpo-34651. It's not just FreeIPA that is affected by the issue. Any application running in mod_wsgi is potentially affected and broken by bpo-34651.

    1a) (modify FreeIPA) is not possible. IPA requires the additional features of the subprocess module.
    1b) (modify ctypes) should be done in a separate ticket. I'm not sure why subprocess does not use posix_spawn() here. I guess it's the default value "close_fds=True"?
    2) (avoid subinterpreters) would require a rewrite of mod_wsgi
    3) (revert bpo-34651) is IMHO required for _posixsubprocess.fork_exec().

    bpo-34651 is a backwards incompatible change that breaks existing applications that uses mod_wsgi. At least _posixsubprocess.fork_exec() should be reverted and the removal of fork() support should go through a proper deprecation cycle of two releases.

    I'm bumping this up to release blocker and CC Łukasz.

    @ambv
    Copy link
    Contributor

    ambv commented Aug 26, 2019

    Christian, you're right to treat this as Release Blocker. Let's have this fixed. Assigning Eric?

    @gpshead
    Copy link
    Member

    gpshead commented Aug 26, 2019

    FWIW, _posixsubprocess.fork_exec() should be safe to allow.

    The only thing within it to disallow, if you're going to bother to check this at all, is any use of the legacy preexec_fn support.

    @tiran
    Copy link
    Member Author

    tiran commented Aug 27, 2019

    I have created a PR that implements Greg's proposal https://bugs.python.org/issue34651#msg325302

    @tiran tiran added the type-bug An unexpected behavior, bug, or error label Aug 27, 2019
    @tiran
    Copy link
    Member Author

    tiran commented Aug 27, 2019

    New changeset 98d90f7 by Christian Heimes in branch 'master':
    bpo-37951: Lift subprocess's fork() restriction (GH-15544)
    98d90f7

    @miss-islington
    Copy link
    Contributor

    New changeset 03c52f2 by Miss Islington (bot) in branch '3.8':
    bpo-37951: Lift subprocess's fork() restriction (GH-15544)
    03c52f2

    @tiran
    Copy link
    Member Author

    tiran commented Aug 27, 2019

    Thanks Victor and Gregory!

    I'm reducing the severity from release blocker to high and keep the ticket in pending to give Eric a change to review the commits.

    @adamwill
    Copy link
    Mannequin

    adamwill mannequin commented Oct 7, 2019

    Well, now our (Fedora QA's) automated testing of FreeIPA is showing what looks like a problem with preexec_fn (rather than fork) being disallowed:

    https://bugzilla.redhat.com/show_bug.cgi?id=1759290

    Login to the FreeIPA webUI is failing, and at the time it fails we see this error message on the server end:

    [Mon Oct 07 09:22:19.521604 2019] [wsgi:error] [pid 32989:tid 139746234119936] [remote 10.0.2.102:56054] ipa: DEBUG: args=['/usr/bin/kinit', 'admin', '-c', '/run/ipa/ccaches/kinit_32989', '-E']
    [Mon Oct 07 09:22:19.521996 2019] [wsgi:error] [pid 32989:tid 139746234119936] [remote 10.0.2.102:56054] ipa: DEBUG: Process execution failed
    [Mon Oct 07 09:22:19.522189 2019] [wsgi:error] [pid 32989:tid 139746234119936] [remote 10.0.2.102:56054] ipa: INFO: 401 Unauthorized: preexec_fn not supported within subinterpreters

    @gpshead
    Copy link
    Member

    gpshead commented Oct 7, 2019

    preexec_fn is fundamentally unsupportable.

    what code is using it, there should be a way not to rely on that.

    @adamwill
    Copy link
    Mannequin

    adamwill mannequin commented Oct 7, 2019

    It's this function:

    https://github.com/freeipa/freeipa/blob/master/ipalib/install/kinit.py#L66

    The function run is imported from ipapython.ipautil, it's defined here:

    https://github.com/freeipa/freeipa/blob/master/ipapython/ipautil.py#L391

    all of this is being run inside a WSGI.

    @tiran
    Copy link
    Member Author

    tiran commented Oct 7, 2019

    I'll address the issue in FreeIPA.

    The ipautil.run() function is a helper around subprocess.Popen. The function always installs a preexec_fn in case it needs to change umask or drop priviliges. The WSGI server does not need these features.

    @tiran
    Copy link
    Member Author

    tiran commented Oct 7, 2019

    freeipa/freeipa#3769 should address the issue.

    @vstinner
    Copy link
    Member

    vstinner commented Mar 9, 2020

    I'm reducing the severity from release blocker to high and keep the ticket in pending to give Eric a change to review the commits.

    Python 3.8.0 is released with the fix. It's now time to close the issue.

    @vstinner vstinner closed this as completed Mar 9, 2020
    @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 extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants