[v6,5/6] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

Message ID 20171110200827.32265-6-lukeshu@lukeshu.com
State New
Headers show
Series
  • Fixup linux ttyname and ttyname_r [BZ #22145]
Related show

Commit Message

Luke Shumaker Nov. 10, 2017, 8:08 p.m.
From: Luke Shumaker <lukeshu@parabola.nu>

Commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 introduced logic for
ttyname() sending back ENODEV to signal that we can't get a name for the
TTY because we inherited it from a different mount namespace.

However, just because we inherited it from a different mount namespace and
it isn't available at its original path, doesn't mean that its name is
unknowable; we can still try to find it by allowing the normal fall back on
iterating through devices.

An example scenario where this happens is with "/dev/console" in
containers.  It's a common practice among container managers (including
systemd-nspawn) to allocate a PTY master/slave pair in the host's mount
namespace (the slave having a path like "/dev/pty/$X"), bind mount the
slave to "/dev/console" in the container's mount namespace, and send the
slave FD to a process in the container.  Inside of the container, the
slave-end isn't available at its original path ("/dev/pts/$X"), since the
container mount namespace has a separate devpts instance from the host
(that path may or may not exist in the container; if it does exist, it's
not the same PTY slave device).  Currently ttyname{_r} sees that the file
at the original "/dev/pts/$X" path doesn't match the FD passed to it, and
fails early and gives up, even though if it kept searching it would find
the TTY at "/dev/console".  Fix that; don't have the ENODEV path force an
early return inhibiting the fall-back search.

This change is based on the previous patch that adds use of is_mytty in
getttyname and getttyname_r.  Without that change, this effectively reverts
15e9a4f, which made us disregard the false similarity of file pointed to by
"/proc/self/fd/$Y", because if it doesn't bail prematurely then that file
("/dev/pts/$X") will just come up again anyway in the fall-back search.

v2:
 - Rearrange commit order
 - Revise commit message
v3:
 - Revise commit message
 - Revise ChangeLog message
 - Fix style of block comments
---
 ChangeLog                           |  5 +++++
 sysdeps/unix/sysv/linux/ttyname.c   | 19 ++++++++++++-------
 sysdeps/unix/sysv/linux/ttyname_r.c | 20 ++++++++++++--------
 3 files changed, 29 insertions(+), 15 deletions(-)

Comments

Christian Brauner Nov. 11, 2017, 11:27 p.m. | #1
On Fri, Nov 10, 2017 at 03:08:26PM -0500, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> Commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 introduced logic for
> ttyname() sending back ENODEV to signal that we can't get a name for the
> TTY because we inherited it from a different mount namespace.
> 
> However, just because we inherited it from a different mount namespace and
> it isn't available at its original path, doesn't mean that its name is
> unknowable; we can still try to find it by allowing the normal fall back on
> iterating through devices.
> 
> An example scenario where this happens is with "/dev/console" in
> containers.  It's a common practice among container managers (including
> systemd-nspawn) to allocate a PTY master/slave pair in the host's mount
> namespace (the slave having a path like "/dev/pty/$X"), bind mount the
> slave to "/dev/console" in the container's mount namespace, and send the
> slave FD to a process in the container.  Inside of the container, the
> slave-end isn't available at its original path ("/dev/pts/$X"), since the
> container mount namespace has a separate devpts instance from the host
> (that path may or may not exist in the container; if it does exist, it's
> not the same PTY slave device).  Currently ttyname{_r} sees that the file
> at the original "/dev/pts/$X" path doesn't match the FD passed to it, and
> fails early and gives up, even though if it kept searching it would find
> the TTY at "/dev/console".  Fix that; don't have the ENODEV path force an
> early return inhibiting the fall-back search.
> 
> This change is based on the previous patch that adds use of is_mytty in
> getttyname and getttyname_r.  Without that change, this effectively reverts
> 15e9a4f, which made us disregard the false similarity of file pointed to by
> "/proc/self/fd/$Y", because if it doesn't bail prematurely then that file
> ("/dev/pts/$X") will just come up again anyway in the fall-back search.
> 
> v2:
>  - Rearrange commit order
>  - Revise commit message
> v3:
>  - Revise commit message
>  - Revise ChangeLog message
>  - Fix style of block comments
> ---
>  ChangeLog                           |  5 +++++
>  sysdeps/unix/sysv/linux/ttyname.c   | 19 ++++++++++++-------
>  sysdeps/unix/sysv/linux/ttyname_r.c | 20 ++++++++++++--------
>  3 files changed, 29 insertions(+), 15 deletions(-)

Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>

