Message ID | 20171102185346.1386-5-lukeshu@parabola.nu |
---|---|
State | New, archived |
Headers |
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: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> 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 <lukeshu@parabola.nu> 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> |
Commit Message
Luke Shumaker
Nov. 2, 2017, 6:53 p.m. UTC
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 --- ChangeLog | 5 +++++ sysdeps/unix/sysv/linux/ttyname.c | 19 ++++++++++++------- sysdeps/unix/sysv/linux/ttyname_r.c | 20 ++++++++++++-------- 3 files changed, 29 insertions(+), 15 deletions(-)
Comments
On Thu, Nov 02, 2017 at 02:53:45PM -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. > > 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 > --- > ChangeLog | 5 +++++ > sysdeps/unix/sysv/linux/ttyname.c | 19 ++++++++++++------- > sysdeps/unix/sysv/linux/ttyname_r.c | 20 ++++++++++++-------- > 3 files changed, 29 insertions(+), 15 deletions(-) Same, as before minor improvements to the commit messages code itself looks fine to me. Reviewed-By: Christian Brauner <christian.brauner@ubuntu.com> > > diff --git a/ChangeLog b/ChangeLog > index 4c282f8f47..b5d214ae6f 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,5 +1,10 @@ > 2017-11-02 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_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; > } > > -- > 2.15.0 >
On Mon, 06 Nov 2017 08:30:24 -0500, Christian Brauner wrote: > Same, as before minor improvements to the commit messages code itself looks fine > to me. What improvements do you want to make? I'd be happy to go ahead and make them and submit a v3 today with revised commit messages.
On Tue, Nov 07, 2017 at 11:00:37AM -0500, Luke Shumaker wrote: > On Mon, 06 Nov 2017 08:30:24 -0500, > Christian Brauner wrote: > > Same, as before minor improvements to the commit messages code itself looks fine > > to me. > > What improvements do you want to make? I'd be happy to go ahead and > make them and submit a v3 today with revised commit messages. It's just spelling mistakes or double words in the commit message. It's really not worth resending for this imho.
On Thu, Nov 02, 2017 at 02:53:45PM -0400, Luke Shumaker wrote: s/bail/give up/ > 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. Like in other commit messages, this is getting too personal. We usually mention changes introduced with commits, not people authored those commits. > 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. s/find/try to find/ > A common scenario where this happens is with "/dev/console" in s/A common/An example/ > containers. Common container managers (including systemd-nspawn) will "It's a common practice among container managers ... to ...". > 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 what is bind-mounted at "/dev/console" in the container's mount namespace? Is it a /dev/pty/<idx> path in the host's mount namespace? > 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, doesn't 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. Like in the previous patch, wouldn't it be better to mention only those test cases that are affected by this commit, and leave the whole picture to the commit that introduces the test itself? > 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. Why do you want to go in details explaining how things will break if the previous patch is not applied? Wouldn't it be enough to say that this change is essentially based on the previous one that adds use of is_mytty in getttyname and getttyname_r? > > v2: > - Rearranged commit order > - Expanded/reworded commit message > --- > 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 <lukeshu@parabola.nu> > > + [BZ #22145] > + * sysdeps/unix/sysv/linux/ttyname.c (ttyname): > + Defer is_pty check until end. until the end of function? > + * 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; > } > The change looks OK. Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>
On Wed, Nov 08, 2017 at 01:53:39AM +0300, Dmitry V. Levin wrote: > On Thu, Nov 02, 2017 at 02:53:45PM -0400, Luke Shumaker wrote: > > s/bail/give up/ > > > 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. > > Like in other commit messages, this is getting too personal. > We usually mention changes introduced with commits, not people authored > those commits. > > > 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. > > s/find/try to find/ > > > A common scenario where this happens is with "/dev/console" in > > s/A common/An example/ > > > containers. Common container managers (including systemd-nspawn) will > > "It's a common practice among container managers ... to ...". > > > 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 > > what is bind-mounted at "/dev/console" in the container's mount namespace? > Is it a /dev/pty/<idx> path in the host's mount namespace? Usually, but it doesn't need to be. You could mount another devpts instance somewhere e.g. /mnt and open a master/slave pair manually without openpty() (thereby circumventing the glibc hard-coded /dev/pts/<idx> check) and send the slave to the container and mount by /proc/<pid>/fd/<idx>. So probably "and mount the slave on /dev/console inside the container's mount namespace". > > > 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, > > doesn't 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. > > Like in the previous patch, wouldn't it be better to mention only those > test cases that are affected by this commit, and leave the whole picture > to the commit that introduces the test itself? > > > 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. > > Why do you want to go in details explaining how things will break > if the previous patch is not applied? > > Wouldn't it be enough to say that this change is essentially based > on the previous one that adds use of is_mytty in getttyname and getttyname_r? > > > > > v2: > > - Rearranged commit order > > - Expanded/reworded commit message > > --- > > 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 <lukeshu@parabola.nu> > > > > + [BZ #22145] > > + * sysdeps/unix/sysv/linux/ttyname.c (ttyname): > > + Defer is_pty check until end. > > until the end of function? > > > + * 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; > > } > > > > The change looks OK. > > Reviewed-by: Dmitry V. Levin <ldv@altlinux.org> > > > -- > ldv
On Nov 02 2017, Luke Shumaker <lukeshu@parabola.nu> wrote: > @@ -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. */ Style: no '*', two spaces after period. > @@ -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. */ Likewise. Andreas.
diff --git a/ChangeLog b/ChangeLog index 4c282f8f47..b5d214ae6f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ 2017-11-02 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_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; }