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

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

Open
cagney opened this issue Apr 11, 2019 · 4 comments
Open

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

cagney opened this issue Apr 11, 2019 · 4 comments

Comments

@cagney
Copy link

cagney commented Apr 11, 2019

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
Copy link

dluyer commented Aug 6, 2019

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
Copy link
Author

cagney commented Aug 7, 2019

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
Copy link

dluyer commented Aug 7, 2019

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

@takluyver
Copy link
Member

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

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

3 participants