[PATCH 1/2] openpty: close slave pty fd on error

[PATCH 1/2] openpty: close slave pty fd on error

[Christian Brauner]

When openpty() failed only the master fd was closed so far. Let's close the
slave fd as well.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 ChangeLog       | 4 ++++
 login/openpty.c | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index bc1cf94dc3..bc5fb8e27f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
+
+	* login/openpty.c (openpty): Close slave pty file descriptor on error.
+
 2017-08-25  H.J. Lu  <hongjiu.lu@intel.com>
 
 	* sysdeps/x86/cpu-features.h [__ASSEMBLER__]
diff --git a/login/openpty.c b/login/openpty.c
index 41ab0483e2..8fbc66a3ef 100644
--- a/login/openpty.c
+++ b/login/openpty.c
@@ -92,7 +92,7 @@ openpty (int *amaster, int *aslave, char *name,
   char _buf[512];
 #endif
   char *buf = _buf;
-  int master, slave;
+  int master, slave = -1;
 
   master = getpt ();
   if (master == -1)
@@ -135,6 +135,8 @@ openpty (int *amaster, int *aslave, char *name,
 
  fail:
   close (master);
+  if (slave != -1)
+    close(slave);
   return -1;
 }
 libutil_hidden_def (openpty)
-- 
2.14.1

[PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd

[Christian Brauner]

Newer kernels expose the ioctl TIOCGPTPEER [1] call to userspace which allows to
safely allocate a file descriptor for a pty slave based solely on the master
file descriptor. This allows us to avoid path-based operations and makes this
function a lot safer in the face of devpts mounts in different mount namespaces.

[1]: https://patchwork.kernel.org/patch/9760743/

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 ChangeLog       |  5 +++++
 login/openpty.c | 12 +++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index bc5fb8e27f..30829e4c16 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
+
+	* login/openpty.c (openpty): If defined, use the TIOCGPTPEER ioctl call
+	to allocate the slave pty file descriptor.
+
 2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
 
 	* login/openpty.c (openpty): Close slave pty file descriptor on error.
diff --git a/login/openpty.c b/login/openpty.c
index 8fbc66a3ef..293cc0a0db 100644
--- a/login/openpty.c
+++ b/login/openpty.c
@@ -104,10 +104,14 @@ openpty (int *amaster, int *aslave, char *name,
   if (unlockpt (master))
     goto fail;
 
+#ifdef TIOCGPTPEER
+  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
+#else
   if (pts_name (master, &buf, sizeof (_buf)))
     goto fail;
 
   slave = open (buf, O_RDWR | O_NOCTTY);
+#endif
   if (slave == -1)
     {
       if (buf != _buf)
@@ -127,7 +131,13 @@ openpty (int *amaster, int *aslave, char *name,
   *amaster = master;
   *aslave = slave;
   if (name != NULL)
-    strcpy (name, buf);
+    {
+#ifdef TIOCGPTPEER
+      if (pts_name (master, &buf, sizeof (_buf)))
+        goto fail;
+#endif
+      strcpy (name, buf);
+    }
 
   if (buf != _buf)
     free (buf);
-- 
2.14.1

Re: [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd

[Florian Weimer]

On 08/26/2017 03:44 PM, Christian Brauner wrote:
> +#ifdef TIOCGPTPEER
> +  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
> +#else
>    if (pts_name (master, &buf, sizeof (_buf)))
>      goto fail;
>  
>    slave = open (buf, O_RDWR | O_NOCTTY);
> +#endif

I don't think you can #ifdef out existing code this way without
introducing failures on older kernels.  You need to try the ioctl first,
and if that fails, use the old pts_name code.

Thanks,
Florian

Re: [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd

[Christian Brauner]

On Mon, Aug 28, 2017 at 09:34:11AM +0200, Florian Weimer wrote:
> On 08/26/2017 03:44 PM, Christian Brauner wrote:
> > +#ifdef TIOCGPTPEER
> > +  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
> > +#else
> >    if (pts_name (master, &buf, sizeof (_buf)))
> >      goto fail;
> >  
> >    slave = open (buf, O_RDWR | O_NOCTTY);
> > +#endif
> 
> I don't think you can #ifdef out existing code this way without
> introducing failures on older kernels.  You need to try the ioctl first,
> and if that fails, use the old pts_name code.

Cool. Will resend the [PATCH 2/2] soon. I take it that [PATH 1/1] holds up as it
stands.

Thanks!
Christian

> 
> Thanks,
> Florian

Re: [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd

[Joseph Myers]

On Mon, 28 Aug 2017, Florian Weimer wrote:

> On 08/26/2017 03:44 PM, Christian Brauner wrote:
> > +#ifdef TIOCGPTPEER
> > +  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
> > +#else
> >    if (pts_name (master, &buf, sizeof (_buf)))
> >      goto fail;
> >  
> >    slave = open (buf, O_RDWR | O_NOCTTY);
> > +#endif
> 
> I don't think you can #ifdef out existing code this way without
> introducing failures on older kernels.  You need to try the ioctl first,
> and if that fails, use the old pts_name code.

And in principle there should be appropriate __ASSUME_* conditionals so 
that when building with a new-enough --enable-kernel the old code can be 
conditioned out (however, because this code is not Linux-specific, the old 
case would need to stay in the code even when __ASSUME_TIOCGPTPEER is 
defined unconditionally in the Linux kernel-features.h).

-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH 2/2 v2] openpty: use TIOCGPTPEER to open slave side fd

[Christian Brauner]

Newer kernels expose the ioctl TIOCGPTPEER [1] call to userspace which allows to
safely allocate a file descriptor for a pty slave based solely on the master
file descriptor. This allows us to avoid path-based operations and makes this
function a lot safer in the face of devpts mounts in different mount namespaces.

[1]: https://patchwork.kernel.org/patch/9760743/

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
Changelog 2017-08-28:
* Instead of #ifdefing the TIOCGPTPEER ioctl flag we now try the ioctl() first
  and if it fails we fallback to path-based allocation of the slave fd. This
  allows us retain backward compatibility with kernels that do not support this
  ioctl call.
* A note on the following codepath

   if (name != NULL)
     {
       if (*buf == '\0')
         if (pts_name (master, &buf, sizeof (_buf)))
           goto fail;

       strcpy (name, buf);
     }

  "buf" is guaranteed to be allocated in this case. If the pts_name() call above
  failed we would have never reached this code path. If it has been called
  succesfully it will either have handed us a valid buffer or "buf" will still
  point to the static char array "_buf" which is initialized to 0.
---
 ChangeLog       |  5 +++++
 login/openpty.c | 36 +++++++++++++++++++++++++-----------
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index bc5fb8e27f..30829e4c16 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
+
+	* login/openpty.c (openpty): If defined, use the TIOCGPTPEER ioctl call
+	to allocate the slave pty file descriptor.
+
 2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
 
 	* login/openpty.c (openpty): Close slave pty file descriptor on error.
diff --git a/login/openpty.c b/login/openpty.c
index 8fbc66a3ef..43b34d7a9c 100644
--- a/login/openpty.c
+++ b/login/openpty.c
@@ -87,9 +87,9 @@ openpty (int *amaster, int *aslave, char *name,
 	 const struct termios *termp, const struct winsize *winp)
 {
 #ifdef PATH_MAX
-  char _buf[PATH_MAX];
+  char _buf[PATH_MAX] = {0};
 #else
-  char _buf[512];
+  char _buf[512] = {0};
 #endif
   char *buf = _buf;
   int master, slave = -1;
@@ -104,16 +104,24 @@ openpty (int *amaster, int *aslave, char *name,
   if (unlockpt (master))
     goto fail;
 
-  if (pts_name (master, &buf, sizeof (_buf)))
-    goto fail;
-
-  slave = open (buf, O_RDWR | O_NOCTTY);
+  /* Try to allocate slave fd solely based on master fd first. */
+  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
   if (slave == -1)
     {
-      if (buf != _buf)
-	free (buf);
-
-      goto fail;
+      /* Fallback to path-based slave fd allocation in case kernel doesn't
+       * support TIOCGPTPEER.
+       */
+      if (pts_name (master, &buf, sizeof (_buf)))
+        goto fail;
+
+      slave = open (buf, O_RDWR | O_NOCTTY);
+      if (slave == -1)
+        {
+          if (buf != _buf)
+	    free (buf);
+
+          goto fail;
+        }
     }
 
   /* XXX Should we ignore errors here?  */
@@ -127,7 +135,13 @@ openpty (int *amaster, int *aslave, char *name,
   *amaster = master;
   *aslave = slave;
   if (name != NULL)
-    strcpy (name, buf);
+    {
+      if (*buf == '\0')
+        if (pts_name (master, &buf, sizeof (_buf)))
+          goto fail;
+
+      strcpy (name, buf);
+    }
 
   if (buf != _buf)
     free (buf);
-- 
2.14.1

Re: [PATCH 2/2 v2] openpty: use TIOCGPTPEER to open slave side fd

[Joseph Myers]

On Mon, 28 Aug 2017, Christian Brauner wrote:

> +  /* Try to allocate slave fd solely based on master fd first. */
> +  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);

You still need the #ifdef.  openpty is used on non-Linux systems, and the 
oldest kernel headers supported for building glibc are 3.2.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 2/2 v2] openpty: use TIOCGPTPEER to open slave side fd

[Andreas Schwab]

On Aug 28 2017, Christian Brauner <christian.brauner@ubuntu.com> wrote:

> diff --git a/login/openpty.c b/login/openpty.c
> index 8fbc66a3ef..43b34d7a9c 100644
> --- a/login/openpty.c
> +++ b/login/openpty.c
> @@ -87,9 +87,9 @@ openpty (int *amaster, int *aslave, char *name,
>  	 const struct termios *termp, const struct winsize *winp)
>  {
>  #ifdef PATH_MAX
> -  char _buf[PATH_MAX];
> +  char _buf[PATH_MAX] = {0};
>  #else
> -  char _buf[512];
> +  char _buf[512] = {0};
>  #endif

No need to clear the whole array, just the first byte.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH 2/2 v3] openpty: use TIOCGPTPEER to open slave side fd

[Christian Brauner]

Newer kernels expose the ioctl TIOCGPTPEER [1] call to userspace which allows to
safely allocate a file descriptor for a pty slave based solely on the master
file descriptor. This allows us to avoid path-based operations and makes this
function a lot safer in the face of devpts mounts in different mount namespaces.

[1]: https://patchwork.kernel.org/patch/9760743/

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
Changelog 2017-08-28:
* Instead of #ifdefing the TIOCGPTPEER ioctl flag we now try the ioctl() first
  and if it fails we fallback to path-based allocation of the slave fd. This
  allows us retain backward compatibility with kernels that do not support this
  ioctl call.
* A note on the following codepath

   if (name != NULL)
     {
       if (*buf == '\0')
         if (pts_name (master, &buf, sizeof (_buf)))
           goto fail;

       strcpy (name, buf);
     }

  "buf" is guaranteed to be allocated in this case. If the pts_name() call above
  failed we would have never reached this code path. If it has been called
  succesfully it will either have handed us a valid buffer or "buf" will still
  point to the static char array "_buf" which is initialized to 0.
Changelog 2017-08-28:
* Preserve #ifdef for TIOCGPTPEER since it needs to work on non-Linux distros
  too.
* Only intialize first byte of "_buf".
---
 ChangeLog       |  5 +++++
 login/openpty.c | 36 +++++++++++++++++++++++++++---------
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index bc5fb8e27f..30829e4c16 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
+
+	* login/openpty.c (openpty): If defined, use the TIOCGPTPEER ioctl call
+	to allocate the slave pty file descriptor.
+
 2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
 
 	* login/openpty.c (openpty): Close slave pty file descriptor on error.
diff --git a/login/openpty.c b/login/openpty.c
index 8fbc66a3ef..33754e18a7 100644
--- a/login/openpty.c
+++ b/login/openpty.c
@@ -94,6 +94,8 @@ openpty (int *amaster, int *aslave, char *name,
   char *buf = _buf;
   int master, slave = -1;
 
+  *buf = '\0';
+
   master = getpt ();
   if (master == -1)
     return -1;
@@ -104,16 +106,26 @@ openpty (int *amaster, int *aslave, char *name,
   if (unlockpt (master))
     goto fail;
 
-  if (pts_name (master, &buf, sizeof (_buf)))
-    goto fail;
-
-  slave = open (buf, O_RDWR | O_NOCTTY);
+#ifdef TIOCGPTPEER
+  /* Try to allocate slave fd solely based on master fd first. */
+  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
+#endif
   if (slave == -1)
     {
-      if (buf != _buf)
-	free (buf);
-
-      goto fail;
+      /* Fallback to path-based slave fd allocation in case kernel doesn't
+       * support TIOCGPTPEER.
+       */
+      if (pts_name (master, &buf, sizeof (_buf)))
+        goto fail;
+
+      slave = open (buf, O_RDWR | O_NOCTTY);
+      if (slave == -1)
+        {
+          if (buf != _buf)
+	    free (buf);
+
+          goto fail;
+        }
     }
 
   /* XXX Should we ignore errors here?  */
@@ -127,7 +139,13 @@ openpty (int *amaster, int *aslave, char *name,
   *amaster = master;
   *aslave = slave;
   if (name != NULL)
-    strcpy (name, buf);
+    {
+      if (*buf == '\0')
+        if (pts_name (master, &buf, sizeof (_buf)))
+          goto fail;
+
+      strcpy (name, buf);
+    }
 
   if (buf != _buf)
     free (buf);
-- 
2.14.1

Re: [PATCH 1/2] openpty: close slave pty fd on error

[Florian Weimer]

On 08/26/2017 03:44 PM, Christian Brauner wrote:
>   fail:
>    close (master);
> +  if (slave != -1)
> +    close(slave);
>    return -1;

This is inconsistent with how the code frees buf if there is an error:
For buf, the free operation happens before the fail tail.  I think we
should keep this consistent: either free exactly what is needed, or have
a single function exit which checks for initialization and frees what
has been allocated.

Thanks,
Florian

[PATCH 1/2 v4] openpty: close slave pty fd on error

[Christian Brauner]

When openpty() failed only the master fd was closed so far. Let's close the
slave fd as well. Also, let's unify the error handling.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
Changelog 2017-08-29:
* Unify error handling: use a common function exit that frees everything that
  needs freeing. (@Florian)
---
 ChangeLog       |  4 ++++
 login/openpty.c | 28 ++++++++++++++--------------
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index bc1cf94dc3..bc5fb8e27f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
+
+	* login/openpty.c (openpty): Close slave pty fd and unify error handling.
+
 2017-08-25  H.J. Lu  <hongjiu.lu@intel.com>
 
 	* sysdeps/x86/cpu-features.h [__ASSEMBLER__]
diff --git a/login/openpty.c b/login/openpty.c
index 41ab0483e2..a7b1ab5dde 100644
--- a/login/openpty.c
+++ b/login/openpty.c
@@ -92,29 +92,24 @@ openpty (int *amaster, int *aslave, char *name,
   char _buf[512];
 #endif
   char *buf = _buf;
-  int master, slave;
+  int master, ret = -1, slave = -1;
 
   master = getpt ();
   if (master == -1)
     return -1;
 
   if (grantpt (master))
-    goto fail;
+    goto on_error;
 
   if (unlockpt (master))
-    goto fail;
+    goto on_error;
 
   if (pts_name (master, &buf, sizeof (_buf)))
-    goto fail;
+    goto on_error;
 
   slave = open (buf, O_RDWR | O_NOCTTY);
   if (slave == -1)
-    {
-      if (buf != _buf)
-	free (buf);
-
-      goto fail;
-    }
+    goto on_error;
 
   /* XXX Should we ignore errors here?  */
   if (termp)
@@ -129,12 +124,17 @@ openpty (int *amaster, int *aslave, char *name,
   if (name != NULL)
     strcpy (name, buf);
 
+  ret = 0;
+
+ on_error:
+  close (master);
+
+  if (slave != -1)
+    close(slave);
+
   if (buf != _buf)
     free (buf);
-  return 0;
 
- fail:
-  close (master);
-  return -1;
+  return ret;
 }
 libutil_hidden_def (openpty)
-- 
2.14.1

[PATCH 2/2 v4] openpty: use TIOCGPTPEER to open slave side fd

[Christian Brauner]

Newer kernels expose the ioctl TIOCGPTPEER [1] call to userspace which allows to
safely allocate a file descriptor for a pty slave based solely on the master
file descriptor. This allows us to avoid path-based operations and makes this
function a lot safer in the face of devpts mounts in different mount namespaces.

[1]: https://patchwork.kernel.org/patch/9760743/

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
Changelog 2017-08-28:
* Instead of #ifdefing the TIOCGPTPEER ioctl flag we now try the ioctl() first
  and if it fails we fallback to path-based allocation of the slave fd. This
  allows us retain backward compatibility with kernels that do not support this
  ioctl call.
* A note on the following codepath

   if (name != NULL)
     {
       if (*buf == '\0')
         if (pts_name (master, &buf, sizeof (_buf)))
           goto fail;

       strcpy (name, buf);
     }

  "buf" is guaranteed to be allocated in this case. If the pts_name() call above
  failed we would have never reached this code path. If it has been called
  succesfully it will either have handed us a valid buffer or "buf" will still
  point to the static char array "_buf" which is initialized to 0.
Changelog 2017-08-28:
* Preserve #ifdef for TIOCGPTPEER since it needs to work on non-Linux distros
  too.
* Only intialize first byte of "_buf".
Changelog 2017-08-29:
* Adapt to unified error handling as suggested by Florian.
---
 ChangeLog       |  5 +++++
 login/openpty.c | 30 ++++++++++++++++++++++++------
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index bc5fb8e27f..30829e4c16 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
+
+	* login/openpty.c (openpty): If defined, use the TIOCGPTPEER ioctl call
+	to allocate the slave pty file descriptor.
+
 2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
 
 	* login/openpty.c (openpty): Close slave pty file descriptor on error.
diff --git a/login/openpty.c b/login/openpty.c
index a7b1ab5dde..766aa958ff 100644
--- a/login/openpty.c
+++ b/login/openpty.c
@@ -94,6 +94,8 @@ openpty (int *amaster, int *aslave, char *name,
   char *buf = _buf;
   int master, ret = -1, slave = -1;
 
+  *buf = '\0';
+
   master = getpt ();
   if (master == -1)
     return -1;
@@ -104,12 +106,22 @@ openpty (int *amaster, int *aslave, char *name,
   if (unlockpt (master))
     goto on_error;
 
-  if (pts_name (master, &buf, sizeof (_buf)))
-    goto on_error;
-
-  slave = open (buf, O_RDWR | O_NOCTTY);
+#ifdef TIOCGPTPEER
+  /* Try to allocate slave fd solely based on master fd first. */
+  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
+#endif
   if (slave == -1)
-    goto on_error;
+    {
+      /* Fallback to path-based slave fd allocation in case kernel doesn't
+       * support TIOCGPTPEER.
+       */
+      if (pts_name (master, &buf, sizeof (_buf)))
+        goto on_error;
+
+      slave = open (buf, O_RDWR | O_NOCTTY);
+      if (slave == -1)
+        goto on_error;
+    }
 
   /* XXX Should we ignore errors here?  */
   if (termp)
@@ -122,7 +134,13 @@ openpty (int *amaster, int *aslave, char *name,
   *amaster = master;
   *aslave = slave;
   if (name != NULL)
-    strcpy (name, buf);
+    {
+      if (*buf == '\0')
+        if (pts_name (master, &buf, sizeof (_buf)))
+          goto on_error;
+
+      strcpy (name, buf);
+    }
 
   ret = 0;
 
-- 
2.14.1

Re: [PATCH 1/2 v4] openpty: close slave pty fd on error

[Andreas Schwab]

On Aug 29 2017, Christian Brauner <christian.brauner@ubuntu.com> wrote:

> @@ -129,12 +124,17 @@ openpty (int *amaster, int *aslave, char *name,
>    if (name != NULL)
>      strcpy (name, buf);
>  
> +  ret = 0;
> +
> + on_error:
> +  close (master);
> +
> +  if (slave != -1)
> +    close(slave);
> +

You don't want to close the fds on success.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 1/2 v4] openpty: close slave pty fd on error

[Christian Brauner]

On Tue, Aug 29, 2017 at 04:00:26PM +0200, Andreas Schwab wrote:
> On Aug 29 2017, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> 
> > @@ -129,12 +124,17 @@ openpty (int *amaster, int *aslave, char *name,
> >    if (name != NULL)
> >      strcpy (name, buf);
> >  
> > +  ret = 0;
> > +
> > + on_error:
> > +  close (master);
> > +
> > +  if (slave != -1)
> > +    close(slave);
> > +
> 
> You don't want to close the fds on success.

Sorry, I was inatentive.

> 
> Andreas.
> 
> -- 
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

[PATCH 1/2 v5] openpty: close slave pty fd on error

[Christian Brauner]

When openpty() failed only the master fd was closed so far. Let's close the
slave fd as well. Also, let's unify the error handling.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
Changelog 2017-08-29:
* Unify error handling: use a common function exit that frees everything that
  needs freeing. (@Florian)
Changelog 2017-08-29:
* Do not be stupid and only close the file descriptors on error! Duh. (Thanks,
  @Andreas)
---
 ChangeLog       |  4 ++++
 login/openpty.c | 30 ++++++++++++++++--------------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index bc1cf94dc3..bc5fb8e27f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
+
+	* login/openpty.c (openpty): Close slave pty file descriptor on error.
+
 2017-08-25  H.J. Lu  <hongjiu.lu@intel.com>
 
 	* sysdeps/x86/cpu-features.h [__ASSEMBLER__]
diff --git a/login/openpty.c b/login/openpty.c
index 41ab0483e2..9e556c27a5 100644
--- a/login/openpty.c
+++ b/login/openpty.c
@@ -92,29 +92,24 @@ openpty (int *amaster, int *aslave, char *name,
   char _buf[512];
 #endif
   char *buf = _buf;
-  int master, slave;
+  int master, ret = -1, slave = -1;
 
   master = getpt ();
   if (master == -1)
     return -1;
 
   if (grantpt (master))
-    goto fail;
+    goto on_error;
 
   if (unlockpt (master))
-    goto fail;
+    goto on_error;
 
   if (pts_name (master, &buf, sizeof (_buf)))
-    goto fail;
+    goto on_error;
 
   slave = open (buf, O_RDWR | O_NOCTTY);
   if (slave == -1)
-    {
-      if (buf != _buf)
-	free (buf);
-
-      goto fail;
-    }
+    goto on_error;
 
   /* XXX Should we ignore errors here?  */
   if (termp)
@@ -129,12 +124,19 @@ openpty (int *amaster, int *aslave, char *name,
   if (name != NULL)
     strcpy (name, buf);
 
+  ret = 0;
+
+ on_error:
+  if (ret == -1) {
+    close (master);
+
+    if (slave != -1)
+      close (slave);
+  }
+
   if (buf != _buf)
     free (buf);
-  return 0;
 
- fail:
-  close (master);
-  return -1;
+  return ret;
 }
 libutil_hidden_def (openpty)
-- 
2.14.1

[PATCH 2/2 v5] openpty: use TIOCGPTPEER to open slave side fd

[Christian Brauner]

Newer kernels expose the ioctl TIOCGPTPEER [1] call to userspace which allows to
safely allocate a file descriptor for a pty slave based solely on the master
file descriptor. This allows us to avoid path-based operations and makes this
function a lot safer in the face of devpts mounts in different mount namespaces.

[1]: https://patchwork.kernel.org/patch/9760743/

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
Changelog 2017-08-28:
* Instead of #ifdefing the TIOCGPTPEER ioctl flag we now try the ioctl() first
  and if it fails we fallback to path-based allocation of the slave fd. This
  allows us retain backward compatibility with kernels that do not support this
  ioctl call.
* A note on the following codepath

   if (name != NULL)
     {
       if (*buf == '\0')
         if (pts_name (master, &buf, sizeof (_buf)))
           goto fail;

       strcpy (name, buf);
     }

  "buf" is guaranteed to be allocated in this case. If the pts_name() call above
  failed we would have never reached this code path. If it has been called
  succesfully it will either have handed us a valid buffer or "buf" will still
  point to the static char array "_buf" which is initialized to 0.
Changelog 2017-08-28:
* Preserve #ifdef for TIOCGPTPEER since it needs to work on non-Linux distros
  too.
* Only intialize first byte of "_buf".
Changelog 2017-08-29:
* Adapt to unified error handling as suggested by Florian.
---
 ChangeLog       |  5 +++++
 login/openpty.c | 30 ++++++++++++++++++++++++------
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index bc5fb8e27f..30829e4c16 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
+
+	* login/openpty.c (openpty): If defined, use the TIOCGPTPEER ioctl call
+	to allocate the slave pty file descriptor.
+
 2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
 
 	* login/openpty.c (openpty): Close slave pty file descriptor on error.
diff --git a/login/openpty.c b/login/openpty.c
index 9e556c27a5..6703128ea8 100644
--- a/login/openpty.c
+++ b/login/openpty.c
@@ -94,6 +94,8 @@ openpty (int *amaster, int *aslave, char *name,
   char *buf = _buf;
   int master, ret = -1, slave = -1;
 
+  *buf = '\0';
+
   master = getpt ();
   if (master == -1)
     return -1;
@@ -104,12 +106,22 @@ openpty (int *amaster, int *aslave, char *name,
   if (unlockpt (master))
     goto on_error;
 
-  if (pts_name (master, &buf, sizeof (_buf)))
-    goto on_error;
-
-  slave = open (buf, O_RDWR | O_NOCTTY);
+#ifdef TIOCGPTPEER
+  /* Try to allocate slave fd solely based on master fd first. */
+  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
+#endif
   if (slave == -1)
-    goto on_error;
+    {
+      /* Fallback to path-based slave fd allocation in case kernel doesn't
+       * support TIOCGPTPEER.
+       */
+      if (pts_name (master, &buf, sizeof (_buf)))
+        goto on_error;
+
+      slave = open (buf, O_RDWR | O_NOCTTY);
+      if (slave == -1)
+        goto on_error;
+    }
 
   /* XXX Should we ignore errors here?  */
   if (termp)
@@ -122,7 +134,13 @@ openpty (int *amaster, int *aslave, char *name,
   *amaster = master;
   *aslave = slave;
   if (name != NULL)
-    strcpy (name, buf);
+    {
+      if (*buf == '\0')
+        if (pts_name (master, &buf, sizeof (_buf)))
+          goto on_error;
+
+      strcpy (name, buf);
+    }
 
   ret = 0;
 
-- 
2.14.1

Re: [PATCH 1/2 v5] openpty: close slave pty fd on error

[Christian Brauner]

Hi guys,

Any update on whether this is acceptable for inclusion or not now. Linux 4.13
has been released which measn TIOCGPTPEER is now stable API.

Christian

On Tue, Aug 29, 2017 at 04:30:36PM +0200, Christian Brauner wrote:
> When openpty() failed only the master fd was closed so far. Let's close the
> slave fd as well. Also, let's unify the error handling.
> 
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> Changelog 2017-08-29:
> * Unify error handling: use a common function exit that frees everything that
>   needs freeing. (@Florian)
> Changelog 2017-08-29:
> * Do not be stupid and only close the file descriptors on error! Duh. (Thanks,
>   @Andreas)
> ---
>  ChangeLog       |  4 ++++
>  login/openpty.c | 30 ++++++++++++++++--------------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index bc1cf94dc3..bc5fb8e27f 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
> +
> +	* login/openpty.c (openpty): Close slave pty file descriptor on error.
> +
>  2017-08-25  H.J. Lu  <hongjiu.lu@intel.com>
>  
>  	* sysdeps/x86/cpu-features.h [__ASSEMBLER__]
> diff --git a/login/openpty.c b/login/openpty.c
> index 41ab0483e2..9e556c27a5 100644
> --- a/login/openpty.c
> +++ b/login/openpty.c
> @@ -92,29 +92,24 @@ openpty (int *amaster, int *aslave, char *name,
>    char _buf[512];
>  #endif
>    char *buf = _buf;
> -  int master, slave;
> +  int master, ret = -1, slave = -1;
>  
>    master = getpt ();
>    if (master == -1)
>      return -1;
>  
>    if (grantpt (master))
> -    goto fail;
> +    goto on_error;
>  
>    if (unlockpt (master))
> -    goto fail;
> +    goto on_error;
>  
>    if (pts_name (master, &buf, sizeof (_buf)))
> -    goto fail;
> +    goto on_error;
>  
>    slave = open (buf, O_RDWR | O_NOCTTY);
>    if (slave == -1)
> -    {
> -      if (buf != _buf)
> -	free (buf);
> -
> -      goto fail;
> -    }
> +    goto on_error;
>  
>    /* XXX Should we ignore errors here?  */
>    if (termp)
> @@ -129,12 +124,19 @@ openpty (int *amaster, int *aslave, char *name,
>    if (name != NULL)
>      strcpy (name, buf);
>  
> +  ret = 0;
> +
> + on_error:
> +  if (ret == -1) {
> +    close (master);
> +
> +    if (slave != -1)
> +      close (slave);
> +  }
> +
>    if (buf != _buf)
>      free (buf);
> -  return 0;
>  
> - fail:
> -  close (master);
> -  return -1;
> +  return ret;
>  }
>  libutil_hidden_def (openpty)
> -- 
> 2.14.1
> 

On Tue, Aug 29, 2017 at 04:30:37PM +0200, Christian Brauner wrote:
> Newer kernels expose the ioctl TIOCGPTPEER [1] call to userspace which allows to
> safely allocate a file descriptor for a pty slave based solely on the master
> file descriptor. This allows us to avoid path-based operations and makes this
> function a lot safer in the face of devpts mounts in different mount namespaces.
> 
> [1]: https://patchwork.kernel.org/patch/9760743/
> 
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> Changelog 2017-08-28:
> * Instead of #ifdefing the TIOCGPTPEER ioctl flag we now try the ioctl() first
>   and if it fails we fallback to path-based allocation of the slave fd. This
>   allows us retain backward compatibility with kernels that do not support this
>   ioctl call.
> * A note on the following codepath
> 
>    if (name != NULL)
>      {
>        if (*buf == '\0')
>          if (pts_name (master, &buf, sizeof (_buf)))
>            goto fail;
> 
>        strcpy (name, buf);
>      }
> 
>   "buf" is guaranteed to be allocated in this case. If the pts_name() call above
>   failed we would have never reached this code path. If it has been called
>   succesfully it will either have handed us a valid buffer or "buf" will still
>   point to the static char array "_buf" which is initialized to 0.
> Changelog 2017-08-28:
> * Preserve #ifdef for TIOCGPTPEER since it needs to work on non-Linux distros
>   too.
> * Only intialize first byte of "_buf".
> Changelog 2017-08-29:
> * Adapt to unified error handling as suggested by Florian.
> ---
>  ChangeLog       |  5 +++++
>  login/openpty.c | 30 ++++++++++++++++++++++++------
>  2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index bc5fb8e27f..30829e4c16 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
> +
> +	* login/openpty.c (openpty): If defined, use the TIOCGPTPEER ioctl call
> +	to allocate the slave pty file descriptor.
> +
>  2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
>  
>  	* login/openpty.c (openpty): Close slave pty file descriptor on error.
> diff --git a/login/openpty.c b/login/openpty.c
> index 9e556c27a5..6703128ea8 100644
> --- a/login/openpty.c
> +++ b/login/openpty.c
> @@ -94,6 +94,8 @@ openpty (int *amaster, int *aslave, char *name,
>    char *buf = _buf;
>    int master, ret = -1, slave = -1;
>  
> +  *buf = '\0';
> +
>    master = getpt ();
>    if (master == -1)
>      return -1;
> @@ -104,12 +106,22 @@ openpty (int *amaster, int *aslave, char *name,
>    if (unlockpt (master))
>      goto on_error;
>  
> -  if (pts_name (master, &buf, sizeof (_buf)))
> -    goto on_error;
> -
> -  slave = open (buf, O_RDWR | O_NOCTTY);
> +#ifdef TIOCGPTPEER
> +  /* Try to allocate slave fd solely based on master fd first. */
> +  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
> +#endif
>    if (slave == -1)
> -    goto on_error;
> +    {
> +      /* Fallback to path-based slave fd allocation in case kernel doesn't
> +       * support TIOCGPTPEER.
> +       */
> +      if (pts_name (master, &buf, sizeof (_buf)))
> +        goto on_error;
> +
> +      slave = open (buf, O_RDWR | O_NOCTTY);
> +      if (slave == -1)
> +        goto on_error;
> +    }
>  
>    /* XXX Should we ignore errors here?  */
>    if (termp)
> @@ -122,7 +134,13 @@ openpty (int *amaster, int *aslave, char *name,
>    *amaster = master;
>    *aslave = slave;
>    if (name != NULL)
> -    strcpy (name, buf);
> +    {
> +      if (*buf == '\0')
> +        if (pts_name (master, &buf, sizeof (_buf)))
> +          goto on_error;
> +
> +      strcpy (name, buf);
> +    }
>  
>    ret = 0;
>  
> -- 
> 2.14.1
> 

Re: [PATCH 1/2 v5] openpty: close slave pty fd on error

[Christian Brauner]

On Sun, Sep 10, 2017 at 07:45:27PM +0200, Christian Brauner wrote:
> Hi guys,
> 
> Any update on whether this is acceptable for inclusion or not now. Linux 4.13
> has been released which measn TIOCGPTPEER is now stable API.
> 
> Christian

Hi everyone,

Friendly ping about this patch again. I'm not sure where we left of. Last time
it seemed you were fine with the patch. Any change that you'll merge this soon?

Thanks!
Christian

> 
> On Tue, Aug 29, 2017 at 04:30:36PM +0200, Christian Brauner wrote:
> > When openpty() failed only the master fd was closed so far. Let's close the
> > slave fd as well. Also, let's unify the error handling.
> > 
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > Changelog 2017-08-29:
> > * Unify error handling: use a common function exit that frees everything that
> >   needs freeing. (@Florian)
> > Changelog 2017-08-29:
> > * Do not be stupid and only close the file descriptors on error! Duh. (Thanks,
> >   @Andreas)
> > ---
> >  ChangeLog       |  4 ++++
> >  login/openpty.c | 30 ++++++++++++++++--------------
> >  2 files changed, 20 insertions(+), 14 deletions(-)
> > 
> > diff --git a/ChangeLog b/ChangeLog
> > index bc1cf94dc3..bc5fb8e27f 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,7 @@
> > +2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
> > +
> > +	* login/openpty.c (openpty): Close slave pty file descriptor on error.
> > +
> >  2017-08-25  H.J. Lu  <hongjiu.lu@intel.com>
> >  
> >  	* sysdeps/x86/cpu-features.h [__ASSEMBLER__]
> > diff --git a/login/openpty.c b/login/openpty.c
> > index 41ab0483e2..9e556c27a5 100644
> > --- a/login/openpty.c
> > +++ b/login/openpty.c
> > @@ -92,29 +92,24 @@ openpty (int *amaster, int *aslave, char *name,
> >    char _buf[512];
> >  #endif
> >    char *buf = _buf;
> > -  int master, slave;
> > +  int master, ret = -1, slave = -1;
> >  
> >    master = getpt ();
> >    if (master == -1)
> >      return -1;
> >  
> >    if (grantpt (master))
> > -    goto fail;
> > +    goto on_error;
> >  
> >    if (unlockpt (master))
> > -    goto fail;
> > +    goto on_error;
> >  
> >    if (pts_name (master, &buf, sizeof (_buf)))
> > -    goto fail;
> > +    goto on_error;
> >  
> >    slave = open (buf, O_RDWR | O_NOCTTY);
> >    if (slave == -1)
> > -    {
> > -      if (buf != _buf)
> > -	free (buf);
> > -
> > -      goto fail;
> > -    }
> > +    goto on_error;
> >  
> >    /* XXX Should we ignore errors here?  */
> >    if (termp)
> > @@ -129,12 +124,19 @@ openpty (int *amaster, int *aslave, char *name,
> >    if (name != NULL)
> >      strcpy (name, buf);
> >  
> > +  ret = 0;
> > +
> > + on_error:
> > +  if (ret == -1) {
> > +    close (master);
> > +
> > +    if (slave != -1)
> > +      close (slave);
> > +  }
> > +
> >    if (buf != _buf)
> >      free (buf);
> > -  return 0;
> >  
> > - fail:
> > -  close (master);
> > -  return -1;
> > +  return ret;
> >  }
> >  libutil_hidden_def (openpty)
> > -- 
> > 2.14.1
> > 
> 
> On Tue, Aug 29, 2017 at 04:30:37PM +0200, Christian Brauner wrote:
> > Newer kernels expose the ioctl TIOCGPTPEER [1] call to userspace which allows to
> > safely allocate a file descriptor for a pty slave based solely on the master
> > file descriptor. This allows us to avoid path-based operations and makes this
> > function a lot safer in the face of devpts mounts in different mount namespaces.
> > 
> > [1]: https://patchwork.kernel.org/patch/9760743/
> > 
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > Changelog 2017-08-28:
> > * Instead of #ifdefing the TIOCGPTPEER ioctl flag we now try the ioctl() first
> >   and if it fails we fallback to path-based allocation of the slave fd. This
> >   allows us retain backward compatibility with kernels that do not support this
> >   ioctl call.
> > * A note on the following codepath
> > 
> >    if (name != NULL)
> >      {
> >        if (*buf == '\0')
> >          if (pts_name (master, &buf, sizeof (_buf)))
> >            goto fail;
> > 
> >        strcpy (name, buf);
> >      }
> > 
> >   "buf" is guaranteed to be allocated in this case. If the pts_name() call above
> >   failed we would have never reached this code path. If it has been called
> >   succesfully it will either have handed us a valid buffer or "buf" will still
> >   point to the static char array "_buf" which is initialized to 0.
> > Changelog 2017-08-28:
> > * Preserve #ifdef for TIOCGPTPEER since it needs to work on non-Linux distros
> >   too.
> > * Only intialize first byte of "_buf".
> > Changelog 2017-08-29:
> > * Adapt to unified error handling as suggested by Florian.
> > ---
> >  ChangeLog       |  5 +++++
> >  login/openpty.c | 30 ++++++++++++++++++++++++------
> >  2 files changed, 29 insertions(+), 6 deletions(-)
> > 
> > diff --git a/ChangeLog b/ChangeLog
> > index bc5fb8e27f..30829e4c16 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
> > +
> > +	* login/openpty.c (openpty): If defined, use the TIOCGPTPEER ioctl call
> > +	to allocate the slave pty file descriptor.
> > +
> >  2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
> >  
> >  	* login/openpty.c (openpty): Close slave pty file descriptor on error.
> > diff --git a/login/openpty.c b/login/openpty.c
> > index 9e556c27a5..6703128ea8 100644
> > --- a/login/openpty.c
> > +++ b/login/openpty.c
> > @@ -94,6 +94,8 @@ openpty (int *amaster, int *aslave, char *name,
> >    char *buf = _buf;
> >    int master, ret = -1, slave = -1;
> >  
> > +  *buf = '\0';
> > +
> >    master = getpt ();
> >    if (master == -1)
> >      return -1;
> > @@ -104,12 +106,22 @@ openpty (int *amaster, int *aslave, char *name,
> >    if (unlockpt (master))
> >      goto on_error;
> >  
> > -  if (pts_name (master, &buf, sizeof (_buf)))
> > -    goto on_error;
> > -
> > -  slave = open (buf, O_RDWR | O_NOCTTY);
> > +#ifdef TIOCGPTPEER
> > +  /* Try to allocate slave fd solely based on master fd first. */
> > +  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
> > +#endif
> >    if (slave == -1)
> > -    goto on_error;
> > +    {
> > +      /* Fallback to path-based slave fd allocation in case kernel doesn't
> > +       * support TIOCGPTPEER.
> > +       */
> > +      if (pts_name (master, &buf, sizeof (_buf)))
> > +        goto on_error;
> > +
> > +      slave = open (buf, O_RDWR | O_NOCTTY);
> > +      if (slave == -1)
> > +        goto on_error;
> > +    }
> >  
> >    /* XXX Should we ignore errors here?  */
> >    if (termp)
> > @@ -122,7 +134,13 @@ openpty (int *amaster, int *aslave, char *name,
> >    *amaster = master;
> >    *aslave = slave;
> >    if (name != NULL)
> > -    strcpy (name, buf);
> > +    {
> > +      if (*buf == '\0')
> > +        if (pts_name (master, &buf, sizeof (_buf)))
> > +          goto on_error;
> > +
> > +      strcpy (name, buf);
> > +    }
> >  
> >    ret = 0;
> >  
> > -- 
> > 2.14.1
> > 
>