[v6,6/6] linux ttyname and ttyname_r: Add tests

  From: Luke Shumaker <lukeshu@parabola.nu>

Add a new tst-ttyname test that includes several named sub-testcases.

This patch is ordered after the patches with the fixes that it tests for
(to avoid breaking `git bisect`), but for reference, here's how each
relevant change so far affected the testcases in this commit, starting with
15e9a4f378c8607c2ae1aa465436af4321db0e23:

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

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

Each of the failing tests before 15e9a4f are all essentially the same bug:
that it returns a PTY slave with the correct minor device number, but from
the wrong devpts filesystem instance.

15e9a4f sought to fix this, but missed several of the cases that can cause
this to happen, and also broke the case where both the erroneous PTY and
the correct PTY exist.

v2:
 - Rearrange commit order
 - Fix code style
 - Expand commit message
 - Pull xtouch, become-root, and chroot setup into functions
 - Better tracking of test failures
 - Fix the "with search-path trap" test case
 - Add test cases for the readlink target directly
 - More readable diagnostic output
 - Test with both shared and private procfs
v3:
 - Revise commit message
 - Fix style of block comments
v4:
 - Fix brace style
 - Clarify comments
 - Use if/else blocks instead of ternary operators
 - Say "if (!...) ok = false;" rather than "ok = ... && ok;"
 - Use xstrdup instead of strdup
v5:
 - Avoid English idiom in a comment
---
 ChangeLog                             |   4 +
 sysdeps/unix/sysv/linux/Makefile      |   3 +-
 sysdeps/unix/sysv/linux/tst-ttyname.c | 625 ++++++++++++++++++++++++++++++++++
 3 files changed, 631 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c
  

On Fri, Nov 10, 2017 at 03:08:27PM -0500, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> Add a new tst-ttyname test that includes several named sub-testcases.
> 
> This patch is ordered after the patches with the fixes that it tests for
> (to avoid breaking `git bisect`), but for reference, here's how each
> relevant change so far affected the testcases in this commit, starting with
> 15e9a4f378c8607c2ae1aa465436af4321db0e23:
> 
>   |                                 | before  |         | make checks | don't |
>   |                                 | 15e9a4f | 15e9a4f | consistent  | bail  |
>   |---------------------------------+---------+---------+-------------+-------|
>   | basic smoketest                 | PASS    | PASS    | PASS        | PASS  |
>   | no conflict, no match           | PASS[1] | PASS    | PASS        | PASS  |
>   | no conflict, console            | PASS    | FAIL!   | FAIL        | PASS! |
>   | conflict, no match              | FAIL    | PASS!   | PASS        | PASS  |
>   | conflict, console               | FAIL    | FAIL    | FAIL        | PASS! |
>   | with readlink target            | PASS    | PASS    | PASS        | PASS  |
>   | with readlink trap; fallback    | FAIL    | FAIL    | FAIL        | PASS! |
>   | with readlink trap; no fallback | FAIL    | PASS!   | PASS        | PASS  |
>   | with search-path trap           | FAIL    | FAIL    | PASS!       | PASS  |
>   |---------------------------------+---------+---------+-------------+-------|
>   |                                 | 4/9     | 5/9     | 6/9         | 9/9   |
> 
>   [1]: 15e9a4f introduced a semantic that, under certain failure
>        conditions, ttyname sets errno=ENODEV, where previously it didn't
>        set errno; it's not quite fair to hold "before 15e9a4f" ttyname to
>        those new semantics.  This testcase actually fails, but would have
>        passed if we tested for the old the semantics.
> 
> Each of the failing tests before 15e9a4f are all essentially the same bug:
> that it returns a PTY slave with the correct minor device number, but from
> the wrong devpts filesystem instance.
> 
> 15e9a4f sought to fix this, but missed several of the cases that can cause
> this to happen, and also broke the case where both the erroneous PTY and
> the correct PTY exist.
> 
> v2:
>  - Rearrange commit order
>  - Fix code style
>  - Expand commit message
>  - Pull xtouch, become-root, and chroot setup into functions
>  - Better tracking of test failures
>  - Fix the "with search-path trap" test case
>  - Add test cases for the readlink target directly
>  - More readable diagnostic output
>  - Test with both shared and private procfs
> v3:
>  - Revise commit message
>  - Fix style of block comments
> v4:
>  - Fix brace style
>  - Clarify comments
>  - Use if/else blocks instead of ternary operators
>  - Say "if (!...) ok = false;" rather than "ok = ... && ok;"
>  - Use xstrdup instead of strdup
> v5:
>  - Avoid English idiom in a comment
> ---
>  ChangeLog                             |   4 +
>  sysdeps/unix/sysv/linux/Makefile      |   3 +-
>  sysdeps/unix/sysv/linux/tst-ttyname.c | 625 ++++++++++++++++++++++++++++++++++
>  3 files changed, 631 insertions(+), 1 deletion(-)
>  create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c

