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

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

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>

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.

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.

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

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.

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.