Re: [PATCH v2 3/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

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

On Thu, Nov 02, 2017 at 02:53:44PM -0400, Luke Shumaker wrote:
> In the ttyname and ttyname_r routines on Linux, at several points it
> it needs to test if a given TTY is the TTY we are looking for.  It used to
> be that this test was (to see if `maybe` is `mytty`):
> 
>        __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
>     #ifdef _STATBUF_ST_RDEV
>        && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
>     #else
>        && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev
>     #endif
> 
> This check appears in several places.
> 
> Then, in commit 15e9a4f3 by Christian Brauner
> <christian.brauner@canonical.com>, that check changed to:
> 
>        __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
>     #ifdef _STATBUF_ST_RDEV
>        && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
>     #endif
>        && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev
> 
> That is, he made st_ino and st_dev checks happen even if we have the
> st_rdev member.  This is a good change, because kernel namespaces allow you
> to create a new namespace, and to create a new device with the same
> st_rdev, but isn't the same file.
> 
> However, this check appears twice in each file (ttyname.c and
> ttyname_r.c), but Christian only changed it in one place in each file.

Firstly, the way BZ #22145 is mentioned in the subject line creates
a false impression that this commit fixes the bug.

Secondly, the text describes commit 15e9a4f3 in fine details
(imo it goes too far) yet fails to explain the difference between the case
addressed by 15e9a4f3 (ttyname and __ttyname_r) and the other case
(getttyname and getttyname_r).

> Make it easy to keep consistent by pulling the check in to a static inline
> function, is_mytty.
> 
> While we're at it, go ahead and have the existing is_pty return a bool
> (instead of an int) to keep ttyname.h consistent, per
> <https://sourceware.org/ml/libc-alpha/2017-10/msg00532.html>.
> 
>   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 |
>   |                                 | 15e9a4f | 15e9a4f | consistent |
>   |---------------------------------+---------+---------+------------|
>   | basic smoketest                 | PASS    | PASS    | PASS       |
>   | no conflict, no match           | PASS[1] | PASS    | PASS       |
>   | no conflict, console            | PASS    | FAIL!   | FAIL       |
>   | conflict, no match              | FAIL    | PASS!   | PASS       |
>   | conflict, console               | FAIL    | FAIL    | FAIL       |
>   | with readlink target            | PASS    | PASS    | PASS       |
>   | with readlink trap; fallback    | FAIL    | FAIL    | FAIL       |
>   | with readlink trap; no fallback | FAIL    | PASS!   | PASS       |
>   | with search-path trap           | FAIL    | FAIL    | PASS!      |
>   |---------------------------------+---------+---------+------------|
>   |                                 | 4/9     | 5/9     | 6/9        |

Wouldn't it be sufficient 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?

> 
>   [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.
> 
>   This only fixed one testcase, but most of the remaining failures
>   require both this fix and the subsequent fix in order to pass.
> 
> v2:
>  - Rearranged commit order
>  - Expanded commit message
>  - Reformatted is_mytty
>  - Made is_pty and is_mytty return bool instead of int
>  - Specify 'const' on pointer arguments as necessary
> ---
>  ChangeLog                           |  7 +++++++
>  sysdeps/unix/sysv/linux/ttyname.c   | 40 ++++++++----------------------------
>  sysdeps/unix/sysv/linux/ttyname.h   | 14 ++++++++++++-
>  sysdeps/unix/sysv/linux/ttyname_r.c | 41 ++++++++-----------------------------
>  4 files changed, 37 insertions(+), 65 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 2c7770fbbb..4c282f8f47 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,12 @@
>  2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
>  
> +	[BZ #22145]
> +	* sysdeps/unix/sysv/linux/ttyname.h
> +	(is_pty): Change return type from int to bool.
> +	(is_mytty): New function.
> +	* sysdeps/unix/sysv/linux/ttyname.c: Call is_mytty.

This should mention all affected functions: getttyname and ttyname.

> +	* sysdeps/unix/sysv/linux/ttyname_r.c: Likewise.

Likewise, this should mention all affected functions: getttyname_r
and __ttyname_r.

> +
>  	* sysdeps/unix/sysv/linux/ttyname.h (is_pty): Update doc reference.
>  
>  	* manual/terminal.texi (Is It a Terminal):
> diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
> index 116cf350e5..6e97d2d455 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
> @@ -35,14 +35,14 @@
>  char *__ttyname;
>  #endif
>  
> -static char *getttyname (const char *dev, dev_t mydev,
> -			 ino64_t myino, int save, int *dostat);
> +static char *getttyname (const char *dev, const struct stat64 *mytty,
> +			 int save, int *dostat);
>  
>  libc_freeres_ptr (static char *getttyname_name);
>  
>  static char *
>  attribute_compat_text_section
> -getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
> +getttyname (const char *dev, const struct stat64 *mytty, int save, int *dostat)
>  {
>    static size_t namelen;
>    struct stat64 st;
> @@ -63,7 +63,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
>      *((char *) __mempcpy (getttyname_name, dev, devlen - 1)) = '/';
>  
>    while ((d = __readdir64 (dirstream)) != NULL)
> -    if ((d->d_fileno == myino || *dostat)
> +    if ((d->d_fileno == mytty->st_ino || *dostat)
>  	&& strcmp (d->d_name, "stdin")
>  	&& strcmp (d->d_name, "stdout")
>  	&& strcmp (d->d_name, "stderr"))
> @@ -85,12 +85,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
>  	  }
>  	memcpy (&getttyname_name[devlen], d->d_name, dlen);
>  	if (__xstat64 (_STAT_VER, getttyname_name, &st) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
> -#else
> -	    && d->d_fileno == myino && st.st_dev == mydev
> -#endif
> -	   )
> +	    && is_mytty (mytty, &st))
>  	  {
>  	    (void) __closedir (dirstream);
>  #if 0
> @@ -167,12 +162,7 @@ ttyname (int fd)
>        /* Verify readlink result, fall back on iterating through devices.  */
>        if (ttyname_buf[0] == '/'
>  	  && __xstat64 (_STAT_VER, ttyname_buf, &st1) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	  && S_ISCHR (st1.st_mode)
> -	  && st1.st_rdev == st.st_rdev
> -#endif
> -	  && st1.st_ino == st.st_ino
> -	  && st1.st_dev == st.st_dev)
> +	  && is_mytty (&st, &st1))
>  	return ttyname_buf;
>  
>        /* If the link doesn't exist, then it points to a device in another
> @@ -186,11 +176,7 @@ ttyname (int fd)
>  
>    if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
>      {
> -#ifdef _STATBUF_ST_RDEV
> -      name = getttyname ("/dev/pts", st.st_rdev, st.st_ino, save, &dostat);
> -#else
> -      name = getttyname ("/dev/pts", st.st_dev, st.st_ino, save, &dostat);
> -#endif
> +      name = getttyname ("/dev/pts", &st, save, &dostat);
>      }
>    else
>      {
> @@ -200,21 +186,13 @@ ttyname (int fd)
>  
>    if (!name && dostat != -1)
>      {
> -#ifdef _STATBUF_ST_RDEV
> -      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
> -#else
> -      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
> -#endif
> +      name = getttyname ("/dev", &st, save, &dostat);
>      }
>  
>    if (!name && dostat != -1)
>      {
>        dostat = 1;
> -#ifdef _STATBUF_ST_RDEV
> -      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
> -#else
> -      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
> -#endif
> +      name = getttyname ("/dev", &st, save, &dostat);
>      }
>  
>    return name;
> diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
> index dd7873d1ff..2b125cfd0b 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.h
> +++ b/sysdeps/unix/sysv/linux/ttyname.h
> @@ -23,7 +23,7 @@
>  /* Return true if this is a UNIX98 pty device, as defined in
>     linux/Documentation/devices.txt (on linux < 4.10) or
>     linux/Documentation/admin-guide/devices.txt (on linux >= 4.10). */
> -static inline int
> +static inline bool
>  is_pty (struct stat64 *sb)
>  {
>  #ifdef _STATBUF_ST_RDEV
> @@ -33,3 +33,15 @@ is_pty (struct stat64 *sb)
>    return false;
>  #endif
>  }
> +
> +static inline bool
> +is_mytty(const struct stat64 *mytty, const struct stat64 *maybe)
> +{
> +  return (maybe->st_ino == mytty->st_ino
> +	  && maybe->st_dev == mytty->st_dev
> +#ifdef _STATBUF_ST_RDEV
> +	  && S_ISCHR (maybe->st_mode)
> +	  && maybe->st_rdev == mytty->st_rdev
> +#endif
> +	  );
> +}
> diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
> index 18f35ef2b7..58eb919c3f 100644
> --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> @@ -31,12 +31,12 @@
>  #include "ttyname.h"
>  
>  static int getttyname_r (char *buf, size_t buflen,
> -			 dev_t mydev, ino64_t myino, int save,
> +			 const struct stat64 *mytty, int save,
>  			 int *dostat);
>  
>  static int
>  attribute_compat_text_section
> -getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
> +getttyname_r (char *buf, size_t buflen, const struct stat64 *mytty,
>  	      int save, int *dostat)
>  {
>    struct stat64 st;
> @@ -52,7 +52,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
>      }
>  
>    while ((d = __readdir64 (dirstream)) != NULL)
> -    if ((d->d_fileno == myino || *dostat)
> +    if ((d->d_fileno == mytty->st_ino || *dostat)
>  	&& strcmp (d->d_name, "stdin")
>  	&& strcmp (d->d_name, "stdout")
>  	&& strcmp (d->d_name, "stderr"))
> @@ -72,12 +72,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
>  	cp[0] = '\0';
>  
>  	if (__xstat64 (_STAT_VER, buf, &st) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
> -#else
> -	    && d->d_fileno == myino && st.st_dev == mydev
> -#endif
> -	   )
> +	    && is_mytty (mytty, &st))
>  	  {
>  	    (void) __closedir (dirstream);
>  	    __set_errno (save);
> @@ -151,12 +146,7 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>        /* Verify readlink result, fall back on iterating through devices.  */
>        if (buf[0] == '/'
>  	  && __xstat64 (_STAT_VER, buf, &st1) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	  && S_ISCHR (st1.st_mode)
> -	  && st1.st_rdev == st.st_rdev
> -#endif
> -	  && st1.st_ino == st.st_ino
> -	  && st1.st_dev == st.st_dev)
> +	  && is_mytty (&st, &st1))
>  	return 0;
>  
>        /* If the link doesn't exist, then it points to a device in another
> @@ -175,13 +165,8 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>  
>    if (__xstat64 (_STAT_VER, buf, &st1) == 0 && S_ISDIR (st1.st_mode))
>      {
> -#ifdef _STATBUF_ST_RDEV
> -      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
> +      ret = getttyname_r (buf, buflen, &st, save,
>  			  &dostat);
> -#else
> -      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
> -			  &dostat);
> -#endif
>      }
>    else
>      {
> @@ -193,26 +178,16 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>      {
>        buf[sizeof ("/dev/") - 1] = '\0';
>        buflen += sizeof ("pts/") - 1;
> -#ifdef _STATBUF_ST_RDEV
> -      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
> -			  &dostat);
> -#else
> -      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
> +      ret = getttyname_r (buf, buflen, &st, save,
>  			  &dostat);
> -#endif
>      }
>  
>    if (ret && dostat != -1)
>      {
>        buf[sizeof ("/dev/") - 1] = '\0';
>        dostat = 1;
> -#ifdef _STATBUF_ST_RDEV
> -      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino,
> -			  save, &dostat);
> -#else
> -      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino,
> +      ret = getttyname_r (buf, buflen, &st,
>  			  save, &dostat);
> -#endif
>      }
>  
>    return ret;

The change itself looks OK, but as it essentially combines two changes
(the first one is refactoring that is almost no-op, the second one is
a fix by using is_mytty in getttyname and getttyname_r),  I suggest
to split the commit in two.


-- 
ldv

Attachment: signature.asc
Description: PGP signature