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

gh-98610: Adjust the Optional Restrictions on Subinterpreters #98618

Merged

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Oct 24, 2022

Previously, the optional restrictions on subinterpreters were: disallow fork, subprocess, and threads. By default, we were disallowing all three for "isolated" interpreters. We always allowed all three for the main interpreter and those created through the legacy Py_NewInterpreter() API.

Those settings were a bit conservative, so here we've adjusted the optional restrictions to: fork, exec, threads, and daemon threads. The default for "isolated" interpreters disables fork, exec, and daemon threads. Regular threads are allowed by default. We continue always allowing everything For the main interpreter and the legacy API.

In the code, we add _PyInterpreterConfig.allow_exec and _PyInterpreterConfig.allow_daemon_threads. We also add Py_RTFLAGS_DAEMON_THREADS and Py_RTFLAGS_EXEC.

Automerge-Triggered-By: GH:ericsnowcurrently

@ericsnowcurrently
Copy link
Member Author

ericsnowcurrently commented Oct 24, 2022

@gpshead, we talked about this at the core sprint.

@ericsnowcurrently ericsnowcurrently added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 24, 2022
@bedevere-bot
Copy link

bedevere-bot commented Oct 24, 2022

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 77314bc 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 24, 2022
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review Oct 25, 2022
@ericsnowcurrently ericsnowcurrently requested a review from gpshead as a code owner Oct 25, 2022
@ericsnowcurrently ericsnowcurrently marked this pull request as draft Oct 25, 2022
@ericsnowcurrently ericsnowcurrently force-pushed the interpreter-better-restrictions branch 2 times, most recently from 4b8c004 to 6d62590 Compare Oct 26, 2022
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review Oct 26, 2022
@ericsnowcurrently ericsnowcurrently force-pushed the interpreter-better-restrictions branch from 6d62590 to e13a305 Compare Oct 27, 2022
Copy link
Member

@gpshead gpshead left a comment

minor tweaks suggested, overall good.

Lib/threading.py Outdated
@@ -899,6 +900,8 @@ class is implemented.
self._args = args
self._kwargs = kwargs
if daemon is not None:
if daemon and not _daemon_threads_allowed():
raise RuntimeError('daemon threads are disabled in this interpreter')
Copy link
Member

@gpshead gpshead Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps word the error message here and below as "daemon threads are disabled in this (sub)interpreter" to provide a better hint as to why.

Copy link
Member Author

@ericsnowcurrently ericsnowcurrently Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

PyDoc_STRVAR(daemon_threads_allowed_doc,
"daemon_threads_allowed()\n\
\n\
Return True if daemon threads are allowed in the current interpreter,\n\
Copy link
Member

@gpshead gpshead Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to be a function? Just to prevent people from attempting to set it?

Copy link
Member Author

@ericsnowcurrently ericsnowcurrently Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Daemon threads are implemented exclusively in threading.py, so we need a way to access the info from Python.

Copy link
Member Author

@ericsnowcurrently ericsnowcurrently Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean why not an attribute, it's because I was thinking of it as an interpreter setting, rather than state of the _thread module.

@@ -0,0 +1,5 @@
Disallowing subprocess in subinterpreters is no longer supported. Instead,
Copy link
Member

@gpshead gpshead Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double negative... how about wording this like

The use of the :mod:`subprocess` is now allowed from subinterpreters. Instead
:func:`os.exec` can now be disallowed.  ... existing text ...

Copy link
Member Author

@ericsnowcurrently ericsnowcurrently Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@ericsnowcurrently ericsnowcurrently added the 🤖 automerge PR will be merged once it's been approved and all CI passed label Oct 31, 2022
@miss-islington miss-islington merged commit 4702552 into python:main Oct 31, 2022
15 checks passed
@ericsnowcurrently ericsnowcurrently deleted the interpreter-better-restrictions branch Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 automerge PR will be merged once it's been approved and all CI passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants