subprocess: child_exec() uses _Py_set_inheritable() which is not async-signal-safe

[izbyshev]

BPO	32777
Nosy	@gpshead, @vstinner, @izbyshev
PRs	bpo-32777: subprocess: Fix usage of _Py_set_inheritable() in child_exec() #5560 [3.7] bpo-32777: Fix _Py_set_inheritable async-safety in subprocess (GH-5560) #5562 [3.6] bpo-32777: Fix _Py_set_inheritable async-safety in subprocess (GH-5560) #5563

^{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/gpshead'
closed_at = <Date 2018-02-06.06:52:35.951>
created_at = <Date 2018-02-06.01:34:02.804>
labels = ['extension-modules', 'type-bug', 'library', '3.7']
title = 'subprocess: child_exec() uses _Py_set_inheritable() which is not async-signal-safe'
updated_at = <Date 2018-02-06.07:25:34.347>
user = 'https://github.com/izbyshev'

bugs.python.org fields:

activity = <Date 2018-02-06.07:25:34.347>
actor = 'izbyshev'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2018-02-06.06:52:35.951>
closer = 'gregory.p.smith'
components = ['Extension Modules', 'Library (Lib)']
creation = <Date 2018-02-06.01:34:02.804>
creator = 'izbyshev'
dependencies = []
files = []
hgrepos = []
issue_num = 32777
keywords = ['patch']
message_count = 6.0
messages = ['311699', '311706', '311707', '311708', '311709', '311712']
nosy_count = 3.0
nosy_names = ['gregory.p.smith', 'vstinner', 'izbyshev']
pr_nums = ['5560', '5562', '5563']
priority = 'normal'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32777'
versions = ['Python 3.6', 'Python 3.7']

[izbyshev]

_Py_set_inheritable() raises a Python-level exception on error and thus is not async-signal-safe, but child_exec() must use only async-signal-safe functions because it's executed between fork() and exec().

Since a non-raising version is already implemented in Python/fileutils.c for internal use (set_inheritable), I suggest to simply expose it via another public function (similar to _Py_open_noraise(), etc.).

[gpshead]

out of curiosity, did you actually diagnose a process deadlocked due to this or was it noted via code inspection?

this issue has been around since 3.4 and the file descriptor inheritance changes from the looks of things.

[gpshead]

New changeset c1e46e9 by Gregory P. Smith (Alexey Izbyshev) in branch 'master':
bpo-32777: Fix _Py_set_inheritable async-safety in subprocess (GH-5560)
c1e46e9

[gpshead]

New changeset 2bb0bfa by Gregory P. Smith (Miss Islington (bot)) in branch '3.7':
bpo-32777: Fix _Py_set_inheritable async-safety in subprocess (GH-5560) (GH-5562)
2bb0bfa

[gpshead]

New changeset b90c685 by Gregory P. Smith (Miss Islington (bot)) in branch '3.6':
bpo-32777: Fix _Py_set_inheritable async-safety in subprocess (GH-5560) (GH-5563)
b90c685

[izbyshev]

out of curiosity, did you actually diagnose a process deadlocked due to this or was it noted via code inspection?

The latter. I noted it while working on another issue. While it was easy to trigger a malloc() in child_exec() by e.g. specifying an invalid fd in pass_fds for Popen, I haven't tried to arrange a deadlock.