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

Deprecate os.popen() function #86807

Closed
vstinner opened this issue Dec 14, 2020 · 14 comments
Closed

Deprecate os.popen() function #86807

vstinner opened this issue Dec 14, 2020 · 14 comments
Labels
3.10 only security fixes stdlib Python modules in the Lib dir

Comments

@vstinner
Copy link
Member

BPO 42641
Nosy @vstinner, @stevendaprano, @serhiy-storchaka
PRs
  • bpo-42641: Enhance test_select.test_select() #23782
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-09-21.20:03:59.565>
    created_at = <Date 2020-12-14.22:52:43.279>
    labels = ['library', '3.10']
    title = 'Deprecate os.popen() function'
    updated_at = <Date 2021-09-21.20:03:59.565>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-09-21.20:03:59.565>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-09-21.20:03:59.565>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2020-12-14.22:52:43.279>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42641
    keywords = ['patch']
    message_count = 14.0
    messages = ['383012', '383013', '383014', '383015', '383016', '383017', '383020', '383037', '383074', '383079', '383080', '383100', '383143', '402353']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'steven.daprano', 'serhiy.storchaka']
    pr_nums = ['23782']
    priority = 'normal'
    resolution = 'rejected'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue42641'
    versions = ['Python 3.10']

    @vstinner
    Copy link
    Member Author

    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 dcf97b9, then commit f5a4292 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?

    @vstinner vstinner added 3.10 only security fixes stdlib Python modules in the Lib dir labels Dec 14, 2020
    @vstinner
    Copy link
    Member Author

    In the past, multiple os.popen() usage have been replaced with subprocess in the stdlib to prevent the risk of shell injection:

    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).

    @vstinner
    Copy link
    Member Author

    os.popen() doesn't emit a ResourceWarning when close() is not called, leading to weird issues like bpo-15408.

    @vstinner
    Copy link
    Member Author

    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

    @vstinner
    Copy link
    Member Author

    About the pipes module, see bpo-41150: "... unapplicable for processing binary data and text data non-encodable with the locale encoding".

    @vstinner
    Copy link
    Member Author

    See also bpo-26124: "shlex.quote and pipes.quote do not quote shell keywords".

    @vstinner
    Copy link
    Member Author

    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()".

    @serhiy-storchaka
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member Author

    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 :-(

    @vstinner
    Copy link
    Member Author

    New changeset 7f14a37 by Victor Stinner in branch 'master':
    bpo-42641: Enhance test_select.test_select() (GH-23782)
    7f14a37

    @vstinner
    Copy link
    Member Author

    I created bpo-42648: "subprocess: add a helper/parameter to catch exec() OSError exception".

    @vstinner
    Copy link
    Member Author

    document drawbacks and limitations of os.popen and advertise alternatives.

    This sounds like a good idea in any case ;-)

    @stevendaprano
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member Author

    It seems like they are legit use cases for os.popen(), so I abandon my idea of deprecating it.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants