[PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

[Luke Shumaker <lukeshu at parabola dot nu>]

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.

  The linux tst-ttyname test hasn't been added yet (to avoid breaking
  `git bisect`), but for reference, here's how each relevent change so
  far affects the testcases that will be in it:

  |                                 | before  |         | make tests | don't |
  |                                 | 15e9a4f | 15e9a4f | consistent | bail  |
  |---------------------------------+---------+---------+------------+-------|
  | basic smoketest                 | PASS    | PASS    | PASS       | PASS  |
  | no conflict, no match           | PASS[1] | PASS    | PASS       | PASS  |
  | no conflict, console            | PASS    | FAIL!   | FAIL       | PASS! |
  | conflict, no match              | FAIL    | PASS!   | PASS       | PASS  |
  | conflict, console               | FAIL    | FAIL    | FAIL       | PASS! |
  | with readlink target            | PASS    | PASS    | PASS       | PASS  |
  | with readlink trap; fallback    | FAIL    | FAIL    | FAIL       | PASS! |
  | with readlink trap; no fallback | FAIL    | PASS!   | PASS       | PASS  |
  | with search-path trap           | FAIL    | FAIL    | PASS!      | PASS  |
  |---------------------------------+---------+---------+------------+-------|
  |                                 | 4/9     | 5/9     | 6/9        | 9/9   |

  [1]: 15e9a4f introduced a semantic that, under certain failure
       conditions, ttyname sets errno=ENODEV, where previously it
       didn't set errno; it's not quite fair to hold "before 15e9a4f"
       ttyname to those new semantics.  This testcase actually fails,
       but would have passed if we tested for the old the semantics.

  Without the previous ("make tests consistent") patch, this change
  would revert the test status to the pre-15e9a4f state (except for
  the change in errno semantics affecting "no conflict, no match").
  That's surprising; this doesn't revert the changes that Christian
  made in that commit.  Christian's change made us disregard the false
  similarity of file pointed to by /proc/self/fd/$X; but that file
  ("/dev/pts/$Y") will just come up again anyway in the fallback
  search; so it is now even more important than before that the
  fallback search isn't confused by the false similarity.

v2:
 - Rearranged commit order
 - Expanded/reworded commit message
---
 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 4c282f8f47..b5d214ae6f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2017-11-02  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_pty): Change return type from int to bool.
diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 6e97d2d455..9287758b59 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..a109e79068 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