Hi
I think putting these in libutil is a good idea. tmux could use
them. I'd like to have openptmfd() as you suggest as well - it'd be nice
to hide PATH_PTMDEV as well as the PTMGET.
Life would be a lot easier for portable if there was fdforkpty() as
well.
On Mon, Feb 27, 2017 at 07:00:03PM +0100, Reyk Floeter wrote:
> On Mon, Feb 27, 2017 at 10:19:28AM -0700, Theo de Raadt wrote:
> > > On Mon, Feb 27, 2017 at 10:55:31AM +0100, Reyk Floeter wrote:
> > > > The following diff is not really needed without just yet, but:
> > > > - openening /dev/ptm in advance might allow better pledge in the future
> > > > - customizing "openpty" will allow to do what we need next
> > > > Since openpty(4) is libutil and not libc, it should be fine not using
> > > > it.
> > > >
> > > > OK?
> > > >
> > > > Replace openpty(3) with local function that uses pre-opened
> > > > /dev/ptm fd
> > > >
> > > > This allows more flexibility for upcoming changes and better pledge.
> > > > We also didn't use half of the features of libutil's openpty
> > > > function.
> > > > Additionally, make sure that the ttys are closed correctly on
> > > > shutdown.
> >
> > This is related to the change that happened in tmux, when PTM ioctl was
> > locked down in pledge ioctl. The primary purpose of the change, was that
> > review of the PTM ioctl codepaths showed it was a lot of risk to keep in
> > pledge "tty" programs, so it makes sense that some of them get restructured.
> >
> > That means the libutil functions aren't always used. We should consider
> > whether raw ioctl PTMGET in programs is "good" or "bad". vmd is openbsd
> > only, but tmux now has this in the openbsd-specific code.
> >
> > If this goes PTMGET approach goes any further, we should think about a
> > new interface in libutil which hides the ioctl in a differnet way, and
> > convert programs to it. Just a thought..
> >
>
> I had a diff for libutil but millert@ (or was it nicm@?) told me that
> it is better to keep it as a local change. I cannot remember and
> unfortunately I cannot find the mail where it was mentioned.
>
> I attached the old diff for libutil as a reference.
>
> To fully abstract /dev/ptm in libutil, the API below would have to be
> extended to have another function to open /dev/ptm in libutil as well,
> eg. (better names would be desired):
>
> fd = openptmfd()
> pledge()
> fdopenpty(fd, ...)
> fdopenpty(fd, ...)
> fdopenpty(fd, ...)
> fdopenpty(fd, ...)
>
> Reyk
>
> Index: lib/libutil/openpty.3
> ===================================================================
> RCS file: /cvs/src/lib/libutil/openpty.3,v
> retrieving revision 1.17
> diff -u -p -u -p -r1.17 openpty.3
> --- lib/libutil/openpty.3 28 Aug 2015 19:54:06 -0000 1.17
> +++ lib/libutil/openpty.3 25 Jan 2017 08:40:46 -0000
> @@ -44,6 +44,8 @@
> .Ft int
> .Fn openpty "int *amaster" "int *aslave" "char *name" "struct termios
> *termp" "struct winsize *winp"
> .Ft int
> +.Fn fdopenpty "int ptmfd" "int *amaster" "int *aslave" "char *name" "struct
> termios *termp" "struct winsize *winp"
> +.Ft int
> .Fn login_tty "int fd"
> .Ft pid_t
> .Fn forkpty "int *amaster" "char *name" "struct termios *termp" "struct
> winsize *winp"
> @@ -88,6 +90,15 @@ the caller, permissions are set to corre
> uses of that device are revoked (see
> .Xr revoke 2
> for details).
> +.Pp
> +The
> +.Fn fdopenpty
> +function works like
> +.Fn openpty
> +but expects a pre-opened
> +.Pa /dev/ptm
> +file descriptor
> +.Fa ptmfd .
> .Pp
> The
> .Fn login_tty
> Index: lib/libutil/pty.c
> ===================================================================
> RCS file: /cvs/src/lib/libutil/pty.c,v
> retrieving revision 1.20
> diff -u -p -u -p -r1.20 pty.c
> --- lib/libutil/pty.c 30 Aug 2016 14:44:45 -0000 1.20
> +++ lib/libutil/pty.c 25 Jan 2017 08:40:46 -0000
> @@ -57,11 +57,30 @@ openpty(int *amaster, int *aslave, char
> fd = open(PATH_PTMDEV, O_RDWR|O_CLOEXEC);
> if (fd == -1)
> return (-1);
> - if ((ioctl(fd, PTMGET, &ptm) == -1)) {
> +
> + if (fdopenpty(fd, amaster, aslave, name, termp, winp) == -1) {
> close(fd);
> return (-1);
> }
> close(fd);
> +
> + return (0);
> +}
> +
> +int
> +fdopenpty(int ptmfd, int *amaster, int *aslave, char *name,
> + struct termios *termp, struct winsize *winp)
> +{
> + int master, slave;
> + struct ptmget ptm;
> +
> + /*
> + * Use /dev/ptm and the PTMGET ioctl to get a properly set up and
> + * owned pty/tty pair.
> + */
> + if ((ioctl(ptmfd, PTMGET, &ptm) == -1))
> + return (-1);
> +
> master = ptm.cfd;
> slave = ptm.sfd;
> if (name) {
> Index: lib/libutil/util.h
> ===================================================================
> RCS file: /cvs/src/lib/libutil/util.h,v
> retrieving revision 1.34
> diff -u -p -u -p -r1.34 util.h
> --- lib/libutil/util.h 3 Jun 2013 21:07:02 -0000 1.34
> +++ lib/libutil/util.h 25 Jan 2017 08:40:46 -0000
> @@ -99,6 +99,7 @@ void pw_copy(int, int, const struct pass
> int pw_scan(char *, struct passwd *, int *);
> void pw_error(const char *, int, int);
> int openpty(int *, int *, char *, struct termios *, struct winsize *);
> +int fdopenpty(int, int *, int *, char *, struct termios *, struct winsize
> *);
> int opendisk(const char *, int, char *, size_t, int);
> pid_t forkpty(int *, char *, struct termios *, struct winsize *);
> int getmaxpartitions(void);
>
