classification
Title: Deprecate os.popen() function
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.10
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: serhiy.storchaka, steven.daprano, vstinner
Priority: normal Keywords: patch

Created on 2020-12-14 22:52 by vstinner, last changed 2021-09-21 20:03 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 23782 merged vstinner, 2020-12-15 16:38
Messages (14)
msg383012 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-14 22:52
The os.popen() function uses a shell by default which usually leads to shell injection vulnerability.

It also has a weird API:

* closing the file waits until the process completes.
* close() returns a "wait status" (*) not a "returncode"

(*) see https://docs.python.org/dev/library/os.html#os.waitstatus_to_exitcode for the meaning of a "wait status".

IMO the subprocess module provides better and safer alternatives with a clean API. The subprocess module already explains how to replace os.popen() with subprocess:
https://docs.python.org/dev/library/subprocess.html#replacing-os-popen-os-popen2-os-popen3

In Python 2, os.popen() was deprecated since Python 2.6, but Python 3.0 removed the deprecation (commit dcf97b98ec5cad972b3a8b4989001c45da87d0ea, then commit f5a429295d855267c33c5ef110fbf05ee7a3013e extended os.popen() documentation again: bpo-6490).

platform.popen() existed until Python 3.8 (bpo-35345). It was deprecated since Python 3.3 (bpo-11377).

--

There is also the os.system() function which exposes the libc system() function. Should we deprecate this one as well?
msg383013 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-14 23:01
In the past, multiple os.popen() usage have been replaced with subprocess in the stdlib to prevent the risk of shell injection:

* bpo-22636: ctypes modules
* bpo-22637: uuid module

By the way, there is an open issue bpo-21557 "os.popen & os.system lack shell-related security warnings".

See also bpo-6490 "os.popen documentation is probably wrong" (fixed).
msg383014 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-14 23:07
os.popen() doesn't emit a ResourceWarning when close() is not called, leading to weird issues like bpo-15408.
msg383015 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-14 23:13
The pipes module uses os.popen(): The open_r() and open_w() methods of pipes.Template are implemented with os.popen().

Multiple tests still use os.popen():

* test_select: SelectTestCase.test_select()
* test_posix: PosixTester.test_getgroups()
* test_os: EnvironTests._test_underlying_process_env()
* test_os: EnvironTests.test_update2()

And os.popen() tests:

* test_os: EnvironTests.test_os_popen_iter()
* test_popen
msg383016 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-14 23:19
About the pipes module, see bpo-41150: "... unapplicable for processing binary data and text data non-encodable with the locale encoding".
msg383017 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-14 23:20
See also bpo-26124: "shlex.quote and pipes.quote do not quote shell keywords".
msg383020 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-14 23:29
About shell injection, subprocess.getstatusoutput() uses subprocess.Popen(shell=True).
https://docs.python.org/dev/library/subprocess.html#subprocess.getstatusoutput

It's done on purpose: "Execute the string cmd in a shell with Popen.check_output()".
msg383037 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-15 07:45
Searching os.popen in code on GitHub gives around 4.5 millions of results. Seems that most of them are with literal strings which are very specific to the program, like

    check2 = os.popen('grep "net\.ipv4\.ip_forward" /etc/sysctl.conf /etc/sysctl.d/*').read()

They are not vulnerable to shell injection and other drawbacks of os.popen do not matter in that cases. Most of that code looks like specialized scripts rather than parts of general libraries.

Yes, some examples can be vulnerable to shell injection (although in they use cases, with restricted data and environment, they can be pretty safe). But deprecating os.popen can break millions of scripts and cause more harm than prevent bugs.

It may be better strategy to document drawbacks and limitations of os.popen and advertise alternatives.
msg383074 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-15 16:17
> check2 = os.popen('grep "net\.ipv4\.ip_forward" /etc/sysctl.conf /etc/sysctl.d/*').read()

Such code leaks a zombi process when the child process completes, because the parent never reads its exit status :-(
msg383079 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-15 17:06
New changeset 7f14a3756b61272cc15f24302589874b125c2f04 by Victor Stinner in branch 'master':
bpo-42641: Enhance test_select.test_select() (GH-23782)
https://github.com/python/cpython/commit/7f14a3756b61272cc15f24302589874b125c2f04
msg383080 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-15 17:06
I created bpo-42648: "subprocess: add a helper/parameter to catch exec() OSError exception".
msg383100 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-15 22:12
> document drawbacks and limitations of os.popen and advertise alternatives.

This sounds like a good idea in any case ;-)
msg383143 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2020-12-16 10:36
> There is also the os.system() function which exposes the libc system() function. Should we deprecate this one as well?

Please don't deprecate os.system. For quick and dirty scripts used in trusted environments with trusted data, it is simple to use, effective and safe enough.
msg402353 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-09-21 20:03
It seems like they are legit use cases for os.popen(), so I abandon my idea of deprecating it.
History
Date User Action Args
2021-09-21 20:03:59vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg402353

stage: patch review -> resolved
2020-12-16 10:36:11steven.dapranosetnosy: + steven.daprano
messages: + msg383143
2020-12-15 22:12:52vstinnersetmessages: + msg383100
2020-12-15 17:06:57vstinnersetmessages: + msg383080
2020-12-15 17:06:45vstinnersetmessages: + msg383079
2020-12-15 16:38:46vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request22640
2020-12-15 16:17:51vstinnersetmessages: + msg383074
2020-12-15 07:45:48serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg383037
2020-12-14 23:29:29vstinnersetmessages: + msg383020
2020-12-14 23:20:51vstinnersetmessages: + msg383017
2020-12-14 23:19:17vstinnersetmessages: + msg383016
2020-12-14 23:13:51vstinnersetmessages: + msg383015
2020-12-14 23:07:06vstinnersetmessages: + msg383014
2020-12-14 23:01:17vstinnersetmessages: + msg383013
2020-12-14 22:52:43vstinnercreate