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

[Christian Brauner <christian dot brauner at canonical dot com>]

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