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

Soft deprecate os.spawn*(), os.popen() and os.system() functions #120743

Closed
vstinner opened this issue Jun 19, 2024 · 9 comments
Closed

Soft deprecate os.spawn*(), os.popen() and os.system() functions #120743

vstinner opened this issue Jun 19, 2024 · 9 comments

Comments

@vstinner
Copy link
Member

vstinner commented Jun 19, 2024

See the discussion for the rationale: Is time to remove os module spawn* functions?

See also the discussion: How to deal with unsafe/broken os.spawn* arg handling behavior on Windows (Nov 2022).

Advantages of the subprocess module:

  • subprocess restores the signal handlers (reliable behavior).
  • subprocess closes all file descriptors (more secure).
  • subprocess doesn't use a shell by default, shell=True must be used explicitly.
  • subprocess is the defacto standard way to spawn processes in Python.

Examples of os functions issues:

  • os.popen() and os.system() always use shell=True, it cannot be disabled. (higher risk of shell code injection)
  • os.popen().close() return value depends on the platform. (not portable)
  • os.popen().close() return value is a "wait status", not an exit code: waitstatus_to_exitcode() must be used to get a return code.
  • There are 8 os.spawn*() functions, it's more error prone and harder to use than subprocess APIs.
  • os.spawn*() functions return an exit code 1 if the program is not found instead of raising an exception. It's harder to distinguish if the program exists and has an exit code of 1, or if it doesn't exist.
  • The os.spawn*() functions are not safe for use in multithreaded programs on POSIX systems as they use fork()+exec() from Python there.

Linked PRs

@vstinner
Copy link
Member Author

vstinner added a commit to vstinner/cpython that referenced this issue Jun 19, 2024
Soft deprecate os.popen(), os.spawn*() and os.system() functions.
@vstinner
Copy link
Member Author

Reminder: https://docs.python.org/dev/glossary.html#term-soft-deprecated

  • a soft deprecation does not imply scheduling the removal of the deprecated API.
  • a soft deprecation does not issue a warning.

@gpshead
Copy link
Member

gpshead commented Jun 24, 2024

As a soft-deprecation with docs only, no warning raised, and no planned removal... I like this for all of the listed APIs. Including the widely used os.system(). Our goal is to always point users towards the best way to do things.

@adorilson
Copy link
Contributor

Our goal is to always point users towards the best way to do things.

Could we do that as soon as possible?

For a pattern issue, the deprecation note is only at the section's end, right?
However, the spawn* functions, for instance, have a long section (almost two pages), so I think this notice must be earlier.

We have a note suggesting subprocess use, but it doesn't mention the deprecation state.

image

So, what do you guys think about:

  1. Move this paragraph from here and merge it with the deprecation note at the bottom. It's more detailed than the current note.
  2. In this place, say something like These functions are soft deprecated. See note at the bottom. in a highlighted way, if possible.

vstinner added a commit that referenced this issue Jul 1, 2024
Soft deprecate os.popen() and os.spawn*() functions.
@vstinner
Copy link
Member Author

vstinner commented Jul 1, 2024

Implemented as the change d44c550.

@vstinner vstinner closed this as completed Jul 1, 2024
@vstinner
Copy link
Member Author

vstinner commented Jul 1, 2024

Could we do that as soon as possible?

I don't think that we have to be more aggressive about deprecating these functions. A soft deprecation mention at the end of the doc should be enough for now.

@eryksun
Copy link
Contributor

eryksun commented Jul 1, 2024

On Windows, bear in mind that sharing file descriptors with a child process is non-obvious if you're not using os.spawn*(). You have to get the OS handle value via mscvrt.get_osfhandle(). Make it inheritable, at least temporarily. Spawn the child process via subprocess.Popen with close_fds=False (or use the STARTUPINFO handle list). Communicate the handle value to the child (as opposed to the fd value). Then implement the child process to wrap the handle back as a new fd via C _open_osfhandle() (e.g. msvcrt.open_osfhandle()). With os.spawn*(), if the child process uses CRT file descriptors, then one just has to make the file descriptor inheritable and communicate the fd value to the child (e.g. with a command-line argument or environment variable).

@vstinner
Copy link
Member Author

vstinner commented Jul 1, 2024

On Windows, bear in mind that sharing file descriptors with a child process is non-obvious if you're not using os.spawn*().

On Windows, I prefer to pass handles to child processes :-) https://docs.python.org/dev/library/subprocess.html#subprocess.STARTUPINFO.lpAttributeList

@eryksun
Copy link
Contributor

eryksun commented Jul 1, 2024

On Windows, I prefer to pass handles to child processes :-) https://docs.python.org/dev/library/subprocess.html#subprocess.STARTUPINFO.lpAttributeList

If one's current workflow is based around file descriptors, then switching from spawn*() to subprocess.Popen entails adapting the code in both the Python script as well as every affected child script or application to use the CRT's _get_osfhandle() and _open_osfhandle() functions, such as via msvcrt.get_osfhandle() and msvcrt.open_osfhandle(). If you don't control the source code for the spawned processes, the latter is obviously impossible.

Even from Python script to a child Python script, we have nothing implemented to make this workflow obvious and simple.

  • In the parent script, an fd_list list has to be constructed manually by duplicating each file descriptor via os.dup(). Then build the handle_list by calling msvcrt.get_osfhandle() on each file descriptor in fd_list, making it inheritable via os.set_handle_inheritable(), and appending it to handle_list. Next create a startup record via subprocess.STARTUPINFO(lpAttributeList={"handle_list": handle_list}) and spawn the child process with this startupinfo parameter, as well as with the handle values, and whether each should be opened in append mode, passed to the child in some way (e.g. command line, environment variable, pipe, shared memory, etc). Finally, close all of the duped file descriptors in fd_list.
  • In the child Python script, at startup get the communicated list of handles as handle_list in order to build fd_list. Make each handle non-inheritable via os.set_handle_inheritable() and, depending on whether it was opened in append mode, open a file descriptor via msvcrt.open_osfhandle(h, os.O_NOINHERIT) or msvcrt.open_osfhandle(h, os.O_APPEND | os.O_NOINHERIT), and append the opened fd to fd_list.

Akasurde pushed a commit to Akasurde/cpython that referenced this issue Jul 3, 2024
Soft deprecate os.popen() and os.spawn*() functions.
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
Soft deprecate os.popen() and os.spawn*() functions.
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
Soft deprecate os.popen() and os.spawn*() functions.
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

No branches or pull requests

4 participants