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

support for creating ptys and a login_tty fork_action #531

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

avsm
Copy link
Contributor

@avsm avsm commented May 28, 2023

Just a draft to figure out the right way to support spawning a shell with a pty:

  • do the pty fds need to created as cloexec, and then inherited in a fork_action to the subshell? Right now they are not cloexec, but there is no close Fork_action and so there is too much open on the other side.
  • we use login_tty as a compromise between manually doing the tty setup ourselves, and do not need low-level bindings to setsid and other terminal manipulation bits.
  • unsure how to expose this more portably in Eio, as right now Fork_actions require a direct dependency on either Eio_linux or Eio_posix, and cannot be called via Eio_unix. See avsm/eeww@3cbac4c#diff-81dfb4951962ecf602592af852995decc04ccce063dd78977949ed086c803adfR45 for some code that mimics Eio.Process.spawn, but with a pty. Should this plumbed through Eio.Process.spawn somehow?
  • still need to add tests
  • only Linux at the moment, need to do the configure bits to work on macOS/*BSD

/cc @RyanGibb

Co-authored-by: Ryan Gibb <ryan@freumh.org>
@RyanGibb
Copy link
Contributor

RyanGibb commented May 28, 2023

This looks great! Using login_tty is much nicer than manually doing tty setup.

[ ] do the pty fds need to created as cloexec, and then inherited in a fork_action to the subshell? Right now they are not cloexec, but there is no close Fork_action and so there is too much open on the other side.

I saw that they're manually closed in https://github.com/avsm/melange/blob/e92240e6dc8a440cafa91488a1fc367e2ba57de1/lib/ounix/ounix.ml#L66. Perhaps they should similarly be closed in a Pty.close after the fork returns? I think setting cloexec would require manually creating a pty by opening /dev/ptmx (/dev/pty on bsd I think), and calling ioctl TIOCGPTPEER, etc, as openpty doesn't doesn't support it.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding login_tty seems reasonable. I guess the other option is to add fork actions for creating sessions and setting controlling terminals, but maybe that's less portable. Plumbing it through as an extra argument to spawn_unix seems fine.

The opentty situation seems like a huge mess, but maybe we just need to document that it only works in single-domain programs (and set close-on-exec quickly after getting the FDs). Or this could be in a separate library.

value v_pty = Field(v_config, 1);

if (login_tty(Int_val(v_pty)) == -1)
dprintf(errors, "action_login_tty Error logging in tty: %s", strerror(errno));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dprintf(errors, "action_login_tty Error logging in tty: %s", strerror(errno));
eio_unix_fork_error(errors, "action_login_tty", strerror(errno));
_exit(1);

It needs to exit if there's an error, since the parent will assume the child failed to start. I'm also using eio_unix_fork_error here for consistency with the others, but maybe the others should be changed instead.

int i, masterfd, slavefd;
CAMLlocal1(v_ret);

i = openpty(&masterfd, &slavefd, namebuf, NULL, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The man-page notes that:

Nobody knows how much space should be reserved for name. So, calling openpty() or forkpty() with non-NULL name may not be secure.

However, ptsname_r is Linux-only.

In a multi-domain program, all FDs must be created close-on-exec atomically, or they can leak. https://www.austingroupbugs.net/view.php?id=411 says:

The function posix_openpt( ) with the O_CLOEXEC flag is necessary to
avoid a data race in multi-threaded applications. Without it, a file
descriptor is leaked into a child process created by one thread in the
window between another thread creating a file descriptor with
posix_openpt( ) and then using fcntl( ) to set the FD_CLOEXEC flag.

However, it's not clear how widely-supported that is.

The man-page for posix_openpt notes that you can just open /dev/ptmx directly, which would avoid these problems, but require some other manual setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to come down to using either BSD-style pty handling (openpty) or Unix98-style handling (posix_openpt). The latter seems very much preferred, and a quick smoke test on macOS, Linux and OpenBSD shows posix_openpt is present there. I'll have a go at switching this PR to using /dev/ptmx instead -- there's code in portable OpenSSH to use as a guide.

Comment on lines +2 to +3
masterfd : Unix.file_descr;
slavefd : Unix.file_descr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The public interface should use Eio_unix.Fd.t, and attach them to a switch (or maybe two switches?).

@@ -0,0 +1,16 @@
type pty = {
masterfd : Unix.file_descr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is the terminology used by pseudoterminal at some point, but it's problematic and not even that technical. Could we change it ? I've seen others use primary and replica and the like?

Copy link
Contributor

@RyanGibb RyanGibb Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this while experimenting with https://github.com/RyanGibb/ocaml-exec-shell. It seems UNIX/POSIX/Linux use the existing terminology and I couldn't find any discussions around changing it. A relevant thread is in the glibc mail archive where a number of alternatives are discussed. I couldn't see a consensus on new terminology, but changing master and slave to pty and tty respectively, with 'pseudoterminal device' and 'terminal device' in prose, seemed popular.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the pty and terminal device terminology is fine by me, of course. I'll do that in the next revision of this diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reinforcement that this is established terminology is it's usage in the openssh source: https://github.com/openssh/openssh-portable/blob/2709809fd616a0991dc18e3a58dea10fb383c3f0/sshpty.c#L71

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

Successfully merging this pull request may close these issues.

4 participants