[PATCH v2 0/5] Fixup linux ttyname and ttyname_r [BZ #22145]

[PATCH v2 0/5] Fixup linux ttyname and ttyname_r [BZ #22145]

[Luke Shumaker]

The theme of this patchset is to fixup the changes made in 15e9a4f.
Fix a bug [BZ #22145] introduced in the commit (and add tests for both
the bug it fixed and the bug it introduced!), update documentation to
reflect the behavior introduced in the commit.

The FSF should have my copyright assignment paperwork on file (though
I never received a confirmation from them that everything was in
order).

Changes from v1 of this patchset are enumerated in each commit's
message.

Luke Shumaker (5):
  manual: Update to mention ENODEV for ttyname and ttyname_r
  linux ttyname: Update a reference to kernel docs for kernel 4.10
  linux ttyname and ttyname_r: Make the TTY equivalence tests consistent
    [BZ #22145]
  linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]
  linux ttyname and ttyname_r: Add tests [BZ #22145]

 ChangeLog                             |  23 ++
 manual/terminal.texi                  |   5 +
 sysdeps/unix/sysv/linux/Makefile      |   3 +-
 sysdeps/unix/sysv/linux/tst-ttyname.c | 582 ++++++++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/ttyname.c     |  59 ++--
 sysdeps/unix/sysv/linux/ttyname.h     |  17 +-
 sysdeps/unix/sysv/linux/ttyname_r.c   |  61 ++--
 7 files changed, 668 insertions(+), 82 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c

-- 
2.15.0

[PATCH v2 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Luke Shumaker]

Commit 15e9a4f3 by Christian Brauner <christian.brauner@canonical.com>
introduced ENODEV as a possible error condition for ttyname and ttyname_r.
The manual should mention this.

v2:
 - Fix typo: psuedo->pseudo
 - Improve wording
 - Fix ChangeLog item name
---
 ChangeLog            | 5 +++++
 manual/terminal.texi | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 0125305f6e..f6137669bf 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
+
+	* manual/terminal.texi (Is It a Terminal):
+	Mention ENODEV for ttyname and ttyname_r.
+
 2017-11-02  Mike FABIAN  <mfabian@redhat.com>
 
 	[BZ #22382]
diff --git a/manual/terminal.texi b/manual/terminal.texi
index 4fef5045b8..4aace48b14 100644
--- a/manual/terminal.texi
+++ b/manual/terminal.texi
@@ -109,6 +109,11 @@ The @var{filedes} is not associated with a terminal.
 @item ERANGE
 The buffer length @var{len} is too small to store the string to be
 returned.
+
+@item ENODEV
+The @var{filedes} is associated with a terminal device that is a slave
+pseudo-terminal, but the file name associated with that device could
+not be determined.  This is a GNU extension.
 @end table
 @end deftypefun
 
-- 
2.15.0

Re: [PATCH v2 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Christian Brauner]

On Thu, Nov 02, 2017 at 02:53:42PM -0400, Luke Shumaker wrote:
> Commit 15e9a4f3 by Christian Brauner <christian.brauner@canonical.com>
> introduced ENODEV as a possible error condition for ttyname and ttyname_r.
> The manual should mention this.
> 
> v2:
>  - Fix typo: psuedo->pseudo
>  - Improve wording
>  - Fix ChangeLog item name
> ---
>  ChangeLog            | 5 +++++
>  manual/terminal.texi | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 0125305f6e..f6137669bf 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
> +
> +	* manual/terminal.texi (Is It a Terminal):
> +	Mention ENODEV for ttyname and ttyname_r.
> +
>  2017-11-02  Mike FABIAN  <mfabian@redhat.com>
>  
>  	[BZ #22382]
> diff --git a/manual/terminal.texi b/manual/terminal.texi
> index 4fef5045b8..4aace48b14 100644
> --- a/manual/terminal.texi
> +++ b/manual/terminal.texi
> @@ -109,6 +109,11 @@ The @var{filedes} is not associated with a terminal.
>  @item ERANGE
>  The buffer length @var{len} is too small to store the string to be
>  returned.
> +
> +@item ENODEV
> +The @var{filedes} is associated with a terminal device that is a slave
> +pseudo-terminal, but the file name associated with that device could
> +not be determined.  This is a GNU extension.
>  @end table
>  @end deftypefun

Reviewed-By: Christian Brauner <christian.brauner@ubuntu.com>

>  
> -- 
> 2.15.0
> 

Re: [PATCH v2 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]

On Thu, Nov 02, 2017 at 02:53:42PM -0400, Luke Shumaker wrote:
> Commit 15e9a4f3 by Christian Brauner <christian.brauner@canonical.com>
> introduced ENODEV as a possible error condition for ttyname and ttyname_r.
> The manual should mention this.

This short abbreviated form of commit names may become ambiguous in the
future, let's use something less ambiguous, or just the full commit name.
 
Unless the referenced commit lacks a proper attribution, additional
attribution that duplicates the information in the referenced commit
is redundant.

A commit message looks inconsistent when it says that A should be changed
while the commit implements the change.

Summarising, I'd expect a commit message like this:

Commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 introduced ENODEV
as a possible error condition for ttyname and ttyname_r.
Update the manual to mention this GNU extension.
 
> v2:
>  - Fix typo: psuedo->pseudo
>  - Improve wording
>  - Fix ChangeLog item name
> ---
>  ChangeLog            | 5 +++++
>  manual/terminal.texi | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 0125305f6e..f6137669bf 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
> +
> +	* manual/terminal.texi (Is It a Terminal):
> +	Mention ENODEV for ttyname and ttyname_r.
> +
>  2017-11-02  Mike FABIAN  <mfabian@redhat.com>
>  
>  	[BZ #22382]
> diff --git a/manual/terminal.texi b/manual/terminal.texi
> index 4fef5045b8..4aace48b14 100644
> --- a/manual/terminal.texi
> +++ b/manual/terminal.texi
> @@ -109,6 +109,11 @@ The @var{filedes} is not associated with a terminal.
>  @item ERANGE
>  The buffer length @var{len} is too small to store the string to be
>  returned.
> +
> +@item ENODEV
> +The @var{filedes} is associated with a terminal device that is a slave
> +pseudo-terminal, but the file name associated with that device could
> +not be determined.  This is a GNU extension.
>  @end table
>  @end deftypefun

The change itself is OK, thanks for updating documentation.

Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]

On Mon, Nov 06, 2017 at 07:09:43PM +0300, Dmitry V. Levin wrote:
> On Thu, Nov 02, 2017 at 02:53:42PM -0400, Luke Shumaker wrote:
> > Commit 15e9a4f3 by Christian Brauner <christian.brauner@canonical.com>
> > introduced ENODEV as a possible error condition for ttyname and ttyname_r.
> > The manual should mention this.
> 
> This short abbreviated form of commit names may become ambiguous in the
> future, let's use something less ambiguous, or just the full commit name.
>  
> Unless the referenced commit lacks a proper attribution, additional
> attribution that duplicates the information in the referenced commit
> is redundant.
> 
> A commit message looks inconsistent when it says that A should be changed
> while the commit implements the change.
> 
> Summarising, I'd expect a commit message like this:
> 
> Commit 15e9a4f378c8607c2ae1aa465436af4321db0e23 introduced ENODEV
> as a possible error condition for ttyname and ttyname_r.
> Update the manual to mention this GNU extension.
>  
> > v2:
> >  - Fix typo: psuedo->pseudo
> >  - Improve wording
> >  - Fix ChangeLog item name
> > ---
> >  ChangeLog            | 5 +++++
> >  manual/terminal.texi | 5 +++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/ChangeLog b/ChangeLog
> > index 0125305f6e..f6137669bf 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
> > +
> > +	* manual/terminal.texi (Is It a Terminal):
> > +	Mention ENODEV for ttyname and ttyname_r.
> > +
> >  2017-11-02  Mike FABIAN  <mfabian@redhat.com>
> >  
> >  	[BZ #22382]
> > diff --git a/manual/terminal.texi b/manual/terminal.texi
> > index 4fef5045b8..4aace48b14 100644
> > --- a/manual/terminal.texi
> > +++ b/manual/terminal.texi
> > @@ -109,6 +109,11 @@ The @var{filedes} is not associated with a terminal.
> >  @item ERANGE
> >  The buffer length @var{len} is too small to store the string to be
> >  returned.
> > +
> > +@item ENODEV
> > +The @var{filedes} is associated with a terminal device that is a slave
> > +pseudo-terminal, but the file name associated with that device could
> > +not be determined.  This is a GNU extension.
> >  @end table
> >  @end deftypefun
> 
> The change itself is OK, thanks for updating documentation.
> 
> Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>

I'm going to change the wording in some of the commit messages anyway so I would
just do this substitution when I apply, i.e. if people are fine with that.

> 
> 
> -- 
> ldv



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

On Mon, Nov 06, 2017 at 07:19:46PM +0100, Christian Brauner wrote:
[...]
> I'm going to change the wording in some of the commit messages anyway so I would
> just do this substitution when I apply, i.e. if people are fine with that.

As commit messages are part of commits, they are also subject of review.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On Mon, Nov 06, 2017 at 10:56:28PM +0300, Dmitry V. Levin wrote:
> On Mon, Nov 06, 2017 at 07:19:46PM +0100, Christian Brauner wrote:
> [...]
> > I'm going to change the wording in some of the commit messages anyway so I would
> > just do this substitution when I apply, i.e. if people are fine with that.
> 
> As commit messages are part of commits, they are also subject of review.

Thanks. Good to know. I've just seen commit messages being changed when applying
patches to master that's why I assumed grammatical and syntactial errors can be
fixed by the committer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 1/5] manual: Update to mention ENODEV for ttyname and ttyname_r

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Mon, Nov 06, 2017 at 10:18:01PM +0100, Christian Brauner wrote:
> On Mon, Nov 06, 2017 at 10:56:28PM +0300, Dmitry V. Levin wrote:
> > On Mon, Nov 06, 2017 at 07:19:46PM +0100, Christian Brauner wrote:
> > [...]
> > > I'm going to change the wording in some of the commit messages anyway so I would
> > > just do this substitution when I apply, i.e. if people are fine with that.
> > 
> > As commit messages are part of commits, they are also subject of review.
> 
> Thanks. Good to know. I've just seen commit messages being changed when applying
> patches to master that's why I assumed grammatical and syntactial errors can be
> fixed by the committer.

Trivial fixes of spelling or grammatical errors do not require a review.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v2 2/5] linux ttyname: Update a reference to kernel docs for kernel 4.10

[Luke Shumaker]

Linux 4.10 moved many of the documentation files around.

It came out between the time the patch adding the comment (commit
15e9a4f3) was submitted and the time it was applied (in February,
January, and March 2017; respectively).
---
 ChangeLog                         | 2 ++
 sysdeps/unix/sysv/linux/ttyname.h | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index f6137669bf..2c7770fbbb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,7 @@
 2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
 
+	* sysdeps/unix/sysv/linux/ttyname.h (is_pty): Update doc reference.
+
 	* manual/terminal.texi (Is It a Terminal):
 	Mention ENODEV for ttyname and ttyname_r.
 
diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
index 2e415e4e9c..dd7873d1ff 100644
--- a/sysdeps/unix/sysv/linux/ttyname.h
+++ b/sysdeps/unix/sysv/linux/ttyname.h
@@ -21,7 +21,8 @@
 #include <sys/stat.h>
 
 /* Return true if this is a UNIX98 pty device, as defined in
-   linux/Documentation/devices.txt.  */
+   linux/Documentation/devices.txt (on linux < 4.10) or
+   linux/Documentation/admin-guide/devices.txt (on linux >= 4.10). */
 static inline int
 is_pty (struct stat64 *sb)
 {
-- 
2.15.0

Re: [PATCH v2 2/5] linux ttyname: Update a reference to kernel docs for kernel 4.10

[Christian Brauner]

On Thu, Nov 02, 2017 at 02:53:43PM -0400, Luke Shumaker wrote:
> Linux 4.10 moved many of the documentation files around.
> 
> It came out between the time the patch adding the comment (commit
> 15e9a4f3) was submitted and the time it was applied (in February,
> January, and March 2017; respectively).
> ---
>  ChangeLog                         | 2 ++
>  sysdeps/unix/sysv/linux/ttyname.h | 3 ++-
>  2 files changed, 4 insertions(+), 1 deletion(-)

Reviewed-By: Christian Brauner <christian.brauner@ubuntu.com>

> 
> diff --git a/ChangeLog b/ChangeLog
> index f6137669bf..2c7770fbbb 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,7 @@
>  2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
>  
> +	* sysdeps/unix/sysv/linux/ttyname.h (is_pty): Update doc reference.
> +
>  	* manual/terminal.texi (Is It a Terminal):
>  	Mention ENODEV for ttyname and ttyname_r.
>  
> diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
> index 2e415e4e9c..dd7873d1ff 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.h
> +++ b/sysdeps/unix/sysv/linux/ttyname.h
> @@ -21,7 +21,8 @@
>  #include <sys/stat.h>
>  
>  /* Return true if this is a UNIX98 pty device, as defined in
> -   linux/Documentation/devices.txt.  */
> +   linux/Documentation/devices.txt (on linux < 4.10) or
> +   linux/Documentation/admin-guide/devices.txt (on linux >= 4.10). */
>  static inline int
>  is_pty (struct stat64 *sb)
>  {
> -- 
> 2.15.0
> 

Re: [PATCH v2 2/5] linux ttyname: Update a reference to kernel docs for kernel 4.10

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Thu, Nov 02, 2017 at 02:53:43PM -0400, Luke Shumaker wrote:
> Linux 4.10 moved many of the documentation files around.
> 
> It came out between the time the patch adding the comment (commit
> 15e9a4f3) was submitted and the time it was applied (in February,
> January, and March 2017; respectively).

s/15e9a4f3/15e9a4f378c8607c2ae1aa465436af4321db0e23/
s/;/,/

The change is OK.

Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/5] linux ttyname: Update a reference to kernel docs for kernel 4.10

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

On Mon, Nov 06, 2017 at 07:15:31PM +0300, Dmitry V. Levin wrote:
> On Thu, Nov 02, 2017 at 02:53:43PM -0400, Luke Shumaker wrote:
> > Linux 4.10 moved many of the documentation files around.
> > 
> > It came out between the time the patch adding the comment (commit
> > 15e9a4f3) was submitted and the time it was applied (in February,
> > January, and March 2017; respectively).
> 
> s/15e9a4f3/15e9a4f378c8607c2ae1aa465436af4321db0e23/
> s/;/,/
> 
> The change is OK.
> 
> Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>

I'm going to change the wording in some of the commit messages anyway so I would
just do this substitution when I apply, i.e. if people are fine with that.

> 
> 
> -- 
> ldv



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/5] linux ttyname: Update a reference to kernel docs for kernel 4.10

[Luke Shumaker]

On Thu, 02 Nov 2017 14:53:43 -0400,
Luke Shumaker wrote:
> --- a/sysdeps/unix/sysv/linux/ttyname.h
> +++ b/sysdeps/unix/sysv/linux/ttyname.h
> @@ -21,7 +21,8 @@
>  #include <sys/stat.h>
>  
>  /* Return true if this is a UNIX98 pty device, as defined in
> -   linux/Documentation/devices.txt.  */
> +   linux/Documentation/devices.txt (on linux < 4.10) or
> +   linux/Documentation/admin-guide/devices.txt (on linux >= 4.10). */

Style: two spaces after period.

-- 
Happy hacking,
~ Luke Shumaker

[PATCH v2 3/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

[Luke Shumaker]

In the ttyname and ttyname_r routines on Linux, at several points it
it needs to test if a given TTY is the TTY we are looking for.  It used to
be that this test was (to see if `maybe` is `mytty`):

       __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
    #ifdef _STATBUF_ST_RDEV
       && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
    #else
       && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev
    #endif

This check appears in several places.

Then, in commit 15e9a4f3 by Christian Brauner
<christian.brauner@canonical.com>, that check changed to:

       __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
    #ifdef _STATBUF_ST_RDEV
       && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
    #endif
       && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev

That is, he made st_ino and st_dev checks happen even if we have the
st_rdev member.  This is a good change, because kernel namespaces allow you
to create a new namespace, and to create a new device with the same
st_rdev, but isn't the same file.

However, this check appears twice in each file (ttyname.c and
ttyname_r.c), but Christian only changed it in one place in each file.

Make it easy to keep consistent by pulling the check in to a static inline
function, is_mytty.

While we're at it, go ahead and have the existing is_pty return a bool
(instead of an int) to keep ttyname.h consistent, per
<https://sourceware.org/ml/libc-alpha/2017-10/msg00532.html>.

  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 |
  |                                 | 15e9a4f | 15e9a4f | consistent |
  |---------------------------------+---------+---------+------------|
  | basic smoketest                 | PASS    | PASS    | PASS       |
  | no conflict, no match           | PASS[1] | PASS    | PASS       |
  | no conflict, console            | PASS    | FAIL!   | FAIL       |
  | conflict, no match              | FAIL    | PASS!   | PASS       |
  | conflict, console               | FAIL    | FAIL    | FAIL       |
  | with readlink target            | PASS    | PASS    | PASS       |
  | with readlink trap; fallback    | FAIL    | FAIL    | FAIL       |
  | with readlink trap; no fallback | FAIL    | PASS!   | PASS       |
  | with search-path trap           | FAIL    | FAIL    | PASS!      |
  |---------------------------------+---------+---------+------------|
  |                                 | 4/9     | 5/9     | 6/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.

  This only fixed one testcase, but most of the remaining failures
  require both this fix and the subsequent fix in order to pass.

v2:
 - Rearranged commit order
 - Expanded commit message
 - Reformatted is_mytty
 - Made is_pty and is_mytty return bool instead of int
 - Specify 'const' on pointer arguments as necessary
---
 ChangeLog                           |  7 +++++++
 sysdeps/unix/sysv/linux/ttyname.c   | 40 ++++++++----------------------------
 sysdeps/unix/sysv/linux/ttyname.h   | 14 ++++++++++++-
 sysdeps/unix/sysv/linux/ttyname_r.c | 41 ++++++++-----------------------------
 4 files changed, 37 insertions(+), 65 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2c7770fbbb..4c282f8f47 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
 
+	[BZ #22145]
+	* sysdeps/unix/sysv/linux/ttyname.h
+	(is_pty): Change return type from int to bool.
+	(is_mytty): New function.
+	* sysdeps/unix/sysv/linux/ttyname.c: Call is_mytty.
+	* sysdeps/unix/sysv/linux/ttyname_r.c: Likewise.
+
 	* sysdeps/unix/sysv/linux/ttyname.h (is_pty): Update doc reference.
 
 	* manual/terminal.texi (Is It a Terminal):
diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 116cf350e5..6e97d2d455 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -35,14 +35,14 @@
 char *__ttyname;
 #endif
 
-static char *getttyname (const char *dev, dev_t mydev,
-			 ino64_t myino, int save, int *dostat);
+static char *getttyname (const char *dev, const struct stat64 *mytty,
+			 int save, int *dostat);
 
 libc_freeres_ptr (static char *getttyname_name);
 
 static char *
 attribute_compat_text_section
-getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
+getttyname (const char *dev, const struct stat64 *mytty, int save, int *dostat)
 {
   static size_t namelen;
   struct stat64 st;
@@ -63,7 +63,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
     *((char *) __mempcpy (getttyname_name, dev, devlen - 1)) = '/';
 
   while ((d = __readdir64 (dirstream)) != NULL)
-    if ((d->d_fileno == myino || *dostat)
+    if ((d->d_fileno == mytty->st_ino || *dostat)
 	&& strcmp (d->d_name, "stdin")
 	&& strcmp (d->d_name, "stdout")
 	&& strcmp (d->d_name, "stderr"))
@@ -85,12 +85,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
 	  }
 	memcpy (&getttyname_name[devlen], d->d_name, dlen);
 	if (__xstat64 (_STAT_VER, getttyname_name, &st) == 0
-#ifdef _STATBUF_ST_RDEV
-	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
-#else
-	    && d->d_fileno == myino && st.st_dev == mydev
-#endif
-	   )
+	    && is_mytty (mytty, &st))
 	  {
 	    (void) __closedir (dirstream);
 #if 0
@@ -167,12 +162,7 @@ ttyname (int fd)
       /* Verify readlink result, fall back on iterating through devices.  */
       if (ttyname_buf[0] == '/'
 	  && __xstat64 (_STAT_VER, ttyname_buf, &st1) == 0
-#ifdef _STATBUF_ST_RDEV
-	  && S_ISCHR (st1.st_mode)
-	  && st1.st_rdev == st.st_rdev
-#endif
-	  && st1.st_ino == st.st_ino
-	  && st1.st_dev == st.st_dev)
+	  && is_mytty (&st, &st1))
 	return ttyname_buf;
 
       /* If the link doesn't exist, then it points to a device in another
@@ -186,11 +176,7 @@ ttyname (int fd)
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
     {
-#ifdef _STATBUF_ST_RDEV
-      name = getttyname ("/dev/pts", st.st_rdev, st.st_ino, save, &dostat);
-#else
-      name = getttyname ("/dev/pts", st.st_dev, st.st_ino, save, &dostat);
-#endif
+      name = getttyname ("/dev/pts", &st, save, &dostat);
     }
   else
     {
@@ -200,21 +186,13 @@ ttyname (int fd)
 
   if (!name && dostat != -1)
     {
-#ifdef _STATBUF_ST_RDEV
-      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
-#else
-      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
-#endif
+      name = getttyname ("/dev", &st, save, &dostat);
     }
 
   if (!name && dostat != -1)
     {
       dostat = 1;
-#ifdef _STATBUF_ST_RDEV
-      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
-#else
-      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
-#endif
+      name = getttyname ("/dev", &st, save, &dostat);
     }
 
   return name;
diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
index dd7873d1ff..2b125cfd0b 100644
--- a/sysdeps/unix/sysv/linux/ttyname.h
+++ b/sysdeps/unix/sysv/linux/ttyname.h
@@ -23,7 +23,7 @@
 /* Return true if this is a UNIX98 pty device, as defined in
    linux/Documentation/devices.txt (on linux < 4.10) or
    linux/Documentation/admin-guide/devices.txt (on linux >= 4.10). */
-static inline int
+static inline bool
 is_pty (struct stat64 *sb)
 {
 #ifdef _STATBUF_ST_RDEV
@@ -33,3 +33,15 @@ is_pty (struct stat64 *sb)
   return false;
 #endif
 }
+
+static inline bool
+is_mytty(const struct stat64 *mytty, const struct stat64 *maybe)
+{
+  return (maybe->st_ino == mytty->st_ino
+	  && maybe->st_dev == mytty->st_dev
+#ifdef _STATBUF_ST_RDEV
+	  && S_ISCHR (maybe->st_mode)
+	  && maybe->st_rdev == mytty->st_rdev
+#endif
+	  );
+}
diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index 18f35ef2b7..58eb919c3f 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -31,12 +31,12 @@
 #include "ttyname.h"
 
 static int getttyname_r (char *buf, size_t buflen,
-			 dev_t mydev, ino64_t myino, int save,
+			 const struct stat64 *mytty, int save,
 			 int *dostat);
 
 static int
 attribute_compat_text_section
-getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
+getttyname_r (char *buf, size_t buflen, const struct stat64 *mytty,
 	      int save, int *dostat)
 {
   struct stat64 st;
@@ -52,7 +52,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
     }
 
   while ((d = __readdir64 (dirstream)) != NULL)
-    if ((d->d_fileno == myino || *dostat)
+    if ((d->d_fileno == mytty->st_ino || *dostat)
 	&& strcmp (d->d_name, "stdin")
 	&& strcmp (d->d_name, "stdout")
 	&& strcmp (d->d_name, "stderr"))
@@ -72,12 +72,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
 	cp[0] = '\0';
 
 	if (__xstat64 (_STAT_VER, buf, &st) == 0
-#ifdef _STATBUF_ST_RDEV
-	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
-#else
-	    && d->d_fileno == myino && st.st_dev == mydev
-#endif
-	   )
+	    && is_mytty (mytty, &st))
 	  {
 	    (void) __closedir (dirstream);
 	    __set_errno (save);
@@ -151,12 +146,7 @@ __ttyname_r (int fd, char *buf, size_t buflen)
       /* Verify readlink result, fall back on iterating through devices.  */
       if (buf[0] == '/'
 	  && __xstat64 (_STAT_VER, buf, &st1) == 0
-#ifdef _STATBUF_ST_RDEV
-	  && S_ISCHR (st1.st_mode)
-	  && st1.st_rdev == st.st_rdev
-#endif
-	  && st1.st_ino == st.st_ino
-	  && st1.st_dev == st.st_dev)
+	  && is_mytty (&st, &st1))
 	return 0;
 
       /* If the link doesn't exist, then it points to a device in another
@@ -175,13 +165,8 @@ __ttyname_r (int fd, char *buf, size_t buflen)
 
   if (__xstat64 (_STAT_VER, buf, &st1) == 0 && S_ISDIR (st1.st_mode))
     {
-#ifdef _STATBUF_ST_RDEV
-      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
+      ret = getttyname_r (buf, buflen, &st, save,
 			  &dostat);
-#else
-      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
-			  &dostat);
-#endif
     }
   else
     {
@@ -193,26 +178,16 @@ __ttyname_r (int fd, char *buf, size_t buflen)
     {
       buf[sizeof ("/dev/") - 1] = '\0';
       buflen += sizeof ("pts/") - 1;
-#ifdef _STATBUF_ST_RDEV
-      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
-			  &dostat);
-#else
-      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
+      ret = getttyname_r (buf, buflen, &st, save,
 			  &dostat);
-#endif
     }
 
   if (ret && dostat != -1)
     {
       buf[sizeof ("/dev/") - 1] = '\0';
       dostat = 1;
-#ifdef _STATBUF_ST_RDEV
-      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino,
-			  save, &dostat);
-#else
-      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino,
+      ret = getttyname_r (buf, buflen, &st,
 			  save, &dostat);
-#endif
     }
 
   return ret;
-- 
2.15.0

Re: [PATCH v2 3/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

[Christian Brauner]

On Thu, Nov 02, 2017 at 02:53:44PM -0400, Luke Shumaker wrote:
> In the ttyname and ttyname_r routines on Linux, at several points it
> it needs to test if a given TTY is the TTY we are looking for.  It used to
> be that this test was (to see if `maybe` is `mytty`):
> 
>        __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
>     #ifdef _STATBUF_ST_RDEV
>        && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
>     #else
>        && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev
>     #endif
> 
> This check appears in several places.
> 
> Then, in commit 15e9a4f3 by Christian Brauner
> <christian.brauner@canonical.com>, that check changed to:
> 
>        __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
>     #ifdef _STATBUF_ST_RDEV
>        && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
>     #endif
>        && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev
> 
> That is, he made st_ino and st_dev checks happen even if we have the
> st_rdev member.  This is a good change, because kernel namespaces allow you
> to create a new namespace, and to create a new device with the same
> st_rdev, but isn't the same file.
> 
> However, this check appears twice in each file (ttyname.c and
> ttyname_r.c), but Christian only changed it in one place in each file.
> 
> Make it easy to keep consistent by pulling the check in to a static inline
> function, is_mytty.
> 
> While we're at it, go ahead and have the existing is_pty return a bool
> (instead of an int) to keep ttyname.h consistent, per
> <https://sourceware.org/ml/libc-alpha/2017-10/msg00532.html>.
> 
>   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 |
>   |                                 | 15e9a4f | 15e9a4f | consistent |
>   |---------------------------------+---------+---------+------------|
>   | basic smoketest                 | PASS    | PASS    | PASS       |
>   | no conflict, no match           | PASS[1] | PASS    | PASS       |
>   | no conflict, console            | PASS    | FAIL!   | FAIL       |
>   | conflict, no match              | FAIL    | PASS!   | PASS       |
>   | conflict, console               | FAIL    | FAIL    | FAIL       |
>   | with readlink target            | PASS    | PASS    | PASS       |
>   | with readlink trap; fallback    | FAIL    | FAIL    | FAIL       |
>   | with readlink trap; no fallback | FAIL    | PASS!   | PASS       |
>   | with search-path trap           | FAIL    | FAIL    | PASS!      |
>   |---------------------------------+---------+---------+------------|
>   |                                 | 4/9     | 5/9     | 6/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.
> 
>   This only fixed one testcase, but most of the remaining failures
>   require both this fix and the subsequent fix in order to pass.
> 
> v2:
>  - Rearranged commit order
>  - Expanded commit message
>  - Reformatted is_mytty
>  - Made is_pty and is_mytty return bool instead of int
>  - Specify 'const' on pointer arguments as necessary
> ---
>  ChangeLog                           |  7 +++++++
>  sysdeps/unix/sysv/linux/ttyname.c   | 40 ++++++++----------------------------
>  sysdeps/unix/sysv/linux/ttyname.h   | 14 ++++++++++++-
>  sysdeps/unix/sysv/linux/ttyname_r.c | 41 ++++++++-----------------------------
>  4 files changed, 37 insertions(+), 65 deletions(-)

Fyi, I'm going to fix up the commit message in a few places. The code itself
looks fine to me.

Reviewed-By: Christian Brauner <christian.brauner@ubuntu.com>

> 
> diff --git a/ChangeLog b/ChangeLog
> index 2c7770fbbb..4c282f8f47 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,12 @@
>  2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
>  
> +	[BZ #22145]
> +	* sysdeps/unix/sysv/linux/ttyname.h
> +	(is_pty): Change return type from int to bool.
> +	(is_mytty): New function.
> +	* sysdeps/unix/sysv/linux/ttyname.c: Call is_mytty.
> +	* sysdeps/unix/sysv/linux/ttyname_r.c: Likewise.
> +
>  	* sysdeps/unix/sysv/linux/ttyname.h (is_pty): Update doc reference.
>  
>  	* manual/terminal.texi (Is It a Terminal):
> diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
> index 116cf350e5..6e97d2d455 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
> @@ -35,14 +35,14 @@
>  char *__ttyname;
>  #endif
>  
> -static char *getttyname (const char *dev, dev_t mydev,
> -			 ino64_t myino, int save, int *dostat);
> +static char *getttyname (const char *dev, const struct stat64 *mytty,
> +			 int save, int *dostat);
>  
>  libc_freeres_ptr (static char *getttyname_name);
>  
>  static char *
>  attribute_compat_text_section
> -getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
> +getttyname (const char *dev, const struct stat64 *mytty, int save, int *dostat)
>  {
>    static size_t namelen;
>    struct stat64 st;
> @@ -63,7 +63,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
>      *((char *) __mempcpy (getttyname_name, dev, devlen - 1)) = '/';
>  
>    while ((d = __readdir64 (dirstream)) != NULL)
> -    if ((d->d_fileno == myino || *dostat)
> +    if ((d->d_fileno == mytty->st_ino || *dostat)
>  	&& strcmp (d->d_name, "stdin")
>  	&& strcmp (d->d_name, "stdout")
>  	&& strcmp (d->d_name, "stderr"))
> @@ -85,12 +85,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
>  	  }
>  	memcpy (&getttyname_name[devlen], d->d_name, dlen);
>  	if (__xstat64 (_STAT_VER, getttyname_name, &st) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
> -#else
> -	    && d->d_fileno == myino && st.st_dev == mydev
> -#endif
> -	   )
> +	    && is_mytty (mytty, &st))
>  	  {
>  	    (void) __closedir (dirstream);
>  #if 0
> @@ -167,12 +162,7 @@ ttyname (int fd)
>        /* Verify readlink result, fall back on iterating through devices.  */
>        if (ttyname_buf[0] == '/'
>  	  && __xstat64 (_STAT_VER, ttyname_buf, &st1) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	  && S_ISCHR (st1.st_mode)
> -	  && st1.st_rdev == st.st_rdev
> -#endif
> -	  && st1.st_ino == st.st_ino
> -	  && st1.st_dev == st.st_dev)
> +	  && is_mytty (&st, &st1))
>  	return ttyname_buf;
>  
>        /* If the link doesn't exist, then it points to a device in another
> @@ -186,11 +176,7 @@ ttyname (int fd)
>  
>    if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
>      {
> -#ifdef _STATBUF_ST_RDEV
> -      name = getttyname ("/dev/pts", st.st_rdev, st.st_ino, save, &dostat);
> -#else
> -      name = getttyname ("/dev/pts", st.st_dev, st.st_ino, save, &dostat);
> -#endif
> +      name = getttyname ("/dev/pts", &st, save, &dostat);
>      }
>    else
>      {
> @@ -200,21 +186,13 @@ ttyname (int fd)
>  
>    if (!name && dostat != -1)
>      {
> -#ifdef _STATBUF_ST_RDEV
> -      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
> -#else
> -      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
> -#endif
> +      name = getttyname ("/dev", &st, save, &dostat);
>      }
>  
>    if (!name && dostat != -1)
>      {
>        dostat = 1;
> -#ifdef _STATBUF_ST_RDEV
> -      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
> -#else
> -      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
> -#endif
> +      name = getttyname ("/dev", &st, save, &dostat);
>      }
>  
>    return name;
> diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
> index dd7873d1ff..2b125cfd0b 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.h
> +++ b/sysdeps/unix/sysv/linux/ttyname.h
> @@ -23,7 +23,7 @@
>  /* Return true if this is a UNIX98 pty device, as defined in
>     linux/Documentation/devices.txt (on linux < 4.10) or
>     linux/Documentation/admin-guide/devices.txt (on linux >= 4.10). */
> -static inline int
> +static inline bool
>  is_pty (struct stat64 *sb)
>  {
>  #ifdef _STATBUF_ST_RDEV
> @@ -33,3 +33,15 @@ is_pty (struct stat64 *sb)
>    return false;
>  #endif
>  }
> +
> +static inline bool
> +is_mytty(const struct stat64 *mytty, const struct stat64 *maybe)
> +{
> +  return (maybe->st_ino == mytty->st_ino
> +	  && maybe->st_dev == mytty->st_dev
> +#ifdef _STATBUF_ST_RDEV
> +	  && S_ISCHR (maybe->st_mode)
> +	  && maybe->st_rdev == mytty->st_rdev
> +#endif
> +	  );
> +}
> diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
> index 18f35ef2b7..58eb919c3f 100644
> --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> @@ -31,12 +31,12 @@
>  #include "ttyname.h"
>  
>  static int getttyname_r (char *buf, size_t buflen,
> -			 dev_t mydev, ino64_t myino, int save,
> +			 const struct stat64 *mytty, int save,
>  			 int *dostat);
>  
>  static int
>  attribute_compat_text_section
> -getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
> +getttyname_r (char *buf, size_t buflen, const struct stat64 *mytty,
>  	      int save, int *dostat)
>  {
>    struct stat64 st;
> @@ -52,7 +52,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
>      }
>  
>    while ((d = __readdir64 (dirstream)) != NULL)
> -    if ((d->d_fileno == myino || *dostat)
> +    if ((d->d_fileno == mytty->st_ino || *dostat)
>  	&& strcmp (d->d_name, "stdin")
>  	&& strcmp (d->d_name, "stdout")
>  	&& strcmp (d->d_name, "stderr"))
> @@ -72,12 +72,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
>  	cp[0] = '\0';
>  
>  	if (__xstat64 (_STAT_VER, buf, &st) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
> -#else
> -	    && d->d_fileno == myino && st.st_dev == mydev
> -#endif
> -	   )
> +	    && is_mytty (mytty, &st))
>  	  {
>  	    (void) __closedir (dirstream);
>  	    __set_errno (save);
> @@ -151,12 +146,7 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>        /* Verify readlink result, fall back on iterating through devices.  */
>        if (buf[0] == '/'
>  	  && __xstat64 (_STAT_VER, buf, &st1) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	  && S_ISCHR (st1.st_mode)
> -	  && st1.st_rdev == st.st_rdev
> -#endif
> -	  && st1.st_ino == st.st_ino
> -	  && st1.st_dev == st.st_dev)
> +	  && is_mytty (&st, &st1))
>  	return 0;
>  
>        /* If the link doesn't exist, then it points to a device in another
> @@ -175,13 +165,8 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>  
>    if (__xstat64 (_STAT_VER, buf, &st1) == 0 && S_ISDIR (st1.st_mode))
>      {
> -#ifdef _STATBUF_ST_RDEV
> -      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
> +      ret = getttyname_r (buf, buflen, &st, save,
>  			  &dostat);
> -#else
> -      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
> -			  &dostat);
> -#endif
>      }
>    else
>      {
> @@ -193,26 +178,16 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>      {
>        buf[sizeof ("/dev/") - 1] = '\0';
>        buflen += sizeof ("pts/") - 1;
> -#ifdef _STATBUF_ST_RDEV
> -      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
> -			  &dostat);
> -#else
> -      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
> +      ret = getttyname_r (buf, buflen, &st, save,
>  			  &dostat);
> -#endif
>      }
>  
>    if (ret && dostat != -1)
>      {
>        buf[sizeof ("/dev/") - 1] = '\0';
>        dostat = 1;
> -#ifdef _STATBUF_ST_RDEV
> -      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino,
> -			  save, &dostat);
> -#else
> -      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino,
> +      ret = getttyname_r (buf, buflen, &st,
>  			  save, &dostat);
> -#endif
>      }
>  
>    return ret;
> -- 
> 2.15.0
> 

Re: [PATCH v2 3/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 13289 bytes --]

On Thu, Nov 02, 2017 at 02:53:44PM -0400, Luke Shumaker wrote:
> In the ttyname and ttyname_r routines on Linux, at several points it
> it needs to test if a given TTY is the TTY we are looking for.  It used to
> be that this test was (to see if `maybe` is `mytty`):
> 
>        __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
>     #ifdef _STATBUF_ST_RDEV
>        && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
>     #else
>        && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev
>     #endif
> 
> This check appears in several places.
> 
> Then, in commit 15e9a4f3 by Christian Brauner
> <christian.brauner@canonical.com>, that check changed to:
> 
>        __xstat64(_STAT_VER, maybe_filename, &maybe) == 0
>     #ifdef _STATBUF_ST_RDEV
>        && S_ISCHR(maybe.st_mode) && maybe.st_rdev == mytty.st_rdev
>     #endif
>        && maybe.st_ino == mytty.st_ino && maybe.st_dev == mytty.st_dev
> 
> That is, he made st_ino and st_dev checks happen even if we have the
> st_rdev member.  This is a good change, because kernel namespaces allow you
> to create a new namespace, and to create a new device with the same
> st_rdev, but isn't the same file.
> 
> However, this check appears twice in each file (ttyname.c and
> ttyname_r.c), but Christian only changed it in one place in each file.

Firstly, the way BZ #22145 is mentioned in the subject line creates
a false impression that this commit fixes the bug.

Secondly, the text describes commit 15e9a4f3 in fine details
(imo it goes too far) yet fails to explain the difference between the case
addressed by 15e9a4f3 (ttyname and __ttyname_r) and the other case
(getttyname and getttyname_r).

> Make it easy to keep consistent by pulling the check in to a static inline
> function, is_mytty.
> 
> While we're at it, go ahead and have the existing is_pty return a bool
> (instead of an int) to keep ttyname.h consistent, per
> <https://sourceware.org/ml/libc-alpha/2017-10/msg00532.html>.
> 
>   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 |
>   |                                 | 15e9a4f | 15e9a4f | consistent |
>   |---------------------------------+---------+---------+------------|
>   | basic smoketest                 | PASS    | PASS    | PASS       |
>   | no conflict, no match           | PASS[1] | PASS    | PASS       |
>   | no conflict, console            | PASS    | FAIL!   | FAIL       |
>   | conflict, no match              | FAIL    | PASS!   | PASS       |
>   | conflict, console               | FAIL    | FAIL    | FAIL       |
>   | with readlink target            | PASS    | PASS    | PASS       |
>   | with readlink trap; fallback    | FAIL    | FAIL    | FAIL       |
>   | with readlink trap; no fallback | FAIL    | PASS!   | PASS       |
>   | with search-path trap           | FAIL    | FAIL    | PASS!      |
>   |---------------------------------+---------+---------+------------|
>   |                                 | 4/9     | 5/9     | 6/9        |

Wouldn't it be sufficient 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?

> 
>   [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.
> 
>   This only fixed one testcase, but most of the remaining failures
>   require both this fix and the subsequent fix in order to pass.
> 
> v2:
>  - Rearranged commit order
>  - Expanded commit message
>  - Reformatted is_mytty
>  - Made is_pty and is_mytty return bool instead of int
>  - Specify 'const' on pointer arguments as necessary
> ---
>  ChangeLog                           |  7 +++++++
>  sysdeps/unix/sysv/linux/ttyname.c   | 40 ++++++++----------------------------
>  sysdeps/unix/sysv/linux/ttyname.h   | 14 ++++++++++++-
>  sysdeps/unix/sysv/linux/ttyname_r.c | 41 ++++++++-----------------------------
>  4 files changed, 37 insertions(+), 65 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 2c7770fbbb..4c282f8f47 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,12 @@
>  2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
>  
> +	[BZ #22145]
> +	* sysdeps/unix/sysv/linux/ttyname.h
> +	(is_pty): Change return type from int to bool.
> +	(is_mytty): New function.
> +	* sysdeps/unix/sysv/linux/ttyname.c: Call is_mytty.

This should mention all affected functions: getttyname and ttyname.

> +	* sysdeps/unix/sysv/linux/ttyname_r.c: Likewise.

Likewise, this should mention all affected functions: getttyname_r
and __ttyname_r.

> +
>  	* sysdeps/unix/sysv/linux/ttyname.h (is_pty): Update doc reference.
>  
>  	* manual/terminal.texi (Is It a Terminal):
> diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
> index 116cf350e5..6e97d2d455 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
> @@ -35,14 +35,14 @@
>  char *__ttyname;
>  #endif
>  
> -static char *getttyname (const char *dev, dev_t mydev,
> -			 ino64_t myino, int save, int *dostat);
> +static char *getttyname (const char *dev, const struct stat64 *mytty,
> +			 int save, int *dostat);
>  
>  libc_freeres_ptr (static char *getttyname_name);
>  
>  static char *
>  attribute_compat_text_section
> -getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
> +getttyname (const char *dev, const struct stat64 *mytty, int save, int *dostat)
>  {
>    static size_t namelen;
>    struct stat64 st;
> @@ -63,7 +63,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
>      *((char *) __mempcpy (getttyname_name, dev, devlen - 1)) = '/';
>  
>    while ((d = __readdir64 (dirstream)) != NULL)
> -    if ((d->d_fileno == myino || *dostat)
> +    if ((d->d_fileno == mytty->st_ino || *dostat)
>  	&& strcmp (d->d_name, "stdin")
>  	&& strcmp (d->d_name, "stdout")
>  	&& strcmp (d->d_name, "stderr"))
> @@ -85,12 +85,7 @@ getttyname (const char *dev, dev_t mydev, ino64_t myino, int save, int *dostat)
>  	  }
>  	memcpy (&getttyname_name[devlen], d->d_name, dlen);
>  	if (__xstat64 (_STAT_VER, getttyname_name, &st) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
> -#else
> -	    && d->d_fileno == myino && st.st_dev == mydev
> -#endif
> -	   )
> +	    && is_mytty (mytty, &st))
>  	  {
>  	    (void) __closedir (dirstream);
>  #if 0
> @@ -167,12 +162,7 @@ ttyname (int fd)
>        /* Verify readlink result, fall back on iterating through devices.  */
>        if (ttyname_buf[0] == '/'
>  	  && __xstat64 (_STAT_VER, ttyname_buf, &st1) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	  && S_ISCHR (st1.st_mode)
> -	  && st1.st_rdev == st.st_rdev
> -#endif
> -	  && st1.st_ino == st.st_ino
> -	  && st1.st_dev == st.st_dev)
> +	  && is_mytty (&st, &st1))
>  	return ttyname_buf;
>  
>        /* If the link doesn't exist, then it points to a device in another
> @@ -186,11 +176,7 @@ ttyname (int fd)
>  
>    if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
>      {
> -#ifdef _STATBUF_ST_RDEV
> -      name = getttyname ("/dev/pts", st.st_rdev, st.st_ino, save, &dostat);
> -#else
> -      name = getttyname ("/dev/pts", st.st_dev, st.st_ino, save, &dostat);
> -#endif
> +      name = getttyname ("/dev/pts", &st, save, &dostat);
>      }
>    else
>      {
> @@ -200,21 +186,13 @@ ttyname (int fd)
>  
>    if (!name && dostat != -1)
>      {
> -#ifdef _STATBUF_ST_RDEV
> -      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
> -#else
> -      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
> -#endif
> +      name = getttyname ("/dev", &st, save, &dostat);
>      }
>  
>    if (!name && dostat != -1)
>      {
>        dostat = 1;
> -#ifdef _STATBUF_ST_RDEV
> -      name = getttyname ("/dev", st.st_rdev, st.st_ino, save, &dostat);
> -#else
> -      name = getttyname ("/dev", st.st_dev, st.st_ino, save, &dostat);
> -#endif
> +      name = getttyname ("/dev", &st, save, &dostat);
>      }
>  
>    return name;
> diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
> index dd7873d1ff..2b125cfd0b 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.h
> +++ b/sysdeps/unix/sysv/linux/ttyname.h
> @@ -23,7 +23,7 @@
>  /* Return true if this is a UNIX98 pty device, as defined in
>     linux/Documentation/devices.txt (on linux < 4.10) or
>     linux/Documentation/admin-guide/devices.txt (on linux >= 4.10). */
> -static inline int
> +static inline bool
>  is_pty (struct stat64 *sb)
>  {
>  #ifdef _STATBUF_ST_RDEV
> @@ -33,3 +33,15 @@ is_pty (struct stat64 *sb)
>    return false;
>  #endif
>  }
> +
> +static inline bool
> +is_mytty(const struct stat64 *mytty, const struct stat64 *maybe)
> +{
> +  return (maybe->st_ino == mytty->st_ino
> +	  && maybe->st_dev == mytty->st_dev
> +#ifdef _STATBUF_ST_RDEV
> +	  && S_ISCHR (maybe->st_mode)
> +	  && maybe->st_rdev == mytty->st_rdev
> +#endif
> +	  );
> +}
> diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
> index 18f35ef2b7..58eb919c3f 100644
> --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> @@ -31,12 +31,12 @@
>  #include "ttyname.h"
>  
>  static int getttyname_r (char *buf, size_t buflen,
> -			 dev_t mydev, ino64_t myino, int save,
> +			 const struct stat64 *mytty, int save,
>  			 int *dostat);
>  
>  static int
>  attribute_compat_text_section
> -getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
> +getttyname_r (char *buf, size_t buflen, const struct stat64 *mytty,
>  	      int save, int *dostat)
>  {
>    struct stat64 st;
> @@ -52,7 +52,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
>      }
>  
>    while ((d = __readdir64 (dirstream)) != NULL)
> -    if ((d->d_fileno == myino || *dostat)
> +    if ((d->d_fileno == mytty->st_ino || *dostat)
>  	&& strcmp (d->d_name, "stdin")
>  	&& strcmp (d->d_name, "stdout")
>  	&& strcmp (d->d_name, "stderr"))
> @@ -72,12 +72,7 @@ getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
>  	cp[0] = '\0';
>  
>  	if (__xstat64 (_STAT_VER, buf, &st) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	    && S_ISCHR (st.st_mode) && st.st_rdev == mydev
> -#else
> -	    && d->d_fileno == myino && st.st_dev == mydev
> -#endif
> -	   )
> +	    && is_mytty (mytty, &st))
>  	  {
>  	    (void) __closedir (dirstream);
>  	    __set_errno (save);
> @@ -151,12 +146,7 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>        /* Verify readlink result, fall back on iterating through devices.  */
>        if (buf[0] == '/'
>  	  && __xstat64 (_STAT_VER, buf, &st1) == 0
> -#ifdef _STATBUF_ST_RDEV
> -	  && S_ISCHR (st1.st_mode)
> -	  && st1.st_rdev == st.st_rdev
> -#endif
> -	  && st1.st_ino == st.st_ino
> -	  && st1.st_dev == st.st_dev)
> +	  && is_mytty (&st, &st1))
>  	return 0;
>  
>        /* If the link doesn't exist, then it points to a device in another
> @@ -175,13 +165,8 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>  
>    if (__xstat64 (_STAT_VER, buf, &st1) == 0 && S_ISDIR (st1.st_mode))
>      {
> -#ifdef _STATBUF_ST_RDEV
> -      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
> +      ret = getttyname_r (buf, buflen, &st, save,
>  			  &dostat);
> -#else
> -      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
> -			  &dostat);
> -#endif
>      }
>    else
>      {
> @@ -193,26 +178,16 @@ __ttyname_r (int fd, char *buf, size_t buflen)
>      {
>        buf[sizeof ("/dev/") - 1] = '\0';
>        buflen += sizeof ("pts/") - 1;
> -#ifdef _STATBUF_ST_RDEV
> -      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino, save,
> -			  &dostat);
> -#else
> -      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino, save,
> +      ret = getttyname_r (buf, buflen, &st, save,
>  			  &dostat);
> -#endif
>      }
>  
>    if (ret && dostat != -1)
>      {
>        buf[sizeof ("/dev/") - 1] = '\0';
>        dostat = 1;
> -#ifdef _STATBUF_ST_RDEV
> -      ret = getttyname_r (buf, buflen, st.st_rdev, st.st_ino,
> -			  save, &dostat);
> -#else
> -      ret = getttyname_r (buf, buflen, st.st_dev, st.st_ino,
> +      ret = getttyname_r (buf, buflen, &st,
>  			  save, &dostat);
> -#endif
>      }
>  
>    return ret;

The change itself looks OK, but as it essentially combines two changes
(the first one is refactoring that is almost no-op, the second one is
a fix by using is_mytty in getttyname and getttyname_r),  I suggest
to split the commit in two.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 3/5] linux ttyname and ttyname_r: Make the TTY equivalence tests consistent [BZ #22145]

[Luke Shumaker]

On Thu, 02 Nov 2017 14:53:44 -0400,
Luke Shumaker wrote:
> @@ -33,3 +33,15 @@ is_pty (struct stat64 *sb)
>    return false;
>  #endif
>  }
> +
> +static inline bool
> +is_mytty(const struct stat64 *mytty, const struct stat64 *maybe)

Style: there should be a space before the "(".

-- 
Happy hacking,
~ Luke Shumaker

[PATCH v2 5/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Luke Shumaker]

Add a new tst-ttyname test that includes several named sub-testcases.

This patch is ordered the fixes it tests for are applied (to avoid
breaking `git bisect`), but for reference, here's how each relevent
change so far affected the testcases in this commit:

  |                                 | 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.

Each of the failing tests before 15e9a4f is 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.

v2:
 - Rearranged commit order
 - Fix code style
 - Expanded commit message
 - Pulled xtouch, become-root, and chroot setup into functions
 - Better tracking of test failures
 - Fixed the "with search-path trap" test case
 - Added test cases for the readlink target directly
 - More readable diagnostic output
 - Test with both shared and private procfs
---
 ChangeLog                             |   4 +
 sysdeps/unix/sysv/linux/Makefile      |   3 +-
 sysdeps/unix/sysv/linux/tst-ttyname.c | 582 ++++++++++++++++++++++++++++++++++
 3 files changed, 588 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c

diff --git a/ChangeLog b/ChangeLog
index b5d214ae6f..1982a498ce 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
 
+	[BZ #22145]
+	* sysdeps/unix/sysv/linux/tst-ttyname.c: New file.
+	* sysdeps/unix/sysv/linux/Makefile: Add tst-ttyname to tests.
+
 	[BZ #22145]
 	* sysdeps/unix/sysv/linux/ttyname.c (ttyname):
 	Defer is_pty check until end.
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 0c8a009b5e..69edb75209 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -50,7 +50,8 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
 		  bits/siginfo-arch.h bits/siginfo-consts-arch.h
 
 tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
-	 tst-quota tst-sync_file_range test-errno-linux tst-sysconf-iov_max
+	 tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
+	 test-errno-linux
 
 # Generate the list of SYS_* macros for the system calls (__NR_*
 # macros).  The file syscall-names.list contains all possible system
diff --git a/sysdeps/unix/sysv/linux/tst-ttyname.c b/sysdeps/unix/sysv/linux/tst-ttyname.c
new file mode 100644
index 0000000000..45d356cab4
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-ttyname.c
@@ -0,0 +1,582 @@
+/* Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#include <dirent.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mount.h>
+#include <sys/prctl.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+
+/* generic utilities */
+
+#define VERIFY(expr)                                                    \
+  do {                                                                  \
+    if (!(expr))                                                        \
+      {                                                                 \
+        printf ("error: %s:%d: %s: %m\n",                               \
+                __FILE__, __LINE__, #expr);                             \
+        exit (1);                                                       \
+      }                                                                 \
+  } while (0)
+
+static void
+xtouch (const char *path, mode_t mode)
+{
+  xclose (xopen (path, O_WRONLY|O_CREAT|O_NOCTTY, mode));
+}
+
+static size_t
+trim_prefix (char *str, size_t str_len, const char *prefix)
+{
+  size_t prefix_len = strlen (prefix);
+  if (str_len > prefix_len && memcmp (str, prefix, prefix_len) == 0)
+    {
+      memmove (str, str + prefix_len, str_len - prefix_len);
+      return str_len - prefix_len;
+    }
+  return str_len;
+}
+
+/* returns a pointer to static storage */
+static char *
+xreadlink (const char *linkname)
+{
+  static char target[PATH_MAX+1];
+  ssize_t target_len = readlink (linkname, target, PATH_MAX);
+  VERIFY (target_len > 0);
+  target_len = trim_prefix (target, target_len, "(unreachable)");
+  target[target_len] = '\0';
+  return target;
+}
+
+static void
+become_root_in_mount_ns (void) {
+  uid_t orig_uid = getuid ();
+  gid_t orig_gid = getgid ();
+
+  support_become_root ();
+
+  if (unshare (CLONE_NEWNS) < 0)
+    FAIL_UNSUPPORTED ("could not enter new mount namespace");
+
+  /* support_become_root might have put us in a new user namespace;
+     most filesystems (including tmpfs) don't allow file or directory
+     creation from a user namespace unless uid and gid maps are set,
+     even if we have root privileges in the namespace (failing with
+     EOVERFLOW, since the uid overflows the empty (0-length) uid map).
+
+     Also, stat always reports that uid and gid maps are empty, so we
+     have to try actually reading from them to check if they are
+     empty. */
+  int fd;
+
+  if ((fd = open ("/proc/self/uid_map", O_RDWR, 0)) >= 0)
+    {
+      char buf;
+      if (read (fd, &buf, 1) == 0)
+	{
+	  char *str = xasprintf ("0 %ld 1\n", (long)orig_uid);
+	  if (write (fd, str, strlen (str)) < 0)
+	    FAIL_EXIT1 ("write (uid_map, \"%s\"): %m", str);
+	  free (str);
+	}
+      xclose (fd);
+    }
+
+  /* Oh, but we can't set the gid map until we turn off setgroups. */
+  if ((fd = open ("/proc/self/setgroups", O_WRONLY, 0)) >= 0)
+    {
+      const char *str = "deny";
+      if (write (fd, str, strlen (str)) < 0)
+	FAIL_EXIT1 ("write (setroups, \"%s\"): %m", str);
+      xclose (fd);
+    }
+
+  if ((fd = open ("/proc/self/gid_map", O_RDWR, 0)) >= 0)
+    {
+      char buf;
+      if (read (fd, &buf, 1) == 0)
+	{
+	  char *str = xasprintf ("0 %ld 1\n", (long)orig_gid);
+	  if (write (fd, str, strlen (str)) < 0)
+	    FAIL_EXIT1 ("write (gid_map, \"%s\"): %m", str);
+	  free (str);
+	}
+      xclose (fd);
+    }
+}
+
+/* plain ttyname runner */
+
+struct result {
+  const char *name;
+  int err;
+};
+
+/* strings in result structure are in static storage */
+static struct result
+run_ttyname (int fd)
+{
+  struct result ret;
+  errno = 0;
+  ret.name = ttyname (fd);
+  ret.err = errno;
+  return ret;
+}
+
+static bool
+eq_ttyname (struct result actual, struct result expected)
+{
+  if ((actual.err == expected.err) &&
+      (!actual.name == !expected.name) &&
+      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
+    {
+      char *name = expected.name
+	? xasprintf ("\"%s\"", expected.name)
+	: strdup ("NULL");
+      printf ("info:      ttyname: PASS {name=%s, errno=%d}\n",
+	      name, expected.err);
+      free (name);
+      return true;
+    }
+
+  char *actual_name = actual.name
+    ? xasprintf ("\"%s\"", actual.name)
+    : strdup ("NULL");
+  char *expected_name = expected.name
+    ? xasprintf ("\"%s\"", expected.name)
+    : strdup ("NULL");
+  printf ("error:     ttyname: actual {name=%s, errno=%d} != expected {name=%s, errno=%d}\n",
+	  actual_name, actual.err,
+	  expected_name, expected.err);
+  free (actual_name);
+  free (expected_name);
+  return false;
+}
+
+/* ttyname_r runner */
+
+struct result_r {
+  const char *name;
+  int ret;
+  int err;
+};
+
+/* strings in result structure are in static storage */
+static struct result_r
+run_ttyname_r (int fd)
+{
+  static char buf[TTY_NAME_MAX];
+
+  struct result_r ret;
+  errno = 0;
+  ret.ret = ttyname_r (fd, buf, TTY_NAME_MAX);
+  ret.err = errno;
+  if (ret.ret == 0)
+    ret.name = buf;
+  else
+    ret.name = NULL;
+  return ret;
+}
+
+static bool
+eq_ttyname_r (struct result_r actual, struct result_r expected)
+{
+  if ((actual.err == expected.err) &&
+      (actual.ret == expected.ret) &&
+      (!actual.name == !expected.name) &&
+      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
+    {
+      char *name = expected.name
+	? xasprintf ("\"%s\"", expected.name)
+	: strdup ("NULL");
+      printf ("info:      ttyname_r: PASS {name=%s, ret=%d, errno=%d}\n",
+              name, expected.ret, expected.err);
+      free (name);
+      return true;
+    }
+
+  char *actual_name = actual.name
+    ? xasprintf ("\"%s\"", actual.name)
+    : strdup ("NULL");
+  char *expected_name = expected.name
+    ? xasprintf ("\"%s\"", expected.name)
+    : strdup ("NULL");
+  printf ("error:     ttyname_r: actual {name=%s, ret=%d, errno=%d} != expected {name=%s, ret=%d, errno=%d}\n",
+	  actual_name, actual.ret, actual.err,
+	  expected_name, expected.ret, expected.err);
+  free (actual_name);
+  free (expected_name);
+  return false;
+}
+
+/* combined runner */
+
+static bool
+doit (int fd, const char *testname, struct result_r expected_r) {
+  struct result expected = {.name=expected_r.name, .err=expected_r.ret};
+  bool ret = true;
+
+  printf ("info:    testcase: %s\n", testname);
+
+  ret = eq_ttyname (run_ttyname (fd), expected) && ret;
+  ret = eq_ttyname_r (run_ttyname_r (fd), expected_r) && ret;
+
+  if (!ret)
+    support_record_failure ();
+
+  return ret;
+}
+
+/* chroot setup */
+
+static char *chrootdir;
+
+static void
+prepare (int argc, char **argv)
+{
+  chrootdir = xasprintf ("%s/tst-ttyname-XXXXXX", test_dir);
+  if (mkdtemp (chrootdir) == NULL)
+    FAIL_EXIT1 ("mkdtemp (\"%s\"): %m", chrootdir);
+  add_temp_file (chrootdir);
+}
+#define PREPARE prepare
+
+/* These chroot setup functions put the TTY at at "/console" (where it
+   won't be found by ttyname), and create "/dev/console" as an
+   ordinary file.  This way, it's easier to write test-cases that
+   expect ttyname to fail; test-cases that expect it to succeed need
+   to explicitly remount it at "/dev/console". */
+
+static int
+do_in_chroot_1 (int (*cb)(const char *, int)) {
+  printf ("info:  entering chroot 1\n");
+
+  /* Open the PTS that we'll be testing on. */
+  int master;
+  char *slavename;
+  VERIFY ((master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK)) >= 0);
+  VERIFY ((slavename = ptsname (master)));
+  VERIFY (unlockpt (master) == 0);
+  if (strncmp (slavename, "/dev/pts/", 9) != 0)
+    FAIL_UNSUPPORTED ("slave pseudo-terminal is not under /dev/pts/: %s",
+                      slavename);
+  int slave = xopen (slavename, O_RDWR, 0);
+  if (!doit (slave, "basic smoketest",
+             (struct result_r){.name=slavename, .ret=0, .err=0}))
+    return 1;
+
+  pid_t pid = xfork ();
+  if (pid == 0)
+    {
+      xclose (master);
+
+      become_root_in_mount_ns ();
+
+      VERIFY (mount ("tmpfs", chrootdir, "tmpfs", 0, "mode=755") == 0);
+      VERIFY (chdir (chrootdir) == 0);
+
+      xmkdir ("proc", 0755);
+      xmkdir ("dev", 0755);
+      xmkdir ("dev/pts", 0755);
+
+      VERIFY (mount ("/proc", "proc", NULL, MS_BIND|MS_REC, NULL) == 0);
+      VERIFY (mount ("devpts", "dev/pts", "devpts",
+                     MS_NOSUID|MS_NOEXEC,
+                     "newinstance,ptmxmode=0666,mode=620") == 0);
+      VERIFY (symlink ("pts/ptmx", "dev/ptmx") == 0);
+
+      xtouch ("console", 0);
+      xtouch ("dev/console", 0);
+      VERIFY (mount (slavename, "console", NULL, MS_BIND, NULL) == 0);
+
+      xchroot (".");
+
+      char *linkname = xasprintf ("/proc/self/fd/%d", slave);
+      char *target = xreadlink (linkname);
+      VERIFY (strcmp (target, slavename) == 0);
+      free (linkname);
+
+      _exit (cb (slavename, slave));
+    }
+  int status;
+  xwaitpid (pid, &status, 0);
+  VERIFY (WIFEXITED (status));
+  xclose (master);
+  xclose (slave);
+  return WEXITSTATUS (status);
+}
+
+static int
+do_in_chroot_2 (int (*cb)(const char *, int)) {
+  printf ("info:  entering chroot 2\n");
+
+  int pid_pipe[2];
+  xpipe (pid_pipe);
+  int exit_pipe[2];
+  xpipe (exit_pipe);
+
+  /* Open the PTS that we'll be testing on. */
+  int master;
+  char *slavename;
+  VERIFY ((master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK)) >= 0);
+  VERIFY ((slavename = ptsname (master)));
+  VERIFY (unlockpt (master) == 0);
+  if (strncmp (slavename, "/dev/pts/", 9) != 0)
+    FAIL_UNSUPPORTED ("slave pseudo-terminal is not under /dev/pts/: %s",
+                      slavename);
+  /* wait until in a new mount ns to open the slave */
+
+  /* enable `wait`ing on grandchildren */
+  VERIFY (prctl (PR_SET_CHILD_SUBREAPER, 1) == 0);
+
+  pid_t pid = xfork (); /* outer child */
+  if (pid == 0)
+    {
+      xclose (master);
+      xclose (pid_pipe[0]);
+      xclose (exit_pipe[1]);
+
+      become_root_in_mount_ns ();
+
+      int slave = xopen (slavename, O_RDWR, 0);
+      if (!doit (slave, "basic smoketest",
+                 (struct result_r){.name=slavename, .ret=0, .err=0}))
+        _exit (1);
+
+      VERIFY (mount ("tmpfs", chrootdir, "tmpfs", 0, "mode=755") == 0);
+      VERIFY (chdir (chrootdir) == 0);
+
+      xmkdir ("proc", 0755);
+      xmkdir ("dev", 0755);
+      xmkdir ("dev/pts", 0755);
+
+      VERIFY (mount ("devpts", "dev/pts", "devpts",
+                     MS_NOSUID|MS_NOEXEC,
+                     "newinstance,ptmxmode=0666,mode=620") == 0);
+      VERIFY (symlink ("pts/ptmx", "dev/ptmx") == 0);
+
+      xtouch ("console", 0);
+      xtouch ("dev/console", 0);
+      VERIFY (mount (slavename, "console", NULL, MS_BIND, NULL) == 0);
+
+      xchroot (".");
+
+      if (unshare (CLONE_NEWNS | CLONE_NEWPID) < 0)
+        FAIL_UNSUPPORTED ("could not enter new PID namespace");
+      pid = xfork (); /* inner child */
+      if (pid == 0)
+        {
+          xclose (pid_pipe[1]);
+
+          /* wait until the outer child has exited */
+          char c;
+          VERIFY (read (exit_pipe[0], &c, 1) == 0);
+          xclose (exit_pipe[0]);
+
+          VERIFY (mount ("proc", "/proc", "proc",
+                         MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) == 0);
+
+          char *linkname = xasprintf ("/proc/self/fd/%d", slave);
+          char *target = xreadlink (linkname);
+          VERIFY (strcmp (target, strrchr (slavename, '/')) == 0);
+          free (linkname);
+
+          _exit (cb (slavename, slave));
+        }
+      xwrite (pid_pipe[1], &pid, sizeof pid);
+      _exit (0);
+    }
+  xclose (pid_pipe[1]);
+  xclose (exit_pipe[0]);
+  xclose (exit_pipe[1]);
+
+  /* wait for the outer child */
+  int status;
+  xwaitpid (pid, &status, 0);
+  VERIFY (WIFEXITED (status));
+  int ret = WEXITSTATUS (status);
+  if (ret != 0)
+    return ret;
+
+  /* set 'pid' to the inner child */
+  VERIFY (read (pid_pipe[0], &pid, sizeof pid) == sizeof pid);
+  xclose (pid_pipe[0]);
+
+  /* wait for the inner child */
+  xwaitpid (pid, &status, 0);
+  VERIFY (WIFEXITED (status));
+  xclose (master);
+  return WEXITSTATUS (status);
+}
+
+/* main test */
+
+static int
+run_chroot_tests (const char *slavename, int slave)
+{
+  struct stat st;
+  bool ok = true;
+
+  /* There are 3 groups of tests here.  The first group throws a
+     wrench into the works in a generic way, and verifies that ttyname
+     copes correctly.  The remaining groups are increasingly
+     convoluted, as we direct that wrench towards specific parts of
+     the routine, and try to break it in specific ways. */
+
+  /* Basic tests that it doesn't get confused by multiple devpts
+     instances. */
+  {
+    VERIFY (stat (slavename, &st) < 0); /* sanity check */
+    ok = doit (slave, "no conflict, no match",
+               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
+    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+    ok = doit (slave, "no conflict, console",
+               (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
+    VERIFY (umount ("/dev/console") == 0);
+
+    /* keep creating PTYs until we we get a name collision */
+    while (stat (slavename, &st) < 0)
+      posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK);
+    VERIFY (stat (slavename, &st) == 0);
+
+    ok = doit (slave, "conflict, no match",
+               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
+    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+    ok = doit (slave, "conflict, console",
+               (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
+    VERIFY (umount ("/dev/console") == 0);
+  }
+
+  /* The first tests kinda assumed that they hit certain code-paths
+     based on assuming that the readlink target is 'slavename', but
+     that's not quite always true.  They're still a good preliminary
+     sanity check, so keep them, but let's add tests that make sure
+     that those code-paths are hit by doing a readlink ourself. */
+  {
+    char *linkname = xasprintf ("/proc/self/fd/%d", slave);
+    char *target = xreadlink (linkname);
+    free (linkname);
+    /* Depeding on how we set up the chroot, the kernel may or may not
+     * trim the leading path to the target (it may give us "/6",
+     * instead of "/dev/pts/6").  This test group relies on the target
+     * existing, so guarantee that (so create a file at "/6" if
+     * necessary). */
+    if (stat (target, &st) < 0)
+      {
+        VERIFY (errno == ENOENT);
+        xtouch (target, 0);
+      }
+
+    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+    VERIFY (mount ("/console", target, NULL, MS_BIND, NULL) == 0);
+    ok = doit (slave, "with readlink target",
+               (struct result_r){.name=target, .ret=0, .err=0}) && ok;
+    VERIFY (umount (target) == 0);
+    VERIFY (umount ("/dev/console") == 0);
+
+    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
+    VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
+    ok = doit (slave, "with readlink trap; fallback",
+               (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
+    VERIFY (umount (target) == 0);
+    VERIFY (umount ("/dev/console") == 0);
+
+    VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
+    ok = doit (slave, "with readlink trap; no fallback",
+               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
+    VERIFY (umount (target) == 0);
+  }
+
+  /* This test is extra-convoluted because we need to control the
+     order of files within the directory.  So, just create 3 files,
+     then use opendir/readdir to see what order they are in, and
+     assign meaning based on that order, not by name.  This assumes
+     that (on tmpfs) ordering within the directory is stable in the
+     absence of modification, which seems reasonably safe. */
+  {
+    /* since we're testing the fallback search, disable the readlink
+       happy-path */
+    VERIFY (umount2 ("/proc", MNT_DETACH) == 0);
+
+    xtouch ("/dev/console1", 0);
+    xtouch ("/dev/console2", 0);
+    xtouch ("/dev/console3", 0);
+
+    char *c[3];
+    int ci = 0;
+    DIR *dirstream = opendir ("/dev");
+    VERIFY (dirstream != NULL);
+    struct dirent *d;
+    while ((d = readdir (dirstream)) != NULL && ci < 3)
+      {
+        if (strcmp (d->d_name, "console1") &&
+            strcmp (d->d_name, "console2") &&
+            strcmp (d->d_name, "console3") )
+          continue;
+        c[ci++] = xasprintf ("/dev/%s", d->d_name);
+      }
+    VERIFY (ci == 3);
+    VERIFY (closedir (dirstream) == 0);
+
+    VERIFY (mount (slavename, c[0], NULL, MS_BIND, NULL) == 0);
+    VERIFY (mount ("/console", c[1], NULL, MS_BIND, NULL) == 0);
+    VERIFY (mount (slavename, c[2], NULL, MS_BIND, NULL) == 0);
+    VERIFY (umount2 ("/dev/pts", MNT_DETACH) == 0);
+    ok = doit (slave, "with search-path trap",
+               (struct result_r){.name=c[1], .ret=0, .err=0}) && ok;
+    for (int i = 0; i < 3; i++)
+      {
+        VERIFY (umount (c[i]) == 0);
+        VERIFY (unlink (c[i]) == 0);
+        free (c[i]);
+      }
+  }
+
+  return ok ? 0 : 1;
+}
+
+static int
+do_test (void)
+{
+  int ret1 = do_in_chroot_1 (run_chroot_tests);
+  if (ret1 == EXIT_UNSUPPORTED)
+    return ret1;
+
+  int ret2 = do_in_chroot_2 (run_chroot_tests);
+  if (ret2 == EXIT_UNSUPPORTED)
+    return ret2;
+
+  return  ret1 | ret2;
+}
+
+#include <support/test-driver.c>
-- 
2.15.0

Re: [PATCH v2 5/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Luke Shumaker]

On Thu, 02 Nov 2017 14:53:46 -0400,
Luke Shumaker wrote:
> +  /* support_become_root might have put us in a new user namespace;
> +     most filesystems (including tmpfs) don't allow file or directory
> +     creation from a user namespace unless uid and gid maps are set,
> +     even if we have root privileges in the namespace (failing with
> +     EOVERFLOW, since the uid overflows the empty (0-length) uid map).
> +
> +     Also, stat always reports that uid and gid maps are empty, so we
> +     have to try actually reading from them to check if they are
> +     empty. */

Style: two spaces after the trailing period.  This mistake is
consistent throughout the file.

> +    /* Depeding on how we set up the chroot, the kernel may or may not
> +     * trim the leading path to the target (it may give us "/6",
> +     * instead of "/dev/pts/6").  This test group relies on the target
> +     * existing, so guarantee that (so create a file at "/6" if
> +     * necessary). */

Style: no '*', two spaces after the trailing period.

Andreas Schwab made me aware of these style issues on the previous
commit.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH v2 5/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Christian Brauner]

On Thu, Nov 02, 2017 at 02:53:46PM -0400, Luke Shumaker wrote:
> Add a new tst-ttyname test that includes several named sub-testcases.
> 
> This patch is ordered the fixes it tests for are applied (to avoid
> breaking `git bisect`), but for reference, here's how each relevent
> change so far affected the testcases in this commit:
> 
>   |                                 | 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.
> 
> Each of the failing tests before 15e9a4f is 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.
> 
> v2:
>  - Rearranged commit order
>  - Fix code style
>  - Expanded commit message
>  - Pulled xtouch, become-root, and chroot setup into functions
>  - Better tracking of test failures
>  - Fixed the "with search-path trap" test case
>  - Added test cases for the readlink target directly
>  - More readable diagnostic output
>  - Test with both shared and private procfs
> ---
>  ChangeLog                             |   4 +
>  sysdeps/unix/sysv/linux/Makefile      |   3 +-
>  sysdeps/unix/sysv/linux/tst-ttyname.c | 582 ++++++++++++++++++++++++++++++++++
>  3 files changed, 588 insertions(+), 1 deletion(-)
>  create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index b5d214ae6f..1982a498ce 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,9 @@
>  2017-11-02  Luke Shumaker  <lukeshu@parabola.nu>
>  
> +	[BZ #22145]
> +	* sysdeps/unix/sysv/linux/tst-ttyname.c: New file.
> +	* sysdeps/unix/sysv/linux/Makefile: Add tst-ttyname to tests.
> +
>  	[BZ #22145]
>  	* sysdeps/unix/sysv/linux/ttyname.c (ttyname):
>  	Defer is_pty check until end.
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 0c8a009b5e..69edb75209 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -50,7 +50,8 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
>  		  bits/siginfo-arch.h bits/siginfo-consts-arch.h
>  
>  tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
> -	 tst-quota tst-sync_file_range test-errno-linux tst-sysconf-iov_max
> +	 tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
> +	 test-errno-linux
>  
>  # Generate the list of SYS_* macros for the system calls (__NR_*
>  # macros).  The file syscall-names.list contains all possible system
> diff --git a/sysdeps/unix/sysv/linux/tst-ttyname.c b/sysdeps/unix/sysv/linux/tst-ttyname.c
> new file mode 100644
> index 0000000000..45d356cab4
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-ttyname.c
> @@ -0,0 +1,582 @@
> +/* Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public License as
> +   published by the Free Software Foundation; either version 2.1 of the
> +   License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; see the file COPYING.LIB.  If
> +   not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <dirent.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <sched.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mount.h>
> +#include <sys/prctl.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include <support/check.h>
> +#include <support/namespace.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +
> +/* generic utilities */
> +
> +#define VERIFY(expr)                                                    \
> +  do {                                                                  \
> +    if (!(expr))                                                        \
> +      {                                                                 \
> +        printf ("error: %s:%d: %s: %m\n",                               \
> +                __FILE__, __LINE__, #expr);                             \
> +        exit (1);                                                       \
> +      }                                                                 \
> +  } while (0)
> +
> +static void
> +xtouch (const char *path, mode_t mode)
> +{
> +  xclose (xopen (path, O_WRONLY|O_CREAT|O_NOCTTY, mode));
> +}
> +
> +static size_t
> +trim_prefix (char *str, size_t str_len, const char *prefix)
> +{
> +  size_t prefix_len = strlen (prefix);
> +  if (str_len > prefix_len && memcmp (str, prefix, prefix_len) == 0)
> +    {
> +      memmove (str, str + prefix_len, str_len - prefix_len);
> +      return str_len - prefix_len;
> +    }
> +  return str_len;
> +}
> +
> +/* returns a pointer to static storage */
> +static char *
> +xreadlink (const char *linkname)
> +{
> +  static char target[PATH_MAX+1];
> +  ssize_t target_len = readlink (linkname, target, PATH_MAX);
> +  VERIFY (target_len > 0);
> +  target_len = trim_prefix (target, target_len, "(unreachable)");
> +  target[target_len] = '\0';
> +  return target;
> +}
> +
> +static void
> +become_root_in_mount_ns (void) {
> +  uid_t orig_uid = getuid ();
> +  gid_t orig_gid = getgid ();
> +
> +  support_become_root ();
> +
> +  if (unshare (CLONE_NEWNS) < 0)
> +    FAIL_UNSUPPORTED ("could not enter new mount namespace");
> +
> +  /* support_become_root might have put us in a new user namespace;
> +     most filesystems (including tmpfs) don't allow file or directory
> +     creation from a user namespace unless uid and gid maps are set,
> +     even if we have root privileges in the namespace (failing with
> +     EOVERFLOW, since the uid overflows the empty (0-length) uid map).
> +
> +     Also, stat always reports that uid and gid maps are empty, so we
> +     have to try actually reading from them to check if they are
> +     empty. */
> +  int fd;
> +
> +  if ((fd = open ("/proc/self/uid_map", O_RDWR, 0)) >= 0)
> +    {
> +      char buf;
> +      if (read (fd, &buf, 1) == 0)
> +	{
> +	  char *str = xasprintf ("0 %ld 1\n", (long)orig_uid);
> +	  if (write (fd, str, strlen (str)) < 0)
> +	    FAIL_EXIT1 ("write (uid_map, \"%s\"): %m", str);

I haven't looked at the test code a lot yet but is it generally considered ok to
not cleanup before exit? That's mostly style but still it'd be good to clarify
this. I usually do all the cleanup even when exiting.

> +	  free (str);
> +	}
> +      xclose (fd);
> +    }
> +
> +  /* Oh, but we can't set the gid map until we turn off setgroups. */

That sounds like a discovery you just made. I think - nitpick - a simple:
/* We need to set setgroups to "deny" otherwise we won't be able to write a gid
*  mapping. */

> +  if ((fd = open ("/proc/self/setgroups", O_WRONLY, 0)) >= 0)
> +    {
> +      const char *str = "deny";
> +      if (write (fd, str, strlen (str)) < 0)
> +	FAIL_EXIT1 ("write (setroups, \"%s\"): %m", str);
> +      xclose (fd);
> +    }
> +
> +  if ((fd = open ("/proc/self/gid_map", O_RDWR, 0)) >= 0)
> +    {
> +      char buf;
> +      if (read (fd, &buf, 1) == 0)
> +	{
> +	  char *str = xasprintf ("0 %ld 1\n", (long)orig_gid);
> +	  if (write (fd, str, strlen (str)) < 0)
> +	    FAIL_EXIT1 ("write (gid_map, \"%s\"): %m", str);
> +	  free (str);
> +	}
> +      xclose (fd);
> +    }
> +}
> +
> +/* plain ttyname runner */
> +
> +struct result {
> +  const char *name;
> +  int err;
> +};
> +
> +/* strings in result structure are in static storage */
> +static struct result
> +run_ttyname (int fd)
> +{
> +  struct result ret;
> +  errno = 0;
> +  ret.name = ttyname (fd);
> +  ret.err = errno;
> +  return ret;
> +}
> +
> +static bool
> +eq_ttyname (struct result actual, struct result expected)
> +{
> +  if ((actual.err == expected.err) &&
> +      (!actual.name == !expected.name) &&
> +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> +    {
> +      char *name = expected.name
> +	? xasprintf ("\"%s\"", expected.name)
> +	: strdup ("NULL");
> +      printf ("info:      ttyname: PASS {name=%s, errno=%d}\n",
> +	      name, expected.err);
> +      free (name);
> +      return true;
> +    }
> +
> +  char *actual_name = actual.name
> +    ? xasprintf ("\"%s\"", actual.name)
> +    : strdup ("NULL");

I'd prefer if this would be proper if () branches as this is pretty unreadable.
Also, I'd prefer - even though newer C standards allow this - if new variables
wouldn't be declared mid-stack especially if the function is rather small.

> +  char *expected_name = expected.name
> +    ? xasprintf ("\"%s\"", expected.name)
> +    : strdup ("NULL");

Likewise.

> +  printf ("error:     ttyname: actual {name=%s, errno=%d} != expected {name=%s, errno=%d}\n",
> +	  actual_name, actual.err,
> +	  expected_name, expected.err);
> +  free (actual_name);
> +  free (expected_name);
> +  return false;
> +}
> +
> +/* ttyname_r runner */
> +
> +struct result_r {
> +  const char *name;
> +  int ret;
> +  int err;
> +};
> +
> +/* strings in result structure are in static storage */
> +static struct result_r
> +run_ttyname_r (int fd)
> +{
> +  static char buf[TTY_NAME_MAX];
> +
> +  struct result_r ret;
> +  errno = 0;
> +  ret.ret = ttyname_r (fd, buf, TTY_NAME_MAX);
> +  ret.err = errno;
> +  if (ret.ret == 0)
> +    ret.name = buf;
> +  else
> +    ret.name = NULL;
> +  return ret;
> +}
> +
> +static bool
> +eq_ttyname_r (struct result_r actual, struct result_r expected)
> +{
> +  if ((actual.err == expected.err) &&
> +      (actual.ret == expected.ret) &&
> +      (!actual.name == !expected.name) &&
> +      (actual.name ? strcmp (actual.name, expected.name) == 0 : true))
> +    {
> +      char *name = expected.name
> +	? xasprintf ("\"%s\"", expected.name)
> +	: strdup ("NULL");
> +      printf ("info:      ttyname_r: PASS {name=%s, ret=%d, errno=%d}\n",
> +              name, expected.ret, expected.err);
> +      free (name);
> +      return true;
> +    }
> +
> +  char *actual_name = actual.name
> +    ? xasprintf ("\"%s\"", actual.name)
> +    : strdup ("NULL");

Likewise.

> +  char *expected_name = expected.name
> +    ? xasprintf ("\"%s\"", expected.name)
> +    : strdup ("NULL");

Likewise.

> +  printf ("error:     ttyname_r: actual {name=%s, ret=%d, errno=%d} != expected {name=%s, ret=%d, errno=%d}\n",
> +	  actual_name, actual.ret, actual.err,
> +	  expected_name, expected.ret, expected.err);
> +  free (actual_name);
> +  free (expected_name);
> +  return false;
> +}
> +
> +/* combined runner */
> +
> +static bool
> +doit (int fd, const char *testname, struct result_r expected_r) {
> +  struct result expected = {.name=expected_r.name, .err=expected_r.ret};
> +  bool ret = true;
> +
> +  printf ("info:    testcase: %s\n", testname);
> +
> +  ret = eq_ttyname (run_ttyname (fd), expected) && ret;
> +  ret = eq_ttyname_r (run_ttyname_r (fd), expected_r) && ret;

Imho, this is not very nice. It looks clever but is hard to parse semantically.
I'd prefer if the assignment from eq_ttyname{_r}() would be entangled from the
following "&& ret" check. This is just easier to maintain.

> +
> +  if (!ret)
> +    support_record_failure ();
> +
> +  return ret;
> +}
> +
> +/* chroot setup */
> +
> +static char *chrootdir;
> +
> +static void
> +prepare (int argc, char **argv)
> +{
> +  chrootdir = xasprintf ("%s/tst-ttyname-XXXXXX", test_dir);
> +  if (mkdtemp (chrootdir) == NULL)
> +    FAIL_EXIT1 ("mkdtemp (\"%s\"): %m", chrootdir);
> +  add_temp_file (chrootdir);
> +}
> +#define PREPARE prepare
> +
> +/* These chroot setup functions put the TTY at at "/console" (where it
> +   won't be found by ttyname), and create "/dev/console" as an
> +   ordinary file.  This way, it's easier to write test-cases that
> +   expect ttyname to fail; test-cases that expect it to succeed need
> +   to explicitly remount it at "/dev/console". */
> +
> +static int
> +do_in_chroot_1 (int (*cb)(const char *, int)) {
> +  printf ("info:  entering chroot 1\n");
> +
> +  /* Open the PTS that we'll be testing on. */
> +  int master;
> +  char *slavename;
> +  VERIFY ((master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK)) >= 0);
> +  VERIFY ((slavename = ptsname (master)));
> +  VERIFY (unlockpt (master) == 0);
> +  if (strncmp (slavename, "/dev/pts/", 9) != 0)
> +    FAIL_UNSUPPORTED ("slave pseudo-terminal is not under /dev/pts/: %s",
> +                      slavename);
> +  int slave = xopen (slavename, O_RDWR, 0);
> +  if (!doit (slave, "basic smoketest",
> +             (struct result_r){.name=slavename, .ret=0, .err=0}))
> +    return 1;
> +
> +  pid_t pid = xfork ();
> +  if (pid == 0)
> +    {
> +      xclose (master);
> +
> +      become_root_in_mount_ns ();
> +
> +      VERIFY (mount ("tmpfs", chrootdir, "tmpfs", 0, "mode=755") == 0);
> +      VERIFY (chdir (chrootdir) == 0);
> +
> +      xmkdir ("proc", 0755);
> +      xmkdir ("dev", 0755);
> +      xmkdir ("dev/pts", 0755);
> +
> +      VERIFY (mount ("/proc", "proc", NULL, MS_BIND|MS_REC, NULL) == 0);
> +      VERIFY (mount ("devpts", "dev/pts", "devpts",
> +                     MS_NOSUID|MS_NOEXEC,
> +                     "newinstance,ptmxmode=0666,mode=620") == 0);
> +      VERIFY (symlink ("pts/ptmx", "dev/ptmx") == 0);
> +
> +      xtouch ("console", 0);
> +      xtouch ("dev/console", 0);
> +      VERIFY (mount (slavename, "console", NULL, MS_BIND, NULL) == 0);
> +
> +      xchroot (".");
> +
> +      char *linkname = xasprintf ("/proc/self/fd/%d", slave);
> +      char *target = xreadlink (linkname);
> +      VERIFY (strcmp (target, slavename) == 0);
> +      free (linkname);
> +
> +      _exit (cb (slavename, slave));
> +    }
> +  int status;
> +  xwaitpid (pid, &status, 0);
> +  VERIFY (WIFEXITED (status));
> +  xclose (master);
> +  xclose (slave);
> +  return WEXITSTATUS (status);
> +}
> +
> +static int
> +do_in_chroot_2 (int (*cb)(const char *, int)) {
> +  printf ("info:  entering chroot 2\n");
> +
> +  int pid_pipe[2];
> +  xpipe (pid_pipe);
> +  int exit_pipe[2];
> +  xpipe (exit_pipe);
> +
> +  /* Open the PTS that we'll be testing on. */
> +  int master;
> +  char *slavename;
> +  VERIFY ((master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK)) >= 0);
> +  VERIFY ((slavename = ptsname (master)));
> +  VERIFY (unlockpt (master) == 0);
> +  if (strncmp (slavename, "/dev/pts/", 9) != 0)
> +    FAIL_UNSUPPORTED ("slave pseudo-terminal is not under /dev/pts/: %s",
> +                      slavename);
> +  /* wait until in a new mount ns to open the slave */
> +
> +  /* enable `wait`ing on grandchildren */
> +  VERIFY (prctl (PR_SET_CHILD_SUBREAPER, 1) == 0);

For future reference in case you want to avoid this clutch you can also use
clone() with CLONE_PARENT.

> +
> +  pid_t pid = xfork (); /* outer child */
> +  if (pid == 0)
> +    {
> +      xclose (master);
> +      xclose (pid_pipe[0]);
> +      xclose (exit_pipe[1]);
> +
> +      become_root_in_mount_ns ();
> +
> +      int slave = xopen (slavename, O_RDWR, 0);
> +      if (!doit (slave, "basic smoketest",
> +                 (struct result_r){.name=slavename, .ret=0, .err=0}))
> +        _exit (1);
> +
> +      VERIFY (mount ("tmpfs", chrootdir, "tmpfs", 0, "mode=755") == 0);
> +      VERIFY (chdir (chrootdir) == 0);
> +
> +      xmkdir ("proc", 0755);
> +      xmkdir ("dev", 0755);
> +      xmkdir ("dev/pts", 0755);
> +
> +      VERIFY (mount ("devpts", "dev/pts", "devpts",
> +                     MS_NOSUID|MS_NOEXEC,
> +                     "newinstance,ptmxmode=0666,mode=620") == 0);
> +      VERIFY (symlink ("pts/ptmx", "dev/ptmx") == 0);
> +
> +      xtouch ("console", 0);
> +      xtouch ("dev/console", 0);
> +      VERIFY (mount (slavename, "console", NULL, MS_BIND, NULL) == 0);
> +
> +      xchroot (".");
> +
> +      if (unshare (CLONE_NEWNS | CLONE_NEWPID) < 0)
> +        FAIL_UNSUPPORTED ("could not enter new PID namespace");
> +      pid = xfork (); /* inner child */
> +      if (pid == 0)
> +        {
> +          xclose (pid_pipe[1]);
> +
> +          /* wait until the outer child has exited */
> +          char c;
> +          VERIFY (read (exit_pipe[0], &c, 1) == 0);
> +          xclose (exit_pipe[0]);
> +
> +          VERIFY (mount ("proc", "/proc", "proc",
> +                         MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) == 0);
> +
> +          char *linkname = xasprintf ("/proc/self/fd/%d", slave);
> +          char *target = xreadlink (linkname);
> +          VERIFY (strcmp (target, strrchr (slavename, '/')) == 0);
> +          free (linkname);
> +
> +          _exit (cb (slavename, slave));
> +        }
> +      xwrite (pid_pipe[1], &pid, sizeof pid);
> +      _exit (0);
> +    }
> +  xclose (pid_pipe[1]);
> +  xclose (exit_pipe[0]);
> +  xclose (exit_pipe[1]);
> +
> +  /* wait for the outer child */
> +  int status;
> +  xwaitpid (pid, &status, 0);
> +  VERIFY (WIFEXITED (status));
> +  int ret = WEXITSTATUS (status);
> +  if (ret != 0)
> +    return ret;
> +
> +  /* set 'pid' to the inner child */
> +  VERIFY (read (pid_pipe[0], &pid, sizeof pid) == sizeof pid);
> +  xclose (pid_pipe[0]);
> +
> +  /* wait for the inner child */
> +  xwaitpid (pid, &status, 0);
> +  VERIFY (WIFEXITED (status));
> +  xclose (master);
> +  return WEXITSTATUS (status);
> +}
> +
> +/* main test */
> +
> +static int
> +run_chroot_tests (const char *slavename, int slave)
> +{
> +  struct stat st;
> +  bool ok = true;
> +
> +  /* There are 3 groups of tests here.  The first group throws a
> +     wrench into the works in a generic way, and verifies that ttyname

That's using a pretty (for a non-native speaker) specific English idiom. Could
you maybe simply say "creates problems" or something similar.

> +     copes correctly.  The remaining groups are increasingly
> +     convoluted, as we direct that wrench towards specific parts of

Likewise.

> +     the routine, and try to break it in specific ways. */
> +
> +  /* Basic tests that it doesn't get confused by multiple devpts
> +     instances. */
> +  {
> +    VERIFY (stat (slavename, &st) < 0); /* sanity check */
> +    ok = doit (slave, "no conflict, no match",
> +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
> +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> +    ok = doit (slave, "no conflict, console",
> +               (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
> +    VERIFY (umount ("/dev/console") == 0);
> +
> +    /* keep creating PTYs until we we get a name collision */
> +    while (stat (slavename, &st) < 0)
> +      posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK);
> +    VERIFY (stat (slavename, &st) == 0);
> +
> +    ok = doit (slave, "conflict, no match",
> +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
> +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> +    ok = doit (slave, "conflict, console",
> +               (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
> +    VERIFY (umount ("/dev/console") == 0);
> +  }
> +
> +  /* The first tests kinda assumed that they hit certain code-paths
> +     based on assuming that the readlink target is 'slavename', but
> +     that's not quite always true.  They're still a good preliminary
> +     sanity check, so keep them, but let's add tests that make sure
> +     that those code-paths are hit by doing a readlink ourself. */
> +  {
> +    char *linkname = xasprintf ("/proc/self/fd/%d", slave);
> +    char *target = xreadlink (linkname);
> +    free (linkname);
> +    /* Depeding on how we set up the chroot, the kernel may or may not
> +     * trim the leading path to the target (it may give us "/6",

Uhm? I'm sorry, I don't understand. That sounds like a bug we fixed a while back
upstream whereby the kernel recorded the wrong vfs mount for devpts. That's one
scenario where this can happen. In any case the comment should outline when the
kernel can *legitimately* show you the contents of the symlink as /<n> instead
of the correct /dev/pts/<n>. Right now it sounds like this code doesn't
understand how such a scenario can happen but only that it can happen.

> +     * instead of "/dev/pts/6").  This test group relies on the target
> +     * existing, so guarantee that (so create a file at "/6" if
> +     * necessary). */
> +    if (stat (target, &st) < 0)
> +      {
> +        VERIFY (errno == ENOENT);
> +        xtouch (target, 0);
> +      }
> +
> +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> +    VERIFY (mount ("/console", target, NULL, MS_BIND, NULL) == 0);
> +    ok = doit (slave, "with readlink target",
> +               (struct result_r){.name=target, .ret=0, .err=0}) && ok;
> +    VERIFY (umount (target) == 0);
> +    VERIFY (umount ("/dev/console") == 0);
> +
> +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> +    VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
> +    ok = doit (slave, "with readlink trap; fallback",
> +               (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
> +    VERIFY (umount (target) == 0);
> +    VERIFY (umount ("/dev/console") == 0);
> +
> +    VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
> +    ok = doit (slave, "with readlink trap; no fallback",
> +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;

Same comment as above: Imho, this is not very nice. It looks clever but is hard
to parse semantically. I'd prefer if the assignment would be entangled from the
following "&& ret" check. This is just easier to maintain.

> +    VERIFY (umount (target) == 0);
> +  }
> +
> +  /* This test is extra-convoluted because we need to control the
> +     order of files within the directory.  So, just create 3 files,
> +     then use opendir/readdir to see what order they are in, and
> +     assign meaning based on that order, not by name.  This assumes

Why do you want to assign meaning based on order not name? If there's a
legitimate reason please mention it.

> +     that (on tmpfs) ordering within the directory is stable in the
> +     absence of modification, which seems reasonably safe. */
> +  {
> +    /* since we're testing the fallback search, disable the readlink
> +       happy-path */
> +    VERIFY (umount2 ("/proc", MNT_DETACH) == 0);
> +
> +    xtouch ("/dev/console1", 0);
> +    xtouch ("/dev/console2", 0);
> +    xtouch ("/dev/console3", 0);
> +
> +    char *c[3];
> +    int ci = 0;
> +    DIR *dirstream = opendir ("/dev");
> +    VERIFY (dirstream != NULL);
> +    struct dirent *d;
> +    while ((d = readdir (dirstream)) != NULL && ci < 3)
> +      {
> +        if (strcmp (d->d_name, "console1") &&
> +            strcmp (d->d_name, "console2") &&
> +            strcmp (d->d_name, "console3") )
> +          continue;
> +        c[ci++] = xasprintf ("/dev/%s", d->d_name);
> +      }
> +    VERIFY (ci == 3);
> +    VERIFY (closedir (dirstream) == 0);
> +
> +    VERIFY (mount (slavename, c[0], NULL, MS_BIND, NULL) == 0);
> +    VERIFY (mount ("/console", c[1], NULL, MS_BIND, NULL) == 0);
> +    VERIFY (mount (slavename, c[2], NULL, MS_BIND, NULL) == 0);
> +    VERIFY (umount2 ("/dev/pts", MNT_DETACH) == 0);
> +    ok = doit (slave, "with search-path trap",
> +               (struct result_r){.name=c[1], .ret=0, .err=0}) && ok;

Same comment as above: Imho, this is not very nice. It looks clever but is hard
to parse semantically. I'd prefer if the assignment would be entangled from the
following "&& ret" check. This is just easier to maintain.

> +    for (int i = 0; i < 3; i++)
> +      {
> +        VERIFY (umount (c[i]) == 0);
> +        VERIFY (unlink (c[i]) == 0);
> +        free (c[i]);
> +      }
> +  }
> +
> +  return ok ? 0 : 1;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int ret1 = do_in_chroot_1 (run_chroot_tests);
> +  if (ret1 == EXIT_UNSUPPORTED)
> +    return ret1;
> +
> +  int ret2 = do_in_chroot_2 (run_chroot_tests);
> +  if (ret2 == EXIT_UNSUPPORTED)
> +    return ret2;
> +
> +  return  ret1 | ret2;
> +}
> +
> +#include <support/test-driver.c>
> -- 
> 2.15.0
> 

Re: [PATCH v2 5/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Luke Shumaker]

On Wed, 08 Nov 2017 14:43:07 -0500,
Christian Brauner wrote:
> > +  if ((fd = open ("/proc/self/uid_map", O_RDWR, 0)) >= 0)
> > +    {
> > +      char buf;
> > +      if (read (fd, &buf, 1) == 0)
> > +	{
> > +	  char *str = xasprintf ("0 %ld 1\n", (long)orig_uid);
> > +	  if (write (fd, str, strlen (str)) < 0)
> > +	    FAIL_EXIT1 ("write (uid_map, \"%s\"): %m", str);
> 
> I haven't looked at the test code a lot yet but is it generally considered ok to
> not cleanup before exit? That's mostly style but still it'd be good to clarify
> this. I usually do all the cleanup even when exiting.

Clean up the file system, or clean up memory?  FS cleanup isn't
necessary because we do everything on a tmpfs in a new mount
namespace.  Looking at other tests, it seems to be OK to leave memory
messy when exiting with an error.

> > +	  free (str);
> > +	}
> > +      xclose (fd);
> > +    }
> > +
> > +  /* Oh, but we can't set the gid map until we turn off setgroups. */
> 
> That sounds like a discovery you just made. I think - nitpick - a simple:
> /* We need to set setgroups to "deny" otherwise we won't be able to write a gid
> *  mapping. */

My intent was for it to read as a postscript to the previous comment,
but I'm happy to change it.

> > +    }
> > +
> > +  char *actual_name = actual.name
> > +    ? xasprintf ("\"%s\"", actual.name)
> > +    : strdup ("NULL");
> 
> I'd prefer if this would be proper if () branches as this is pretty unreadable.
> Also, I'd prefer - even though newer C standards allow this - if new variables
> wouldn't be declared mid-stack especially if the function is rather small.

Sure.

> > +  char *expected_name = expected.name
> > +    ? xasprintf ("\"%s\"", expected.name)
> > +    : strdup ("NULL");
> 
> Likewise.

Ack.

> > +    }
> > +
> > +  char *actual_name = actual.name
> > +    ? xasprintf ("\"%s\"", actual.name)
> > +    : strdup ("NULL");
> 
> Likewise.

Ack.

> > +  char *expected_name = expected.name
> > +    ? xasprintf ("\"%s\"", expected.name)
> > +    : strdup ("NULL");
> 
> Likewise.

Ack.

> > +static bool
> > +doit (int fd, const char *testname, struct result_r expected_r) {

(whoops, that brace needs to be on a new line)

> > +  bool ret = true;
> > +
> > +  printf ("info:    testcase: %s\n", testname);
> > +
> > +  ret = eq_ttyname (run_ttyname (fd), expected) && ret;
> > +  ret = eq_ttyname_r (run_ttyname_r (fd), expected_r) && ret;
> 
> Imho, this is not very nice. It looks clever but is hard to parse semantically.
> I'd prefer if the assignment from eq_ttyname{_r}() would be entangled from the
> following "&& ret" check. This is just easier to maintain.

Ok, see below.

> > +  /* enable `wait`ing on grandchildren */
> > +  VERIFY (prctl (PR_SET_CHILD_SUBREAPER, 1) == 0);
> 
> For future reference in case you want to avoid this clutch you can also use
> clone() with CLONE_PARENT.

I wanted to do that, but unfortunately glibc's clone(2) wrapper
requires passing in the stack explicitly, which requires knowing if
the stack grows up or down; and I didn't want to have to deal with
that in this test.

> > +  /* There are 3 groups of tests here.  The first group throws a
> > +     wrench into the works in a generic way, and verifies that ttyname
> 
> That's using a pretty (for a non-native speaker) specific English idiom. Could
> you maybe simply say "creates problems" or something similar.

Ok.

> > +     copes correctly.  The remaining groups are increasingly
> > +     convoluted, as we direct that wrench towards specific parts of
> 
> Likewise.

Ok.

> > +  {
> > +    char *linkname = xasprintf ("/proc/self/fd/%d", slave);
> > +    char *target = xreadlink (linkname);
> > +    free (linkname);
> > +    /* Depeding on how we set up the chroot, the kernel may or may not
> > +     * trim the leading path to the target (it may give us "/6",
> 
> Uhm? I'm sorry, I don't understand. That sounds like a bug we fixed a while back
> upstream whereby the kernel recorded the wrong vfs mount for devpts. That's one
> scenario where this can happen. In any case the comment should outline when the
> kernel can *legitimately* show you the contents of the symlink as /<n> instead
> of the correct /dev/pts/<n>. Right now it sounds like this code doesn't
> understand how such a scenario can happen but only that it can happen.

You're right, this comment should be clearer.  The test has two
chroot-setup routines, and runs this code in each.  One of the setup
routines does it in a way that results in /<n> and the other does it
in a way that results in /dev/pts/<n>; this code doesn't have context
to know which of the setup routines was called.

> 
> > +     * instead of "/dev/pts/6").  This test group relies on the target
> > +     * existing, so guarantee that (so create a file at "/6" if
> > +     * necessary). */
> > +    if (stat (target, &st) < 0)
> > +      {
> > +        VERIFY (errno == ENOENT);
> > +        xtouch (target, 0);
> > +      }
> > +
> > +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> > +    VERIFY (mount ("/console", target, NULL, MS_BIND, NULL) == 0);
> > +    ok = doit (slave, "with readlink target",
> > +               (struct result_r){.name=target, .ret=0, .err=0}) && ok;
> > +    VERIFY (umount (target) == 0);
> > +    VERIFY (umount ("/dev/console") == 0);
> > +
> > +    VERIFY (mount ("/console", "/dev/console", NULL, MS_BIND, NULL) == 0);
> > +    VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
> > +    ok = doit (slave, "with readlink trap; fallback",
> > +               (struct result_r){.name="/dev/console", .ret=0, .err=0}) && ok;
> > +    VERIFY (umount (target) == 0);
> > +    VERIFY (umount ("/dev/console") == 0);
> > +
> > +    VERIFY (mount (slavename, target, NULL, MS_BIND, NULL) == 0);
> > +    ok = doit (slave, "with readlink trap; no fallback",
> > +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
> 
> Same comment as above: Imho, this is not very nice. It looks clever but is hard
> to parse semantically. I'd prefer if the assignment would be entangled from the
> following "&& ret" check. This is just easier to maintain.

How would you rather it be written?

    static void
    doit (..., bool *ok)
    {
      ...

      if (!eq_ttyname (run_ttyname (fd), expected))
        *ok = false;
      if (!eq_ttyname_r (run_ttyname_r (fd), expected_r))
        *ok = false;
    }
  
    doit(..., &ok);

?

> > +    VERIFY (umount (target) == 0);
> > +  }
> > +
> > +  /* This test is extra-convoluted because we need to control the
> > +     order of files within the directory.  So, just create 3 files,
> > +     then use opendir/readdir to see what order they are in, and
> > +     assign meaning based on that order, not by name.  This assumes
> 
> Why do you want to assign meaning based on order not name? If there's a
> legitimate reason please mention it.

Reason: for this test "we need to control the order of files within
the directory."

Conceptually, the test is making sure that an pseudo-match from the
wrong devpts that readdir() finds before it finds the real match
doesn't mess anything up.

To test that, we need to set up the pseudo-match and the actual match
such that readdir finds the pseudo-match first.  There's no good way
to control the order that readdir will find the files in (maybe I
should look in to the whitebox testing infrastructure?).  So we create
two files, and whichever one readdir finds first, we assign that one
to be the pseudo-match; and whichever one readdir finds second, we
assign that one to be the actual match.

(We also test that readdir finding a pseudo-match after the actual
match doesn't mess anything up either, hence 3 files instead of 2.)

> > +    ok = doit (slave, "with search-path trap",
> > +               (struct result_r){.name=c[1], .ret=0, .err=0}) && ok;
> 
> Same comment as above: Imho, this is not very nice. It looks clever but is hard
> to parse semantically. I'd prefer if the assignment would be entangled from the
> following "&& ret" check. This is just easier to maintain.

See above.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH v2 5/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Luke Shumaker]

On Wed, 08 Nov 2017 18:23:49 -0500,
Luke Shumaker wrote:
> > > +	    FAIL_EXIT1 ("write (uid_map, \"%s\"): %m", str);
> > 
> > I haven't looked at the test code a lot yet but is it generally considered ok to
> > not cleanup before exit? That's mostly style but still it'd be good to clarify
> > this. I usually do all the cleanup even when exiting.
> 
> Clean up the file system, or clean up memory?  FS cleanup isn't
> necessary because we do everything on a tmpfs in a new mount
> namespace.  Looking at other tests, it seems to be OK to leave memory
> messy when exiting with an error.

(if it weren't OK to leave the memory a mess, then the x* functions in
support/ wouldn't be calling FAIL_EXIT1 behind your back)

> > > +    ok = doit (slave, "with readlink trap; no fallback",
> > > +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
> > 
> > Same comment as above: Imho, this is not very nice. It looks clever but is hard
> > to parse semantically. I'd prefer if the assignment would be entangled from the
> > following "&& ret" check. This is just easier to maintain.
> 
> How would you rather it be written?
> 
>     static void
>     doit (..., bool *ok)
>     {
>       ...
> 
>       if (!eq_ttyname (run_ttyname (fd), expected))
>         *ok = false;
>       if (!eq_ttyname_r (run_ttyname_r (fd), expected_r))
>         *ok = false;
>     }
>   
>     doit(..., &ok);
> 
> ?

Disregard that, I was being silly,

    if (!doit (...))
      ok = false;

doesn't add too much noise.

-- 
Happy hacking,
~ Luke Shumaker

[PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

[Luke Shumaker]

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(-)

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

Re: [PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

[Christian Brauner]

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
> 

Re: [PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

[Luke Shumaker]

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.

-- 
Happy hacking,

Re: [PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 8426 bytes --]

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>


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

[Christian Brauner]

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

Re: [PATCH v2 4/5] linux ttyname and ttyname_r: Don't bail prematurely [BZ #22145]

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."