Some variable declarations in the middle-of-the stack I pointed out before are
still left. I don't see this coding style used in the codebase a lot but if
people don't care and are fine with this I don't care.

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

> 
> diff --git a/ChangeLog b/ChangeLog
> index 9ea727499c..f677ef4d72 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,9 @@
>  2017-11-07  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.
> +
>  	[BZ #22145]
>  	* sysdeps/unix/sysv/linux/ttyname.c (ttyname):
>  	Defer is_pty check until end of the function.
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index bf76b8773d..c6675b3aa5 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -43,7 +43,8 @@ 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-sysconf-iov_max
> +	 tst-quota tst-sync_file_range tst-sysconf-iov_max 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..a5a4bb9689
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-ttyname.c
> @@ -0,0 +1,625 @@
> +/* 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 <dirent.h>
> +#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/prctl.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>
> +
> +/* generic utilities */
> +
> +#define VERIFY(expr)                                                    \
> +  do {                                                                  \
> +    if (!(expr))                                                        \
> +      {                                                                 \
> +        printf ("error: %s:%d: %s: %m\n",                               \
> +                __FILE__, __LINE__, #expr);                             \
> +        exit (1);                                                       \
> +      }                                                                 \
> +  } while (0)
> +
> +static void
> +xtouch (const char *path, mode_t mode)
> +{
> +  xclose (xopen (path, O_WRONLY|O_CREAT|O_NOCTTY, mode));
> +}
> +
> +static size_t
> +trim_prefix (char *str, size_t str_len, const char *prefix)
> +{
> +  size_t prefix_len = strlen (prefix);
> +  if (str_len > prefix_len && memcmp (str, prefix, prefix_len) == 0)
> +    {
> +      memmove (str, str + prefix_len, str_len - prefix_len);
> +      return str_len - prefix_len;
> +    }
> +  return str_len;
> +}
> +
> +/* returns a pointer to static storage */
> +static char *
> +xreadlink (const char *linkname)
> +{
> +  static char target[PATH_MAX+1];
> +  ssize_t target_len = readlink (linkname, target, PATH_MAX);
> +  VERIFY (target_len > 0);
> +  target_len = trim_prefix (target, target_len, "(unreachable)");
> +  target[target_len] = '\0';
> +  return target;
> +}
> +
> +static void
> +become_root_in_mount_ns (void)
> +{
> +  uid_t orig_uid = getuid ();
> +  gid_t orig_gid = getgid ();
> +
> +  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 (failing with
> +     EOVERFLOW, since the uid overflows the empty (0-length) uid map).
> +
> +     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;

nit: middle-of-stack variable

> +
> +  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);
> +    }
> +
> +  /* Setting the gid map has the additional complexity that we have to
> +     first 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);
> +    }
> +}
> +
> +/* plain ttyname runner */
> +
> +struct result
> +{
> +  const char *name;
> +  int err;
> +};
> +
> +/* strings in result structure are in static storage */
> +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)
> +{
> +  char *actual_name, *expected_name;
> +
> +  if ((actual.err == expected.err) &&
> +      (!actual.name == !expected.name) &&
> +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> +    {
> +      if (expected.name)
> +        expected_name = xasprintf ("\"%s\"", expected.name);
> +      else
> +	expected_name = xstrdup ("NULL");
> +
> +      printf ("info:      ttyname: PASS {name=%s, errno=%d}\n",
> +	      expected_name, expected.err);
> +
> +      free (expected_name);
> +      return true;
> +    }
> +
> +  if (actual.name)
> +    actual_name = xasprintf ("\"%s\"", actual.name);
> +  else
> +    actual_name = xstrdup ("NULL");
> +
> +  if (expected.name)
> +    expected_name = xasprintf ("\"%s\"", expected.name);
> +  else
> +    expected_name = xstrdup ("NULL");
> +
> +  printf ("error:     ttyname: 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;
> +};
> +
> +/* strings in result structure are in static storage */
> +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)
> +{
> +  char *actual_name, *expected_name;
> +
> +  if ((actual.err == expected.err) &&
> +      (actual.ret == expected.ret) &&
> +      (!actual.name == !expected.name) &&
> +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> +    {
> +      if (expected.name)
> +        expected_name = xasprintf ("\"%s\"", expected.name);
> +      else
> +        expected_name = xstrdup ("NULL");
> +
> +      printf ("info:      ttyname_r: PASS {name=%s, ret=%d, errno=%d}\n",
> +              expected_name, expected.ret, expected.err);
> +
> +      free (expected_name);
> +      return true;
> +    }
> +
> +  if (actual.name)
> +    actual_name = xasprintf ("\"%s\"", actual.name);
> +  else
> +    actual_name = xstrdup ("NULL");
> +
> +  if (expected.name)
> +    expected_name = xasprintf ("\"%s\"", expected.name);
> +  else
> +    expected_name = xstrdup ("NULL");
> +
> +  printf ("error:     ttyname_r: 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:    testcase: %s\n", testname);
> +
> +  if (!eq_ttyname (run_ttyname (fd), expected))
> +    ret = false;
> +  if (!eq_ttyname_r (run_ttyname_r (fd), expected_r))
> +    ret = false;
> +
> +  if (!ret)
> +    support_record_failure ();
> +
> +  return ret;
> +}
> +
> +/* chroot setup */
> +
> +static char *chrootdir;
> +
> +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);
> +}
> +#define PREPARE prepare
> +
> +/* These chroot setup functions put the TTY at at "/console" (where it
> +   won't be found by ttyname), and create "/dev/console" as an
> +   ordinary file.  This way, it's easier to write test-cases that
> +   expect ttyname to fail; test-cases that expect it to succeed need
> +   to explicitly remount it at "/dev/console".  */
> +
> +static int
> +do_in_chroot_1 (int (*cb)(const char *, int))
> +{
> +  printf ("info:  entering chroot 1\n");
> +
> +  /* 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);

nit: middle-of-stack variable

> +  if (!doit (slave, "basic smoketest",
> +             (struct result_r){.name=slavename, .ret=0, .err=0}))
> +    return 1;
> +
> +  pid_t pid = xfork ();

nit: middle-of-stack variable

> +  if (pid == 0)
> +    {
> +      xclose (master);
> +
> +      become_root_in_mount_ns ();
> +
> +      VERIFY (mount ("tmpfs", chrootdir, "tmpfs", 0, "mode=755") == 0);
> +      VERIFY (chdir (chrootdir) == 0);
> +
> +      xmkdir ("proc", 0755);
> +      xmkdir ("dev", 0755);
> +      xmkdir ("dev/pts", 0755);
> +
> +      VERIFY (mount ("/proc", "proc", NULL, MS_BIND|MS_REC, NULL) == 0);
> +      VERIFY (mount ("devpts", "dev/pts", "devpts",
> +                     MS_NOSUID|MS_NOEXEC,
> +                     "newinstance,ptmxmode=0666,mode=620") == 0);
> +      VERIFY (symlink ("pts/ptmx", "dev/ptmx") == 0);
> +
> +      xtouch ("console", 0);
> +      xtouch ("dev/console", 0);
> +      VERIFY (mount (slavename, "console", NULL, MS_BIND, NULL) == 0);
> +
> +      xchroot (".");
> +
> +      char *linkname = xasprintf ("/proc/self/fd/%d", slave);
> +      char *target = xreadlink (linkname);
> +      VERIFY (strcmp (target, slavename) == 0);
> +      free (linkname);
> +
> +      _exit (cb (slavename, slave));
> +    }
> +  int status;

nit: middle-of-stack variable

> +  xwaitpid (pid, &status, 0);
> +  VERIFY (WIFEXITED (status));
> +  xclose (master);
> +  xclose (slave);
> +  return WEXITSTATUS (status);
> +}
> +
> +static int
> +do_in_chroot_2 (int (*cb)(const char *, int))
> +{
> +  printf ("info:  entering chroot 2\n");
> +
> +  int pid_pipe[2];
> +  xpipe (pid_pipe);
> +  int exit_pipe[2];
> +  xpipe (exit_pipe);

int pid_pipe[2];
int exit_pipe[2];

xpipe (exit_pipe);
xpipe (pid_pipe);


> +
> +  /* 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);
> +  /* wait until in a new mount ns to open the slave */
> +
> +  /* enable `wait`ing on grandchildren */
> +  VERIFY (prctl (PR_SET_CHILD_SUBREAPER, 1) == 0);
> +
> +  pid_t pid = xfork (); /* outer child */

nit: middle-of-stack variable

> +  if (pid == 0)
> +    {
> +      xclose (master);
> +      xclose (pid_pipe[0]);
> +      xclose (exit_pipe[1]);
> +
> +      become_root_in_mount_ns ();
> +
> +      int slave = xopen (slavename, O_RDWR, 0);

nit: middle-of-stack variable

> +      if (!doit (slave, "basic smoketest",
> +                 (struct result_r){.name=slavename, .ret=0, .err=0}))
> +        _exit (1);
> +
> +      VERIFY (mount ("tmpfs", chrootdir, "tmpfs", 0, "mode=755") == 0);
> +      VERIFY (chdir (chrootdir) == 0);
> +
> +      xmkdir ("proc", 0755);
> +      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);
> +
> +      xtouch ("console", 0);
> +      xtouch ("dev/console", 0);
> +      VERIFY (mount (slavename, "console", NULL, MS_BIND, NULL) == 0);
> +
> +      xchroot (".");
> +
> +      if (unshare (CLONE_NEWNS | CLONE_NEWPID) < 0)
> +        FAIL_UNSUPPORTED ("could not enter new PID namespace");
> +      pid = xfork (); /* inner child */
> +      if (pid == 0)
> +        {
> +          xclose (pid_pipe[1]);
> +
> +          /* wait until the outer child has exited */
> +          char c;

nit: middle-of-stack variable

> +          VERIFY (read (exit_pipe[0], &c, 1) == 0);
> +          xclose (exit_pipe[0]);
> +
> +          VERIFY (mount ("proc", "/proc", "proc",
> +                         MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) == 0);
> +
> +          char *linkname = xasprintf ("/proc/self/fd/%d", slave);
> +          char *target = xreadlink (linkname);

nit: middle-of-stack variable

> +          VERIFY (strcmp (target, strrchr (slavename, '/')) == 0);
> +          free (linkname);
> +
> +          _exit (cb (slavename, slave));
> +        }
> +      xwrite (pid_pipe[1], &pid, sizeof pid);
> +      _exit (0);
> +    }
> +  xclose (pid_pipe[1]);
> +  xclose (exit_pipe[0]);
> +  xclose (exit_pipe[1]);
> +
> +  /* wait for the outer child */
> +  int status;

