-
Notifications
You must be signed in to change notification settings - Fork 72
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Ryan Gibb <ryan@freumh.org>
This looks great! Using
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 |
There was a problem hiding this 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
masterfd : Unix.file_descr; | ||
slavefd : Unix.file_descr; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Just a draft to figure out the right way to support spawning a shell with a pty:
/cc @RyanGibb