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 v2 5/5] linux ttyname and ttyname_r: Add tests [BZ #22145]


On Wed, 08 Nov 2017 14:43:07 -0500,
Christian Brauner wrote:
> > +  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);
> 
> I haven't looked at the test code a lot yet but is it generally considered ok to
> not cleanup before exit? That's mostly style but still it'd be good to clarify
> this. I usually do all the cleanup even when exiting.

Clean up the file system, or clean up memory?  FS cleanup isn't
necessary because we do everything on a tmpfs in a new mount
namespace.  Looking at other tests, it seems to be OK to leave memory
messy when exiting with an error.

> > +	  free (str);
> > +	}
> > +      xclose (fd);
> > +    }
> > +
> > +  /* Oh, but we can't set the gid map until we turn off setgroups. */
> 
> That sounds like a discovery you just made. I think - nitpick - a simple:
> /* We need to set setgroups to "deny" otherwise we won't be able to write a gid
> *  mapping. */

My intent was for it to read as a postscript to the previous comment,
but I'm happy to change it.

> > +    }
> > +
> > +  char *actual_name = actual.name
> > +    ? xasprintf ("\"%s\"", actual.name)
> > +    : strdup ("NULL");
> 
> I'd prefer if this would be proper if () branches as this is pretty unreadable.
> Also, I'd prefer - even though newer C standards allow this - if new variables
> wouldn't be declared mid-stack especially if the function is rather small.

Sure.

> > +  char *expected_name = expected.name
> > +    ? xasprintf ("\"%s\"", expected.name)
> > +    : strdup ("NULL");
> 
> Likewise.

Ack.

> > +    }
> > +
> > +  char *actual_name = actual.name
> > +    ? xasprintf ("\"%s\"", actual.name)
> > +    : strdup ("NULL");
> 
> Likewise.

Ack.

> > +  char *expected_name = expected.name
> > +    ? xasprintf ("\"%s\"", expected.name)
> > +    : strdup ("NULL");
> 
> Likewise.

Ack.

> > +static bool
> > +doit (int fd, const char *testname, struct result_r expected_r) {

(whoops, that brace needs to be on a new line)

> > +  bool ret = true;
> > +
> > +  printf ("info:    testcase: %s\n", testname);
> > +
> > +  ret = eq_ttyname (run_ttyname (fd), expected) && ret;
> > +  ret = eq_ttyname_r (run_ttyname_r (fd), expected_r) && ret;
> 
> Imho, this is not very nice. It looks clever but is hard to parse semantically.
> I'd prefer if the assignment from eq_ttyname{_r}() would be entangled from the
> following "&& ret" check. This is just easier to maintain.

Ok, see below.

> > +  /* enable `wait`ing on grandchildren */
> > +  VERIFY (prctl (PR_SET_CHILD_SUBREAPER, 1) == 0);
> 
> For future reference in case you want to avoid this clutch you can also use
> clone() with CLONE_PARENT.

I wanted to do that, but unfortunately glibc's clone(2) wrapper
requires passing in the stack explicitly, which requires knowing if
the stack grows up or down; and I didn't want to have to deal with
that in this test.

> > +  /* There are 3 groups of tests here.  The first group throws a
> > +     wrench into the works in a generic way, and verifies that ttyname
> 
> That's using a pretty (for a non-native speaker) specific English idiom. Could
> you maybe simply say "creates problems" or something similar.

Ok.

> > +     copes correctly.  The remaining groups are increasingly
> > +     convoluted, as we direct that wrench towards specific parts of
> 
> Likewise.

Ok.

> > +  {
> > +    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",
> 
> Uhm? I'm sorry, I don't understand. That sounds like a bug we fixed a while back
> upstream whereby the kernel recorded the wrong vfs mount for devpts. That's one
> scenario where this can happen. In any case the comment should outline when the
> kernel can *legitimately* show you the contents of the symlink as /<n> instead
> of the correct /dev/pts/<n>. Right now it sounds like this code doesn't
> understand how such a scenario can happen but only that it can happen.

You're right, this comment should be clearer.  The test has two
chroot-setup routines, and runs this code in each.  One of the setup
routines does it in a way that results in /<n> and the other does it
in a way that results in /dev/pts/<n>; this code doesn't have context
to know which of the setup routines was called.

> 
> > +     * instead of "/dev/pts/6").  This test group relies on the target
> > +     * existing, so guarantee that (so create a file at "/6" 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);
> > +    ok = doit (slave, "with readlink target",
> > +               (struct result_r){.name=target, .ret=0, .err=0}) && ok;
> > +    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);
> > +    ok = doit (slave, "with readlink trap; fallback",
> > +               (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
> > +    VERIFY (umount (target) == 0);
> > +    VERIFY (umount ("/dev/console") == 0);
> > +
> > +    VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
> > +    ok = doit (slave, "with readlink trap; no fallback",
> > +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
> 
> Same comment as above: Imho, this is not very nice. It looks clever but is hard
> to parse semantically. I'd prefer if the assignment would be entangled from the
> following "&& ret" check. This is just easier to maintain.

How would you rather it be written?

    static void
    doit (..., bool *ok)
    {
      ...

      if (!eq_ttyname (run_ttyname (fd), expected))
        *ok = false;
      if (!eq_ttyname_r (run_ttyname_r (fd), expected_r))
        *ok = false;
    }
  
    doit(..., &ok);

?

> > +    VERIFY (umount (target) == 0);
> > +  }
> > +
> > +  /* This test is extra-convoluted because we need to control the
> > +     order of files within the directory.  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.  This assumes
> 
> Why do you want to assign meaning based on order not name? If there's a
> legitimate reason please mention it.

Reason: for this test "we need to control the order of files within
the directory."

Conceptually, the test is making sure that an pseudo-match from the
wrong devpts that readdir() finds before it finds the real match
doesn't mess anything up.

To test that, we need to set up the pseudo-match and the actual match
such that readdir finds the pseudo-match first.  There's no good way
to control the order that readdir will find the files in (maybe I
should look in to the whitebox testing infrastructure?).  So we create
two files, and whichever one readdir finds first, we assign that one
to be the pseudo-match; and whichever one readdir finds second, we
assign that one to be the actual match.

(We also test that readdir finding a pseudo-match after the actual
match doesn't mess anything up either, hence 3 files instead of 2.)

> > +    ok = doit (slave, "with search-path trap",
> > +               (struct result_r){.name=c[1], .ret=0, .err=0}) && ok;
> 
> Same comment as above: Imho, this is not very nice. It looks clever but is hard
> to parse semantically. I'd prefer if the assignment would be entangled from the
> following "&& ret" check. This is just easier to maintain.

See above.

-- 
Happy hacking,
~ Luke Shumaker


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