nit: middle-of-stack variable

> +  xwaitpid (pid, &status, 0);
> +  VERIFY (WIFEXITED (status));
> +  int ret = WEXITSTATUS (status);

nit: middle-of-stack variable

> +  if (ret != 0)
> +    return ret;
> +
> +  /* set 'pid' to the inner child */
> +  VERIFY (read (pid_pipe[0], &pid, sizeof pid) == sizeof pid);
> +  xclose (pid_pipe[0]);
> +
> +  /* wait for the inner child */
> +  xwaitpid (pid, &status, 0);
> +  VERIFY (WIFEXITED (status));
> +  xclose (master);
> +  return WEXITSTATUS (status);
> +}
> +
> +/* main test */
> +
> +static int
> +run_chroot_tests (const char *slavename, int slave)
> +{
> +  struct stat st;
> +  bool ok = true;
> +
> +  /* There are 3 groups of tests here.  The first group fairly
> +     generically does things known to mess up ttyname, and verifies
> +     that ttyname copes correctly.  The remaining groups are
> +     increasingly convoluted, as we target specific parts of ttyname
> +     to try to confuse.  */
> +
> +  /* Basic tests that it doesn't get confused by multiple devpts
> +     instances.  */
> +  {
> +    VERIFY (stat (slavename, &st) < 0); /* sanity check */
> +    if (!doit (slave, "no conflict, no match",
> +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}))
> +      ok = false;

I mean these are just test so it's not super crucial in terms of coding style
but this functions would be well-served by a single goto-based exit point.

> +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> +    if (!doit (slave, "no conflict, console",
> +               (struct result_r){.name="/dev/console", .ret=0, .err=0}))
> +      ok = false;
> +    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);
> +
> +    if (!doit (slave, "conflict, no match",
> +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}))
> +      ok = false;
> +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> +    if (!doit (slave, "conflict, console",
> +               (struct result_r){.name="/dev/console", .ret=0, .err=0}))
> +      ok = false;
> +    VERIFY (umount ("/dev/console") == 0);
> +  }
> +
> +  /* The first tests kinda assumed that they hit certain code-paths
> +     based on assuming that the readlink target is 'slavename', but
> +     that's not quite always true.  They're still a good preliminary
> +     sanity check, so keep them, but let's add tests that make sure
> +     that those code-paths are hit by doing a readlink ourself.  */
> +  {
> +    char *linkname = xasprintf ("/proc/self/fd/%d", slave);
> +    char *target = xreadlink (linkname);
> +    free (linkname);
> +    /* Depeding on how we set up the chroot, the kernel may or may not
> +       trim the leading path to the target (it may give us "/6",
> +       instead of "/dev/pts/6").  We test it both ways (do_in_chroot_1
> +       and do_in_chroot_2).  This test group relies on the target
> +       existing, so guarantee that it does exist by creating it if
> +       necessary.  */
> +    if (stat (target, &st) < 0)
> +      {
> +        VERIFY (errno == ENOENT);
> +        xtouch (target, 0);
> +      }
> +
> +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> +    VERIFY (mount ("/console", target, NULL, MS_BIND, NULL) == 0);
> +    if (!doit (slave, "with readlink target",
> +               (struct result_r){.name=target, .ret=0, .err=0}))
> +      ok = false;
> +    VERIFY (umount (target) == 0);
> +    VERIFY (umount ("/dev/console") == 0);
> +
> +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> +    VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
> +    if (!doit (slave, "with readlink trap; fallback",
> +               (struct result_r){.name="/dev/console", .ret=0, .err=0}))
> +      ok = false;
> +    VERIFY (umount (target) == 0);
> +    VERIFY (umount ("/dev/console") == 0);
> +
> +    VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
> +    if (!doit (slave, "with readlink trap; no fallback",
> +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}))
> +      ok = false;
> +    VERIFY (umount (target) == 0);
> +  }
> +
> +  /* This test makes sure that everything still works OK if readdir
> +     finds a pseudo-match before and/or after the actual match.  Now,
> +     to do that, we need to control that readdir finds the
> +     pseudo-matches before and after the actual match; and there's no
> +     good way to control that order in absence of whitebox testing.
> +     So, just create 3 files, then use opendir/readdir to see what
> +     order they are in, and assign meaning based on that order, not by
> +     name; assigning the first to be a pseudo-match, the second to be
> +     the actual match, and the third to be a pseudo-match.  This
> +     assumes that (on tmpfs) ordering within the directory is stable
> +     in the absence of modification, which seems reasonably safe.  */
> +  {
> +    /* since we're testing the fallback search, disable the readlink
> +       happy-path */
> +    VERIFY (umount2 ("/proc", MNT_DETACH) == 0);
> +
> +    xtouch ("/dev/console1", 0);
> +    xtouch ("/dev/console2", 0);
> +    xtouch ("/dev/console3", 0);
> +
> +    char *c[3];
> +    int ci = 0;
> +    DIR *dirstream = opendir ("/dev");
> +    VERIFY (dirstream != NULL);
> +    struct dirent *d;

nit: middle-of-stack variable

> +    while ((d = readdir (dirstream)) != NULL && ci < 3)
> +      {
> +        if (strcmp (d->d_name, "console1") &&
> +            strcmp (d->d_name, "console2") &&
> +            strcmp (d->d_name, "console3") )
> +          continue;
> +        c[ci++] = xasprintf ("/dev/%s", d->d_name);
> +      }
> +    VERIFY (ci == 3);
> +    VERIFY (closedir (dirstream) == 0);
> +
> +    VERIFY (mount (slavename, c[0], NULL, MS_BIND, NULL) == 0);
> +    VERIFY (mount ("/console", c[1], NULL, MS_BIND, NULL) == 0);
> +    VERIFY (mount (slavename, c[2], NULL, MS_BIND, NULL) == 0);
> +    VERIFY (umount2 ("/dev/pts", MNT_DETACH) == 0);
> +    if (!doit (slave, "with search-path trap",
> +               (struct result_r){.name=c[1], .ret=0, .err=0}))
> +      ok = false;
> +    for (int i = 0; i < 3; i++)
> +      {
> +        VERIFY (umount (c[i]) == 0);
> +        VERIFY (unlink (c[i]) == 0);
> +        free (c[i]);
> +      }
> +  }
> +
> +  return ok ? 0 : 1;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int ret1 = do_in_chroot_1 (run_chroot_tests);
> +  if (ret1 == EXIT_UNSUPPORTED)
> +    return ret1;
> +
> +  int ret2 = do_in_chroot_2 (run_chroot_tests);

