From patchwork Thu Nov 2 18:53:45 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luke Shumaker X-Patchwork-Id: 24045 Received: (qmail 72659 invoked by alias); 2 Nov 2017 18:59:16 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 71174 invoked by uid 89); 2 Nov 2017 18:59:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=similarity, affecting, christians X-HELO: mav.lukeshu.com From: Luke Shumaker To: libc-alpha@sourceware.org Cc: christian.brauner@mailbox.org Subject: [PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145] Date: Thu, 2 Nov 2017 14:53:45 -0400 Message-Id: <20171102185346.1386-5-lukeshu@parabola.nu> In-Reply-To: <20171102185346.1386-1-lukeshu@parabola.nu> References: <20171102185346.1386-1-lukeshu@parabola.nu> 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. The linux tst-ttyname test hasn't been added yet (to avoid breaking `git bisect`), but for reference, here's how each relevent change so far affects the testcases that will be in it: | | before | | make tests | 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. Without the previous ("make tests consistent") patch, this change would revert the test status to the pre-15e9a4f state (except for the change in errno semantics affecting "no conflict, no match"). That's surprising; this doesn't revert the changes that Christian made in that commit. Christian's change made us disregard the false similarity of file pointed to by /proc/self/fd/$X; but that file ("/dev/pts/$Y") will just come up again anyway in the fallback search; so it is now even more important than before that the fallback search isn't confused by the false similarity. v2: - Rearranged commit order - Expanded/reworded commit message Reviewed-By: Christian Brauner Reviewed-by: Dmitry V. Levin --- 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 4c282f8f47..b5d214ae6f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ 2017-11-02 Luke Shumaker + [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_pty): Change return type from int to bool. diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c index 6e97d2d455..9287758b59 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 58eb919c3f..a109e79068 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; }