Skip to content

support for creating ptys and a login_tty fork_action #531

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib_eio/unix/dune
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
(foreign_stubs
(language c)
(include_dirs include)
(names fork_action stubs))
(names fork_action stubs pty))
(libraries eio unix threads mtime.clock.os))
1 change: 1 addition & 0 deletions lib_eio/unix/eio_unix.ml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[@@@alert "-unstable"]

module Fd = Fd
module Pty = Pty
module Resource = Resource
module Private = Private

Expand Down
3 changes: 3 additions & 0 deletions lib_eio/unix/eio_unix.mli
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ val pipe : Switch.t -> source * sink
module Process = Process
(** Spawning child processes with extra control. *)

module Pty = Pty
(** Pseudoterminal handling functions. *)

(** The set of resources provided to a process on a Unix-compatible system. *)
module Stdenv : sig
type base = <
Expand Down
38 changes: 26 additions & 12 deletions lib_eio/unix/fork_action.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
#include <fcntl.h>
#include <string.h>
#include <errno.h>
#include <sys/ioctl.h>
#include <paths.h>
#include <utmp.h>

#include <caml/mlvalues.h>
#include <caml/unixsupport.h>
Expand Down Expand Up @@ -153,26 +156,26 @@ static void action_dups(int errors, value v_config) {
if (dst == -1) {
// Dup to a temporary FD
if (tmp == -1) {
tmp = dup(src);
if (tmp < 0) {
eio_unix_fork_error(errors, "dup-tmp", strerror(errno));
_exit(1);
}
tmp = dup(src);
if (tmp < 0) {
eio_unix_fork_error(errors, "dup-tmp", strerror(errno));
_exit(1);
}
} else {
int r = dup2(src, tmp);
if (r < 0) {
eio_unix_fork_error(errors, "dup2-tmp", strerror(errno));
_exit(1);
}
int r = dup2(src, tmp);
if (r < 0) {
eio_unix_fork_error(errors, "dup2-tmp", strerror(errno));
_exit(1);
}
}
set_cloexec(errors, tmp, 1);
} else if (src == dst) {
set_cloexec(errors, dst, 0);
} else {
int r = dup2(src, dst);
if (r < 0) {
eio_unix_fork_error(errors, "dup2", strerror(errno));
_exit(1);
eio_unix_fork_error(errors, "dup2", strerror(errno));
_exit(1);
}
}
v_plan = Field(v_plan, 1);
Expand All @@ -189,3 +192,14 @@ static void action_dups(int errors, value v_config) {
CAMLprim value eio_unix_fork_dups(value v_unit) {
return Val_fork_fn(action_dups);
}

static void action_login_tty(int errors, value v_config) {
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.

}

CAMLprim value eio_unix_login_tty(value v_unit) {
return Val_fork_fn(action_login_tty);
}
7 changes: 7 additions & 0 deletions lib_eio/unix/fork_action.ml
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,10 @@ let inherit_fds m =
with_fds m @@ fun m ->
let plan : action list = Inherit_fds.plan m in
{ run = fun k -> k (Obj.repr (action_dups, plan, blocking)) }

external action_login_tty : unit -> fork_fn = "eio_unix_login_tty"
let action_login_tty = action_login_tty ()

let login_tty pty =
Fd.use_exn "login_tty" pty @@ fun pty ->
{ run = fun k -> k (Obj.repr (action_login_tty, pty)) }
3 changes: 3 additions & 0 deletions lib_eio/unix/fork_action.mli
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,6 @@ val inherit_fds : (int * Fd.t * [< blocking]) list -> t
A mapping from an FD to itself simply clears the close-on-exec flag.

After this, the new FDs may also be set as blocking or non-blocking, depending on [flags]. *)

val login_tty : Fd.t -> t
(** [login_tty pty] prepares for a shell login on the [pty] file descriptor. *)
87 changes: 87 additions & 0 deletions lib_eio/unix/pty.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright (c) 2004 Anil Madhavapeddy <[email protected]>
* Copyright (c) 2020–2021 Craig Ferguson <[email protected]>
*
* Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
* copyright notice and this permission notice appear in all copies.
*
* THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
* WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
* MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
* ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
* WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
* ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/


#include <stdio.h>
#include <errno.h>
#include <paths.h>
#include <fcntl.h>
#include <string.h>
#include <termios.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <signal.h>
#include <limits.h>

#include <pty.h>
#include <utmp.h>

#include <caml/mlvalues.h>
#include <caml/alloc.h>
#include <caml/memory.h>
#include <caml/fail.h>
#include <caml/callback.h>
#include <caml/signals.h>
#include <caml/unixsupport.h>

value eio_unix_open_pty(value v_unit)
{
CAMLparam1 (v_unit);
char namebuf[4096]; /* Not PATH_MAX due to portability issues */
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.

if (i < 0)
caml_uerror("openpty", Nothing);

v_ret = caml_alloc_small(3, 0);
Store_field(v_ret, 0, Val_int(masterfd));
Store_field(v_ret, 1, Val_int(slavefd));
Store_field(v_ret, 2, caml_copy_string(namebuf));
CAMLreturn (v_ret);
}

value eio_unix_window_size(value pty, value pty_window)
{
CAMLparam2 (pty, pty_window);
int ptyfd;
struct winsize w;
w.ws_row = Int32_val(Field(pty_window, 0));
w.ws_col = Int32_val(Field(pty_window, 1));
w.ws_xpixel = Int32_val(Field(pty_window, 2));
w.ws_ypixel = Int32_val(Field(pty_window, 3));
ptyfd = Int_val(Field(pty, 0));
ioctl(ptyfd, TIOCSWINSZ, &w);
CAMLreturn (Val_unit);
}

value eio_unix_tty_window_size(value unit)
{
CAMLparam1 (unit);
CAMLlocal1(pty_window);

struct winsize w;
if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &w) == -1)
memset(&w, 0, sizeof(w));
pty_window = caml_alloc_small(4, 0);
Store_field(pty_window, 0, caml_copy_int32(w.ws_row));
Store_field(pty_window, 1, caml_copy_int32(w.ws_col));
Store_field(pty_window, 2, caml_copy_int32(w.ws_xpixel));
Store_field(pty_window, 3, caml_copy_int32(w.ws_ypixel));
CAMLreturn (pty_window);
}
16 changes: 16 additions & 0 deletions lib_eio/unix/pty.ml
Original file line number Diff line number Diff line change
@@ -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

slavefd : Unix.file_descr;
Comment on lines +2 to +3
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?).

name : string;
}

type pty_window = {
row : int32;
col : int32;
xpixel : int32;
ypixel : int32
}

external open_pty : unit -> pty = "eio_unix_open_pty"
external set_window_size : pty -> pty_window -> unit = "eio_unix_window_size"
external tty_window_size : unit -> pty_window = "eio_unix_tty_window_size"