[PATCH 0/5] Fixup linux ttyname and ttyname_r [BZ #22145]

[PATCH 0/5] Fixup linux ttyname and ttyname_r [BZ #22145]

[Luke Shumaker]

The theme of this patchset is to fixup the changes made in 15e9a4f.
Fix a bug [BZ #22145] introduced in the commit (and add tests for both
the bug it fixed and the bug it introduced!), update documentation to
reflect the behavior introduced in the commit.

I am in the process of submitting the FSF copyright assignment
paperwork.

Luke Shumaker (5):
  manual: Update to mention ENODEV for ttyname and ttyname_r
  linux ttyname: Update a reference to kernel docs for kernel 4.10
  linux ttyname and ttyname_r: Add tests
  linux ttyname and ttyname_r: Make the TTY equivalence tests consistent
  linux ttyname and ttyname_r: Fix namespace check

 ChangeLog                             |  20 +++
 manual/terminal.texi                  |   5 +
 sysdeps/unix/sysv/linux/Makefile      |   2 +-
 sysdeps/unix/sysv/linux/tst-ttyname.c | 325 ++++++++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/ttyname.c     |  59 +++---
 sysdeps/unix/sysv/linux/ttyname.h     |  15 +-
 sysdeps/unix/sysv/linux/ttyname_r.c   |  61 +++----
 7 files changed, 406 insertions(+), 81 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c

-- 
Happy hacking,
~ Luke Shumaker

[PATCH 3/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Luke Shumaker]

Some of these tests fail for now, the following commits should resolve
the failures.
---
 ChangeLog                             |   4 +
 sysdeps/unix/sysv/linux/Makefile      |   2 +-
 sysdeps/unix/sysv/linux/tst-ttyname.c | 325 ++++++++++++++++++++++++++++++++++
 3 files changed, 330 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c

diff --git a/ChangeLog b/ChangeLog
index 029c2a265a..1921f8883b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
 
+	[BZ #22145]
+	* sysdeps/unix/sysv/linux/tst-ttyname.c: New file.
+	* sysdeps/unix/sysv/linux/Makefile: Add tst-ttyname to tests.
+
 	* sysdeps/unix/sysv/linux/ttyname.h (is_pty): Update doc reference.
 
 	* manual/terminal.texi: Mention ENODEV for ttyname and ttyname_r.
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 30bd167bae..dcd9208352 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -50,7 +50,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
 		  bits/siginfo-arch.h bits/siginfo-consts-arch.h
 
 tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
-	 tst-quota tst-sync_file_range test-errno-linux
+	 tst-quota tst-sync_file_range tst-ttyname test-errno-linux
 
 # Generate the list of SYS_* macros for the system calls (__NR_*
 # macros).  The file syscall-names.list contains all possible system
diff --git a/sysdeps/unix/sysv/linux/tst-ttyname.c b/sysdeps/unix/sysv/linux/tst-ttyname.c
new file mode 100644
index 0000000000..884e498d35
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-ttyname.c
@@ -0,0 +1,325 @@
+/* Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+
+#define PREPARE prepare
+
+#define VERIFY(expr)					\
+  ({							\
+    if (expr)						\
+      ;							\
+    else						\
+      support_exit_failure_impl				\
+        (1, __FILE__, __LINE__, "%s: %m", #expr);	\
+  })
+
+/* plain ttyname runner */
+
+struct result {
+  const char *name;
+  int err;
+};
+
+static struct result
+run_ttyname (int fd)
+{
+  struct result ret;
+  errno = 0;
+  ret.name = ttyname (fd);
+  ret.err = errno;
+  return ret;
+}
+
+static bool
+eq_ttyname (struct result actual, struct result expected)
+{
+  if ((actual.err == expected.err) &&
+      (!actual.name == !expected.name) &&
+      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
+    {
+      char *name = expected.name
+	? xasprintf("\"%s\"", expected.name)
+	: strdup("NULL");
+      printf ("info: PASS {name=%s, errno=%d}\n",
+	      name, expected.err);
+      free(name);
+      return true;
+    }
+
+  char *actual_name = actual.name
+    ? xasprintf("\"%s\"", actual.name)
+    : strdup("NULL");
+  char *expected_name = expected.name
+    ? xasprintf("\"%s\"", expected.name)
+    : strdup("NULL");
+  printf ("error: actual {name=%s, errno=%d} != expected {name=%s, errno=%d}\n",
+	  actual_name, actual.err,
+	  expected_name, expected.err);
+  free(actual_name);
+  free(expected_name);
+  return false;
+}
+
+/* ttyname_r runner */
+
+struct result_r {
+  const char *name;
+  int ret;
+  int err;
+};
+
+static struct result_r
+run_ttyname_r (int fd)
+{
+  static char buf[TTY_NAME_MAX];
+
+  struct result_r ret;
+  errno = 0;
+  ret.ret = ttyname_r (fd, buf, TTY_NAME_MAX);
+  ret.err = errno;
+  if (ret.ret == 0)
+    ret.name = buf;
+  else
+    ret.name = NULL;
+  return ret;
+}
+
+static bool
+eq_ttyname_r (struct result_r actual, struct result_r expected)
+{
+  if ((actual.err == expected.err) &&
+      (actual.ret == expected.ret) &&
+      (!actual.name == !expected.name) &&
+      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
+    {
+      char *name = expected.name
+	? xasprintf("\"%s\"", expected.name)
+	: strdup("NULL");
+      printf ("info: PASS {name=%s, ret=%d, errno=%d}\n",
+              name, expected.ret, expected.err);
+      free(name);
+      return true;
+    }
+
+  char *actual_name = actual.name
+    ? xasprintf("\"%s\"", actual.name)
+    : strdup("NULL");
+  char *expected_name = expected.name
+    ? xasprintf("\"%s\"", expected.name)
+    : strdup("NULL");
+  printf ("error: actual {name=%s, ret=%d, errno=%d} != expected {name=%s, ret=%d, errno=%d}\n",
+	  actual_name, actual.ret, actual.err,
+	  expected_name, expected.ret, expected.err);
+  free(actual_name);
+  free(expected_name);
+  return false;
+}
+
+/* combined runner */
+
+static bool
+doit (int fd, const char *testname, struct result_r expected_r) {
+  struct result expected = {.name=expected_r.name, .err=expected_r.ret};
+  bool ret = true;
+
+  printf ("info: running %s\n", testname);
+
+  ret = eq_ttyname (run_ttyname (fd), expected) && ret;
+  ret = eq_ttyname_r (run_ttyname_r (fd), expected_r) && ret;
+
+  return ret;
+}
+
+/* main test */
+
+static char *chrootdir;
+static uid_t orig_uid;
+static uid_t orig_gid;
+
+static void
+prepare (int argc, char **argv)
+{
+  chrootdir = xasprintf ("%s/tst-ttyname-XXXXXX", test_dir);
+  if (mkdtemp (chrootdir) == NULL)
+    FAIL_EXIT1 ("mkdtemp (\"%s\"): %m", chrootdir);
+  add_temp_file (chrootdir);
+
+  orig_uid = getuid();
+  orig_gid = getgid();
+}
+
+static int
+do_test (void)
+{
+  struct stat st;
+
+  /* Open the PTS that we'll be testing on. */
+  int master;
+  char *slavename;
+  VERIFY ((master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK)) >= 0);
+  VERIFY ((slavename = ptsname (master)));
+  VERIFY (unlockpt (master) == 0);
+  if (strncmp (slavename, "/dev/pts/", 9) != 0)
+    FAIL_UNSUPPORTED ("slave pseudo-terminal is not under /dev/pts/: %s",
+		      slavename);
+  int slave = xopen (slavename, O_RDWR, 0);
+
+  /* Do a basic smoke test */
+
+  if (!doit (slave, "basic smoketest",
+	     (struct result_r){.name=slavename, .ret=0, .err=0}))
+    return 1;
+
+  /* Do the rest in a new mount namespace. */
+
+  support_become_root ();
+  if (unshare (CLONE_NEWNS) < 0)
+    FAIL_UNSUPPORTED ("could not enter new mount namespace");
+  /* support_become_root might have put us in a new user namespace;
+     most filesystems (including tmpfs) don't allow file or directory
+     creation from a user namespace unless uid and gid maps are set,
+     even if we have root privileges in the namespace.
+
+     Also, stat always reports that uid and gid maps are empty, so we
+     have to try actually reading from them to check if they are
+     empty. */
+  int fd;
+  if ((fd = open("/proc/self/uid_map", O_RDWR, 0)) >= 0)
+    {
+      char buf;
+      if (read(fd, &buf, 1) == 0)
+	{
+	  char *str = xasprintf ("0 %ld 1\n", (long)orig_uid);
+	  if (write(fd, str, strlen(str)) < 0)
+	    FAIL_EXIT1("write(uid_map, \"%s\"): %m", str);
+	  free(str);
+	}
+      xclose (fd);
+    }
+  /* oh, but we can't set the gid map until we turn off setgroups */
+  if ((fd = open ("/proc/self/setgroups", O_WRONLY, 0)) >= 0)
+    {
+      const char *str = "deny";
+      if (write(fd, str, strlen(str)) < 0)
+	FAIL_EXIT1("write(setroups, \"%s\"): %m", str);
+      xclose(fd);
+    }
+  if ((fd = open ("/proc/self/gid_map", O_RDWR, 0)) >= 0)
+    {
+      char buf;
+      if (read(fd, &buf, 1) == 0)
+	{
+	  char *str = xasprintf ("0 %ld 1\n", (long)orig_gid);
+	  if (write(fd, str, strlen(str)) < 0)
+	    FAIL_EXIT1("write(gid_map, \"%s\"): %m", str);
+	  free(str);
+	}
+      xclose (fd);
+    }
+
+  /* Set up the chroot */
+
+  VERIFY (mount ("tmpfs", chrootdir, "tmpfs", 0, "mode=755") == 0);
+  VERIFY (chdir (chrootdir) == 0);
+
+  xmkdir ("proc", 0755);
+  /* We possibly aren't root, but just have our own user-ns and
+     mount-ns; we'd also need our own pid-ns to mount procfs
+     normally. */
+  VERIFY (mount ("/proc", "proc", NULL, MS_BIND|MS_REC, NULL) == 0);
+
+  xmkdir ("dev", 0755);
+
+  xmkdir ("dev/pts", 0755);
+  VERIFY
+    (mount ("devpts", "dev/pts", "devpts",
+            MS_NOSUID|MS_NOEXEC, "newinstance,ptmxmode=0666,mode=620") == 0);
+  VERIFY (symlink ("pts/ptmx", "dev/ptmx") == 0);
+
+  /* Put the console at "/console" (where it won't be found);
+     explicitly remount it at "/dev/console" later when we run a test
+     we expect to succeed. */
+  xclose (xopen ("console", O_WRONLY|O_CREAT|O_NOCTTY, 0));
+  xclose (xopen ("dev/console", O_WRONLY|O_CREAT|O_NOCTTY, 0));
+  VERIFY (mount (slavename, "console", NULL, MS_BIND, NULL) == 0);
+
+  xchroot (chrootdir);
+  VERIFY (chdir ("/") == 0);
+
+  bool ok = true;
+
+  VERIFY (stat (slavename, &st) < 0); /* sanity check */
+  ok = doit (slave, "no conflict, no match",
+	     (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
+  VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+  ok = doit (slave, "no conflict, console",
+	     (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
+  VERIFY (umount ("/dev/console") == 0);
+
+  /* keep creating PTYs until we we get a name collision */
+  while (stat (slavename, &st) < 0)
+    posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK);
+  VERIFY (stat (slavename, &st) == 0);
+  ok = doit (slave, "conflict, no match",
+	     (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
+  VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+  ok = doit (slave, "conflict, console",
+	     (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
+  VERIFY (umount ("/dev/console") == 0);
+
+  VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+  VERIFY (mount ("/console", slavename, NULL, MS_BIND, NULL) == 0);
+  ok = doit (slave, "with bound pts name",
+	     (struct result_r){.name=slavename, .ret=0, .err=0}) && ok;
+  VERIFY (umount (slavename) == 0);
+  VERIFY (umount ("/dev/console") == 0);
+  
+  VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+  xmkdir ("/oldpts", 0755);
+  VERIFY (mount ("/dev/pts", "/oldpts", NULL, MS_MOVE, NULL) == 0);
+  xclose (xopen ("/dev/pts/trap", O_WRONLY|O_CREAT|O_NOCTTY, 0));
+  char *path = xasprintf("/oldpts/%s", strrchr(slavename, '/')+1);
+  VERIFY (mount (path, "/dev/pts/trap", NULL, MS_BIND, NULL) == 0);
+  free(path);
+  ok = doit (slave, "with search-path trap",
+	     (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
+  VERIFY (umount ("/dev/pts/trap") == 0);
+  VERIFY (unlink ("/dev/pts/trap") == 0);
+  VERIFY (umount ("/dev/console") == 0);
+
+  return ok ? 0 : 1;
+}
+
+#include <support/test-driver.c>
-- 
2.14.2

[PATCH 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Luke Shumaker]

Commit 15e9a4f3 by Christian Brauner <christian.brauner@canonical.com>
introduced ENODEV as a possible error condition for ttyname and ttyname_r.
The manual should mention this.
---
 ChangeLog            | 4 ++++
 manual/terminal.texi | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index a3c6b0ab19..cc036f7b88 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
+
+	* manual/terminal.texi: Mention ENODEV for ttyname and ttyname_r.
+
 2017-10-10  Joseph Myers  <joseph@codesourcery.com>
 
 	* sysdeps/generic/libm-alias-double.h (libm_alias_double_other_r):
diff --git a/manual/terminal.texi b/manual/terminal.texi
index 4fef5045b8..f505e93af6 100644
--- a/manual/terminal.texi
+++ b/manual/terminal.texi
@@ -109,6 +109,11 @@ The @var{filedes} is not associated with a terminal.
 @item ERANGE
 The buffer length @var{len} is too small to store the string to be
 returned.
+
+@item ENODEV
+The @var{filedes} is valid, and is associated with a terminal, and
+that terminal is a slave psuedo-terminal, but the associated file name
+could not be determined.  This is a GNU extension.
 @end table
 @end deftypefun
 
-- 
2.14.2

[PATCH 2/5] linux ttyname: Update a reference to kernel docs for kernel 4.10

[Luke Shumaker]

Linux 4.10 moved many of the documentation files around.

It came out between the time the patch adding the comment (commit
15e9a4f3) was submitted and the time it was applied (in February,
January, and March 2017; respectively).
---
 ChangeLog                         | 2 ++
 sysdeps/unix/sysv/linux/ttyname.h | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index cc036f7b88..029c2a265a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,7 @@
 2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
 
+	* sysdeps/unix/sysv/linux/ttyname.h (is_pty): Update doc reference.
+
 	* manual/terminal.texi: Mention ENODEV for ttyname and ttyname_r.
 
 2017-10-10  Joseph Myers  <joseph@codesourcery.com>
diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
index 2e415e4e9c..dd7873d1ff 100644
--- a/sysdeps/unix/sysv/linux/ttyname.h
+++ b/sysdeps/unix/sysv/linux/ttyname.h
@@ -21,7 +21,8 @@
 #include <sys/stat.h>
 
 /* Return true if this is a UNIX98 pty device, as defined in
-   linux/Documentation/devices.txt.  */
+   linux/Documentation/devices.txt (on linux < 4.10) or
+   linux/Documentation/admin-guide/devices.txt (on linux >= 4.10). */
 static inline int
 is_pty (struct stat64 *sb)
 {
-- 
2.14.2

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

[Luke Shumaker]

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.
---
 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

[PATCH 4/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

[Luke Shumaker]

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

This check appears in several places.

Then, in commit 15e9a4f3 by Christian Brauner
<christian.brauner@canonical.com>, that check changed 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.
---
 ChangeLog                           |  5 +++++
 sysdeps/unix/sysv/linux/ttyname.c   | 40 ++++++++----------------------------
 sysdeps/unix/sysv/linux/ttyname.h   | 12 +++++++++++
 sysdeps/unix/sysv/linux/ttyname_r.c | 41 ++++++++-----------------------------
 4 files changed, 34 insertions(+), 64 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1921f8883b..5973b9d50b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
 
+	[BZ #22145]
+	* sysdeps/unix/sysv/linux/ttyname.h (is_mytty): New function.
+	* sysdeps/unix/sysv/linux/ttyname.c: Call it.
+	* sysdeps/unix/sysv/linux/ttyname_r.c: Likewise.
+
 	[BZ #22145]
 	* sysdeps/unix/sysv/linux/tst-ttyname.c: New file.
 	* sysdeps/unix/sysv/linux/Makefile: Add tst-ttyname to tests.
diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 116cf350e5..138a8a57f8 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -35,14 +35,14 @@
 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);
 
 libc_freeres_ptr (static char *getttyname_name);
 
 static char *
 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;
@@ -63,7 +63,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"))
@@ -85,12 +85,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
@@ -167,12 +162,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
@@ -186,11 +176,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
     {
@@ -200,21 +186,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 dd7873d1ff..15f7b066b6 100644
--- a/sysdeps/unix/sysv/linux/ttyname.h
+++ b/sysdeps/unix/sysv/linux/ttyname.h
@@ -33,3 +33,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 18f35ef2b7..d975d95d0d 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -31,12 +31,12 @@
 #include "ttyname.h"
 
 static int getttyname_r (char *buf, size_t buflen,
-			 dev_t mydev, ino64_t myino, int save,
+			 struct stat64 *mytty, int save,
 			 int *dostat);
 
 static int
 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 +52,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 +72,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 +146,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))
 	return 0;
 
       /* If the link doesn't exist, then it points to a device in another
@@ -175,13 +165,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 +178,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.2

Re: [PATCH 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Christian Brauner]

On Wed, Oct 11, 2017 at 11:53:17PM -0400, Luke Shumaker wrote:
> Commit 15e9a4f3 by Christian Brauner <christian.brauner@canonical.com>
> introduced ENODEV as a possible error condition for ttyname and ttyname_r.
> The manual should mention this.
> ---
> ChangeLog            | 4 ++++
> manual/terminal.texi | 5 +++++
> 2 files changed, 9 insertions(+)
> 
> diff --git a/ChangeLog b/ChangeLog
> index a3c6b0ab19..cc036f7b88 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
> +
> +	* manual/terminal.texi: Mention ENODEV for ttyname and ttyname_r.
> +
> 2017-10-10  Joseph Myers  <joseph@codesourcery.com>
> 
> * sysdeps/generic/libm-alias-double.h (libm_alias_double_other_r):
> diff --git a/manual/terminal.texi b/manual/terminal.texi
> index 4fef5045b8..f505e93af6 100644
> --- a/manual/terminal.texi
> +++ b/manual/terminal.texi
> @@ -109,6 +109,11 @@ The @var{filedes} is not associated with a terminal.
> @item ERANGE
> The buffer length @var{len} is too small to store the string to be
> returned.
> +
> +@item ENODEV
> +The @var{filedes} is valid, and is associated with a terminal, and
> +that terminal is a slave psuedo-terminal, but the associated file name
> +could not be determined.  This is a GNU extension.

Makes sense to me and is in line with a discussion I had with some of the
coreutils people.

@end table
@end deftypefun

-- 
2.14.2

Re: [PATCH 3/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Christian Brauner]

On Wed, Oct 11, 2017 at 11:53:19PM -0400, Luke Shumaker wrote:
> Some of these tests fail for now, the following commits should resolve
> the failures.

Why then arrange them in that order? Is there a reason to not put the tests last
after the other commits?

> ---
> ChangeLog                             |   4 +
> sysdeps/unix/sysv/linux/Makefile      |   2 +-
> sysdeps/unix/sysv/linux/tst-ttyname.c | 325 ++++++++++++++++++++++++++++++++++
> 3 files changed, 330 insertions(+), 1 deletion(-)
> create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index 029c2a265a..1921f8883b 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,9 @@
> 2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
> 
> +	[BZ #22145]
> +	* sysdeps/unix/sysv/linux/tst-ttyname.c: New file.
> +	* sysdeps/unix/sysv/linux/Makefile: Add tst-ttyname to tests.
> +
> * sysdeps/unix/sysv/linux/ttyname.h (is_pty): Update doc reference.
> 
> * manual/terminal.texi: Mention ENODEV for ttyname and ttyname_r.
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 30bd167bae..dcd9208352 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -50,7 +50,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
> bits/siginfo-arch.h bits/siginfo-consts-arch.h
> 
> tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
> -	 tst-quota tst-sync_file_range test-errno-linux
> +	 tst-quota tst-sync_file_range tst-ttyname test-errno-linux
> 
> # Generate the list of SYS_* macros for the system calls (__NR_*
> # macros).  The file syscall-names.list contains all possible system
> diff --git a/sysdeps/unix/sysv/linux/tst-ttyname.c b/sysdeps/unix/sysv/linux/tst-ttyname.c
> new file mode 100644
> index 0000000000..884e498d35
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-ttyname.c
> @@ -0,0 +1,325 @@
> +/* Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public License as
> +   published by the Free Software Foundation; either version 2.1 of the
> +   License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; see the file COPYING.LIB.  If
> +   not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <sched.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mount.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include <support/check.h>
> +#include <support/namespace.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +
> +#define PREPARE prepare
> +
> +#define VERIFY(expr)					\
> +  ({							\
> +    if (expr)						\
> +      ;							\
> +    else						\
> +      support_exit_failure_impl				\
> +        (1, __FILE__, __LINE__, "%s: %m", #expr);	\
> +  })
> +
> +/* plain ttyname runner */
> +
> +struct result {
> +  const char *name;
> +  int err;
> +};
> +
> +static struct result
> +run_ttyname (int fd)
> +{
> +  struct result ret;
> +  errno = 0;
> +  ret.name = ttyname (fd);

You very likely want to strdup() this. ttyname() is perfectly free to overwrite
the buffer. Also this pointer is going to be invalid after the funtion returns.

> +  ret.err = errno;
> +  return ret;
> +}

Looks like you're returning stack memory from run_ttyname(). That's invalid
memory after the function returns.

> +
> +static bool
> +eq_ttyname (struct result actual, struct result expected)
> +{
> +  if ((actual.err == expected.err) &&
> +      (!actual.name == !expected.name) &&
> +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> +    {
> +      char *name = expected.name
> +	? xasprintf("\"%s\"", expected.name)
> +	: strdup("NULL");
> +      printf ("info: PASS {name=%s, errno=%d}\n",
> +	      name, expected.err);
> +      free(name);
> +      return true;
> +    }
> +
> +  char *actual_name = actual.name
> +    ? xasprintf("\"%s\"", actual.name)
> +    : strdup("NULL");
> +  char *expected_name = expected.name
> +    ? xasprintf("\"%s\"", expected.name)
> +    : strdup("NULL");
> +  printf ("error: actual {name=%s, errno=%d} != expected {name=%s, errno=%d}\n",
> +	  actual_name, actual.err,
> +	  expected_name, expected.err);
> +  free(actual_name);
> +  free(expected_name);
> +  return false;
> +}

The function layout doesn't make much sense to me. Why wouldn't you pass by
reference here aka

eq_ttyname(struct result *actual, struct result *expected)

?

> +
> +/* ttyname_r runner */
> +
> +struct result_r {
> +  const char *name;
> +  int ret;
> +  int err;
> +};
> +
> +static struct result_r
> +run_ttyname_r (int fd)
> +{
> +  static char buf[TTY_NAME_MAX];
> +
> +  struct result_r ret;
> +  errno = 0;
> +  ret.ret = ttyname_r (fd, buf, TTY_NAME_MAX);
> +  ret.err = errno;
> +  if (ret.ret == 0)
> +    ret.name = buf;
> +  else
> +    ret.name = NULL;
> +  return ret;
> +}

Same problem as before, you're returning stack memory and fail to strdup() the
result of ttyname{_r}().

> +
> +static bool
> +eq_ttyname_r (struct result_r actual, struct result_r expected)
> +{
> +  if ((actual.err == expected.err) &&
> +      (actual.ret == expected.ret) &&
> +      (!actual.name == !expected.name) &&
> +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> +    {
> +      char *name = expected.name
> +	? xasprintf("\"%s\"", expected.name)
> +	: strdup("NULL");
> +      printf ("info: PASS {name=%s, ret=%d, errno=%d}\n",
> +              name, expected.ret, expected.err);
> +      free(name);
> +      return true;
> +    }
> +
> +  char *actual_name = actual.name
> +    ? xasprintf("\"%s\"", actual.name)
> +    : strdup("NULL");
> +  char *expected_name = expected.name
> +    ? xasprintf("\"%s\"", expected.name)
> +    : strdup("NULL");
> +  printf ("error: actual {name=%s, ret=%d, errno=%d} != expected {name=%s, ret=%d, errno=%d}\n",
> +	  actual_name, actual.ret, actual.err,
> +	  expected_name, expected.ret, expected.err);
> +  free(actual_name);
> +  free(expected_name);
> +  return false;
> +}

Should take pointers as arguments.

> +
> +/* combined runner */
> +
> +static bool
> +doit (int fd, const char *testname, struct result_r expected_r) {
> +  struct result expected = {.name=expected_r.name, .err=expected_r.ret};
> +  bool ret = true;
> +
> +  printf ("info: running %s\n", testname);
> +
> +  ret = eq_ttyname (run_ttyname (fd), expected) && ret;
> +  ret = eq_ttyname_r (run_ttyname_r (fd), expected_r) && ret;
> +
> +  return ret;
> +}

Given my points from above I'd be surprised if that won't SEGFAULT all over the
place under the right conditions. :)

> +
> +/* main test */
> +
> +static char *chrootdir;
> +static uid_t orig_uid;
> +static uid_t orig_gid;
> +
> +static void
> +prepare (int argc, char **argv)
> +{
> +  chrootdir = xasprintf ("%s/tst-ttyname-XXXXXX", test_dir);
> +  if (mkdtemp (chrootdir) == NULL)
> +    FAIL_EXIT1 ("mkdtemp (\"%s\"): %m", chrootdir);
> +  add_temp_file (chrootdir);
> +
> +  orig_uid = getuid();
> +  orig_gid = getgid();
> +}

Sorry, but where is this function called in the code?

> +
> +static int
> +do_test (void)
> +{
> +  struct stat st;
> +
> +  /* Open the PTS that we'll be testing on. */
> +  int master;
> +  char *slavename;
> +  VERIFY ((master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK)) >= 0);
> +  VERIFY ((slavename = ptsname (master)));
> +  VERIFY (unlockpt (master) == 0);
> +  if (strncmp (slavename, "/dev/pts/", 9) != 0)
> +    FAIL_UNSUPPORTED ("slave pseudo-terminal is not under /dev/pts/: %s",
> +		      slavename);
> +  int slave = xopen (slavename, O_RDWR, 0);

Why isn't that simply calling openpty()?

+
+  /* Do a basic smoke test */
+
+  if (!doit (slave, "basic smoketest",
+	     (struct result_r){.name=slavename, .ret=0, .err=0}))
+    return 1;
+
+  /* Do the rest in a new mount namespace. */
+
+  support_become_root ();
+  if (unshare (CLONE_NEWNS) < 0)
+    FAIL_UNSUPPORTED ("could not enter new mount namespace");
+  /* support_become_root might have put us in a new user namespace;
+     most filesystems (including tmpfs) don't allow file or directory
+     creation from a user namespace unless uid and gid maps are set,
+     even if we have root privileges in the namespace.
+
+     Also, stat always reports that uid and gid maps are empty, so we
+     have to try actually reading from them to check if they are
+     empty. */
+  int fd;
+  if ((fd = open("/proc/self/uid_map", O_RDWR, 0)) >= 0)
+    {
+      char buf;
+      if (read(fd, &buf, 1) == 0)
+	{
+	  char *str = xasprintf ("0 %ld 1\n", (long)orig_uid);
+	  if (write(fd, str, strlen(str)) < 0)
+	    FAIL_EXIT1("write(uid_map, \"%s\"): %m", str);
+	  free(str);
+	}
+      xclose (fd);
+    }
+  /* oh, but we can't set the gid map until we turn off setgroups */
+  if ((fd = open ("/proc/self/setgroups", O_WRONLY, 0)) >= 0)
+    {
+      const char *str = "deny";
+      if (write(fd, str, strlen(str)) < 0)
+	FAIL_EXIT1("write(setroups, \"%s\"): %m", str);
+      xclose(fd);
+    }
+  if ((fd = open ("/proc/self/gid_map", O_RDWR, 0)) >= 0)
+    {
+      char buf;
+      if (read(fd, &buf, 1) == 0)
+	{
+	  char *str = xasprintf ("0 %ld 1\n", (long)orig_gid);
+	  if (write(fd, str, strlen(str)) < 0)
+	    FAIL_EXIT1("write(gid_map, \"%s\"): %m", str);
+	  free(str);
+	}
+      xclose (fd);
+    }
+
+  /* Set up the chroot */
+
+  VERIFY (mount ("tmpfs", chrootdir, "tmpfs", 0, "mode=755") == 0);
+  VERIFY (chdir (chrootdir) == 0);
+
+  xmkdir ("proc", 0755);
+  /* We possibly aren't root, but just have our own user-ns and
+     mount-ns; we'd also need our own pid-ns to mount procfs
+     normally. */
+  VERIFY (mount ("/proc", "proc", NULL, MS_BIND|MS_REC, NULL) == 0);
+
+  xmkdir ("dev", 0755);
+
+  xmkdir ("dev/pts", 0755);
+  VERIFY
+    (mount ("devpts", "dev/pts", "devpts",
+            MS_NOSUID|MS_NOEXEC, "newinstance,ptmxmode=0666,mode=620") == 0);
+  VERIFY (symlink ("pts/ptmx", "dev/ptmx") == 0);
+
+  /* Put the console at "/console" (where it won't be found);
+     explicitly remount it at "/dev/console" later when we run a test
+     we expect to succeed. */
+  xclose (xopen ("console", O_WRONLY|O_CREAT|O_NOCTTY, 0));
+  xclose (xopen ("dev/console", O_WRONLY|O_CREAT|O_NOCTTY, 0));
+  VERIFY (mount (slavename, "console", NULL, MS_BIND, NULL) == 0);
+
+  xchroot (chrootdir);
+  VERIFY (chdir ("/") == 0);
+
+  bool ok = true;
+
+  VERIFY (stat (slavename, &st) < 0); /* sanity check */
+  ok = doit (slave, "no conflict, no match",
+	     (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
+  VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+  ok = doit (slave, "no conflict, console",
+	     (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
+  VERIFY (umount ("/dev/console") == 0);
+
+  /* keep creating PTYs until we we get a name collision */
+  while (stat (slavename, &st) < 0)
+    posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK);
+  VERIFY (stat (slavename, &st) == 0);
+  ok = doit (slave, "conflict, no match",
+	     (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
+  VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+  ok = doit (slave, "conflict, console",
+	     (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
+  VERIFY (umount ("/dev/console") == 0);
+
+  VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+  VERIFY (mount ("/console", slavename, NULL, MS_BIND, NULL) == 0);
+  ok = doit (slave, "with bound pts name",
+	     (struct result_r){.name=slavename, .ret=0, .err=0}) && ok;
+  VERIFY (umount (slavename) == 0);
+  VERIFY (umount ("/dev/console") == 0);
+  
+  VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+  xmkdir ("/oldpts", 0755);
+  VERIFY (mount ("/dev/pts", "/oldpts", NULL, MS_MOVE, NULL) == 0);
+  xclose (xopen ("/dev/pts/trap", O_WRONLY|O_CREAT|O_NOCTTY, 0));
+  char *path = xasprintf("/oldpts/%s", strrchr(slavename, '/')+1);
+  VERIFY (mount (path, "/dev/pts/trap", NULL, MS_BIND, NULL) == 0);
+  free(path);
+  ok = doit (slave, "with search-path trap",
+	     (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
+  VERIFY (umount ("/dev/pts/trap") == 0);
+  VERIFY (unlink ("/dev/pts/trap") == 0);
+  VERIFY (umount ("/dev/console") == 0);
+
+  return ok ? 0 : 1;
+}

Tbh, this is very convoluted atm.

+
+#include <support/test-driver.c>
-- 
2.14.2

Re: [PATCH 3/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Andreas Schwab]

On Okt 12 2017, Christian Brauner <christian.brauner@mailbox.org> wrote:

>> +static struct result_r
>> +run_ttyname_r (int fd)
>> +{
>> +  static char buf[TTY_NAME_MAX];
>> +
>> +  struct result_r ret;
>> +  errno = 0;
>> +  ret.ret = ttyname_r (fd, buf, TTY_NAME_MAX);
>> +  ret.err = errno;
>> +  if (ret.ret == 0)
>> +    ret.name = buf;
>> +  else
>> +    ret.name = NULL;
>> +  return ret;
>> +}
>
> Same problem as before, you're returning stack memory and fail to strdup() the
> result of ttyname{_r}().

In which way?  buf is static, and ret is copied to the caller.

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 4/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

[Christian Brauner]

On Wed, Oct 11, 2017 at 11:53:20PM -0400, Luke Shumaker wrote:
> 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
> 
> This check appears in several places.
> 
> Then, in commit 15e9a4f3 by Christian Brauner
> <christian.brauner@canonical.com>, that check changed 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.
> ---
> ChangeLog                           |  5 +++++
> sysdeps/unix/sysv/linux/ttyname.c   | 40 ++++++++----------------------------
> sysdeps/unix/sysv/linux/ttyname.h   | 12 +++++++++++
> sysdeps/unix/sysv/linux/ttyname_r.c | 41 ++++++++-----------------------------
> 4 files changed, 34 insertions(+), 64 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 1921f8883b..5973b9d50b 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,10 @@
> 2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
> 
> +	[BZ #22145]
> +	* sysdeps/unix/sysv/linux/ttyname.h (is_mytty): New function.
> +	* sysdeps/unix/sysv/linux/ttyname.c: Call it.
> +	* sysdeps/unix/sysv/linux/ttyname_r.c: Likewise.
> +
> [BZ #22145]
> * sysdeps/unix/sysv/linux/tst-ttyname.c: New file.
> * sysdeps/unix/sysv/linux/Makefile: Add tst-ttyname to tests.
> diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
> index 116cf350e5..138a8a57f8 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
> @@ -35,14 +35,14 @@
> 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);
> 
> libc_freeres_ptr (static char *getttyname_name);
> 
> static char *
> 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;
> @@ -63,7 +63,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"))
> @@ -85,12 +85,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
> @@ -167,12 +162,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
> @@ -186,11 +176,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
> {
> @@ -200,21 +186,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 dd7873d1ff..15f7b066b6 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.h
> +++ b/sysdeps/unix/sysv/linux/ttyname.h
> @@ -33,3 +33,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;
> +}

I find that hard to read. At first I thought this would unconditionally return
1. I'd rather prefer something like (In the appropriate GNU coding style
eventually of course.):

static inline bool is_mytty(struct stat64 *mytty, struct stat64 *maybe)
{
	if (maybe->st_ino == mytty->st_ino && maybe->st_dev == mytty->st_dev
#ifdef _STATBUF_ST_RDEV
	    && S_ISCHR(maybe->st_mode) && maybe->st_rdev == mytty->st_rdev
#endif
	    )
		return true;

	return false;
}

But others might be fine with it. But I think having it return a bool here is
perfectly fine since there's no other possible outcome than true or false.

diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index 18f35ef2b7..d975d95d0d 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -31,12 +31,12 @@
#include "ttyname.h"

static int getttyname_r (char *buf, size_t buflen,
-			 dev_t mydev, ino64_t myino, int save,
+			 struct stat64 *mytty, int save,
int *dostat);

static int
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 +52,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 +72,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 +146,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))
return 0;

/* If the link doesn't exist, then it points to a device in another
@@ -175,13 +165,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 +178,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.2

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

[Christian Brauner]

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

Re: [PATCH 3/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Luke Shumaker]

On Thu, 12 Oct 2017 06:32:59 -0400,
Christian Brauner wrote:
> 
> On Wed, Oct 11, 2017 at 11:53:19PM -0400, Luke Shumaker wrote:
> > Some of these tests fail for now, the following commits should resolve
> > the failures.
> 
> Why then arrange them in that order? Is there a reason to not put the tests last
> after the other commits?

Because it's too easy to accidentally write a test that passes even
without the fix.  By putting the test first, we can verify that it
fails without the fix, and passes with the fix.

> > +#define PREPARE prepare

Remember this, we'll come back to it.

> > +static struct result
> > +run_ttyname (int fd)
> > +{
> > +  struct result ret;
> > +  errno = 0;
> > +  ret.name = ttyname (fd);
> 
> You very likely want to strdup() this. ttyname() is perfectly free to overwrite
> the buffer. Also this pointer is going to be invalid after the funtion returns.

No, I didn't (similarly, run_ttyname_r() uses a static buffer that it
is free to overwrite upon subsequent invocations).  The pointer only
needs to be valid through the call to doit().

> > +  ret.err = errno;
> > +  return ret;
> > +}
> 
> Looks like you're returning stack memory from run_ttyname(). That's invalid
> memory after the function returns.

It's returning the full structure as a value, not a reference.

> > +
> > +static bool
> > +eq_ttyname (struct result actual, struct result expected)
> > +{
> > +  if ((actual.err == expected.err) &&
> > +      (!actual.name == !expected.name) &&
> > +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> > +    {
> > +      char *name = expected.name
> > +	? xasprintf("\"%s\"", expected.name)
> > +	: strdup("NULL");
> > +      printf ("info: PASS {name=%s, errno=%d}\n",
> > +	      name, expected.err);
> > +      free(name);
> > +      return true;
> > +    }
> > +
> > +  char *actual_name = actual.name
> > +    ? xasprintf("\"%s\"", actual.name)
> > +    : strdup("NULL");
> > +  char *expected_name = expected.name
> > +    ? xasprintf("\"%s\"", expected.name)
> > +    : strdup("NULL");
> > +  printf ("error: actual {name=%s, errno=%d} != expected {name=%s, errno=%d}\n",
> > +	  actual_name, actual.err,
> > +	  expected_name, expected.err);
> > +  free(actual_name);
> > +  free(expected_name);
> > +  return false;
> > +}
> 
> The function layout doesn't make much sense to me. Why wouldn't you pass by
> reference here aka
> 
> eq_ttyname(struct result *actual, struct result *expected)
> 
> ?

What reason is there to pass by reference?  The size of the structs is
known ahead of time, it won't be mutating them, and they aren't
optional (null-able).  As an optimization, perhaps (avoid a memcpy),
but I prefer to code for clarity first (especially for test code that
doesn't actually make it in to the library).

> > +static struct result_r
> > +run_ttyname_r (int fd)
> > +{
> > +  static char buf[TTY_NAME_MAX];
> > +
> > +  struct result_r ret;
> > +  errno = 0;
> > +  ret.ret = ttyname_r (fd, buf, TTY_NAME_MAX);
> > +  ret.err = errno;
> > +  if (ret.ret == 0)
> > +    ret.name = buf;
> > +  else
> > +    ret.name = NULL;
> > +  return ret;
> > +}
> 
> Same problem as before, you're returning stack memory and fail to strdup() the
> result of ttyname{_r}().

Same as before, the string is valid until this function is called
again, and that's fine as it only needs to be valid through one
invocation of doit().  And it is returning a value, not a reference.

> 
> > +
> > +static bool
> > +eq_ttyname_r (struct result_r actual, struct result_r expected)
> > +{
> > +  if ((actual.err == expected.err) &&
> > +      (actual.ret == expected.ret) &&
> > +      (!actual.name == !expected.name) &&
> > +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> > +    {
> > +      char *name = expected.name
> > +	? xasprintf("\"%s\"", expected.name)
> > +	: strdup("NULL");
> > +      printf ("info: PASS {name=%s, ret=%d, errno=%d}\n",
> > +              name, expected.ret, expected.err);
> > +      free(name);
> > +      return true;
> > +    }
> > +
> > +  char *actual_name = actual.name
> > +    ? xasprintf("\"%s\"", actual.name)
> > +    : strdup("NULL");
> > +  char *expected_name = expected.name
> > +    ? xasprintf("\"%s\"", expected.name)
> > +    : strdup("NULL");
> > +  printf ("error: actual {name=%s, ret=%d, errno=%d} != expected {name=%s, ret=%d, errno=%d}\n",
> > +	  actual_name, actual.ret, actual.err,
> > +	  expected_name, expected.ret, expected.err);
> > +  free(actual_name);
> > +  free(expected_name);
> > +  return false;
> > +}
> 
> Should take pointers as arguments.

Again, why?

> > +
> > +/* combined runner */
> > +
> > +static bool
> > +doit (int fd, const char *testname, struct result_r expected_r) {
> > +  struct result expected = {.name=expected_r.name, .err=expected_r.ret};
> > +  bool ret = true;
> > +
> > +  printf ("info: running %s\n", testname);
> > +
> > +  ret = eq_ttyname (run_ttyname (fd), expected) && ret;
> > +  ret = eq_ttyname_r (run_ttyname_r (fd), expected_r) && ret;
> > +
> > +  return ret;
> > +}
> 
> Given my points from above I'd be surprised if that won't SEGFAULT all over the
> place under the right conditions. :)
> 
> > +
> > +/* main test */
> > +
> > +static char *chrootdir;
> > +static uid_t orig_uid;
> > +static uid_t orig_gid;
> > +
> > +static void
> > +prepare (int argc, char **argv)
> > +{
> > +  chrootdir = xasprintf ("%s/tst-ttyname-XXXXXX", test_dir);
> > +  if (mkdtemp (chrootdir) == NULL)
> > +    FAIL_EXIT1 ("mkdtemp (\"%s\"): %m", chrootdir);
> > +  add_temp_file (chrootdir);
> > +
> > +  orig_uid = getuid();
> > +  orig_gid = getgid();
> > +}
> 
> Sorry, but where is this function called in the code?

Up top, at `#define PREPARE prepare`.

If PREPARE is defined, the test-driver will run that function in the
supervisor process before it forks and runs do_test.

> 
> > +
> > +static int
> > +do_test (void)
> > +{
> > +  struct stat st;
> > +
> > +  /* Open the PTS that we'll be testing on. */
> > +  int master;
> > +  char *slavename;
> > +  VERIFY ((master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK)) >= 0);
> > +  VERIFY ((slavename = ptsname (master)));
> > +  VERIFY (unlockpt (master) == 0);
> > +  if (strncmp (slavename, "/dev/pts/", 9) != 0)
> > +    FAIL_UNSUPPORTED ("slave pseudo-terminal is not under /dev/pts/: %s",
> > +		      slavename);
> > +  int slave = xopen (slavename, O_RDWR, 0);
> 
> Why isn't that simply calling openpty()?

Because systemd-nspawn doesn't simply call openpty() (I don't know
why), and I was largely just mimicking what it does.

> Tbh, this is very convoluted atm.

Yes, I agree.  But given that the point of it is to screw with
ttyname{_r} in convoluted ways, I'm not sure it can be improved too
much.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 4/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

[Luke Shumaker]

On Thu, 12 Oct 2017 07:16:15 -0400,
Christian Brauner wrote:
> On Wed, Oct 11, 2017 at 11:53:20PM -0400, Luke Shumaker wrote:
> > +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;
> > +}
> 
> I find that hard to read. At first I thought this would unconditionally return
> 1. I'd rather prefer something like (In the appropriate GNU coding style
> eventually of course.):
> 
> static inline bool is_mytty(struct stat64 *mytty, struct stat64 *maybe)
> {
> 	if (maybe->st_ino == mytty->st_ino && maybe->st_dev == mytty->st_dev
> #ifdef _STATBUF_ST_RDEV
> 	    && S_ISCHR(maybe->st_mode) && maybe->st_rdev == mytty->st_rdev
> #endif
> 	    )
> 		return true;
> 
> 	return false;
> }
> 
> But others might be fine with it.

I'm fine either way.  If the consensus is reformat it, I'll reformat
it.

>                                   But I think having it return a bool here is
> perfectly fine since there's no other possible outcome than true or false.

So why doesn't is_pty return a bool?  I was mimicing the style already
in the file.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 4/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

[Christian Brauner]

On Thu, Oct 12, 2017 at 08:18:50AM -0400, Luke Shumaker wrote:
> On Thu, 12 Oct 2017 07:16:15 -0400,
> Christian Brauner wrote:
> > On Wed, Oct 11, 2017 at 11:53:20PM -0400, Luke Shumaker wrote:
> > > +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;
> > > +}
> >
> > I find that hard to read. At first I thought this would unconditionally return
> > 1. I'd rather prefer something like (In the appropriate GNU coding style
> > eventually of course.):
> >
> > static inline bool is_mytty(struct stat64 *mytty, struct stat64 *maybe)
> > {
> > if (maybe->st_ino == mytty->st_ino && maybe->st_dev == mytty->st_dev
> > #ifdef _STATBUF_ST_RDEV
> >    && S_ISCHR(maybe->st_mode) && maybe->st_rdev == mytty->st_rdev
> > #endif
> >    )
> > return true;
> >
> > return false;
> > }
> >
> > But others might be fine with it.
>
> I'm fine either way.  If the consensus is reformat it, I'll reformat
> it.
>
> >                                   But I think having it return a bool here is
> > perfectly fine since there's no other possible outcome than true or false.
>
> So why doesn't is_pty return a bool?  I was mimicing the style already
> in the file.

Likely because it was overseen as well. I think it makes more sense to have them
return proper bools when they can't return anything else. Seems fine to me since
they're used all over the codebase as well.

>
> --
> Happy hacking,
> ~ Luke Shumaker

Re: [PATCH 3/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Christian Brauner]

On Thu, Oct 12, 2017 at 08:12:21AM -0400, Luke Shumaker wrote:
> On Thu, 12 Oct 2017 06:32:59 -0400,
> Christian Brauner wrote:
> >
> > On Wed, Oct 11, 2017 at 11:53:19PM -0400, Luke Shumaker wrote:
> > > Some of these tests fail for now, the following commits should resolve
> > > the failures.
> >
> > Why then arrange them in that order? Is there a reason to not put the tests last
> > after the other commits?
>
> Because it's too easy to accidentally write a test that passes even
> without the fix.  By putting the test first, we can verify that it
> fails without the fix, and passes with the fix.
>
> > > +#define PREPARE prepare
>
> Remember this, we'll come back to it.
>
> > > +static struct result
> > > +run_ttyname (int fd)
> > > +{
> > > +  struct result ret;
> > > +  errno = 0;
> > > +  ret.name = ttyname (fd);
> >
> > You very likely want to strdup() this. ttyname() is perfectly free to overwrite
> > the buffer. Also this pointer is going to be invalid after the funtion returns.
>
> No, I didn't (similarly, run_ttyname_r() uses a static buffer that it
> is free to overwrite upon subsequent invocations).  The pointer only
> needs to be valid through the call to doit().

>
> > > +  ret.err = errno;
> > > +  return ret;
> > > +}
> >
> > Looks like you're returning stack memory from run_ttyname(). That's invalid
> > memory after the function returns.
>
> It's returning the full structure as a value, not a reference.

Ah, I didn't see that you were returning a copy.
(Declaring functions as
static struct result
run_ttyname (int fd)

instead of
static struct result run_ttyname (int fd)
is still tripping me up.)

>
> > > +
> > > +static bool
> > > +eq_ttyname (struct result actual, struct result expected)
> > > +{
> > > +  if ((actual.err == expected.err) &&
> > > +      (!actual.name == !expected.name) &&
> > > +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> > > +    {
> > > +      char *name = expected.name
> > > + ? xasprintf("\"%s\"", expected.name)
> > > + : strdup("NULL");
> > > +      printf ("info: PASS {name=%s, errno=%d}\n",
> > > +      name, expected.err);
> > > +      free(name);
> > > +      return true;
> > > +    }
> > > +
> > > +  char *actual_name = actual.name
> > > +    ? xasprintf("\"%s\"", actual.name)
> > > +    : strdup("NULL");
> > > +  char *expected_name = expected.name
> > > +    ? xasprintf("\"%s\"", expected.name)
> > > +    : strdup("NULL");
> > > +  printf ("error: actual {name=%s, errno=%d} != expected {name=%s, errno=%d}\n",
> > > +  actual_name, actual.err,
> > > +  expected_name, expected.err);
> > > +  free(actual_name);
> > > +  free(expected_name);
> > > +  return false;
> > > +}
> >
> > The function layout doesn't make much sense to me. Why wouldn't you pass by
> > reference here aka
> >
> > eq_ttyname(struct result *actual, struct result *expected)
> >
> > ?
>
> What reason is there to pass by reference?  The size of the structs is
> known ahead of time, it won't be mutating them, and they aren't
> optional (null-able).  As an optimization, perhaps (avoid a memcpy),
> but I prefer to code for clarity first (especially for test code that
> doesn't actually make it in to the library).

Passing full structures by value is just something I rarely see used in code
since copying one way of the other is costly. But these are tests so: *waves
hands*

>
> > > +static struct result_r
> > > +run_ttyname_r (int fd)
> > > +{
> > > +  static char buf[TTY_NAME_MAX];
> > > +
> > > +  struct result_r ret;
> > > +  errno = 0;
> > > +  ret.ret = ttyname_r (fd, buf, TTY_NAME_MAX);
> > > +  ret.err = errno;
> > > +  if (ret.ret == 0)
> > > +    ret.name = buf;
> > > +  else
> > > +    ret.name = NULL;
> > > +  return ret;
> > > +}
> >
> > Same problem as before, you're returning stack memory and fail to strdup() the
> > result of ttyname{_r}().
>
> Same as before, the string is valid until this function is called
> again, and that's fine as it only needs to be valid through one
> invocation of doit().  And it is returning a value, not a reference.
>
> >
> > > +
> > > +static bool
> > > +eq_ttyname_r (struct result_r actual, struct result_r expected)
> > > +{
> > > +  if ((actual.err == expected.err) &&
> > > +      (actual.ret == expected.ret) &&
> > > +      (!actual.name == !expected.name) &&
> > > +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> > > +    {
> > > +      char *name = expected.name
> > > + ? xasprintf("\"%s\"", expected.name)
> > > + : strdup("NULL");
> > > +      printf ("info: PASS {name=%s, ret=%d, errno=%d}\n",
> > > +              name, expected.ret, expected.err);
> > > +      free(name);
> > > +      return true;
> > > +    }
> > > +
> > > +  char *actual_name = actual.name
> > > +    ? xasprintf("\"%s\"", actual.name)
> > > +    : strdup("NULL");
> > > +  char *expected_name = expected.name
> > > +    ? xasprintf("\"%s\"", expected.name)
> > > +    : strdup("NULL");
> > > +  printf ("error: actual {name=%s, ret=%d, errno=%d} != expected {name=%s, ret=%d, errno=%d}\n",
> > > +  actual_name, actual.ret, actual.err,
> > > +  expected_name, expected.ret, expected.err);
> > > +  free(actual_name);
> > > +  free(expected_name);
> > > +  return false;
> > > +}
> >
> > Should take pointers as arguments.
>
> Again, why?
>
> > > +
> > > +/* combined runner */
> > > +
> > > +static bool
> > > +doit (int fd, const char *testname, struct result_r expected_r) {
> > > +  struct result expected = {.name=expected_r.name, .err=expected_r.ret};
> > > +  bool ret = true;
> > > +
> > > +  printf ("info: running %s\n", testname);
> > > +
> > > +  ret = eq_ttyname (run_ttyname (fd), expected) && ret;
> > > +  ret = eq_ttyname_r (run_ttyname_r (fd), expected_r) && ret;
> > > +
> > > +  return ret;
> > > +}
> >
> > Given my points from above I'd be surprised if that won't SEGFAULT all over the
> > place under the right conditions. :)
> >
> > > +
> > > +/* main test */
> > > +
> > > +static char *chrootdir;
> > > +static uid_t orig_uid;
> > > +static uid_t orig_gid;
> > > +
> > > +static void
> > > +prepare (int argc, char **argv)
> > > +{
> > > +  chrootdir = xasprintf ("%s/tst-ttyname-XXXXXX", test_dir);
> > > +  if (mkdtemp (chrootdir) == NULL)
> > > +    FAIL_EXIT1 ("mkdtemp (\"%s\"): %m", chrootdir);
> > > +  add_temp_file (chrootdir);
> > > +
> > > +  orig_uid = getuid();
> > > +  orig_gid = getgid();
> > > +}
> >
> > Sorry, but where is this function called in the code?
>
> Up top, at `#define PREPARE prepare`.
>
> If PREPARE is defined, the test-driver will run that function in the
> supervisor process before it forks and runs do_test.
>
> >
> > > +
> > > +static int
> > > +do_test (void)
> > > +{
> > > +  struct stat st;
> > > +
> > > +  /* Open the PTS that we'll be testing on. */
> > > +  int master;
> > > +  char *slavename;
> > > +  VERIFY ((master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK)) >= 0);
> > > +  VERIFY ((slavename = ptsname (master)));
> > > +  VERIFY (unlockpt (master) == 0);
> > > +  if (strncmp (slavename, "/dev/pts/", 9) != 0)
> > > +    FAIL_UNSUPPORTED ("slave pseudo-terminal is not under /dev/pts/: %s",
> > > +      slavename);
> > > +  int slave = xopen (slavename, O_RDWR, 0);
> >
> > Why isn't that simply calling openpty()?
>
> Because systemd-nspawn doesn't simply call openpty() (I don't know
> why), and I was largely just mimicking what it does.
>
> > Tbh, this is very convoluted atm.
>
> Yes, I agree.  But given that the point of it is to screw with
> ttyname{_r} in convoluted ways, I'm not sure it can be improved too
> much.

Yeah.

>
> --
> Happy hacking,
> ~ Luke Shumaker

Re: [PATCH 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

On Wed, Oct 11, 2017 at 11:53:17PM -0400, Luke Shumaker wrote:
> Commit 15e9a4f3 by Christian Brauner <christian.brauner@canonical.com>
> introduced ENODEV as a possible error condition for ttyname and ttyname_r.
> The manual should mention this.
> ---
>  ChangeLog            | 4 ++++
>  manual/terminal.texi | 5 +++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/ChangeLog b/ChangeLog
> index a3c6b0ab19..cc036f7b88 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
> +
> +	* manual/terminal.texi: Mention ENODEV for ttyname and ttyname_r.

I think this should be

	* manual/terminal.texi (Is It a Terminal): ...

> --- a/manual/terminal.texi
> +++ b/manual/terminal.texi
> @@ -109,6 +109,11 @@ The @var{filedes} is not associated with a terminal.
>  @item ERANGE
>  The buffer length @var{len} is too small to store the string to be
>  returned.
> +
> +@item ENODEV
> +The @var{filedes} is valid, and is associated with a terminal, and
> +that terminal is a slave psuedo-terminal, but the associated file name
> +could not be determined.  This is a GNU extension.

Something has to be done with the wording, e.g.

The @var{filedes} argument is a valid file descriptor associated with
a slave psuedo-terminal device, but the file name of that device could
not be determined.  This is a GNU extension.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 3/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

On Thu, Oct 12, 2017 at 08:12:21AM -0400, Luke Shumaker wrote:
> On Thu, 12 Oct 2017 06:32:59 -0400, Christian Brauner wrote:
> > On Wed, Oct 11, 2017 at 11:53:19PM -0400, Luke Shumaker wrote:
> > > Some of these tests fail for now, the following commits should resolve
> > > the failures.
> > 
> > Why then arrange them in that order? Is there a reason to not put the tests last
> > after the other commits?
> 
> Because it's too easy to accidentally write a test that passes even
> without the fix.  By putting the test first, we can verify that it
> fails without the fix, and passes with the fix.

Sure, but this would break bisecting, so please reorder.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 4/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

On Wed, Oct 11, 2017 at 11:53:20PM -0400, Luke Shumaker wrote:
[...]
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
> @@ -35,14 +35,14 @@
>  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);

This has to be "const struct stat64 *".

>  
>  libc_freeres_ptr (static char *getttyname_name);
>  
>  static char *
>  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)

Likewise.

[...]
> --- a/sysdeps/unix/sysv/linux/ttyname.h
> +++ b/sysdeps/unix/sysv/linux/ttyname.h
> @@ -33,3 +33,15 @@ is_pty (struct stat64 *sb)
>    return false;
>  #endif
>  }
> +
> +static inline int
> +is_mytty(struct stat64 *mytty, struct stat64 *maybe)
> +{

Likewise.

[...]
> diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
> index 18f35ef2b7..d975d95d0d 100644
> --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> @@ -31,12 +31,12 @@
>  #include "ttyname.h"
>  
>  static int getttyname_r (char *buf, size_t buflen,
> -			 dev_t mydev, ino64_t myino, int save,
> +			 struct stat64 *mytty, int save,
>  			 int *dostat);

Likewise.

>  
>  static int
>  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)
>  {

Likewise.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Luke Shumaker]

On Wed, 11 Oct 2017 23:53:17 -0400,
Luke Shumaker wrote:
> +that terminal is a slave psuedo-terminal, but the associated file name
                            ^^^^^^

Someone mentioned in a private email to me (perhaps the just forgot to
CC the list) that I misspelled "pseudo" here.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Luke Shumaker]

On Thu, 12 Oct 2017 14:44:22 -0400,
Dmitry V. Levin wrote:
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,7 @@
> > +2017-10-11  Luke Shumaker  <lukeshu@parabola.nu>
> > +
> > +	* manual/terminal.texi: Mention ENODEV for ttyname and ttyname_r.
> 
> I think this should be
> 
> 	* manual/terminal.texi (Is It a Terminal): ...

Sure.

On that note, am I otherwise doing the ChangeLog thing correctly?  One
of the wiki pages suggested that the ChangeLog entry should be part of
the commit messages, not part of the patch; looking through the
archives, both ways seem common.

> > --- a/manual/terminal.texi
> > +++ b/manual/terminal.texi
> > @@ -109,6 +109,11 @@ The @var{filedes} is not associated with a terminal.
> >  @item ERANGE
> >  The buffer length @var{len} is too small to store the string to be
> >  returned.
> > +
> > +@item ENODEV
> > +The @var{filedes} is valid, and is associated with a terminal, and
> > +that terminal is a slave psuedo-terminal, but the associated file name
> > +could not be determined.  This is a GNU extension.
> 
> Something has to be done with the wording, e.g.
> 
> The @var{filedes} argument is a valid file descriptor associated with
> a slave psuedo-terminal device, but the file name of that device could
> not be determined.  This is a GNU extension.

The wording is awkward, but the exact case of what ENODEV means is
awkward.  The problem I have with your wording is that it isn't as
immediately clear that "slave pseudo-terminal" is a special case of
"terminal", and that ENODEV does *not* apply to that more general
case.

Now, IMO, ENODEV probably *should* apply to the more general case of
"The @var{filedes} associated with a terminal device, but the file
name associated with that device could not be determined", but that's
not what it currently means, and it's beyond the scope of this
patchset to change that.

What about:

The @var{filedes} is associated with a terminal device that is a slave
pseudo-terminal, but the file name associated with that device could
not be determined.  This is a GNU extension.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 3/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Luke Shumaker]

On Wed, 11 Oct 2017 23:53:19 -0400,
Luke Shumaker wrote:
> +  
> +  VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> +  xmkdir ("/oldpts", 0755);
> +  VERIFY (mount ("/dev/pts", "/oldpts", NULL, MS_MOVE, NULL) == 0);
> +  xclose (xopen ("/dev/pts/trap", O_WRONLY|O_CREAT|O_NOCTTY, 0));
> +  char *path = xasprintf("/oldpts/%s", strrchr(slavename, '/')+1);
> +  VERIFY (mount (path, "/dev/pts/trap", NULL, MS_BIND, NULL) == 0);
> +  free(path);
> +  ok = doit (slave, "with search-path trap",
> +	     (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
> +  VERIFY (umount ("/dev/pts/trap") == 0);
> +  VERIFY (unlink ("/dev/pts/trap") == 0);
> +  VERIFY (umount ("/dev/console") == 0);
> +

When re-ordering the commits, I realized that the "wrong" commit fixed
this test, which means it wasn't really testing what I wanted it to be
testing.  Once I work that out, I'll submit v2 of the patchset with
all of the feedback I've gotten.

-- 
Happy hacking,
~ Luke Shumaker

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

[Christian Brauner]

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

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

[Luke Shumaker]

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 :)

-- 
Happy hacking,
~ Luke Shumaker