Message ID | 20171012035321.22094-6-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:21PM -0400, Luke Shumaker wrote: > 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. > > 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. > > A common scenario where this happens is with "/dev/console" in > containers. Common container managers (including systemd-nspawn) will > 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 > 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, 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. Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> --- 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 5973b9d50b..28f31d345b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ 2017-10-11 Luke Shumaker <lukeshu@parabola.nu> + [BZ #22145] + * sysdeps/unix/sysv/linux/ttyname.c (ttyname): + Defer is_pty check until end. + * sysdeps/unix/sysv/linux/ttyname_r.c (ttyname_r): Likewise. + [BZ #22145] * sysdeps/unix/sysv/linux/ttyname.h (is_mytty): New function. * sysdeps/unix/sysv/linux/ttyname.c: Call it. diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c index 138a8a57f8..ebd916f68e 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 d975d95d0d..adcffacb2c 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; }
Hey Luke, Just a quick ping about the ttyname{_r}() patch. Do you have any time-frame on when you'd be able to send an updated version of the patch and do you have signed an FSF agreement yet? There are only ~2 months left before the Glibc 2.27 development freeze and I'd like to see this regression fixed in 2.27. :) Otherwise I wouldn't nag you. :) Thanks! Christian On Thu, Oct 12, 2017 at 01:35:45PM +0200, Christian Brauner wrote: > On Wed, Oct 11, 2017 at 11:53:21PM -0400, Luke Shumaker wrote: > > 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. > > > > 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. > > > > A common scenario where this happens is with "/dev/console" in > > containers. Common container managers (including systemd-nspawn) will > > 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 > > 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, 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. > > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> > > --- > 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 5973b9d50b..28f31d345b 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,5 +1,10 @@ > 2017-10-11 Luke Shumaker <lukeshu@parabola.nu> > > + [BZ #22145] > + * sysdeps/unix/sysv/linux/ttyname.c (ttyname): > + Defer is_pty check until end. > + * sysdeps/unix/sysv/linux/ttyname_r.c (ttyname_r): Likewise. > + > [BZ #22145] > * sysdeps/unix/sysv/linux/ttyname.h (is_mytty): New function. > * sysdeps/unix/sysv/linux/ttyname.c: Call it. > diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c > index 138a8a57f8..ebd916f68e 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 d975d95d0d..adcffacb2c 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; > } > > -- > 2.14.2
On Wed, 01 Nov 2017 11:51:24 -0400, Christian Brauner wrote: > Hey Luke, > > Just a quick ping about the ttyname{_r}() patch. > Do you have any time-frame on when you'd be able to send an updated version of > the patch and do you have signed an FSF agreement yet? There are only ~2 months > left before the Glibc 2.27 development freeze and I'd like to see this > regression fixed in 2.27. :) Otherwise I wouldn't nag you. :) > > Thanks! > Christian I'm not sure if you missed it, but I submitted v2 of this patchset on Thursday (I did CC you on it). Thanks for the nagging :)
diff --git a/ChangeLog b/ChangeLog index 5973b9d50b..28f31d345b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ 2017-10-11 Luke Shumaker <lukeshu@parabola.nu> + [BZ #22145] + * sysdeps/unix/sysv/linux/ttyname.c (ttyname): + Defer is_pty check until end. + * sysdeps/unix/sysv/linux/ttyname_r.c (ttyname_r): Likewise. + [BZ #22145] * sysdeps/unix/sysv/linux/ttyname.h (is_mytty): New function. * sysdeps/unix/sysv/linux/ttyname.c: Call it. diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c index 138a8a57f8..ebd916f68e 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 d975d95d0d..adcffacb2c 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; }