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

subprocess fails when used as init, vfork() returns EINVAL if PID=1 #91307

Closed
socketpair mannequin opened this issue Mar 29, 2022 · 13 comments
Closed

subprocess fails when used as init, vfork() returns EINVAL if PID=1 #91307

socketpair mannequin opened this issue Mar 29, 2022 · 13 comments
Assignees
Labels
3.10 only security fixes 3.11 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@socketpair
Copy link
Mannequin

socketpair mannequin commented Mar 29, 2022

BPO 47151
Nosy @gpshead, @vstinner, @socketpair, @corona10, @miss-islington
PRs
  • bpo-47151: Fallback to fork when vfork fails in subprocess. #32186
  • [3.10] bpo-47151: Fallback to fork when vfork fails in subprocess. (GH-32186) #32219
  • 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 = 'https://github.com/gpshead'
    closed_at = <Date 2022-03-31.23:12:07.704>
    created_at = <Date 2022-03-29.10:54:10.950>
    labels = ['extension-modules', 'type-bug', '3.10', '3.11']
    title = 'subprocess fails when used as init, vfork() returns EINVAL if PID=1'
    updated_at = <Date 2022-03-31.23:12:07.703>
    user = 'https://github.com/socketpair'

    bugs.python.org fields:

    activity = <Date 2022-03-31.23:12:07.703>
    actor = 'gregory.p.smith'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2022-03-31.23:12:07.704>
    closer = 'gregory.p.smith'
    components = ['Extension Modules']
    creation = <Date 2022-03-29.10:54:10.950>
    creator = 'socketpair'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 47151
    keywords = ['patch']
    message_count = 7.0
    messages = ['416252', '416316', '416334', '416416', '416452', '416455', '416460']
    nosy_count = 5.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'socketpair', 'corona10', 'miss-islington']
    pr_nums = ['32186', '32219']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue47151'
    versions = ['Python 3.10', 'Python 3.11']

    @socketpair
    Copy link
    Mannequin Author

    socketpair mannequin commented Mar 29, 2022

    The bug introduced here: bpo-35823

    If os.gepid() == 1, vfork() does not work and returns EINVAL.

    Please add check for the current pid in condition where CPython chooses between fork() and vfork().

    @socketpair socketpair mannequin added stdlib Python modules in the Lib dir 3.10 only security fixes 3.11 only security fixes labels Mar 29, 2022
    @gpshead gpshead added extension-modules C modules in the Modules dir and removed stdlib Python modules in the Lib dir labels Mar 29, 2022
    @gpshead gpshead changed the title vfork() returns EINVAL if PID=1 subprocess fails when used as init, vfork() returns EINVAL if PID=1 Mar 29, 2022
    @gpshead gpshead added the type-bug An unexpected behavior, bug, or error label Mar 29, 2022
    @gpshead
    Copy link
    Member

    gpshead commented Mar 30, 2022

    Any possibility that you can test the attached PR as pid 1?

    @vstinner
    Copy link
    Member

    Any possibility that you can test the attached PR as pid 1?

    Python should be built as a static binary and can be used as the init process on the kernel command line, no? I'm not sure that "static binary" is a requirement, since I'm commonly using init=/usr/bin/bash to fix a broken Linux, and on my Fedora, this program is "dynamically linked".

    Who uses Python as pid 1? I'm now curious :-)

    @socketpair
    Copy link
    Mannequin Author

    socketpair mannequin commented Mar 31, 2022

    Well.

    1. We use Python as PID 1. In PID namespace.
    2. Further investigation gave information that vfork()+pid=1 actually WORKS. The problem is connected with another weird thing in kernel (undocumented unshare() flags).
    3. The logic to try fork() after vfork() has failed is NICE. Please merge. I would also write a message to stderr in case vfork() failed.

    P.S. Program may or may not be static in order to work as PID=1.

    @gpshead
    Copy link
    Member

    gpshead commented Mar 31, 2022

    Thanks. I had wondered if this was really a pid=1 restriction or not, but I could definitely imagine kernel scenarios where vfork is simply forbidden regardless. There are a variety of policy mechanisms in kernels, mainline Linux or not, that _could_ do that kind of thing.

    As much as I'd like to expose that the fallback happened, emitting to stderr isn't friendly and using warnings from this code is complicated so I'm inclined to keep the silent fallback on failure simple as is until someone can demonstrate of it causing a practical problem.

    Outside of unusual configurations, if this were ever happening when it is not expected, people observing low subprocess performance could strace and witness the vfork syscall failure.

    I'll merge once our CI is happy.

    @gpshead
    Copy link
    Member

    gpshead commented Mar 31, 2022

    New changeset 4a08c4c by Gregory P. Smith in branch 'main':
    bpo-47151: Fallback to fork when vfork fails in subprocess. (GH-32186)
    4a08c4c

    @miss-islington
    Copy link
    Contributor

    New changeset 9ed179b by Miss Islington (bot) in branch '3.10':
    bpo-47151: Fallback to fork when vfork fails in subprocess. (GH-32186)
    9ed179b

    @gpshead gpshead closed this as completed Mar 31, 2022
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @izbyshev
    Copy link
    Contributor

    izbyshev commented May 1, 2022

    I had wondered if this was really a pid=1 restriction or not, but I could definitely imagine kernel scenarios where vfork is simply forbidden regardless. There are a variety of policy mechanisms in kernels, mainline Linux or not, that could do that kind of thing.

    The reason for this particular EINVAL is explained in https://bugzilla.kernel.org/show_bug.cgi?id=215769#c6. (I suspect the underlying reason is that the parent and the child need different VDSOs if they are in separate time namespaces, and this implies that they can't share the address space).

    In hindsight, I should have realized that vfork()'s address space sharing could cause extra failure modes that plain fork() doesn't have. I was aware that many clone() flags are forbidden in conjunction with CLONE_VM, and it's a small step from there to realize that unshare() + vfork() could cause trouble. Sorry about that, and thanks for adding the fallback!

    Of course, since posix_spawn() is implemented via vfork() in modern glibc, it suffers from the same issue. For example, on Linux >= 5.6 (for time namespaces):

    $ unshare -UrT python3 -c 'import subprocess as s; s.call("/bin/true", close_fds=False)'
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/usr/lib64/python3.8/subprocess.py", line 340, in call
        with Popen(*popenargs, **kwargs) as p:
      File "/usr/lib64/python3.8/subprocess.py", line 858, in __init__
        self._execute_child(args, executable, preexec_fn, close_fds,
      File "/usr/lib64/python3.8/subprocess.py", line 1594, in _execute_child
        self._posix_spawn(args, executable, env, restore_signals,
      File "/usr/lib64/python3.8/subprocess.py", line 1545, in _posix_spawn
        self.pid = os.posix_spawn(executable, args, env, **kwargs)
    PermissionError: [Errno 1] Operation not permitted: '/bin/true'
    

    (EPERM instead of EINVAL is due to a bug in glibc's posix_spawn() error reporting: https://sourceware.org/bugzilla/show_bug.cgi?id=29109).

    Adding a fallback for posix_spawn() path seems more problematic because we can't detect vfork() failure precisely and hence we'd risk unnecessarily retrying in case of unrelated (and probably more widespread) errors. I think it's better to guide users to opt-out from both vfork() and posix_spawn() in this case.

    Alternatively, we could disable posix_spawn() on Linux entirely: its benefits are only theoretical, we don't control it and have to work around its bugs, and using different subprocess backends depending on Popen() arguments can only cause hard-to-understand behavioral differences.

    @vstinner
    Copy link
    Member

    vstinner commented May 2, 2022

    If an application calls directly vfork(), I understand that the syscall can fail with EINVAL in some special case about namespaces. But for posix_spawn(), maybe tomorrow the glibc can detect this case to decide if it can use vfork() or not, no? I don't think that the glibc can "fix" the vfork() function, but it can fix posix_spawn(), no?

    @izbyshev
    Copy link
    Contributor

    izbyshev commented May 2, 2022

    But for posix_spawn(), maybe tomorrow the glibc can detect this case to decide if it can use vfork() or not, no? I don't think that the glibc can "fix" the vfork() function, but it can fix posix_spawn(), no?

    Glibc can't easily switch from vfork() to fork() in posix_spawn() because it currently relies on address space sharing to report any child-setup/execve() errors back to the parent (unlike subprocess, which uses a pipe for that). (Relying on AS sharing also breaks error reporting under qemu-user because it implements vfork() as fork(), but glibc sadly doesn't seem to care about this use case, despite that qemu-user is not uncommonly used in cross-compilation environments).

    Of course glibc can fix that if it wants, but let's ask it instead of guessing: https://sourceware.org/bugzilla/show_bug.cgi?id=29115.

    The general problem with posix_spawn() will remain in any case. It's not clear what the practical benefits of having two vfork()-based subprocess backends are (one of which we don't control), and so far we have only seen downsides.

    @vstinner
    Copy link
    Member

    vstinner commented May 2, 2022

    The general problem with posix_spawn() will remain in any case. It's not clear what the practical benefits of having two vfork()-based subprocess backends are (one of which we don't control), and so far we have only seen downsides.

    On macOS, posix_spawn() is a syscall.

    @izbyshev
    Copy link
    Contributor

    izbyshev commented May 2, 2022

    On macOS, posix_spawn() is a syscall.

    Sure, my comment is in the context of Linux only. We don't currently use vfork() on other platforms, and without vfork() the only efficient alternative to fork() is posix_spawn().

    @izbyshev
    Copy link
    Contributor

    izbyshev commented Aug 9, 2022

    Good news: the glibc bug report led to the kernel fix, so since Linux 5.19 6.2 vfork() doesn't fail anymore if the parent unshared the time namespace.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes 3.11 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants