[Bug libc/22145] ttyname() gives up too early in the face of namespaces

["christian.brauner at mailbox dot org" <sourceware-bugzilla at sourceware dot org>]

https://sourceware.org/bugzilla/show_bug.cgi?id=22145

--- Comment #13 from Christian Brauner <christian.brauner at mailbox dot org> ---
Comment on attachment 10415
  --> https://sourceware.org/bugzilla/attachment.cgi?id=10415
proposed fix part 1

>From 1d8113b396f145ea8fea27c2586b10cc55526be8 Mon Sep 17 00:00:00 2001
>From: Luke Shumaker <lukeshu@lukeshu.com>
>Date: Sun, 17 Sep 2017 01:55:15 -0400
>Subject: [PATCH 1/2] linux ttyname() and ttyname_r(): Make the TTY equivalence
> tests consistent
>
>In the ttyname() and ttyname_r() routines on Linux, at several points it
>it needs to test if a given TTY is the TTY we are looking for.  It used to
>be that this test was (to see if `maybe` is `mytty`):
>
>       __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
>    #ifdef _STATBUF_ST_RDEV
>       && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
>    #else
>       && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev
>    #endif
>
>Now, this check appears a couple of places.
>
>Then Christian Brauner came along with commit 15e9a4f3, and changed the
>check to
>
>       __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
>    #ifdef _STATBUF_ST_RDEV
>       && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
>    #endif
>       && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev
>
>That is, he made st_ino and st_dev checks happen even if we have the
>st_rdev member.  This is a good change, because kernel namespaces allow you
>to create a new namespace, and to create a new device with the same
>st_rdev, but isn't the same file.
>
>However, this check appears twice in each file (ttyname.c and ttyname_r.c),
>but Christian only changed it in one place in each file.  Now that kind-of
>made sense.  The common use-case for this is that you're in a chroot, and
>have inherited a file descriptor to a TTY outside of the chroot.  In the
>ttyname() routine, the first check is on the passed-in file descriptor,
>while the remaining checks are for files we open by their absolute path; so
>we'd expect them to be in the new namespace.  But that's just the "normal"
>situation, users can do all kinds of funny things, so update the check
>everywhere.
>
>Make it easy to keep consistent by pulling the check in to a static inline
>function.
>---
> sysdeps/unix/sysv/linux/ttyname.c   | 41 +++++++++-------------------------
> sysdeps/unix/sysv/linux/ttyname.h   | 12 ++++++++++
> sysdeps/unix/sysv/linux/ttyname_r.c | 44 +++++++++----------------------------
> 3 files changed, 32 insertions(+), 65 deletions(-)
>
>diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
>index 5909cb765f..ba046205ae 100644
>--- a/sysdeps/unix/sysv/linux/ttyname.c
>+++ b/sysdeps/unix/sysv/linux/ttyname.c
>@@ -35,8 +35,9 @@
> char *__ttyname;
> #endif
> 
>-static char *getttyname (const char *dev, dev_t mydev,
>-			 ino64_t myino, int save, int *dostat)
>+static char *getttyname (const char *dev,
>+			 struct stat64 *mytty,
>+			 int save, int *dostat)
>      internal_function;
> 
> 
>@@ -44,7 +45,7 @@ libc_freeres_ptr (static char *getttyname_name);
> 
> static char *
> internal_function attribute_compat_text_section
>-getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
>+getttyname (const char *dev, struct stat64 *mytty, int save, int *dostat)
> {
>   static size_t namelen;
>   struct stat64 st;
>@@ -65,7 +66,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
>     *((char *) __mempcpy (getttyname_name, dev, devlen - 1)) = '/';
> 
>   while ((d = __readdir64 (dirstream)) != NULL)
>-    if ((d->d_fileno == myino || *dostat)
>+    if ((d->d_fileno == mytty->st_ino || *dostat)
> 	&& strcmp (d->d_name, "stdin")
> 	&& strcmp (d->d_name, "stdout")
> 	&& strcmp (d->d_name, "stderr"))
>@@ -87,12 +88,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
> 	  }
> 	memcpy (&getttyname_name[devlen], d->d_name, dlen);
> 	if (__xstat64 (_STAT_VER, getttyname_name, &st) == 0
>-#ifdef _STATBUF_ST_RDEV
>-	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
>-#else
>-	    && d->d_fileno == myino && st.st_dev == mydev
>-#endif
>-	   )
>+	    && is_mytty (mytty, &st))
> 	  {
> 	    (void) __closedir (dirstream);
> #if 0
>@@ -169,12 +165,7 @@ ttyname (int fd)
>       /* Verify readlink result, fall back on iterating through devices.  */
>       if (ttyname_buf[0] == '/'
> 	  && __xstat64 (_STAT_VER, ttyname_buf, &st1) == 0
>-#ifdef _STATBUF_ST_RDEV
>-	  && S_ISCHR (st1.st_mode)
>-	  && st1.st_rdev == st.st_rdev
>-#endif
>-	  && st1.st_ino == st.st_ino
>-	  && st1.st_dev == st.st_dev)
>+	  && is_mytty (&st, &st1))
> 	return ttyname_buf;
> 
>       /* If the link doesn't exist, then it points to a device in another
>@@ -188,11 +179,7 @@ ttyname (int fd)
> 
>   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
>     {
>-#ifdef _STATBUF_ST_RDEV
>-      name = getttyname ("/dev/pts", st.st_rdev, st.st_ino, save, &dostat);
>-#else
>-      name = getttyname ("/dev/pts", st.st_dev, st.st_ino, save, &dostat);
>-#endif
>+      name = getttyname ("/dev/pts", &st, save, &dostat);
>     }
>   else
>     {
>@@ -202,21 +189,13 @@ ttyname (int fd)
> 
>   if (!name && dostat != -1)
>     {
>-#ifdef _STATBUF_ST_RDEV
>-      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
>-#else
>-      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
>-#endif
>+      name = getttyname ("/dev", &st, save, &dostat);
>     }
> 
>   if (!name && dostat != -1)
>     {
>       dostat = 1;
>-#ifdef _STATBUF_ST_RDEV
>-      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
>-#else
>-      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
>-#endif
>+      name = getttyname ("/dev", &st, save, &dostat);
>     }
> 
>   return name;
>diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
>index 2e415e4e9c..cc911baeb0 100644
>--- a/sysdeps/unix/sysv/linux/ttyname.h
>+++ b/sysdeps/unix/sysv/linux/ttyname.h
>@@ -32,3 +32,15 @@ is_pty (struct stat64 *sb)
>   return false;
> #endif
> }
>+
>+static inline int
>+is_mytty(struct stat64 *mytty, struct stat64 *maybe)
>+{
>+  return 1
>+#ifdef _STATBUF_ST_RDEV
>+    && S_ISCHR (maybe->st_mode)
>+    && maybe->st_rdev == mytty->st_rdev
>+#endif
>+    && maybe->st_ino == mytty->st_ino
>+    && maybe->st_dev == mytty->st_dev;
>+}
>diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
>index dc863526ba..8ff8653a2d 100644
>--- a/sysdeps/unix/sysv/linux/ttyname_r.c
>+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
>@@ -31,12 +31,13 @@
> #include "ttyname.h"
> 
> static int getttyname_r (char *buf, size_t buflen,
>-			 dev_t mydev, ino64_t myino, int save,
>-			 int *dostat) internal_function;
>+			 struct stat64 *mytty,
>+			 int save, int *dostat)
>+     internal_function;
> 
> static int
> internal_function attribute_compat_text_section
>-getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
>+getttyname_r (char *buf, size_t buflen, struct stat64 *mytty,
> 	      int save, int *dostat)
> {
>   struct stat64 st;
>@@ -52,7 +53,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
>     }
> 
>   while ((d = __readdir64 (dirstream)) != NULL)
>-    if ((d->d_fileno == myino || *dostat)
>+    if ((d->d_fileno == mytty->st_ino || *dostat)
> 	&& strcmp (d->d_name, "stdin")
> 	&& strcmp (d->d_name, "stdout")
> 	&& strcmp (d->d_name, "stderr"))
>@@ -72,12 +73,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
> 	cp[0] = '\0';
> 
> 	if (__xstat64 (_STAT_VER, buf, &st) == 0
>-#ifdef _STATBUF_ST_RDEV
>-	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
>-#else
>-	    && d->d_fileno == myino && st.st_dev == mydev
>-#endif
>-	   )
>+	    && is_mytty (mytty, &st))
> 	  {
> 	    (void) __closedir (dirstream);
> 	    __set_errno (save);
>@@ -151,12 +147,7 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>       /* Verify readlink result, fall back on iterating through devices.  */
>       if (buf[0] == '/'
> 	  && __xstat64 (_STAT_VER, buf, &st1) == 0
>-#ifdef _STATBUF_ST_RDEV
>-	  && S_ISCHR (st1.st_mode)
>-	  && st1.st_rdev == st.st_rdev
>-#endif
>-	  && st1.st_ino == st.st_ino
>-	  && st1.st_dev == st.st_dev)
>+	  && is_mytty (&st, &st1));