Likewise. Use a single "int ret" declaration at the top of the function and
return directly on error.

> +  if (ret2 == EXIT_UNSUPPORTED)
> +    return ret2;
> +
> +  return  ret1 | ret2;
> +}
> +
> +#include <support/test-driver.c>
> -- 
> 2.15.0
>
  

* Luke Shumaker:

> +/* returns a pointer to static storage */
> +static char *
> +xreadlink (const char *linkname)
> +{
> +  static char target[PATH_MAX+1];
> +  ssize_t target_len = readlink (linkname, target, PATH_MAX);
> +  VERIFY (target_len > 0);
> +  target_len = trim_prefix (target, target_len, "(unreachable)");
> +  target[target_len] = '\0';
> +  return target;
> +}

Please don't use x* names in this way in tests, they should go into
support/ as glibc function wrappers.  (I just added a different
implementation of xreadlink to support/.)
  

On Sat, 11 Nov 2017 19:10:41 -0500,
Christian Brauner wrote:
> On Fri, Nov 10, 2017 at 03:08:27PM -0500, Luke Shumaker wrote:
> Some variable declarations in the middle-of-the stack I pointed out before are
> still left. I don't see this coding style used in the codebase a lot but if
> people don't care and are fine with this I don't care.

(yeah, I was really lazy when looking for those)

