This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]
On Wed, Nov 08, 2017 at 01:53:39AM +0300, Dmitry V. Levin wrote:
> On Thu, Nov 02, 2017 at 02:53:45PM -0400, Luke Shumaker wrote:
>
> s/bail/give up/
>
> > In commit 15e9a4f Christian Brauner introduced logic for ttyname() sending
> > back ENODEV to signal that we can't get a name for the TTY because we
> > inherited it from a different mount namespace.
>
> Like in other commit messages, this is getting too personal.
> We usually mention changes introduced with commits, not people authored
> those commits.
>
> > However, just because we inherited it from a different mount namespace, and
> > it isn't available at its original path, doesn't mean that its name is
> > unknowable; we can still find it by allowing the normal fall back on
> > iterating through devices.
>
> s/find/try to find/
>
> > A common scenario where this happens is with "/dev/console" in
>
> s/A common/An example/
>
> > containers. Common container managers (including systemd-nspawn) will
>
> "It's a common practice among container managers ... to ...".
>
> > call openpty() on a ptmx device in the host's mount namespace to
> > allocate a pty master/slave pair, then send the slave FD to the
> > container, and bind-mounted at "/dev/console" in the container's mount
>
> what is bind-mounted at "/dev/console" in the container's mount namespace?
> Is it a /dev/pty/<idx> path in the host's mount namespace?
Usually, but it doesn't need to be. You could mount another devpts instance
somewhere e.g. /mnt and open a master/slave pair manually without openpty()
(thereby circumventing the glibc hard-coded /dev/pts/<idx> check) and send the
slave to the container and mount by /proc/<pid>/fd/<idx>. So probably "and mount
the slave on /dev/console inside the container's mount namespace".
>
> > namespace. Inside of the container, the slave-end isn't available at
> > its original path ("/dev/pts/$X"), since the container mount namespace
> > has a separate devpts instance from the host (that path may or may not
> > exist in the container; if it does exist, it's not the same PTY slave
> > device). Currently ttyname{_r}() sees that the original path isn't a
> > match,
>
> doesn't match
>
> > and fails early and gives up, even though if it kept searching
> > it would find the TTY at "/dev/console". This fixes that so that the
> > ENODEV path does not force an early return inhibiting the fall-back
> > search.
> >
> > The linux tst-ttyname test hasn't been added yet (to avoid breaking
> > `git bisect`), but for reference, here's how each relevent change so
> > far affects the testcases that will be in it:
> >
> > | | before | | make tests | 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.
>
> Like in the previous patch, wouldn't it be better to mention only those
> test cases that are affected by this commit, and leave the whole picture
> to the commit that introduces the test itself?
>
> > Without the previous ("make tests consistent") patch, this change
> > would revert the test status to the pre-15e9a4f state (except for
> > the change in errno semantics affecting "no conflict, no match").
> > That's surprising; this doesn't revert the changes that Christian
> > made in that commit. Christian's change made us disregard the false
> > similarity of file pointed to by /proc/self/fd/$X; but that file
> > ("/dev/pts/$Y") will just come up again anyway in the fallback
> > search; so it is now even more important than before that the
> > fallback search isn't confused by the false similarity.
>
> Why do you want to go in details explaining how things will break
> if the previous patch is not applied?
>
> Wouldn't it be enough to say that this change is essentially based
> on the previous one that adds use of is_mytty in getttyname and getttyname_r?
>
> >
> > v2:
> > - Rearranged commit order
> > - Expanded/reworded commit message
> > ---
> > ChangeLog | 5 +++++
> > sysdeps/unix/sysv/linux/ttyname.c | 19 ++++++++++++-------
> > sysdeps/unix/sysv/linux/ttyname_r.c | 20 ++++++++++++--------
> > 3 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/ChangeLog b/ChangeLog
> > index 4c282f8f47..b5d214ae6f 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,5 +1,10 @@
> > 2017-11-02 Luke Shumaker <lukeshu@parabola.nu>
> >
> > + [BZ #22145]
> > + * sysdeps/unix/sysv/linux/ttyname.c (ttyname):
> > + Defer is_pty check until end.
>
> until the end of function?
>
> > + * sysdeps/unix/sysv/linux/ttyname_r.c (ttyname_r): Likewise.
> > +
> > [BZ #22145]
> > * sysdeps/unix/sysv/linux/ttyname.h
> > (is_pty): Change return type from int to bool.
> > diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
> > index 6e97d2d455..9287758b59 100644
> > --- a/sysdeps/unix/sysv/linux/ttyname.c
> > +++ b/sysdeps/unix/sysv/linux/ttyname.c
> > @@ -115,6 +115,7 @@ ttyname (int fd)
> > char procname[30];
> > struct stat64 st, st1;
> > int dostat = 0;
> > + int doispty = 0;
> > char *name;
> > int save = errno;
> > struct termios term;
> > @@ -165,13 +166,7 @@ ttyname (int fd)
> > && is_mytty (&st, &st1))
> > return ttyname_buf;
> >
> > - /* If the link doesn't exist, then it points to a device in another
> > - namespace. */
> > - if (is_pty (&st))
> > - {
> > - __set_errno (ENODEV);
> > - return NULL;
> > - }
> > + doispty = 1;
> > }
> >
> > if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
> > @@ -195,5 +190,15 @@ ttyname (int fd)
> > name = getttyname ("/dev", &st, save, &dostat);
> > }
> >
> > + if (!name && doispty && is_pty (&st))
> > + {
> > + /* We failed to figure out the TTY's name, but we can at least
> > + * signal that we did verify that it really is a PTY slave.
> > + * This happens when we have inherited the file descriptor from
> > + * a different mount namespace. */
> > + __set_errno (ENODEV);
> > + return NULL;
> > + }
> > +
> > return name;
> > }
> > diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
> > index 58eb919c3f..a109e79068 100644
> > --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> > +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> > @@ -95,6 +95,7 @@ __ttyname_r (int fd, char *buf, size_t buflen)
> > char procname[30];
> > struct stat64 st, st1;
> > int dostat = 0;
> > + int doispty = 0;
> > int save = errno;
> >
> > /* Test for the absolute minimal size. This makes life easier inside
> > @@ -149,14 +150,7 @@ __ttyname_r (int fd, char *buf, size_t buflen)
> > && is_mytty (&st, &st1))
> > return 0;
> >
> > - /* If the link doesn't exist, then it points to a device in another
> > - * namespace.
> > - */
> > - if (is_pty (&st))
> > - {
> > - __set_errno (ENODEV);
> > - return ENODEV;
> > - }
> > + doispty = 1;
> > }
> >
> > /* Prepare the result buffer. */
> > @@ -190,6 +184,16 @@ __ttyname_r (int fd, char *buf, size_t buflen)
> > save, &dostat);
> > }
> >
> > + if (ret && doispty && is_pty (&st))
> > + {
> > + /* We failed to figure out the TTY's name, but we can at least
> > + * signal that we did verify that it really is a PTY slave.
> > + * This happens when we have inherited the file descriptor from
> > + * a different mount namespace. */
> > + __set_errno (ENODEV);
> > + return ENODEV;
> > + }
> > +
> > return ret;
> > }
> >
>
> The change looks OK.
>
> Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>
>
>
> --
> ldv