This semicolon will cause the following "return 0" to be executed
unconditionally. That's very much not what you want I suspect.

> 	return 0;
> 
>       /* If the link doesn't exist, then it points to a device in another
>@@ -175,13 +166,8 @@ __ttyname_r (int fd, char *buf, size_t buflen)
> 
>   if (__xstat64 (_STAT_VER, buf, &st1) == 0 && S_ISDIR (st1.st_mode))
>     {
>-#ifdef _STATBUF_ST_RDEV
>-      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
>+      ret = getttyname_r (buf, buflen, &st, save,
> 			  &dostat);
>-#else
>-      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
>-			  &dostat);
>-#endif
>     }
>   else
>     {
>@@ -193,26 +179,16 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>     {
>       buf[sizeof ("/dev/") - 1] = '\0';
>       buflen += sizeof ("pts/") - 1;
>-#ifdef _STATBUF_ST_RDEV
>-      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
>-			  &dostat);
>-#else
>-      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
>+      ret = getttyname_r (buf, buflen, &st, save,
> 			  &dostat);
>-#endif
>     }
> 
>   if (ret && dostat != -1)
>     {
>       buf[sizeof ("/dev/") - 1] = '\0';
>       dostat = 1;
>-#ifdef _STATBUF_ST_RDEV
>-      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino,
>-			  save, &dostat);
>-#else
>-      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino,
>+      ret = getttyname_r (buf, buflen, &st,
> 			  save, &dostat);
>-#endif
>     }
> 
>   return ret;
>-- 
>2.14.1
>

-- 
You are receiving this mail because:
You are on the CC list for the bug.