| Message ID | 20171012035321.22094-5-lukeshu@parabola.nu |
|---|---|
| State | New |
| Headers | show |
| Series | Fixup linux ttyname and ttyname_r [BZ #22145] | expand |
On Wed, Oct 11, 2017 at 11:53:20PM -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. > 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. > --- > ChangeLog | 5 +++++ > sysdeps/unix/sysv/linux/ttyname.c | 40 ++++++++---------------------------- > sysdeps/unix/sysv/linux/ttyname.h | 12 +++++++++++ > sysdeps/unix/sysv/linux/ttyname_r.c | 41 ++++++++----------------------------- > 4 files changed, 34 insertions(+), 64 deletions(-) > > diff --git a/ChangeLog b/ChangeLog > index 1921f8883b..5973b9d50b 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,5 +1,10 @@ > 2017-10-11 Luke Shumaker <lukeshu@parabola.nu> > > + [BZ #22145] > + * sysdeps/unix/sysv/linux/ttyname.h (is_mytty): New function. > + * sysdeps/unix/sysv/linux/ttyname.c: Call it. > + * sysdeps/unix/sysv/linux/ttyname_r.c: Likewise. > + > [BZ #22145] > * sysdeps/unix/sysv/linux/tst-ttyname.c: New file. > * sysdeps/unix/sysv/linux/Makefile: Add tst-ttyname to tests. > diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c > index 116cf350e5..138a8a57f8 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, 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, 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..15f7b066b6 100644 > --- a/sysdeps/unix/sysv/linux/ttyname.h > +++ b/sysdeps/unix/sysv/linux/ttyname.h > @@ -33,3 +33,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; > +} I find that hard to read. At first I thought this would unconditionally return 1. I'd rather prefer something like (In the appropriate GNU coding style eventually of course.): static inline bool is_mytty(struct stat64 *mytty, struct stat64 *maybe) { if (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 ) return true; return false; } But others might be fine with it. But I think having it return a bool here is perfectly fine since there's no other possible outcome than true or false. diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c index 18f35ef2b7..d975d95d0d 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, + 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, 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;
On Thu, 12 Oct 2017 07:16:15 -0400, Christian Brauner wrote: > On Wed, Oct 11, 2017 at 11:53:20PM -0400, Luke Shumaker wrote: > > +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; > > +} > > I find that hard to read. At first I thought this would unconditionally return > 1. I'd rather prefer something like (In the appropriate GNU coding style > eventually of course.): > > static inline bool is_mytty(struct stat64 *mytty, struct stat64 *maybe) > { > if (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 > ) > return true; > > return false; > } > > But others might be fine with it. I'm fine either way. If the consensus is reformat it, I'll reformat it. > But I think having it return a bool here is > perfectly fine since there's no other possible outcome than true or false. So why doesn't is_pty return a bool? I was mimicing the style already in the file.
On Thu, Oct 12, 2017 at 08:18:50AM -0400, Luke Shumaker wrote: > On Thu, 12 Oct 2017 07:16:15 -0400, > Christian Brauner wrote: > > On Wed, Oct 11, 2017 at 11:53:20PM -0400, Luke Shumaker wrote: > > > +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; > > > +} > > > > I find that hard to read. At first I thought this would unconditionally return > > 1. I'd rather prefer something like (In the appropriate GNU coding style > > eventually of course.): > > > > static inline bool is_mytty(struct stat64 *mytty, struct stat64 *maybe) > > { > > if (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 > > ) > > return true; > > > > return false; > > } > > > > But others might be fine with it. > > I'm fine either way. If the consensus is reformat it, I'll reformat > it. > > > But I think having it return a bool here is > > perfectly fine since there's no other possible outcome than true or false. > > So why doesn't is_pty return a bool? I was mimicing the style already > in the file. Likely because it was overseen as well. I think it makes more sense to have them return proper bools when they can't return anything else. Seems fine to me since they're used all over the codebase as well. > > -- > Happy hacking, > ~ Luke Shumaker
On Wed, Oct 11, 2017 at 11:53:20PM -0400, Luke Shumaker wrote: [...] > --- 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, struct stat64 *mytty, > + int save, int *dostat); This has to be "const struct stat64 *". > > 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, struct stat64 *mytty, int save, int *dostat) Likewise. [...] > --- a/sysdeps/unix/sysv/linux/ttyname.h > +++ b/sysdeps/unix/sysv/linux/ttyname.h > @@ -33,3 +33,15 @@ is_pty (struct stat64 *sb) > return false; > #endif > } > + > +static inline int > +is_mytty(struct stat64 *mytty, struct stat64 *maybe) > +{ Likewise. [...] > diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c > index 18f35ef2b7..d975d95d0d 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, > + struct stat64 *mytty, int save, > int *dostat); Likewise. > > 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, struct stat64 *mytty, > int save, int *dostat) > { Likewise.
diff --git a/ChangeLog b/ChangeLog index 1921f8883b..5973b9d50b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ 2017-10-11 Luke Shumaker <lukeshu@parabola.nu> + [BZ #22145] + * sysdeps/unix/sysv/linux/ttyname.h (is_mytty): New function. + * sysdeps/unix/sysv/linux/ttyname.c: Call it. + * sysdeps/unix/sysv/linux/ttyname_r.c: Likewise. + [BZ #22145] * sysdeps/unix/sysv/linux/tst-ttyname.c: New file. * sysdeps/unix/sysv/linux/Makefile: Add tst-ttyname to tests. diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c index 116cf350e5..138a8a57f8 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, 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, 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..15f7b066b6 100644 --- a/sysdeps/unix/sysv/linux/ttyname.h +++ b/sysdeps/unix/sysv/linux/ttyname.h @@ -33,3 +33,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 18f35ef2b7..d975d95d0d 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, + 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, 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;
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. 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. --- ChangeLog | 5 +++++ sysdeps/unix/sysv/linux/ttyname.c | 40 ++++++++---------------------------- sysdeps/unix/sysv/linux/ttyname.h | 12 +++++++++++ sysdeps/unix/sysv/linux/ttyname_r.c | 41 ++++++++----------------------------- 4 files changed, 34 insertions(+), 64 deletions(-)