This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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
> 

Attachment: signature.asc
Description: PGP signature


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]