pexpect / ptyprocess

Run a subprocess in a pseudo terminal
https://ptyprocess.readthedocs.io/en/latest/
Other
222 stars 71 forks source link

use os.posix_spawn when available (i.e, 3.8) #53

Open cagney opened 5 years ago

cagney commented 5 years ago

The attached patch is an experiment in using Python 3.8's os.posix_spawn() in ptyprocess. Since it doesn't use fork() it eliminates all those problems. A simple test using pexpect.interact() seemed to work.

I know it has a race with inheritable when called in parallel; and I'm sure there's more.

ptyprocess-posix-spawn.patch.gz

The change looks bigger than it is because I indented all the old code. Below is what matters, which I've included so it is easier to pick it apart....

    if hasattr(os, 'posix_spawn'):
        print("using posix_spawn")
        # Issue 36603: Use os.openpty() (and try to avoid the
        # whole pty module) as that guarentees inheritable (if it
        # ever fails then just file a bug against os.openpty()
        fd, tty = os.openpty()
        # Try to set window size on TTY per below; but is this
        # needed?
        try:
            _setwinsize(tty, *dimensions)
        except IOError as err:
            if err.args[0] not in (errno.EINVAL, errno.ENOTTY):
                raise
        # Try to disable echo if spawn argument echo was unset per
        # below; but does this work?
        if not echo:
            try:
                _setecho(tty, False)
            except (IOError, termios.error) as err:
                if err.args[0] not in (errno.EINVAL, errno.ENOTTY):
                    raise
        # Create the child: convert the tty into STDIO; use the
        # default ENV if needed; and try to make the child the
        # session head using SETSID.  Assume that all files have
        # inheritable (close-on-exec) correctly set.
        file_actions=[
            (os.POSIX_SPAWN_DUP2, tty, STDIN_FILENO),
            (os.POSIX_SPAWN_DUP2, tty, STDOUT_FILENO),
            (os.POSIX_SPAWN_DUP2, tty, STDERR_FILENO),
            (os.POSIX_SPAWN_CLOSE, tty),
            (os.POSIX_SPAWN_CLOSE, fd),
        ]
        spawn_env = env or os.environ
        pid = os.posix_spawn(command, argv, spawn_env,
                             file_actions=file_actions,
                             setsid=True)
        # Child started.  Now close tty and stop PTY(FD) being
        # inherited. Note that there's a race here: a parallel
        # fork/exec would unwittingly inherit this PTY(FD)/TTY
        # pair.  Probably need to wrap all this in a lock?
        os.close(tty)
        os.set_inheritable(fd, False)
dluyer commented 5 years ago

fd & tty should be non-inheritable already (by default), so there would be no race and the os.set_inheritable and POSIX_SPAWN_CLOSE are both unnecessary.

Documentation states that all FDs are non-inheritable by default in Python 3.4+, and I have confirmed this in Python 3.6.6.

cagney commented 5 years ago

fd & tty should be non-inheritable already (by default), so there would be no race and the os.set_inheritable and POSIX_SPAWN_CLOSE are both unnecessary.

Documentation states that all FDs are non-inheritable by default in Python 3.4+, and I have confirmed this in Python 3.6.6.

The race is in os.openpty(). Since openpty() returns an inheritable master and slave, a parallel for made after openpty() returns but before inheritable is cleared would inherit those FDs.

if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) != 0)
    goto posix_error;

if (_Py_set_inheritable(master_fd, 0, NULL) < 0)
    goto error;
if (_Py_set_inheritable(slave_fd, 0, NULL) < 0)
    goto error;

but yes, the os.set_inheritable and POSIX_SPAWN_CLOSE could be dropped

dluyer commented 5 years ago

That region of code is protected by the GIL. No race.

takluyver commented 5 years ago

(Copying my comment from #52)

I don't know how to combine the os.spawn* functions - or anything that doesn't involve a fork - with setting up a new pty as the controlling terminal of the child process. I suspect it's not possible, but it would be very nice if it was. If you figure it out, please let me know!