posix_openpt: allow O_CLOEXEC

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

posix_openpt: allow O_CLOEXEC

joshua stein-3
The spec says the behavior of anything other than O_RDWR and
O_NOCTTY is unspecified, but FreeBSD allows passing O_CLOEXEC.


Index: lib/libc/stdlib/posix_openpt.3
===================================================================
RCS file: /cvs/src/lib/libc/stdlib/posix_openpt.3,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 posix_openpt.3
--- lib/libc/stdlib/posix_openpt.3 25 Jan 2019 00:19:25 -0000 1.4
+++ lib/libc/stdlib/posix_openpt.3 5 Feb 2020 21:41:00 -0000
@@ -45,7 +45,7 @@ argument is formed by bitwise-inclusive
 .Tn OR Ns 'ing
 the following values defined in
 .In fcntl.h :
-.Bl -tag -width O_NOCTTY -offset indent
+.Bl -tag -width O_CLOEXEC -offset indent
 .It Dv O_RDWR
 Open for reading and writing.
 .It Dv O_NOCTTY
@@ -53,6 +53,8 @@ Prevent the device from being made the c
 This flag has no effect on
 .Ox
 and is included for compatibility with other systems.
+.It Dv O_CLOEXEC
+Set the close-on-exec flag for the new file descriptor.
 .El
 .Pp
 The
Index: lib/libc/stdlib/posix_pty.c
===================================================================
RCS file: /cvs/src/lib/libc/stdlib/posix_pty.c,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 posix_pty.c
--- lib/libc/stdlib/posix_pty.c 25 Jan 2019 00:19:25 -0000 1.3
+++ lib/libc/stdlib/posix_pty.c 5 Feb 2020 21:41:00 -0000
@@ -36,13 +36,13 @@ posix_openpt(int oflag)
 
  /* User must specify O_RDWR in oflag. */
  if ((oflag & O_ACCMODE) != O_RDWR ||
-    (oflag & ~(O_ACCMODE | O_NOCTTY)) != 0) {
+    (oflag & ~(O_ACCMODE | O_NOCTTY | O_CLOEXEC)) != 0) {
  errno = EINVAL;
  return -1;
  }
 
  /* Get pty master and slave (this API only uses the master). */
