classification
Title: Disallow fork in a subinterpreter broke subprocesses in mod_wsgi daemon mode
Type: behavior Stage: commit review
Components: Extension Modules, Interpreter Core Versions: Python 3.9, Python 3.8
process
Status: open Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.snow Nosy List: adamwill, christian.heimes, eric.snow, gregory.p.smith, lukasz.langa, miss-islington, vstinner
Priority: high Keywords: 3.8regression, patch

Created on 2019-08-26 08:42 by christian.heimes, last changed 2019-10-07 20:01 by christian.heimes.

Pull Requests
URL Status Linked Edit
PR 15544 merged christian.heimes, 2019-08-27 08:15
PR 15554 merged miss-islington, 2019-08-27 21:37
Messages (14)
msg350511 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-08-26 08:42
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
msg350522 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-08-26 10:46
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.
msg350526 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-08-26 11:22
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.
msg350544 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2019-08-26 16:16
Christian, you're right to treat this as Release Blocker. Let's have this fixed. Assigning Eric?
msg350550 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-08-26 17:28
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.
msg350615 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-08-27 08:21
I have created a PR that implements Greg's proposal https://bugs.python.org/issue34651#msg325302
msg350646 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-08-27 21:37
New changeset 98d90f745d35d5d07bffcb46788b50e05eea56c6 by Christian Heimes in branch 'master':
bpo-37951: Lift subprocess's fork() restriction (GH-15544)
https://github.com/python/cpython/commit/98d90f745d35d5d07bffcb46788b50e05eea56c6
msg350647 - (view) Author: miss-islington (miss-islington) Date: 2019-08-27 21:56
New changeset 03c52f2f63a8abeb4afb75e9da46c7d6c0a8afd5 by Miss Islington (bot) in branch '3.8':
bpo-37951: Lift subprocess's fork() restriction (GH-15544)
https://github.com/python/cpython/commit/03c52f2f63a8abeb4afb75e9da46c7d6c0a8afd5
msg350648 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-08-27 21:57
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.
msg354126 - (view) Author: Adam Williamson (adamwill) Date: 2019-10-07 18:47
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
msg354130 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-10-07 19:15
preexec_fn is fundamentally unsupportable.

what code is using it, there should be a way not to rely on that.
msg354131 - (view) Author: Adam Williamson (adamwill) Date: 2019-10-07 19:21
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.
msg354135 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-10-07 19:55
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.
msg354136 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-10-07 20:01
https://github.com/freeipa/freeipa/pull/3769 should address the issue.
History
Date User Action Args
2019-10-07 20:01:21christian.heimessetmessages: + msg354136
2019-10-07 19:55:39christian.heimessetmessages: + msg354135
2019-10-07 19:21:19adamwillsetmessages: + msg354131
2019-10-07 19:15:15gregory.p.smithsetmessages: + msg354130
2019-10-07 18:47:52adamwillsetstatus: pending -> open
nosy: + adamwill
messages: + msg354126

2019-08-27 21:57:54christian.heimessetstatus: open -> pending
priority: release blocker -> high
messages: + msg350648

resolution: fixed
stage: patch review -> commit review
2019-08-27 21:56:30miss-islingtonsetnosy: + miss-islington
messages: + msg350647
2019-08-27 21:37:28miss-islingtonsetpull_requests: + pull_request15229
2019-08-27 21:37:00christian.heimessetmessages: + msg350646
2019-08-27 08:21:56christian.heimessettype: behavior
messages: + msg350615
2019-08-27 08:15:41christian.heimessetkeywords: + patch
stage: patch review
pull_requests: + pull_request15221
2019-08-26 17:28:10gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg350550
2019-08-26 16:16:16lukasz.langasetassignee: eric.snow
messages: + msg350544
2019-08-26 11:22:16christian.heimessetpriority: critical -> release blocker
nosy: + lukasz.langa
messages: + msg350526

2019-08-26 10:46:04vstinnersetmessages: + msg350522
2019-08-26 08:42:32christian.heimescreate