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

["Dmitry V. Levin" <ldv at altlinux dot org>]

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>


-- 
ldv

Attachment: signature.asc
Description: PGP signature