[v2,4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

  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.

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.

A common scenario where this happens is with "/dev/console" in
containers.  Common container managers (including systemd-nspawn) will
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
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, 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.

  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.

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(-)
  

On Thu, Nov 02, 2017 at 02:53:45PM -0400, Luke Shumaker wrote:
> 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.
> 
> 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.
> 
> A common scenario where this happens is with "/dev/console" in
> containers.  Common container managers (including systemd-nspawn) will
> 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
> 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, 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.
> 
>   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.
> 
> 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(-)

Same, as before minor improvements to the commit messages code itself looks fine
to me.

Reviewed-By: Christian Brauner <christian.brauner@ubuntu.com>

> 
> 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.
> +	* 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;
>  }
>  
> -- 
> 2.15.0
>
  

On Mon, 06 Nov 2017 08:30:24 -0500,
Christian Brauner wrote:
> Same, as before minor improvements to the commit messages code itself looks fine
> to me.

What improvements do you want to make?  I'd be happy to go ahead and
make them and submit a v3 today with revised commit messages.
  

On Tue, Nov 07, 2017 at 11:00:37AM -0500, Luke Shumaker wrote:
> On Mon, 06 Nov 2017 08:30:24 -0500,
> Christian Brauner wrote:
> > Same, as before minor improvements to the commit messages code itself looks fine
> > to me.
> 
> What improvements do you want to make?  I'd be happy to go ahead and
> make them and submit a v3 today with revised commit messages.

It's just spelling mistakes or double words in the commit message. It's really
not worth resending for this imho.
  

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?

> 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>
  

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
  

On Nov 02 2017, Luke Shumaker <lukeshu@parabola.nu> wrote:

> @@ -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. */

Style: no '*', two spaces after period.

> @@ -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. */

Likewise.

Andreas.