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

[Christian Brauner <christian dot brauner at mailbox dot org>]

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
>