> 
> diff --git a/ChangeLog b/ChangeLog
> index f28c7c1afc..9ea727499c 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,10 @@
>  2017-11-07  Luke Shumaker  <lukeshu@parabola.nu>
>  
> +	[BZ #22145]
> +	* sysdeps/unix/sysv/linux/ttyname.c (ttyname):
> +	Defer is_pty check until end of the function.
> +	* sysdeps/unix/sysv/linux/ttyname_r.c (__ttyname_r): Likewise.
> +
>  	[BZ #22145]
>  	* sysdeps/unix/sysv/linux/ttyname.h (is_mytty): New function.
>  	* sysdeps/unix/sysv/linux/ttyname.c (getttyname): Call is_mytty.
> diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
> index 6e97d2d455..f4c955f25b 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
> @@ -115,6 +115,7 @@ ttyname (int fd)
>    char procname[30];
>    struct stat64 st, st1;
>    int dostat = 0;
> +  int doispty = 0;
>    char *name;
>    int save = errno;
>    struct termios term;
> @@ -165,13 +166,7 @@ ttyname (int fd)
>  	  && is_mytty (&st, &st1))
>  	return ttyname_buf;
>  
> -      /* If the link doesn't exist, then it points to a device in another
> -	 namespace. */
> -      if (is_pty (&st))
> -	{
> -	  __set_errno (ENODEV);
> -	  return NULL;
> -	}
> +      doispty = 1;
>      }
>  
>    if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
> @@ -195,5 +190,15 @@ ttyname (int fd)
>        name = getttyname ("/dev", &st, save, &dostat);
>      }
>  
> +  if (!name && doispty && is_pty (&st))
> +    {
> +      /* We failed to figure out the TTY's name, but we can at least
> +         signal that we did verify that it really is a PTY slave.
> +         This happens when we have inherited the file descriptor from
> +         a different mount namespace.  */
> +      __set_errno (ENODEV);
> +      return NULL;
> +    }
> +
>    return name;
>  }
> diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
> index 58eb919c3f..00eefc2c5c 100644
> --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> @@ -95,6 +95,7 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>    char procname[30];
>    struct stat64 st, st1;
>    int dostat = 0;
> +  int doispty = 0;
>    int save = errno;
>  
>    /* Test for the absolute minimal size.  This makes life easier inside
> @@ -149,14 +150,7 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>  	  && is_mytty (&st, &st1))
>  	return 0;
>  
> -      /* If the link doesn't exist, then it points to a device in another
> -       * namespace.
> -       */
> -      if (is_pty (&st))
> -	{
> -	  __set_errno (ENODEV);
> -	  return ENODEV;
> -	}
> +      doispty = 1;
>      }
>  
>    /* Prepare the result buffer.  */
> @@ -190,6 +184,16 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>  			  save, &dostat);
>      }
>  
> +  if (ret && doispty && is_pty (&st))
> +    {
> +      /* We failed to figure out the TTY's name, but we can at least
> +         signal that we did verify that it really is a PTY slave.
> +         This happens when we have inherited the file descriptor from
> +         a different mount namespace.  */
> +      __set_errno (ENODEV);
> +      return ENODEV;
> +    }
> +
>    return ret;
>  }
>  
> -- 
> 2.15.0
>

Patch

diff --git a/ChangeLog b/ChangeLog
index f28c7c1afc..9ea727499c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@ 
 2017-11-07  Luke Shumaker  <lukeshu@parabola.nu>
 
+	[BZ #22145]
+	* sysdeps/unix/sysv/linux/ttyname.c (ttyname):
+	Defer is_pty check until end of the function.
+	* sysdeps/unix/sysv/linux/ttyname_r.c (__ttyname_r): Likewise.
+
 	[BZ #22145]
 	* sysdeps/unix/sysv/linux/ttyname.h (is_mytty): New function.
 	* sysdeps/unix/sysv/linux/ttyname.c (getttyname): Call is_mytty.
diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 6e97d2d455..f4c955f25b 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -115,6 +115,7 @@  ttyname (int fd)
   char procname[30];
   struct stat64 st, st1;
   int dostat = 0;
+  int doispty = 0;
   char *name;
   int save = errno;
   struct termios term;
@@ -165,13 +166,7 @@  ttyname (int fd)
 	  && is_mytty (&st, &st1))
 	return ttyname_buf;
 
-      /* If the link doesn't exist, then it points to a device in another
-	 namespace. */
-      if (is_pty (&st))
-	{
-	  __set_errno (ENODEV);
-	  return NULL;
-	}
+      doispty = 1;
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
@@ -195,5 +190,15 @@  ttyname (int fd)
       name = getttyname ("/dev", &st, save, &dostat);
     }
 
+  if (!name && doispty && is_pty (&st))
+    {
+      /* We failed to figure out the TTY's name, but we can at least
+         signal that we did verify that it really is a PTY slave.
+         This happens when we have inherited the file descriptor from
+         a different mount namespace.  */
+      __set_errno (ENODEV);
+      return NULL;
+    }
+
   return name;
 }
diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index 58eb919c3f..00eefc2c5c 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -95,6 +95,7 @@  __ttyname_r (int fd, char *buf, size_t buflen)
   char procname[30];
   struct stat64 st, st1;
   int dostat = 0;
+  int doispty = 0;
   int save = errno;
 
   /* Test for the absolute minimal size.  This makes life easier inside
@@ -149,14 +150,7 @@  __ttyname_r (int fd, char *buf, size_t buflen)
 	  && is_mytty (&st, &st1))
 	return 0;
 
-      /* If the link doesn't exist, then it points to a device in another
-       * namespace.
-       */
-      if (is_pty (&st))
-	{
-	  __set_errno (ENODEV);
-	  return ENODEV;
-	}
+      doispty = 1;
     }
 
   /* Prepare the result buffer.  */
@@ -190,6 +184,16 @@  __ttyname_r (int fd, char *buf, size_t buflen)
 			  save, &dostat);
     }
 
+  if (ret && doispty && is_pty (&st))
+    {
+      /* We failed to figure out the TTY's name, but we can at least
+         signal that we did verify that it really is a PTY slave.
+         This happens when we have inherited the file descriptor from
+         a different mount namespace.  */
+      __set_errno (ENODEV);
+      return ENODEV;
+    }
+
   return ret;
 }