Patchwork [5/5] linux ttyname and ttyname_r: Fix namespace check [BZ #22145]

login
register
mail settings
Submitter Christian Brauner
Date Oct. 12, 2017, 11:35 a.m.
Message ID <209276812.8483.1507808145541@office.mailbox.org>
Download mbox | patch
Permalink /patch/23500/
State New
Headers show

Comments

Christian Brauner - Oct. 12, 2017, 11:35 a.m.
On Wed, Oct 11, 2017 at 11:53:21PM -0400, Luke Shumaker wrote:
> In commit 15e9a4f Christian Brauner 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 find it by allowing the normal fall back on
> iterating through devices.
> 
> A common scenario where this happens is with "/dev/console" in
> containers.  Common container managers (including systemd-nspawn) will
> call openpty() on a ptmx device in the host's mount namespace to
> allocate a pty master/slave pair, then send the slave FD to the
> container, and bind-mounted at "/dev/console" in the container's mount
> namespace.  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 original path isn't a
> match, and fails early and gives up, even though if it kept searching
> it would find the TTY at "/dev/console".  This fixes that so that the
> ENODEV path does not force an early return inhibiting the fall-back
> search.

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

---
ChangeLog                           |  5 +++++
sysdeps/unix/sysv/linux/ttyname.c   | 19 ++++++++++++-------
sysdeps/unix/sysv/linux/ttyname_r.c | 20 ++++++++++++--------
3 files changed, 29 insertions(+), 15 deletions(-)
Christian Brauner - Nov. 1, 2017, 3:51 p.m.
Hey Luke,

Just a quick ping about the ttyname{_r}() patch.
Do you have any time-frame on when you'd be able to send an updated version of
the patch and do you have signed an FSF agreement yet? There are only ~2 months
left before the Glibc 2.27 development freeze and I'd like to see this
regression fixed in 2.27. :) Otherwise I wouldn't nag you. :)

Thanks!
Christian

On Thu, Oct 12, 2017 at 01:35:45PM +0200, Christian Brauner wrote:
> On Wed, Oct 11, 2017 at 11:53:21PM -0400, Luke Shumaker wrote:
> > In commit 15e9a4f Christian Brauner 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 find it by allowing the normal fall back on
> > iterating through devices.
> > 
> > A common scenario where this happens is with "/dev/console" in
> > containers.  Common container managers (including systemd-nspawn) will
> > call openpty() on a ptmx device in the host's mount namespace to
> > allocate a pty master/slave pair, then send the slave FD to the
> > container, and bind-mounted at "/dev/console" in the container's mount
> > namespace.  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 original path isn't a
> > match, and fails early and gives up, even though if it kept searching
> > it would find the TTY at "/dev/console".  This fixes that so that the
> > ENODEV path does not force an early return inhibiting the fall-back
> > search.
> 
> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> 
> ---
> ChangeLog                           |  5 +++++
> sysdeps/unix/sysv/linux/ttyname.c   | 19 ++++++++++++-------
> sysdeps/unix/sysv/linux/ttyname_r.c | 20 ++++++++++++--------
> 3 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 5973b9d50b..28f31d345b 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,10 @@
> 2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
> 
> +	[BZ #22145]
> +	* sysdeps/unix/sysv/linux/ttyname.c (ttyname):
> +	Defer is_pty check until end.
> +	* 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: Call it.
> diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
> index 138a8a57f8..ebd916f68e 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 d975d95d0d..adcffacb2c 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.14.2
Luke Shumaker - Nov. 4, 2017, 5 p.m.
On Wed, 01 Nov 2017 11:51:24 -0400,
Christian Brauner wrote:
> Hey Luke,
> 
> Just a quick ping about the ttyname{_r}() patch.
> Do you have any time-frame on when you'd be able to send an updated version of
> the patch and do you have signed an FSF agreement yet? There are only ~2 months
> left before the Glibc 2.27 development freeze and I'd like to see this
> regression fixed in 2.27. :) Otherwise I wouldn't nag you. :)
> 
> Thanks!
> Christian

I'm not sure if you missed it, but I submitted v2 of this patchset on
Thursday (I did CC you on it).  Thanks for the nagging :)

Patch

diff --git a/ChangeLog b/ChangeLog
index 5973b9d50b..28f31d345b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@ 
2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>

+	[BZ #22145]
+	* sysdeps/unix/sysv/linux/ttyname.c (ttyname):
+	Defer is_pty check until end.
+	* 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: Call it.
diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 138a8a57f8..ebd916f68e 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 d975d95d0d..adcffacb2c 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;
}