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

[Luke Shumaker <lukeshu at lukeshu dot com>]

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

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

> > +#define PREPARE prepare

Remember this, we'll come back to it.

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

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

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

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

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

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

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

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

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

Again, why?

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

Up top, at `#define PREPARE prepare`.

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

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

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

> Tbh, this is very convoluted atm.

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

-- 
Happy hacking,
~ Luke Shumaker