From patchwork Thu Nov 2 18:53:44 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luke Shumaker X-Patchwork-Id: 24044 Received: (qmail 82140 invoked by alias); 2 Nov 2017 18:53:52 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 82088 invoked by uid 89); 2 Nov 2017 18:53:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=H*F:D*nu, H*r:202, H*r:803, 3112 X-HELO: mav.lukeshu.com From: Luke Shumaker To: libc-alpha@sourceware.org Cc: christian.brauner@mailbox.org Subject: [PATCH v2 3/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145] Date: Thu, 2 Nov 2017 14:53:44 -0400 Message-Id: <20171102185346.1386-4-lukeshu@parabola.nu> In-Reply-To: <20171102185346.1386-1-lukeshu@parabola.nu> References: <20171102185346.1386-1-lukeshu@parabola.nu> 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 , 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. 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 . 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 | [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 Reviewed-By: Christian Brauner --- 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 + [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. + * sysdeps/unix/sysv/linux/ttyname_r.c: Likewise. + * 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;