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-91401: Add a failsafe way to disable vfork. #91490

Merged
merged 5 commits into from
Apr 25, 2022

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Apr 13, 2022

Just in case there is ever an issue with _posixsubprocess's use of
vfork() due to the complexity of using it properly and potential
directions that Linux platforms where it defaults to on could take, this
adds a failsafe so that users can disable its use entirely by setting
a global flag.

No known reason to disable it exists. But it'd be a shame to encounter
one and not be able to use CPython without patching and rebuilding it.

See the linked issue for some discussion on reasoning.

Just in case there is ever an issue with _posixsubprocess's use of
vfork() due to the complexity of using it properly and potential
directions that Linux platforms where it defaults to on could take, this
adds a failsafe so that users can disable its use entirely by setting
a global flag.

No known reason to disable it exists. But it'd be a shame to encounter
one and not be able to use CPython without patching and rebuilding it.

See the linked issue for some discussion on reasoning.
@gpshead gpshead added type-bug An unexpected behavior, bug, or error type-feature A feature request or enhancement labels Apr 13, 2022
@gpshead gpshead self-assigned this Apr 13, 2022
@gpshead gpshead marked this pull request as ready for review April 14, 2022 22:27
@gpshead gpshead requested a review from vstinner April 14, 2022 22:27
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Why not a simple boolean flag?

Can subprocess disable vfork using disable_vfork_reason if vfork is not available? You can expose a simple private flag in _posixsubprocess saying if vfork is available. Something similar to _testcapi.WITH_PYMALLOC or _thread.TIMEOUT_MAX.

Doc/library/subprocess.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

I added subprocess._USE_POSIX_SPAWN so people can opt-out if they encounter problems with posix_spawn().

@gpshead
Copy link
Member Author

gpshead commented Apr 20, 2022

I added subprocess._USE_POSIX_SPAWN so people can opt-out if they encounter problems with posix_spawn().

Being consistent with that seems worthwhile, I'll rework this PR. I expect I'm overthinking it with the whole "make it a string" thing and pleading for people to include an explanation of why.

@vstinner
Copy link
Member

I chose to add a private attribute since the default value must be safe for all users. But for the corner cases, IMO a private attribute is enough.

For vfork, well, it's up to you to add a private or public attribute.

No double negatives. Also documents _USE_POSIX_SPAWN.
Rather than re-encoding our platform selection here (configure.ac
and runtime logic in _posixsubprocess already does that).
@gpshead gpshead added 3.10 only security fixes needs backport to 3.10 only security fixes and removed 3.10 only security fixes labels Apr 25, 2022
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I just have a doubt about the doc format, but you will see when it will be rendered on docs.python.org.

code.

.. versionadded:: 3.8 ``_USE_POSIX_SPAWN``
.. versionadded:: 3.11 ``_USE_VFORK``
Copy link
Member

Choose a reason for hiding this comment

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

I never tried this syntax, I don't know if it's properly rendered. You will see :-)

@gpshead gpshead removed the needs backport to 3.10 only security fixes label Apr 25, 2022
@gpshead gpshead merged commit cd5726f into python:main Apr 25, 2022
@gpshead gpshead deleted the vfork-monsters branch April 25, 2022 23:19
JelleZijlstra added a commit to python/typeshed that referenced this pull request Apr 26, 2022
Debatable whether to include these, but they are now documented: python/cpython#91490
AlexWaygood pushed a commit to python/typeshed that referenced this pull request Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants