This is the mail archive of the
glibc-bugs@sourceware.org
mailing list for the glibc project.
[Bug libc/22145] ttyname() gives up too early in the face of namespaces
- From: "christian.brauner at mailbox dot org" <sourceware-bugzilla at sourceware dot org>
- To: glibc-bugs at sourceware dot org
- Date: Tue, 10 Oct 2017 09:30:03 +0000
- Subject: [Bug libc/22145] ttyname() gives up too early in the face of namespaces
- Auto-submitted: auto-generated
- References: <bug-22145-131@http.sourceware.org/bugzilla/>
https://sourceware.org/bugzilla/show_bug.cgi?id=22145
--- Comment #13 from Christian Brauner <christian.brauner at mailbox dot org> ---
Comment on attachment 10415
--> https://sourceware.org/bugzilla/attachment.cgi?id=10415
proposed fix part 1
>From 1d8113b396f145ea8fea27c2586b10cc55526be8 Mon Sep 17 00:00:00 2001
>From: Luke Shumaker <lukeshu@lukeshu.com>
>Date: Sun, 17 Sep 2017 01:55:15 -0400
>Subject: [PATCH 1/2] linux ttyname() and ttyname_r(): Make the TTY equivalence
> tests consistent
>
>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
>
>Now, this check appears a couple of places.
>
>Then Christian Brauner came along with commit 15e9a4f3, and changed the
>check 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. Now that kind-of
>made sense. The common use-case for this is that you're in a chroot, and
>have inherited a file descriptor to a TTY outside of the chroot. In the
>ttyname() routine, the first check is on the passed-in file descriptor,
>while the remaining checks are for files we open by their absolute path; so
>we'd expect them to be in the new namespace. But that's just the "normal"
>situation, users can do all kinds of funny things, so update the check
>everywhere.
>
>Make it easy to keep consistent by pulling the check in to a static inline
>function.
>---
> sysdeps/unix/sysv/linux/ttyname.c | 41 +++++++++-------------------------
> sysdeps/unix/sysv/linux/ttyname.h | 12 ++++++++++
> sysdeps/unix/sysv/linux/ttyname_r.c | 44 +++++++++----------------------------
> 3 files changed, 32 insertions(+), 65 deletions(-)
>
>diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
>index 5909cb765f..ba046205ae 100644
>--- a/sysdeps/unix/sysv/linux/ttyname.c
>+++ b/sysdeps/unix/sysv/linux/ttyname.c
>@@ -35,8 +35,9 @@
> 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,
>+ struct stat64 *mytty,
>+ int save, int *dostat)
> internal_function;
>
>
>@@ -44,7 +45,7 @@ libc_freeres_ptr (static char *getttyname_name);
>
> static char *
> internal_function attribute_compat_text_section
>-getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
>+getttyname (const char *dev, struct stat64 *mytty, int save, int *dostat)
> {
> static size_t namelen;
> struct stat64 st;
>@@ -65,7 +66,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"))
>@@ -87,12 +88,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
>@@ -169,12 +165,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
>@@ -188,11 +179,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
> {
>@@ -202,21 +189,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 2e415e4e9c..cc911baeb0 100644
>--- a/sysdeps/unix/sysv/linux/ttyname.h
>+++ b/sysdeps/unix/sysv/linux/ttyname.h
>@@ -32,3 +32,15 @@ is_pty (struct stat64 *sb)
> return false;
> #endif
> }
>+
>+static inline int
>+is_mytty(struct stat64 *mytty, struct stat64 *maybe)
>+{
>+ return 1
>+#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;
>+}
>diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
>index dc863526ba..8ff8653a2d 100644
>--- a/sysdeps/unix/sysv/linux/ttyname_r.c
>+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
>@@ -31,12 +31,13 @@
> #include "ttyname.h"
>
> static int getttyname_r (char *buf, size_t buflen,
>- dev_t mydev, ino64_t myino, int save,
>- int *dostat) internal_function;
>+ struct stat64 *mytty,
>+ int save, int *dostat)
>+ internal_function;
>
> static int
> internal_function attribute_compat_text_section
>-getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
>+getttyname_r (char *buf, size_t buflen, struct stat64 *mytty,
> int save, int *dostat)
> {
> struct stat64 st;
>@@ -52,7 +53,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 +73,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 +147,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));
This semicolon will cause the following "return 0" to be executed
unconditionally. That's very much not what you want I suspect.
> return 0;
>
> /* If the link doesn't exist, then it points to a device in another
>@@ -175,13 +166,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 +179,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;
>--
>2.14.1
>
--
You are receiving this mail because:
You are on the CC list for the bug.