Looking around, it isn't common, but it's there and presumably
permissible (example: support/support_test_main.c).  It wasn't allowed
in <= ISO C 90; I figure the reason it's not common in glibc is
because of all of the code written before availability of C99 was to
be relied on.

Since I'll be submitting v7 anyway to address Florian Weimer's
concerns, do you want me to change this?  It's not terrible, but I do
think that it hurts readability of some of the code.

> > +  /* There are 3 groups of tests here.  The first group fairly
> > +     generically does things known to mess up ttyname, and verifies
> > +     that ttyname copes correctly.  The remaining groups are
> > +     increasingly convoluted, as we target specific parts of ttyname
> > +     to try to confuse.  */
> > +
> > +  /* Basic tests that it doesn't get confused by multiple devpts
> > +     instances.  */
> > +  {
> > +    VERIFY (stat (slavename, &st) < 0); /* sanity check */
> > +    if (!doit (slave, "no conflict, no match",
> > +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}))
> > +      ok = false;
> 
> I mean these are just test so it's not super crucial in terms of coding style
> but this functions would be well-served by a single goto-based exit point.

The reason I didn't do that is because even if one case fails, I still
want the remainder to run.  (If that weren't true, I'd probably have
just called FAIL_EXIT1 in doit.)

> > +static int
> > +do_test (void)
> > +{
> > +  int ret1 = do_in_chroot_1 (run_chroot_tests);
> > +  if (ret1 == EXIT_UNSUPPORTED)
> > +    return ret1;
> > +
> > +  int ret2 = do_in_chroot_2 (run_chroot_tests);
> 
> Likewise. Use a single "int ret" declaration at the top of the function and
> return directly on error.

Likewise, even if some testcases failed in do_in_chroot_1, I still
want do_in_chroot_2 to run.  Unless ret1==EXIT_UNSUPPORTED (which
happens if we FAIL_UNSUPPORTED), in which case we bail early not
because a testcase failed, but because something indicated that the
test isn't runnable.
  

* Christian Brauner:

> Some variable declarations in the middle-of-the stack I pointed out
> before are still left. I don't see this coding style used in the
> codebase a lot but if people don't care and are fine with this I
> don't care.

In new code, we generally tend to reduce the scope of declarative
regions.
  

On Sun, Nov 12, 2017 at 05:23:31PM +0100, Florian Weimer wrote:
> * Christian Brauner:
> 
> > Some variable declarations in the middle-of-the stack I pointed out
> > before are still left. I don't see this coding style used in the
> > codebase a lot but if people don't care and are fine with this I
> > don't care.
> 
> In new code, we generally tend to reduce the scope of declarative
> regions.

Ha, ok. Thanks! I mean what I don't like is not small scopes but variable
declarations not at the beginning of a new scope. So to illustrate:

void foo()
  {
    int m, n, p;
    /* do stuff */

    if (bla)
      {
        /* new scope begins so it's fine to declare variable's here. */
	int a, b, c;

	/* do other stuff */
      }

      /* old scope --> Variable declarations here is what I find odd regardless
       * of C standard */
       int i, k, j;
  }

Is the latter encouraged, Florian?

Christian
  

On Sun, Nov 12, 2017 at 10:49:19AM -0500, Luke Shumaker wrote:
> On Sat, 11 Nov 2017 19:10:41 -0500,
> Christian Brauner wrote:
> > On Fri, Nov 10, 2017 at 03:08:27PM -0500, Luke Shumaker wrote:
> > Some variable declarations in the middle-of-the stack I pointed out before are
> > still left. I don't see this coding style used in the codebase a lot but if
> > people don't care and are fine with this I don't care.
> 
> (yeah, I was really lazy when looking for those)
> 
> Looking around, it isn't common, but it's there and presumably
> permissible (example: support/support_test_main.c).  It wasn't allowed
> in <= ISO C 90; I figure the reason it's not common in glibc is
> because of all of the code written before availability of C99 was to
> be relied on.
> 
> Since I'll be submitting v7 anyway to address Florian Weimer's
> concerns, do you want me to change this?  It's not terrible, but I do
> think that it hurts readability of some of the code.

Since these a) are tests and b) if variable declarations not at the beginning of
a new scope but in the middle of the current scope are considered acceptable who
am I to yell. :)

