Skip to content

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

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

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

@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

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

If you want to schedule another build, you need to add the ":hammer: 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 October 25, 2022 03:11
@ericsnowcurrently ericsnowcurrently marked this pull request as draft October 25, 2022 03:11
@ericsnowcurrently ericsnowcurrently force-pushed the interpreter-better-restrictions branch 2 times, most recently from 4b8c004 to 6d62590 Compare October 26, 2022 17:20
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review October 26, 2022 17:56
@ericsnowcurrently ericsnowcurrently force-pushed the interpreter-better-restrictions branch from 6d62590 to e13a305 Compare October 27, 2022 22:22
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

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

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

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

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

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

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

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

Choose a reason for hiding this comment

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

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants