Commit 15e9a4f (which was the fix for #12167) introduced returning ENODEV for when we can't determine the TTY's name because we inherited it from a different mount namespace. However, just because the TTY is from a different namespace, and we can no longer reach it at the original name doesn't mean its unreachable. It shouldn't stop the old fallback iteration code from running; that fallback might very well find the name. For instance, systemd-nspawn arranges for the TTY to be at /dev/console, even though it's original /dev/pts/X location won't be the same TTY. With glibc 2.25 this worked fine, but with 15e9a4f applied in 2.26, not so much: $ sudo systemd-nspawn --register=no -D /path/to/my-container /bin/bash Spawning container my-container on /path/to/my-container. Press ^] three times within 1s to kill container. bash: cannot set terminal process group (-1): Inappropriate ioctl for device bash: no job control in this shell [root@my-container /]# This is because the TTY, originally /dev/pts/${X}, is only available at /dev/console in the new mount namespace. Bash gets the TTY on FD 0, and calls ttyname(STDIN_FILENO). Before 15e9a4f, this correctly returned "/dev/console", but now it returns NULL and sets errno=ENODEV.
Created attachment 10415 [details] proposed fix part 1
Created attachment 10416 [details] proposed fix part 2 I've attached a pair of patches that I believe fixes this. In the morning I'll test it (it's building now), and look in to including this in the test suite. I guess I'll have to add a ChangeLog entry, and update my FSF assignment paperwork to include libc.
Am I correct in my assessment that the test suite has no way to add tests that require root access?
On IRC, djdelorie made me aware of support_become_root(). I have verified that the patches work. I'll get around to writing testcases eventually.
Hi, I'm one of the authors of the original ttyname{_r}() patch you referred to. Florian just made me aware of this bug report. Would've been cool if you could've CCed us before. :) (In reply to Luke Shumaker from comment #0) > Commit 15e9a4f (which was the fix for #12167) introduced returning ENODEV > for when we can't determine the TTY's name because we inherited it from a > different mount namespace. However, just because the TTY is from a > different namespace, and we can no longer reach it at the original name > doesn't mean its unreachable. It shouldn't stop the old fallback iteration > code from running; that fallback might very well find the name. > > For instance, systemd-nspawn arranges for the TTY to be at /dev/console, > even though it's original /dev/pts/X location won't be the same TTY. With > glibc 2.25 this worked fine, but with 15e9a4f applied in 2.26, not so much: > > $ sudo systemd-nspawn --register=no -D /path/to/my-container /bin/bash > Spawning container my-container on /path/to/my-container. > Press ^] three times within 1s to kill container. > bash: cannot set terminal process group (-1): Inappropriate ioctl for > device > bash: no job control in this shell > [root@my-container /]# I'm a little confused. We do the same in liblxc namely, we bind-mount a /dev/pty/<idx> path from the parent's devpts mount in the parent's mount namespace over /dev/console. And things work just fine meaning: chb@conventiont|~ > lxc-execute -n a5 -- bash bash: cannot set terminal process group (1): Inappropriate ioctl for device bash: no job control in this shell root@a5:/# tty /dev/console root@a5:/# findmnt | grep console | `-/dev/console devpts[/9] devpts rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000 root@a5:/# ls -al /dev/pts/ total 0 drwxr-xr-x 2 root root 0 Oct 6 09:18 . drwxr-xr-x 6 root root 360 Oct 6 09:18 .. crw--w---- 1 root tty 136, 0 Oct 6 09:18 0 crw--w---- 1 root tty 136, 1 Oct 6 09:18 1 crw--w---- 1 root tty 136, 2 Oct 6 09:18 2 crw--w---- 1 root tty 136, 3 Oct 6 09:18 3 crw-rw-rw- 1 root root 5, 2 Oct 6 09:18 ptmx you can clearly see that the tty coreutil reports /dev/console and exits 0 so it correctly detects that the program is attached to a pty and reports the correct path even thought bind-mounted /dev/pts/<idx> (where <idx> == 9 in this case) does not exist in the current devpts mount's mount namespace. Internally, tty calls ttyname and isatty so there doesn't seem to be a regression here. Also, I'm not sure how your patch would help with bash. What your patch does is have ttyname() set ENODEV or have ttyname_r() return ENODEV but in both cases no tty name will be returned. So this presupposes that any program that calls ttyname{_r}() recognizes that ENODEV means "is a tty but no path was found" which is not the case for bash so I'm confused how that would help you get around this problem. Maybe you can explain this in a little more detail I might just be dense. > > This is because the TTY, originally /dev/pts/${X}, is only available at > /dev/console in the new mount namespace. Bash gets the TTY on FD 0, and > calls ttyname(STDIN_FILENO). Before 15e9a4f, this correctly returned > "/dev/console", but now it returns NULL and sets errno=ENODEV. I don't completely understand this analysis as well. It seems to me that the error comes from setting the process group leader and not from ttyname{_r}() failures in the bash code. I think the codepath responsible is: /* If (and only if) we just set our process group to our pid, thereby becoming a process group leader, and the terminal is not in the same process group as our (new) process group, then set the terminal's process group to our (new) process group. If that fails, set our process group back to what it was originally (so we can still read from the terminal) and turn off job control. */ if (shell_pgrp != original_pgrp && shell_pgrp != terminal_pgrp) { if (give_terminal_to (shell_pgrp, 0) < 0) { t_errno = errno; setpgid (0, original_pgrp); shell_pgrp = original_pgrp; errno = t_errno; sys_error (_("cannot set terminal process group (%d)"), shell_pgrp); job_control = 0; } }
(In reply to Christian Brauner from comment #5) > Hi, > > I'm one of the authors of the original ttyname{_r}() patch you referred to. > Florian just made me aware of this bug report. Would've been cool if you > could've CCed us before. :) > > (In reply to Luke Shumaker from comment #0) > > Commit 15e9a4f (which was the fix for #12167) introduced returning ENODEV > > for when we can't determine the TTY's name because we inherited it from a > > different mount namespace. However, just because the TTY is from a > > different namespace, and we can no longer reach it at the original name > > doesn't mean its unreachable. It shouldn't stop the old fallback iteration > > code from running; that fallback might very well find the name. > > > > For instance, systemd-nspawn arranges for the TTY to be at /dev/console, > > even though it's original /dev/pts/X location won't be the same TTY. With > > glibc 2.25 this worked fine, but with 15e9a4f applied in 2.26, not so much: > > > > $ sudo systemd-nspawn --register=no -D /path/to/my-container /bin/bash > > Spawning container my-container on /path/to/my-container. > > Press ^] three times within 1s to kill container. > > bash: cannot set terminal process group (-1): Inappropriate ioctl for > > device > > bash: no job control in this shell > > [root@my-container /]# > > I'm a little confused. We do the same in liblxc namely, we bind-mount a > /dev/pty/<idx> path from the parent's devpts mount in the parent's mount > namespace over /dev/console. And things work just fine meaning: > > > chb@conventiont|~ > > lxc-execute -n a5 -- bash > bash: cannot set terminal process group (1): Inappropriate ioctl for device > bash: no job control in this shell > > root@a5:/# tty > /dev/console > > root@a5:/# findmnt | grep console > | `-/dev/console devpts[/9] > devpts rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000 > > root@a5:/# ls -al /dev/pts/ > total 0 > drwxr-xr-x 2 root root 0 Oct 6 09:18 . > drwxr-xr-x 6 root root 360 Oct 6 09:18 .. > crw--w---- 1 root tty 136, 0 Oct 6 09:18 0 > crw--w---- 1 root tty 136, 1 Oct 6 09:18 1 > crw--w---- 1 root tty 136, 2 Oct 6 09:18 2 > crw--w---- 1 root tty 136, 3 Oct 6 09:18 3 > crw-rw-rw- 1 root root 5, 2 Oct 6 09:18 ptmx > > you can clearly see that the tty coreutil reports /dev/console and exits 0 > so it correctly detects that the program is attached to a pty and reports > the correct path even thought bind-mounted /dev/pts/<idx> (where <idx> == 9 > in this case) does not exist in the current devpts mount's mount namespace. > Internally, tty calls ttyname and isatty so there doesn't seem to be a > regression here. > > Also, I'm not sure how your patch would help with bash. What your patch does > is have ttyname() set ENODEV or have ttyname_r() return ENODEV but in both > cases no tty name will be returned. So this presupposes that any program > that calls ttyname{_r}() recognizes that ENODEV means "is a tty but no path > was found" which is not the case for bash so I'm confused how that would > help you get around this problem. Maybe you can explain this in a little > more detail I might just be dense. The fact that you got working job control with prior versions of glibc was caused by an actual bug: Namely that ttyname{_r}() did not detect that /dev/console indeed did a) refer to pty b) the path for the pty exists in that namespace. Serge's and my patch fixed this by having ttyname{_r}() detect that /dev/console indeed refers to a pty and reporting correctly. (Unless I'm misunderstanding something which is entirely possible.). So bash tries to initialize job control but fails so this is more likely a bash bug and should be handled there. > > > > > This is because the TTY, originally /dev/pts/${X}, is only available at > > /dev/console in the new mount namespace. Bash gets the TTY on FD 0, and > > calls ttyname(STDIN_FILENO). Before 15e9a4f, this correctly returned > > "/dev/console", but now it returns NULL and sets errno=ENODEV. > > I don't completely understand this analysis as well. It seems to me that the > error comes from setting the process group leader and not from ttyname{_r}() > failures in the bash code. I think the codepath responsible is: > > > /* If (and only if) we just set our process group to our pid, > thereby becoming a process group leader, and the terminal > is not in the same process group as our (new) process group, > then set the terminal's process group to our (new) process > group. If that fails, set our process group back to what it > was originally (so we can still read from the terminal) and > turn off job control. */ > if (shell_pgrp != original_pgrp && shell_pgrp != terminal_pgrp) > { > if (give_terminal_to (shell_pgrp, 0) < 0) > { > t_errno = errno; > setpgid (0, original_pgrp); > shell_pgrp = original_pgrp; > errno = t_errno; > sys_error (_("cannot set terminal process group (%d)"), shell_pgrp); > job_control = 0; > } > }
Can someone please forward this to bug-bash@?
(In reply to Andreas Schwab from comment #7) > Can someone please forward this to bug-bash@? Sent a mail to bug-bash@ and CCed you and Florian.
(In reply to Christian Brauner from comment #8) > (In reply to Andreas Schwab from comment #7) > > Can someone please forward this to bug-bash@? > > Sent a mail to bug-bash@ and CCed you and Florian. See https://lists.gnu.org/archive/html/bug-bash/2017-10/msg00042.html .
Hi Christian, I'm sorry I didn't CC you, I meant to (I guess that's what I get for thinking I can do things at 3 AM). > > lxc-execute -n a5 -- bash > bash: cannot set terminal process group (1): Inappropriate ioctl for device > bash: no job control in this shell > > root@a5:/# tty > /dev/console Huh? From my understanding, either `bash` should not be spitting out those errors, or `tty` should also be saying "not a tty"; which is what I see with systemd-nspawn: $ sudo systemd-nspawn --register=no --keep-unit -D /path/to/my-container Spawning container my-container on /path/to/my-container. Press ^] three times within 1s to kill container. -bash: cannot set terminal process group (-1): Inappropriate ioctl for device -bash: no job control in this shell [root@my-container ~]# tty not a tty [root@my-container ~]# findmnt|grep console │ └─/dev/console devpts[/6] devpts rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000 [root@my-container ~]# ls -al /dev/pts total 0 drwxr-xr-x 2 root root 0 Oct 6 14:11 . drwxr-xr-x 5 root root 340 Oct 6 14:11 .. crw-rw-rw- 1 root root 5, 2 Oct 6 14:11 ptmx At first I suspected that the difference is maybe you are running an older coreutils; and that the commits in 2.28 (937388c 2c622e1) in response to you reporting the equivalent issue in coreutils made the difference, but I see "not a tty" with both 2.27 and 2.28. Anyway... Because magical TTY reasons that aren't really relevant, Bash needs to re-open(2) its TTY on a new FD to set up job control. It won't do anything with it, it will immediately close it after it opens it. It just needs to open it. First, it tries "/dev/tty", and if that fails, it tries opening the path returned by `ttyname(0)`. Here's what happened with systemd-nspawn/bash/glibc-2.25: - Bash calls `ttyname(0)` to identify the current TTY. - `ttyname(0)` calls `readlink("/proc/self/fd/0", ...)` and gets `"/6"` (or some other number). - `ttyname(0)` calls `xstat64(..., "/6", ...)`, which fails. - `ttyname(0)` then goes to a fall-back loop over all files `/dev/pts/*` and `/dev/*` checking each to see if it matches our TTY. - `ttyname(0)` eventually comes across `/dev/console` which matches, and returns that. Here's what's happening with systemd-nspawn/bash/glibc-2.26: - Bash calls `ttyname(0)` to identify the current TTY. - `ttyname(0)` calls `readlink("/proc/self/fd/0", ...)` and gets `"/6"` (or some other number). - `ttyname(0)` calls `xstat64(..., "/6", ...)`, which fails. - `ttyname(0)` calls `is_pty(...)` to check if FD-0 is a PTY but the device doesn't exist because it's from another namespace (which is the case). - `ttyname(0)` sets `errno=ENODEV` and returns `NULL` because `is_pty(...)` returned true. So, within the container, the TTY is named "/dev/console", and with glibc 2.25, ttyname() correctly returned "/dev/console", but glibc 2.26 ttyname() returns NULL and sets errno=ENODEV. ---- > What your patch does is have ttyname() set ENODEV or have > ttyname_r() return ENODEV but in both cases no tty name will be > returned. So this presupposes that any program that calls > ttyname{_r}() recognizes that ENODEV means "is a tty but no path was > found" which is not the case for bash so I'm confused how that would > help you get around this problem. Wait? Now I'm confused. Isn't that what *your* patch does? My issue is that your is_pty()/ENODEV check ran too early, and prevents the old fallback search from running.
(In reply to Luke Shumaker from comment #10) > Hi Christian, > > I'm sorry I didn't CC you, I meant to (I guess that's what I get for > thinking I can do things at 3 AM). No worries, I just wanted to point it out for the future. :) The more eyes on this code the better! > > > > lxc-execute -n a5 -- bash > > bash: cannot set terminal process group (1): Inappropriate ioctl for > device > > bash: no job control in this shell > > > > root@a5:/# tty > > /dev/console > > Huh? From my understanding, either `bash` should not be spitting out > those errors, or `tty` should also be saying "not a tty"; which is > what I see with systemd-nspawn: Stupid question, but did you make sure that you're running a version with Serge's and my patch in the container? > > $ sudo systemd-nspawn --register=no --keep-unit -D /path/to/my-container > Spawning container my-container on /path/to/my-container. > Press ^] three times within 1s to kill container. > -bash: cannot set terminal process group (-1): Inappropriate ioctl for > device > -bash: no job control in this shell > [root@my-container ~]# tty > not a tty > [root@my-container ~]# findmnt|grep console > │ └─/dev/console devpts[/6] > devpts rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000 > [root@my-container ~]# ls -al /dev/pts > total 0 > drwxr-xr-x 2 root root 0 Oct 6 14:11 . > drwxr-xr-x 5 root root 340 Oct 6 14:11 .. > crw-rw-rw- 1 root root 5, 2 Oct 6 14:11 ptmx > > At first I suspected that the difference is maybe you are running an > older coreutils; and that the commits in 2.28 (937388c 2c622e1) in > response to you reporting the equivalent issue in coreutils made the > difference, but I see "not a tty" with both 2.27 and 2.28. Anyway... I've actually tried to make the tty program in coreutils sane in that regard but upstream didn't like it because POSIX. Let's not get into that. :) What I can tell you is that we've made things a little better for the tty program but it didn't change anything in how the output of ttyname{_r}() is shown. > > Because magical TTY reasons that aren't really relevant, Bash needs to > re-open(2) its TTY on a new FD to set up job control. It won't do > anything with it, it will immediately close it after it opens it. It > just needs to open it. First, it tries "/dev/tty", and if that fails, > it tries opening the path returned by `ttyname(0)`. > > Here's what happened with systemd-nspawn/bash/glibc-2.25: > > - Bash calls `ttyname(0)` to identify the current TTY. > - `ttyname(0)` calls `readlink("/proc/self/fd/0", ...)` and gets > `"/6"` (or some other number). > - `ttyname(0)` calls `xstat64(..., "/6", ...)`, which fails. > - `ttyname(0)` then goes to a fall-back loop over all files > `/dev/pts/*` and `/dev/*` checking each to see if it matches our > TTY. > - `ttyname(0)` eventually comes across `/dev/console` which matches, > and returns that. > > Here's what's happening with systemd-nspawn/bash/glibc-2.26: > > - Bash calls `ttyname(0)` to identify the current TTY. > - `ttyname(0)` calls `readlink("/proc/self/fd/0", ...)` and gets > `"/6"` (or some other number). > - `ttyname(0)` calls `xstat64(..., "/6", ...)`, which fails. > - `ttyname(0)` calls `is_pty(...)` to check if FD-0 is a PTY but the > device doesn't exist because it's from another namespace (which is > the case). > - `ttyname(0)` sets `errno=ENODEV` and returns `NULL` because > `is_pty(...)` returned true. > > So, within the container, the TTY is named "/dev/console", and with > glibc 2.25, ttyname() correctly returned "/dev/console", but glibc > 2.26 ttyname() returns NULL and sets errno=ENODEV. Thanks! I suggest ignoring differences between systemd-nspawn and liblxc console setup from now on since the issue clearly should be generally fixed in bash and glibc. > > ---- > > > What your patch does is have ttyname() set ENODEV or have > > ttyname_r() return ENODEV but in both cases no tty name will be > > returned. So this presupposes that any program that calls > > ttyname{_r}() recognizes that ENODEV means "is a tty but no path was > > found" which is not the case for bash so I'm confused how that would > > help you get around this problem. > > Wait? Now I'm confused. Isn't that what *your* patch does? My issue > is that your is_pty()/ENODEV check ran too early, and prevents the old > fallback search from running. I'm going to build a glibc with your patch now and take a closer look. If things look sane and it passes a couple of tests sending around {p,t}ty fds I think we're good to go. Bash however, still needs to handle the job control case better because as you saw jobctl still fails even if /dev/console is correctly detected.
Comment on attachment 10416 [details] proposed fix part 2 >From f9209704ded0240b39c7b1157d9294d88aab3591 Mon Sep 17 00:00:00 2001 >From: Luke Shumaker <lukeshu@lukeshu.com> >Date: Sun, 17 Sep 2017 03:02:07 -0400 >Subject: [PATCH 2/2] linux ttyname() and ttyname_r(): Fix namespace check > >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. > > This is the case for systemd-nspawn, which arranges for the TTY to be at > /dev/console in the new mount namespace. Christian's check meant that > Bash has broken job control when run directly in systemd-nspawn. I know this is on the verge of bikeshedding but please drop the reference to systemd-nspawn and provide a generic example for the /dev/console case. This makes it clearer that this is a general problem and avoids mentioning tools that might change behavior at any point. Maybe something along the lines of: "A common scenario where this happens is with /dev/console in containers. Usually container runtimes/managers will call openpty() on a ptmx device in the host's mount namespace to safely allocate a {p,t}ty master-slave pair since they can't trust the container's devpts mount after the container's init binary has started (potentially malicious fuse mounts and what not). The slave {p,t}ty fd will then usually be sent to the container and bind-mounted over the container's /dev/console which in this scenario is simply a regular file. This is especially common with unprivileged containers where mknod() syscalls are not possible. This patch enables ttyname{_r}() to correctly report that /dev/console does in fact refer to a {p,t}ty device whose path exists in the current mount namespace but whose origin is a devpts mount in a different mount namespace." >--- > sysdeps/unix/sysv/linux/ttyname.c | 19 ++++++++++++------- > sysdeps/unix/sysv/linux/ttyname_r.c | 20 ++++++++++++-------- > 2 files changed, 24 insertions(+), 15 deletions(-) > >diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c >index ba046205ae..509009bdda 100644 >--- a/sysdeps/unix/sysv/linux/ttyname.c >+++ b/sysdeps/unix/sysv/linux/ttyname.c >@@ -118,6 +118,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; >@@ -168,13 +169,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)) >@@ -198,5 +193,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. 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 8ff8653a2d..9dde9f7443 100644 >--- a/sysdeps/unix/sysv/linux/ttyname_r.c >+++ b/sysdeps/unix/sysv/linux/ttyname_r.c >@@ -96,6 +96,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 >@@ -150,14 +151,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. */ >@@ -191,6 +185,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. This >+ * happens when we have inherited the file descriptor from a >+ * different mount namespace. */ >+ __set_errno (ENODEV); >+ return ENODEV; >+ } >+ > return ret; > } > >-- >2.14.1 >
Comment on attachment 10415 [details] 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 >
Luke, could you please send your patches to the libc-alpha mailing list so that we can discuss them properly there? And if you're able to do so, could you please make the patches apply against current glibc master? That would make our lives a little easier. Thanks!
I've built a glibc with your patches applied. Detecting /dev/console as the path for a {p,t}y fd that stems from a devpts mount in a different mount namespace works correctly. I am inclined to think that your patch is safe but would like to pass a final verdict after we've done a proper review of this on the mailing list.
> Luke, could you please send your patches to the libc-alpha mailing > list Oh, definitely. I had intended to clean the patches up a bit, then send them to libc-alpha in the day or so after I posted them here, but things came up and I never got around to it... I guess I'm doing that today! > Stupid question, but did you make sure that you're running a version > with Serge's and my patch in the container? You're referring to 15e9a4f, right? If so: Yes, my glibc 2.26 is 58270c0 (plus 2 more patches), and 15e9a4f is an ancestor of that.
(In reply to Luke Shumaker from comment #16) > > Luke, could you please send your patches to the libc-alpha mailing > > list > > Oh, definitely. I had intended to clean the patches up a bit, then > send them to libc-alpha in the day or so after I posted them here, but > things came up and I never got around to it... I guess I'm doing that > today! Perfect, if it'd be possible for you to have them apply against current glibc master that would be fantastic. No need to rush it if you have a lot on your plate. :)
In cleaning up my patchset, I went to update the manual, since I figured ENODEV should be mentioned as a possible return value there. And I'm left with a question: Does `errno=ENODEV` really convey any more useful information than `errno != EBADF && errno != ENOTTY`? It has already passed `isatty()` (well `tcgetattr()`). Prior to Serge & your change, name=NULL/errno=unchanged signaled "the file descriptor is valid, and is associated with a terminal, but the associated file name could not be determined". ENODEV is mostly that same thing, but "is associated with a PTY", instead of a generic TTY. Is that a meaningful distinction to make? What about using ENODEV for all cases were the file descriptor is valid, and is associated with a terminal, but the associated file name could not be determined? AFAICT, the only other cases where this can happen are if `/proc` and `/dev` are not mounted/set-up as expected.
(In reply to Luke Shumaker from comment #18) > In cleaning up my patchset, I went to update the manual, since I > figured ENODEV should be mentioned as a possible return value there. > And I'm left with a question: > > Does `errno=ENODEV` really convey any more useful information than > `errno != EBADF && errno != ENOTTY`? It has already passed `isatty()` > (well `tcgetattr()`). > > Prior to Serge & your change, name=NULL/errno=unchanged signaled "the > file descriptor is valid, and is associated with a terminal, but the > associated file name could not be determined". ENODEV is mostly that > same thing, but "is associated with a PTY", instead of a generic TTY. > Is that a meaningful distinction to make? While POSIX does explicitly allow for this case I prefer setting errno to a meaningful value on error. I don't like conveying information implicitly be it in the error or success case. That's best left for cocktail parties in the Oxford common room. :) > > What about using ENODEV for all cases were the file descriptor is > valid, and is associated with a terminal, but the associated file name > could not be determined? This is not the focus of this patch. I'd prefer if this patch fixed the single issue of making parsing through /dev work correctly. In general, I'd consider all {p,t}y codepaths to be tricky since it is very easy to break backwards compatibility without realizing it. Let's not generalize the use of ENODEV right now. > > AFAICT, the only other cases where this can happen are if `/proc` and > `/dev` are not mounted/set-up as expected.
> While POSIX does explicitly allow for this case I prefer setting > errno to a meaningful value on error. I don't like conveying > information implicitly be it in the error or success case. That's > best left for cocktail parties in the Oxford common room. :) It actually doesn't; if isatty(FD) is true, then ttyname(FD) isn't allowed to fail according to POSIX. The behavior I described is just what glibc did :) I suspect that, prior to your patch, no one in glibc had thought very seriously about what happens when isatty(FD) is true but ttyname(FD) has to fail for another reason.
(In reply to Luke Shumaker from comment #20) > > While POSIX does explicitly allow for this case I prefer setting > > errno to a meaningful value on error. I don't like conveying > > information implicitly be it in the error or success case. That's > > best left for cocktail parties in the Oxford common room. :) > > It actually doesn't; if isatty(FD) is true, then ttyname(FD) isn't > allowed to fail according to POSIX. The behavior I described is just > what glibc did :) No, I just meant that POSIX - or the C standard - says it's perfectly fine for a function to return an error code but leave errno unchanged. Which for me should always be the ultima ratio, i.e. when setting errno to anything would be more confusing than not setting it. But we digress...
https://sourceware.org/ml/libc-alpha/2017-10/msg00501.html
This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "GNU C Library master sources". The branch, master has been updated via d9611e308592355718b36fe085b7b61aa52911e5 (commit) via a09dfc19edcbac3f96d5410529b724db0a583879 (commit) via 2fbce9c2031e70b6bd67876accfc34b0ec492878 (commit) via d10d6cab168ffa26ef6a506655ee5dc8537c8ed7 (commit) via 9b5a87502d048905c383b65c51768f4a1db8c685 (commit) via 495a56fdeb05d20a88304ff5da577d23a8e81ae1 (commit) from 78cde19f622cab74e3953c3d0139d51e1076108e (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log ----------------------------------------------------------------- https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=d9611e308592355718b36fe085b7b61aa52911e5 commit d9611e308592355718b36fe085b7b61aa52911e5 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:39:22 2017 +0100 linux ttyname{_r}: Add tests Add a new tst-ttyname test that includes several named sub-testcases. This patch is ordered after the patches with the fixes that it tests for (to avoid breaking `git bisect`), but for reference, here's how each relevant change so far affected the testcases in this commit, starting with 15e9a4f378c8607c2ae1aa465436af4321db0e23: | | before | | make checks | don't | | | 15e9a4f | 15e9a4f | consistent | bail | |---------------------------------+---------+---------+-------------+-------| | basic smoketest | PASS | PASS | PASS | PASS | | no conflict, no match | PASS[1] | PASS | PASS | PASS | | no conflict, console | PASS | FAIL! | FAIL | PASS! | | conflict, no match | FAIL | PASS! | PASS | PASS | | conflict, console | FAIL | FAIL | FAIL | PASS! | | with readlink target | PASS | PASS | PASS | PASS | | with readlink trap; fallback | FAIL | FAIL | FAIL | PASS! | | with readlink trap; no fallback | FAIL | PASS! | PASS | PASS | | with search-path trap | FAIL | FAIL | PASS! | PASS | |---------------------------------+---------+---------+-------------+-------| | | 4/9 | 5/9 | 6/9 | 9/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. Each of the failing tests before 15e9a4f are all essentially the same bug: that it returns a PTY slave with the correct minor device number, but from the wrong devpts filesystem instance. 15e9a4f sought to fix this, but missed several of the cases that can cause this to happen, and also broke the case where both the erroneous PTY and the correct PTY exist. Acked-by: Christian Brauner <christian.brauner@ubuntu.com> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=a09dfc19edcbac3f96d5410529b724db0a583879 commit a09dfc19edcbac3f96d5410529b724db0a583879 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:36:44 2017 +0100 linux ttyname{_r}: Don't bail prematurely [BZ #22145] Commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 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 try to find it by allowing the normal fall back on iterating through devices. An example scenario where this happens is with "/dev/console" in containers. It's a common practice among container managers to allocate a PTY master/slave pair in the host's mount namespace (the slave having a path like "/dev/pty/$X"), bind mount the slave to "/dev/console" in the container's mount namespace, and send the slave FD to a process in the container. 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 file at the original "/dev/pts/$X" path doesn't match the FD passed to it, and fails early and gives up, even though if it kept searching it would find the TTY at "/dev/console". Fix that; don't have the ENODEV path force an early return inhibiting the fall-back search. This change is based on the previous patch that adds use of is_mytty in getttyname and getttyname_r. Without that change, this effectively reverts 15e9a4f, which made us disregard the false similarity of file pointed to by "/proc/self/fd/$Y", because if it doesn't bail prematurely then that file ("/dev/pts/$X") will just come up again anyway in the fall-back search. Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=2fbce9c2031e70b6bd67876accfc34b0ec492878 commit 2fbce9c2031e70b6bd67876accfc34b0ec492878 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:34:30 2017 +0100 linux ttyname{_r}: Make tty checks consistent In the ttyname and ttyname_r routines on Linux, at several points it needs to check if a given TTY is the TTY we are looking for. It used to be that this check 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, one of the changes made in commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 was to change that 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, it made the st_ino and st_dev parts of the check happen even if we have the st_rdev member. This is an important change, because the kernel allows multiple devpts filesystem instances to be created; a device file in one devpts instance may share the same st_rdev with a file in another devpts instance, but they aren't the same file. This check appears twice in each file (ttyname.c and ttyname_r.c), once (in ttyname and __ttyname_r) to check if a candidate file found by inspecting /proc is the desired TTY, and once (in getttyname and getttyname_r) to check if a candidate file found by searching /dev is the desired TTY. However, 15e9a4f only updated the checks for files found via /proc; but the concern about collisions between devpts instances is just as valid for files found via /dev. So, update all 4 occurrences the check to be consistent with the version of the check introduced in 15e9a4f. Make it easy to keep all 4 occurrences of the check consistent by pulling it in to a static inline function, is_mytty. Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=d10d6cab168ffa26ef6a506655ee5dc8537c8ed7 commit d10d6cab168ffa26ef6a506655ee5dc8537c8ed7 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:33:11 2017 +0100 linux ttyname: Change return type of is_pty from int to bool is_pty returning a bool is fine since there's no possible outcome other than true or false, and bool is used throughout the codebase. Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=9b5a87502d048905c383b65c51768f4a1db8c685 commit 9b5a87502d048905c383b65c51768f4a1db8c685 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:31:32 2017 +0100 linux ttyname: Update a reference to kernel docs for kernel 4.10 Linux 4.10 moved many of the documentation files around. 4.10 came out between the time the patch adding the comment (commit 15e9a4f378c8607c2ae1aa465436af4321db0e23) was submitted and the time it was applied (in February, January, and March 2017; respectively). Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=495a56fdeb05d20a88304ff5da577d23a8e81ae1 commit 495a56fdeb05d20a88304ff5da577d23a8e81ae1 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:28:40 2017 +0100 manual: Update to mention ENODEV for ttyname and ttyname_r Commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 introduced ENODEV as a possible error condition for ttyname and ttyname_r. Update the manual to mention this GNU extension. Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> ----------------------------------------------------------------------- Summary of changes: ChangeLog | 26 ++ manual/terminal.texi | 5 + sysdeps/unix/sysv/linux/Makefile | 3 +- sysdeps/unix/sysv/linux/tst-ttyname.c | 625 +++++++++++++++++++++++++++++++++ sysdeps/unix/sysv/linux/ttyname.c | 59 ++-- sysdeps/unix/sysv/linux/ttyname.h | 18 +- sysdeps/unix/sysv/linux/ttyname_r.c | 61 +--- 7 files changed, 715 insertions(+), 82 deletions(-) create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c
If the commits fix the bug, please resolve it as FIXED with target milestone set. If not, given the length of discussions on it, a short description of what remains to be fixed would be helpful.
(In reply to joseph@codesourcery.com from comment #24) > If the commits fix the bug, please resolve it as FIXED with target > milestone set. If not, given the length of discussions on it, a short > description of what remains to be fixed would be helpful. Thanks, I was about to handle this next. Target milestone is probably 2.27.
This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "GNU C Library master sources". The branch, release/2.26/master has been updated via 069c3dd05abc91fced6e1e119e425c361ad97644 (commit) via 89e75d5eda37bf513d5f219a571d7b39b26277c3 (commit) via 4f987759d1108f5be622d7511d984d31fbc05178 (commit) via f43ead291c501dcfd9381e99cacb118b33344b04 (commit) via bd81a9d1e99f04d0b664542ddec2d270789f7ec1 (commit) via 1f0ba053ed6d065b2261f49d77bdcf4a34255689 (commit) via 58a63062a04c05a0e0633e0cdbf315a57cc88b5d (commit) from 5da0de4de5e8b9576475432b94fa0a486fd9389f (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log ----------------------------------------------------------------- https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=069c3dd05abc91fced6e1e119e425c361ad97644 commit 069c3dd05abc91fced6e1e119e425c361ad97644 Author: Florian Weimer <fweimer@redhat.com> Date: Sat Nov 18 14:34:46 2017 +0100 tst-ttyname: Fix namespace setup for Fedora On Fedora, the previous initialization sequence did not work and resulted in failures like: info: entering chroot 1 info: testcase: basic smoketest info: ttyname: PASS {name="/dev/pts/5", errno=0} info: ttyname_r: PASS {name="/dev/pts/5", ret=0, errno=0} error: ../sysdeps/unix/sysv/linux/tst-ttyname.c:122: write (setroups, "deny"): Operation not permitted info: entering chroot 2 error: ../sysdeps/unix/sysv/linux/tst-ttyname.c:122: write (setroups, "deny"): Operation not permitted error: 2 test failures Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> (cherry picked from commit 8db7f48cb74670829df037b2d037df3f36b71ecd) https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=89e75d5eda37bf513d5f219a571d7b39b26277c3 commit 89e75d5eda37bf513d5f219a571d7b39b26277c3 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:39:22 2017 +0100 linux ttyname{_r}: Add tests Add a new tst-ttyname test that includes several named sub-testcases. This patch is ordered after the patches with the fixes that it tests for (to avoid breaking `git bisect`), but for reference, here's how each relevant change so far affected the testcases in this commit, starting with 15e9a4f378c8607c2ae1aa465436af4321db0e23: | | before | | make checks | don't | | | 15e9a4f | 15e9a4f | consistent | bail | |---------------------------------+---------+---------+-------------+-------| | basic smoketest | PASS | PASS | PASS | PASS | | no conflict, no match | PASS[1] | PASS | PASS | PASS | | no conflict, console | PASS | FAIL! | FAIL | PASS! | | conflict, no match | FAIL | PASS! | PASS | PASS | | conflict, console | FAIL | FAIL | FAIL | PASS! | | with readlink target | PASS | PASS | PASS | PASS | | with readlink trap; fallback | FAIL | FAIL | FAIL | PASS! | | with readlink trap; no fallback | FAIL | PASS! | PASS | PASS | | with search-path trap | FAIL | FAIL | PASS! | PASS | |---------------------------------+---------+---------+-------------+-------| | | 4/9 | 5/9 | 6/9 | 9/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. Each of the failing tests before 15e9a4f are all essentially the same bug: that it returns a PTY slave with the correct minor device number, but from the wrong devpts filesystem instance. 15e9a4f sought to fix this, but missed several of the cases that can cause this to happen, and also broke the case where both the erroneous PTY and the correct PTY exist. Acked-by: Christian Brauner <christian.brauner@ubuntu.com> (cherry picked from commit d9611e308592355718b36fe085b7b61aa52911e5) https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=4f987759d1108f5be622d7511d984d31fbc05178 commit 4f987759d1108f5be622d7511d984d31fbc05178 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:36:44 2017 +0100 linux ttyname{_r}: Don't bail prematurely [BZ #22145] Commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 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 try to find it by allowing the normal fall back on iterating through devices. An example scenario where this happens is with "/dev/console" in containers. It's a common practice among container managers to allocate a PTY master/slave pair in the host's mount namespace (the slave having a path like "/dev/pty/$X"), bind mount the slave to "/dev/console" in the container's mount namespace, and send the slave FD to a process in the container. 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 file at the original "/dev/pts/$X" path doesn't match the FD passed to it, and fails early and gives up, even though if it kept searching it would find the TTY at "/dev/console". Fix that; don't have the ENODEV path force an early return inhibiting the fall-back search. This change is based on the previous patch that adds use of is_mytty in getttyname and getttyname_r. Without that change, this effectively reverts 15e9a4f, which made us disregard the false similarity of file pointed to by "/proc/self/fd/$Y", because if it doesn't bail prematurely then that file ("/dev/pts/$X") will just come up again anyway in the fall-back search. Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> (cherry picked from commit a09dfc19edcbac3f96d5410529b724db0a583879) https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=f43ead291c501dcfd9381e99cacb118b33344b04 commit f43ead291c501dcfd9381e99cacb118b33344b04 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Fri Dec 22 15:27:57 2017 +0100 linux ttyname{_r}: Make tty checks consistent In the ttyname and ttyname_r routines on Linux, at several points it needs to check if a given TTY is the TTY we are looking for. It used to be that this check 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, one of the changes made in commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 was to change that 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, it made the st_ino and st_dev parts of the check happen even if we have the st_rdev member. This is an important change, because the kernel allows multiple devpts filesystem instances to be created; a device file in one devpts instance may share the same st_rdev with a file in another devpts instance, but they aren't the same file. This check appears twice in each file (ttyname.c and ttyname_r.c), once (in ttyname and __ttyname_r) to check if a candidate file found by inspecting /proc is the desired TTY, and once (in getttyname and getttyname_r) to check if a candidate file found by searching /dev is the desired TTY. However, 15e9a4f only updated the checks for files found via /proc; but the concern about collisions between devpts instances is just as valid for files found via /dev. So, update all 4 occurrences the check to be consistent with the version of the check introduced in 15e9a4f. Make it easy to keep all 4 occurrences of the check consistent by pulling it in to a static inline function, is_mytty. Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> (cherry picked from commit 2fbce9c2031e70b6bd67876accfc34b0ec492878) https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=bd81a9d1e99f04d0b664542ddec2d270789f7ec1 commit bd81a9d1e99f04d0b664542ddec2d270789f7ec1 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:33:11 2017 +0100 linux ttyname: Change return type of is_pty from int to bool is_pty returning a bool is fine since there's no possible outcome other than true or false, and bool is used throughout the codebase. Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> (cherry picked from commit d10d6cab168ffa26ef6a506655ee5dc8537c8ed7) https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=1f0ba053ed6d065b2261f49d77bdcf4a34255689 commit 1f0ba053ed6d065b2261f49d77bdcf4a34255689 Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:31:32 2017 +0100 linux ttyname: Update a reference to kernel docs for kernel 4.10 Linux 4.10 moved many of the documentation files around. 4.10 came out between the time the patch adding the comment (commit 15e9a4f378c8607c2ae1aa465436af4321db0e23) was submitted and the time it was applied (in February, January, and March 2017; respectively). Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> (cherry picked from commit 9b5a87502d048905c383b65c51768f4a1db8c685) https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=58a63062a04c05a0e0633e0cdbf315a57cc88b5d commit 58a63062a04c05a0e0633e0cdbf315a57cc88b5d Author: Luke Shumaker <lukeshu@parabola.nu> Date: Wed Nov 15 20:28:40 2017 +0100 manual: Update to mention ENODEV for ttyname and ttyname_r Commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 introduced ENODEV as a possible error condition for ttyname and ttyname_r. Update the manual to mention this GNU extension. Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com> (cherry picked from commit 495a56fdeb05d20a88304ff5da577d23a8e81ae1) ----------------------------------------------------------------------- Summary of changes: ChangeLog | 44 +++ NEWS | 1 + manual/terminal.texi | 5 + sysdeps/unix/sysv/linux/Makefile | 3 +- sysdeps/unix/sysv/linux/tst-ttyname.c | 570 +++++++++++++++++++++++++++++++++ sysdeps/unix/sysv/linux/ttyname.c | 63 ++--- sysdeps/unix/sysv/linux/ttyname.h | 18 +- sysdeps/unix/sysv/linux/ttyname_r.c | 65 ++--- 8 files changed, 682 insertions(+), 87 deletions(-) create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c