Thanks!
Christian

> 
> > > +  /* There are 3 groups of tests here.  The first group fairly
> > > +     generically does things known to mess up ttyname, and verifies
> > > +     that ttyname copes correctly.  The remaining groups are
> > > +     increasingly convoluted, as we target specific parts of ttyname
> > > +     to try to confuse.  */
> > > +
> > > +  /* Basic tests that it doesn't get confused by multiple devpts
> > > +     instances.  */
> > > +  {
> > > +    VERIFY (stat (slavename, &st) < 0); /* sanity check */
> > > +    if (!doit (slave, "no conflict, no match",
> > > +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}))
> > > +      ok = false;
> > 
> > I mean these are just test so it's not super crucial in terms of coding style
> > but this functions would be well-served by a single goto-based exit point.
> 
> The reason I didn't do that is because even if one case fails, I still
> want the remainder to run.  (If that weren't true, I'd probably have
> just called FAIL_EXIT1 in doit.)
> 
> > > +static int
> > > +do_test (void)
> > > +{
> > > +  int ret1 = do_in_chroot_1 (run_chroot_tests);
> > > +  if (ret1 == EXIT_UNSUPPORTED)
> > > +    return ret1;
> > > +
> > > +  int ret2 = do_in_chroot_2 (run_chroot_tests);
> > 
> > Likewise. Use a single "int ret" declaration at the top of the function and
> > return directly on error.
> 
> Likewise, even if some testcases failed in do_in_chroot_1, I still
> want do_in_chroot_2 to run.  Unless ret1==EXIT_UNSUPPORTED (which
> happens if we FAIL_UNSUPPORTED), in which case we bail early not
> because a testcase failed, but because something indicated that the
> test isn't runnable.
> 
> -- 
> Happy hacking,
> ~ Luke Shumaker