- fd = open(PATH_PTMDEV, O_RDWR);
+ fd = open(PATH_PTMDEV, oflag);
  if (fd != -1) {
  if (ioctl(fd, PTMGET, &ptm) != -1) {
  close(ptm.sfd);

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

Theo de Raadt-2
Seems very reasonable.

joshua stein <[hidden email]> wrote:

> The spec says the behavior of anything other than O_RDWR and
> O_NOCTTY is unspecified, but FreeBSD allows passing O_CLOEXEC.
>
>
> Index: lib/libc/stdlib/posix_openpt.3
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/posix_openpt.3,v
> retrieving revision 1.4
> diff -u -p -u -p -r1.4 posix_openpt.3
> --- lib/libc/stdlib/posix_openpt.3 25 Jan 2019 00:19:25 -0000 1.4
> +++ lib/libc/stdlib/posix_openpt.3 5 Feb 2020 21:41:00 -0000
> @@ -45,7 +45,7 @@ argument is formed by bitwise-inclusive
>  .Tn OR Ns 'ing
>  the following values defined in
>  .In fcntl.h :
> -.Bl -tag -width O_NOCTTY -offset indent
> +.Bl -tag -width O_CLOEXEC -offset indent
>  .It Dv O_RDWR
>  Open for reading and writing.
>  .It Dv O_NOCTTY
> @@ -53,6 +53,8 @@ Prevent the device from being made the c
>  This flag has no effect on
>  .Ox
>  and is included for compatibility with other systems.
> +.It Dv O_CLOEXEC
> +Set the close-on-exec flag for the new file descriptor.
>  .El
>  .Pp
>  The
> Index: lib/libc/stdlib/posix_pty.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/posix_pty.c,v
> retrieving revision 1.3
> diff -u -p -u -p -r1.3 posix_pty.c
> --- lib/libc/stdlib/posix_pty.c 25 Jan 2019 00:19:25 -0000 1.3
> +++ lib/libc/stdlib/posix_pty.c 5 Feb 2020 21:41:00 -0000
> @@ -36,13 +36,13 @@ posix_openpt(int oflag)
>  
>   /* User must specify O_RDWR in oflag. */
>   if ((oflag & O_ACCMODE) != O_RDWR ||
> -    (oflag & ~(O_ACCMODE | O_NOCTTY)) != 0) {
> +    (oflag & ~(O_ACCMODE | O_NOCTTY | O_CLOEXEC)) != 0) {
>   errno = EINVAL;
>   return -1;
>   }
>  
>   /* Get pty master and slave (this API only uses the master). */
> - fd = open(PATH_PTMDEV, O_RDWR);
> + fd = open(PATH_PTMDEV, oflag);
>   if (fd != -1) {
>   if (ioctl(fd, PTMGET, &ptm) != -1) {
>   close(ptm.sfd);
>

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

Todd C. Miller-3
In reply to this post by joshua stein-3
On Wed, 05 Feb 2020 15:47:37 -0600, joshua stein wrote:

> The spec says the behavior of anything other than O_RDWR and
> O_NOCTTY is unspecified, but FreeBSD allows passing O_CLOEXEC.

OK, but the manual needs to specify that O_CLOEXEC support is an
extension.  E.g., under STANDARDS:

The ability to use
.Dv O_CLOEXEC
is an extension to that standard.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

William Ahern-2
On Wed, Feb 05, 2020 at 05:48:41PM -0700, Todd C. Miller wrote:

> On Wed, 05 Feb 2020 15:47:37 -0600, joshua stein wrote:
>
> > The spec says the behavior of anything other than O_RDWR and
> > O_NOCTTY is unspecified, but FreeBSD allows passing O_CLOEXEC.
>
> OK, but the manual needs to specify that O_CLOEXEC support is an
> extension.  E.g., under STANDARDS:
>
> The ability to use
> .Dv O_CLOEXEC
> is an extension to that standard.
>
>  - todd

FWIW, it looks like a future standard version/revision will add O_CLOEXEC to
posix_openpt. See https://www.austingroupbugs.net/view.php?id=411 (grep for
posix_openpt, which is included among many other similar modifications).

I haven't yet figured out the rules for that group and how things wind up in
the published standard; what counts as a defect and what counts as a new
feature. I'm a little surprised it's not already in the -2017 revision.
IIRC, some other changes related to O_CLOEXEC/FD_CLOEXEC consistency made it
into -2017 and earlier revisions.

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

joshua stein-3
In reply to this post by Todd C. Miller-3
On Wed, 05 Feb 2020 at 17:48:41 -0700, Todd C. Miller wrote:

> On Wed, 05 Feb 2020 15:47:37 -0600, joshua stein wrote:
>
> > The spec says the behavior of anything other than O_RDWR and
> > O_NOCTTY is unspecified, but FreeBSD allows passing O_CLOEXEC.
>
> OK, but the manual needs to specify that O_CLOEXEC support is an
> extension.  E.g., under STANDARDS:
>
> The ability to use
> .Dv O_CLOEXEC
> is an extension to that standard.

Thanks, incorporated.


Index: lib/libc/stdlib/posix_openpt.3
===================================================================
RCS file: /cvs/src/lib/libc/stdlib/posix_openpt.3,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 posix_openpt.3
--- lib/libc/stdlib/posix_openpt.3 25 Jan 2019 00:19:25 -0000 1.4
+++ lib/libc/stdlib/posix_openpt.3 6 Feb 2020 16:01:58 -0000
@@ -45,7 +45,7 @@ argument is formed by bitwise-inclusive
 .Tn OR Ns 'ing
 the following values defined in
 .In fcntl.h :
-.Bl -tag -width O_NOCTTY -offset indent
+.Bl -tag -width O_CLOEXEC -offset indent
 .It Dv O_RDWR
 Open for reading and writing.
 .It Dv O_NOCTTY
@@ -53,6 +53,8 @@ Prevent the device from being made the c
 This flag has no effect on
 .Ox
 and is included for compatibility with other systems.
+.It Dv O_CLOEXEC
+Set the close-on-exec flag for the new file descriptor.
 .El
 .Pp
 The
@@ -95,6 +97,10 @@ The
 .Fn posix_openpt
 function conforms to
 .St -p1003.1-2001 .
+.Pp
+The ability to use
+.Dv O_CLOEXEC
+is an extension to that standard.
 .Sh HISTORY
 The
 .Fn posix_openpt
Index: lib/libc/stdlib/posix_pty.c
===================================================================
RCS file: /cvs/src/lib/libc/stdlib/posix_pty.c,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 posix_pty.c
--- lib/libc/stdlib/posix_pty.c 25 Jan 2019 00:19:25 -0000 1.3
+++ lib/libc/stdlib/posix_pty.c 6 Feb 2020 16:01:58 -0000
@@ -36,13 +36,13 @@ posix_openpt(int oflag)
 
  /* User must specify O_RDWR in oflag. */
  if ((oflag & O_ACCMODE) != O_RDWR ||
-    (oflag & ~(O_ACCMODE | O_NOCTTY)) != 0) {
+    (oflag & ~(O_ACCMODE | O_NOCTTY | O_CLOEXEC)) != 0) {
  errno = EINVAL;
  return -1;
  }
 
  /* Get pty master and slave (this API only uses the master). */
- fd = open(PATH_PTMDEV, O_RDWR);
+ fd = open(PATH_PTMDEV, oflag);
  if (fd != -1) {
  if (ioctl(fd, PTMGET, &ptm) != -1) {
  close(ptm.sfd);

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

Todd C. Miller-3
This doesn't do what you think it does.  You are setting O_CLOEXEC
on the fd for /dev/ptm (which is immediately closed), not on the
pty master or slave fds.  You will need to explicitly set the
close-on-exec flag on the master using something like:

    fcntl(ptm.cfd, F_SETFD, FD_CLOEXEC);

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

Todd C. Miller-3
In reply to this post by joshua stein-3
Alternately, we could teach PTMGET to copy the close-on-exec flag
from the ptm fd.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

Theo de Raadt-2
Todd C. Miller <[hidden email]> wrote:

> Alternately, we could teach PTMGET to copy the close-on-exec flag
> from the ptm fd.

That feels better, and will be more atomic.

Two 1 line diffs in tty_pty.c

In the man page, it is easy also

     device.  The PTMGET command allocates a free pseudo terminal, changes its
     ownership to the caller, revokes the access privileges for all previous
     users, opens the file descriptors for the master and slave devices and
     returns them to the caller in struct ptmget.

something like this

     users, opens the file descriptors for the master and slave devices,
     copy the O_CLOEXEC flag from the ptm device opening, and returns them
     to the caller in struct ptmget.

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

Todd C. Miller-3
On Thu, 06 Feb 2020 10:45:44 -0700, "Theo de Raadt" wrote:

> That feels better, and will be more atomic.

Unfortunately, we can't do this in ptmioctl() since we don't have
the index of the open ptm device to use to check for the presence
of UF_EXCLOSE.  So it has to be done in libc instead.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

joshua stein-3
On Thu, 06 Feb 2020 at 11:21:11 -0700, Todd C. Miller wrote:
> On Thu, 06 Feb 2020 10:45:44 -0700, "Theo de Raadt" wrote:
>
> > That feels better, and will be more atomic.
>
> Unfortunately, we can't do this in ptmioctl() since we don't have
> the index of the open ptm device to use to check for the presence
> of UF_EXCLOSE.  So it has to be done in libc instead.

Alright, so like this then?


Index: stdlib/posix_openpt.3
===================================================================
RCS file: /cvs/src/lib/libc/stdlib/posix_openpt.3,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 posix_openpt.3
--- stdlib/posix_openpt.3 25 Jan 2019 00:19:25 -0000 1.4
+++ stdlib/posix_openpt.3 6 Feb 2020 17:34:15 -0000
@@ -45,7 +45,7 @@ argument is formed by bitwise-inclusive
 .Tn OR Ns 'ing
 the following values defined in
 .In fcntl.h :
-.Bl -tag -width O_NOCTTY -offset indent
+.Bl -tag -width O_CLOEXEC -offset indent
 .It Dv O_RDWR
 Open for reading and writing.
 .It Dv O_NOCTTY
@@ -53,6 +53,8 @@ Prevent the device from being made the c
 This flag has no effect on
 .Ox
 and is included for compatibility with other systems.
+.It Dv O_CLOEXEC
+Set the close-on-exec flag for the new file descriptor.
 .El
 .Pp
 The
@@ -95,6 +97,10 @@ The
 .Fn posix_openpt
 function conforms to
 .St -p1003.1-2001 .
+.Pp
+The ability to use
+.Dv O_CLOEXEC
+is an extension to that standard.
 .Sh HISTORY
 The
 .Fn posix_openpt
Index: stdlib/posix_pty.c
===================================================================
RCS file: /cvs/src/lib/libc/stdlib/posix_pty.c,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 posix_pty.c
--- stdlib/posix_pty.c 25 Jan 2019 00:19:25 -0000 1.3
+++ stdlib/posix_pty.c 6 Feb 2020 17:34:15 -0000
@@ -36,7 +36,7 @@ posix_openpt(int oflag)
 
  /* User must specify O_RDWR in oflag. */
  if ((oflag & O_ACCMODE) != O_RDWR ||
-    (oflag & ~(O_ACCMODE | O_NOCTTY)) != 0) {
+    (oflag & ~(O_ACCMODE | O_NOCTTY | O_CLOEXEC)) != 0) {
  errno = EINVAL;
  return -1;
  }
@@ -46,7 +46,11 @@ posix_openpt(int oflag)
  if (fd != -1) {
  if (ioctl(fd, PTMGET, &ptm) != -1) {
  close(ptm.sfd);
- mfd = ptm.cfd;
+ if ((oflag & O_CLOEXEC) &&
+    fcntl(ptm.cfd, F_SETFD, FD_CLOEXEC) == -1)
+ close(ptm.cfd);
+ else
+ mfd = ptm.cfd;
  }
  close(fd);
  }

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

Theo de Raadt-2
joshua stein <[hidden email]> wrote:

> On Thu, 06 Feb 2020 at 11:21:11 -0700, Todd C. Miller wrote:
> > On Thu, 06 Feb 2020 10:45:44 -0700, "Theo de Raadt" wrote:
> >
> > > That feels better, and will be more atomic.
> >
> > Unfortunately, we can't do this in ptmioctl() since we don't have
> > the index of the open ptm device to use to check for the presence
> > of UF_EXCLOSE.  So it has to be done in libc instead.
>
> Alright, so like this then?
>
>
> Index: stdlib/posix_openpt.3
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/posix_openpt.3,v
> retrieving revision 1.4
> diff -u -p -u -p -r1.4 posix_openpt.3
> --- stdlib/posix_openpt.3 25 Jan 2019 00:19:25 -0000 1.4
> +++ stdlib/posix_openpt.3 6 Feb 2020 17:34:15 -0000
> @@ -45,7 +45,7 @@ argument is formed by bitwise-inclusive
>  .Tn OR Ns 'ing
>  the following values defined in
>  .In fcntl.h :
> -.Bl -tag -width O_NOCTTY -offset indent
> +.Bl -tag -width O_CLOEXEC -offset indent
>  .It Dv O_RDWR
>  Open for reading and writing.
>  .It Dv O_NOCTTY
> @@ -53,6 +53,8 @@ Prevent the device from being made the c
>  This flag has no effect on
>  .Ox
>  and is included for compatibility with other systems.
> +.It Dv O_CLOEXEC
> +Set the close-on-exec flag for the new file descriptor.
>  .El
>  .Pp
>  The
> @@ -95,6 +97,10 @@ The
>  .Fn posix_openpt
>  function conforms to
>  .St -p1003.1-2001 .
> +.Pp
> +The ability to use
> +.Dv O_CLOEXEC
> +is an extension to that standard.
>  .Sh HISTORY
>  The
>  .Fn posix_openpt
> Index: stdlib/posix_pty.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/posix_pty.c,v
> retrieving revision 1.3
> diff -u -p -u -p -r1.3 posix_pty.c
> --- stdlib/posix_pty.c 25 Jan 2019 00:19:25 -0000 1.3
> +++ stdlib/posix_pty.c 6 Feb 2020 17:34:15 -0000
> @@ -36,7 +36,7 @@ posix_openpt(int oflag)
>  
>   /* User must specify O_RDWR in oflag. */
>   if ((oflag & O_ACCMODE) != O_RDWR ||
> -    (oflag & ~(O_ACCMODE | O_NOCTTY)) != 0) {
> +    (oflag & ~(O_ACCMODE | O_NOCTTY | O_CLOEXEC)) != 0) {
>   errno = EINVAL;
>   return -1;
>   }
> @@ -46,7 +46,11 @@ posix_openpt(int oflag)
>   if (fd != -1) {
>   if (ioctl(fd, PTMGET, &ptm) != -1) {
>   close(ptm.sfd);
> - mfd = ptm.cfd;
> + if ((oflag & O_CLOEXEC) &&
> +    fcntl(ptm.cfd, F_SETFD, FD_CLOEXEC) == -1)
> + close(ptm.cfd);
> + else
> + mfd = ptm.cfd;
>   }
>   close(fd);

Only on the child?  The master does not get it?

I suggest an earlier return from fcntl failure.

As it is now, this can return an errno from close(fd), rather than from
the fcntl, hiding the actual facts.  Not that either will error out, but
the fallthrough seems odd.

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

Theo de Raadt-2
In reply to this post by joshua stein-3
Related to previous,

+.It Dv O_CLOEXEC
+Set the close-on-exec flag for the new file descriptor.

Should this say

Set the close-on-exec flag on the
.Va aslave
file descriptor.

Or, what is supposed to happen to the master?  Is that specified
or ignored?

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

joshua stein-3
In reply to this post by Theo de Raadt-2
On Thu, 06 Feb 2020 at 12:41:47 -0700, Theo de Raadt wrote:

> > Index: stdlib/posix_pty.c
> > ===================================================================
> > RCS file: /cvs/src/lib/libc/stdlib/posix_pty.c,v
> > retrieving revision 1.3
> > diff -u -p -u -p -r1.3 posix_pty.c
> > --- stdlib/posix_pty.c 25 Jan 2019 00:19:25 -0000 1.3
> > +++ stdlib/posix_pty.c 6 Feb 2020 17:34:15 -0000
> > @@ -36,7 +36,7 @@ posix_openpt(int oflag)
> >  
> >   /* User must specify O_RDWR in oflag. */
> >   if ((oflag & O_ACCMODE) != O_RDWR ||
> > -    (oflag & ~(O_ACCMODE | O_NOCTTY)) != 0) {
> > +    (oflag & ~(O_ACCMODE | O_NOCTTY | O_CLOEXEC)) != 0) {
> >   errno = EINVAL;
> >   return -1;
> >   }
> > @@ -46,7 +46,11 @@ posix_openpt(int oflag)
> >   if (fd != -1) {
> >   if (ioctl(fd, PTMGET, &ptm) != -1) {
> >   close(ptm.sfd);
> > - mfd = ptm.cfd;
> > + if ((oflag & O_CLOEXEC) &&
> > +    fcntl(ptm.cfd, F_SETFD, FD_CLOEXEC) == -1)
> > + close(ptm.cfd);
> > + else
> > + mfd = ptm.cfd;
> >   }
> >   close(fd);
>
> Only on the child?  The master does not get it?

cfd is the master (controlling), sfd is the child/slave and is
closed already.

Reply | Threaded
Open this post in threaded view
|

Re: posix_openpt: allow O_CLOEXEC

Philip Guenther-2
In reply to this post by joshua stein-3
On Thu, Feb 6, 2020 at 11:38 AM joshua stein <[hidden email]> wrote:

> On Thu, 06 Feb 2020 at 11:21:11 -0700, Todd C. Miller wrote:
> > On Thu, 06 Feb 2020 10:45:44 -0700, "Theo de Raadt" wrote:
> >
> > > That feels better, and will be more atomic.
> >
> > Unfortunately, we can't do this in ptmioctl() since we don't have
> > the index of the open ptm device to use to check for the presence
> > of UF_EXCLOSE.  So it has to be done in libc instead.
>
> Alright, so like this then?
>

I'm sorry, but this is only superficially correct: the otherwise
unobtainable value of O_CLOEXEC (and SOCK_CLOEXEC) is that it's atomic with
the creation of the fd: there is no window where another thread calling
execve() or fork() will catch the fd without the flag set.  Setting the
flag in the libc code doesn't provide that guarantee.

I see two ways to do this in a way that provides the guarantee without
breaking the existing ioctl(PTMGET) ABI:

1) rename PTMGET to PTMGET_O66; add PTMGET which *always* sets FD_CLOEXEC
on both fds and change libc to instead _clear_ FD_CLOEXEC when O_CLOEXEC is
not passed.  Downside: extra syscalls in the common case

2) rename PTMGET to PTMGET_O66 and struct ptmget to struct ptmget_o66;
define a new struct ptmget which includes an "int flags" member which can
be used to pass O_* flags that should be applied by the kernel to both
control and slave fd before returning; change libc to set that from its
flags argument.

2b) bonus: do (2) but also provide a flag which tells the kernel to *not
open* the slave, and use that in posix_openpt(), fixing a subtle bug in our
posix_openpt()'s behavior

I guess I meant three ways.


Side note: posix_openpt() should, like openpty(), unconditionally pass
O_CLOEXEC when opening /dev/ptm.


Philip Guenther