[PATCH v4] linux ttyname and ttyname_r: do not return wrong results

[PATCH v4] linux ttyname and ttyname_r: do not return wrong results

[Christian Brauner]

Hi,

Last we've talked this patches was ready to merge. The only thing left to do was
to sign an FSF agreement. I did and it is fully processed. If you need to see a
PDF I can send it to a specific person privately per mail. I hope we're good to
go now. :)

I've taken over this patch from Serge who has been informed and is CCed on this
thread. There are no significant functional changes. As requested the following
this have been changed:

- remove obsolete comment in ttyname_r.c
- move is_pty() to common header file and mark as static inline

Christian

Christian Brauner (1):
  linux ttyname and ttyname_r: do not return wrong results

 ChangeLog                           | 12 ++++++++++++
 sysdeps/unix/sysv/linux/ttyname.c   | 16 ++++++++++++----
 sysdeps/unix/sysv/linux/ttyname.h   | 35 +++++++++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/ttyname_r.c | 17 +++++++++++++----
 4 files changed, 72 insertions(+), 8 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/ttyname.h

-- 
2.11.0

[PATCH v4] linux ttyname and ttyname_r: do not return wrong results

[Christian Brauner]

From: Christian Brauner <christian.brauner@canonical.com>

If a link (say /proc/self/fd/0) pointing to a device, say /dev/pts/2, in a
parent mount namespace is passed to ttyname, and a /dev/pts/2 exists (in a
different devpts) in the current namespace, then it returns /dev/pts/2. But
/dev/pts/2 is NOT the current tty, it is a different file and device.

Detect this case and return ENODEV. Userspace can choose to take this as a hint
that the fd points to a tty device but to act on the fd rather than the link.

Signed-off-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
Changelog: 2016-11-08
	- remove obsolete comment in ttyname_r.c
	- move is_pty() to common header file and mark as static inline
Changelog: 2016-11-22
	- remove unneeded sys/symacros.h header
Changelog: 2016-11-24
	- add Changelog entry
Changelog: 2017-01-27
	- received my signed FSF copyright agreement back
	- add properly formatted Changelog entry
---
 ChangeLog                           | 12 ++++++++++++
 sysdeps/unix/sysv/linux/ttyname.c   | 16 ++++++++++++----
 sysdeps/unix/sysv/linux/ttyname.h   | 35 +++++++++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/ttyname_r.c | 17 +++++++++++++----
 4 files changed, 72 insertions(+), 8 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/ttyname.h

diff --git a/ChangeLog b/ChangeLog
index e468b59767..63f86b9ce8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2017-01-27  Christian Brauner  <christian.brauner@ubuntu.com>
+
+	* sysdeps/unix/sysv/linux/ttyname.h: New file.
+	* sysdeps/unix/sysv/linux/ttyname.c: Include "ttyname.h".
+	(ttyname) [!_STATBUF_ST_RDEV]: Make code unconditional.
+	Call is_pty when the link does not exist or does not match, fail
+	with ENODEV when it returns true.
+	* sysdeps/unix/sysv/linux/ttyname_r.c: Include "ttyname.h".
+	(__ttyname_r) [!_STATBUF_ST_RDEV]: Make code unconditional.
+	Call is_pty when the link does not exist or does not match, fail
+	with ENODEV when it returns true.
+
 2017-01-27  Florian Weimer  <fweimer@redhat.com>
 
 	* nptl/Makefile (tests): Add tst-robust-fork.
diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index e2686b5a23..5909cb765f 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -28,6 +28,8 @@
 
 #include <_itoa.h>
 
+#include "ttyname.h"
+
 #if 0
 /* Is this used anywhere?  It is not exported.  */
 char *__ttyname;
@@ -170,12 +172,18 @@ ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
-#else
-	  && st1.st_ino == st.st_ino
-	  && st1.st_dev == st.st_dev
 #endif
-	  )
+	  && st1.st_ino == st.st_ino
+	  && st1.st_dev == st.st_dev)
 	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;
+	}
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
new file mode 100644
index 0000000000..a1aa314193
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/ttyname.h
@@ -0,0 +1,35 @@
+/* Copyright (C) 2016 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 <unistd.h>
+#include <sys/sysmacros.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static inline int
+is_pty (struct stat64 *sb)
+{
+#ifdef _STATBUF_ST_RDEV
+  int m = major (sb->st_rdev);
+  return (136 <= m && m <= 143);
+#else
+  return false;
+#endif
+}
+
diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index 10b170a8b5..dc863526ba 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -28,6 +28,8 @@
 
 #include <_itoa.h>
 
+#include "ttyname.h"
+
 static int getttyname_r (char *buf, size_t buflen,
 			 dev_t mydev, ino64_t myino, int save,
 			 int *dostat) internal_function;
@@ -152,12 +154,19 @@ __ttyname_r (int fd, char *buf, size_t buflen)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
-#else
-	  && st1.st_ino == st.st_ino
-	  && st1.st_dev == st.st_dev
 #endif
-	  )
+	  && st1.st_ino == st.st_ino
+	  && st1.st_dev == st.st_dev)
 	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;
+	}
     }
 
   /* Prepare the result buffer.  */
-- 
2.11.0

Re: [PATCH v4] linux ttyname and ttyname_r: do not return wrong results

[Christian Brauner]

[-- Attachment #1.1: Type: text/plain, Size: 1341 bytes --]

Appended to the mail you'll find a program that'll allow you to test the changes
to ttyname() and ttyname_r() on a patched glibc. Are we good about the patch as
it stands?

On Fri, Jan 27, 2017 at 03:59:58PM +0100, Christian Brauner wrote:
> Hi,
> 
> Last we've talked this patches was ready to merge. The only thing left to do was
> to sign an FSF agreement. I did and it is fully processed. If you need to see a
> PDF I can send it to a specific person privately per mail. I hope we're good to
> go now. :)
> 
> I've taken over this patch from Serge who has been informed and is CCed on this
> thread. There are no significant functional changes. As requested the following
> this have been changed:
> 
> - remove obsolete comment in ttyname_r.c
> - move is_pty() to common header file and mark as static inline
> 
> Christian
> 
> Christian Brauner (1):
>   linux ttyname and ttyname_r: do not return wrong results
> 
>  ChangeLog                           | 12 ++++++++++++
>  sysdeps/unix/sysv/linux/ttyname.c   | 16 ++++++++++++----
>  sysdeps/unix/sysv/linux/ttyname.h   | 35 +++++++++++++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/ttyname_r.c | 17 +++++++++++++----
>  4 files changed, 72 insertions(+), 8 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/ttyname.h
> 
> -- 
> 2.11.0

[-- Attachment #1.2: enodev_on_pty_in_different_namespace.c --]
[-- Type: text/x-csrc, Size: 1897 bytes --]

/*
 *
 * Copyright © 2017 Christian Brauner <christian.brauner@ubuntu.com>.
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2, as
 * published by the Free Software Foundation.
 *
 * This program 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 General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License along
 * with this program; if not, write to the Free Software Foundation, Inc.,
 * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 */

#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

/* Test whether ttyname() */
int main(int argc, char *argv[])
{
	int fd;
	char buf[4096];

	fd = open("/proc/self/fd/0", O_RDONLY);
	if (fd < 0) {
		fprintf(stderr, "Could not open \"/proc/self/fd/0\": %s.\n",
			strerror(errno));
		exit(EXIT_FAILURE);
	}

	if (!ttyname(fd)) {
		/* COMMENT(brauner): ENODEV will only be set by a patched
		 * glibc.
		 */
		if (errno == ENODEV) {
			printf("ttyname(): The pty device might exist in a "
			       "different "
			       "namespace: %s\n",
			       strerror(errno));
		} else {
			exit(EXIT_FAILURE);
		}
	}

	if (ttyname_r(fd, buf, sizeof(buf))) {
		/* COMMENT(brauner): ENODEV will only be set by a patched
		 * glibc.
		 */
		if (errno == ENODEV) {
			printf("ttyname_r(): The pty device might exist in a "
			       "different "
			       "namespace: %s\n",
			       strerror(errno));
		} else {
			exit(EXIT_FAILURE);
		}
	}

	exit(EXIT_SUCCESS);
}

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

Re: [PATCH v4] linux ttyname and ttyname_r: do not return wrong results

[Dmitry V. Levin]

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

Hi,

On Fri, Jan 27, 2017 at 03:59:58PM +0100, Christian Brauner wrote:
> Hi,
> 
> Last we've talked this patches was ready to merge. The only thing left to do was
> to sign an FSF agreement. I did and it is fully processed.

Could somebody with access to the corresponding file confirm this, please?


-- 
ldv

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

Re: [PATCH v4] linux ttyname and ttyname_r: do not return wrong results

[Christian Brauner]

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

Hi guys,

On Tue, Feb 7, 2017 at 11:38:52AM +0300 Dmitry V. Levin wrote:
> Hi,
> 
> On Fri, Jan 27, 2017 at 03:59:58PM +0100, Christian Brauner wrote:
> > Hi,
> > 
> > Last we've talked this patches was ready to merge. The only thing left to do was
> > to sign an FSF agreement. I did and it is fully processed.
> 
> Could somebody with access to the corresponding file confirm this, please?

friendly ping about this patch again. As I said, last we spoke this was ready
to merge and all legal barriers have also been removed.
(Btw, aside from the personal copyright assignment I signed Canonical now also
has an agreement with FSFE. So there really shouldn't be any blockers anymore.)

Thanks
Christian

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

Re: [PATCH v4] linux ttyname and ttyname_r: do not return wrong results

[Christian Brauner]

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

Hi guys,

friendly ping about this patch again. When we last spoke this patch was ready to
merge and someone needed to confirm that I actually have signed a copyright
agreement. I hope someone is able to confirm this. :) Aside from the personal
copyright assignment I signed, Canonical now also has signed a company-wide
agreement. I hope this will suffice to get this patch merged.

Thanks,
Christian

On Thu, Feb 16, 2017 at 06:00:45PM +0100, Christian Brauner wrote:
> Hi guys,
> 
> On Tue, Feb 7, 2017 at 11:38:52AM +0300 Dmitry V. Levin wrote:
> > Hi,
> > 
> > On Fri, Jan 27, 2017 at 03:59:58PM +0100, Christian Brauner wrote:
> > > Hi,
> > > 
> > > Last we've talked this patches was ready to merge. The only thing left to do was
> > > to sign an FSF agreement. I did and it is fully processed.
> > 
> > Could somebody with access to the corresponding file confirm this, please?
> 
> friendly ping about this patch again. As I said, last we spoke this was ready
> to merge and all legal barriers have also been removed.
> (Btw, aside from the personal copyright assignment I signed Canonical now also
> has an agreement with FSFE. So there really shouldn't be any blockers anymore.)
> 
> Thanks
> Christian



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

A: check /gd/gnuorg/copyright.list (was: Re: [PATCH v4] linux ttyname and ttyname_r: do not return wrong results)

[Dmitry V. Levin]

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

Hi,

On Fri, Mar 03, 2017 at 10:20:12PM +0100, Christian Brauner wrote:
> Hi guys,
> 
> friendly ping about this patch again. When we last spoke this patch was ready to
> merge and someone needed to confirm that I actually have signed a copyright
> agreement. I hope someone is able to confirm this. :) Aside from the personal
> copyright assignment I signed, Canonical now also has signed a company-wide
> agreement. I hope this will suffice to get this patch merged.
> 
> Thanks,
> Christian

According to https://sourceware.org/glibc/wiki/MAINTAINERS, there are
10 maintainers who are responsible for glibc to the GNU Project.

Could one of these honored people consult
fencepost.gnu.org:/gd/gnuorg/copyright.list file and tell us whether we
can accept legally significant contributions from Christian, please?

> On Thu, Feb 16, 2017 at 06:00:45PM +0100, Christian Brauner wrote:
> > Hi guys,
> > 
> > On Tue, Feb 7, 2017 at 11:38:52AM +0300 Dmitry V. Levin wrote:
> > > Hi,
> > > 
> > > On Fri, Jan 27, 2017 at 03:59:58PM +0100, Christian Brauner wrote:
> > > > Hi,
> > > > 
> > > > Last we've talked this patches was ready to merge. The only thing left to do was
> > > > to sign an FSF agreement. I did and it is fully processed.
> > > 
> > > Could somebody with access to the corresponding file confirm this, please?
> > 
> > friendly ping about this patch again. As I said, last we spoke this was ready
> > to merge and all legal barriers have also been removed.
> > (Btw, aside from the personal copyright assignment I signed Canonical now also
> > has an agreement with FSFE. So there really shouldn't be any blockers anymore.)

-- 
ldv

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

Re: A: check /gd/gnuorg/copyright.list (was: Re: [PATCH v4] linux ttyname and ttyname_r: do not return wrong results)

[Christian Brauner]

Hi guys,

just wanted to check in if there is any news about this patch. It was ready to
be merged and it would be super-nice if someone could confirm that I have in
fact signed a copyright agreement/Canonical has signed a company-wide agreement.
I've seen references to this patch on LKML from around mid-Februrary but I
haven't been CCed. I take it this was partially causing the delayed response
(https://lkml.org/lkml/2017/2/19/98). Would be cool if someone could point out
any potential blockers so we might be able to address them.

Thanks!
Christian

On Fri, Mar 3, 2017 at 11:45 PM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> Hi,
>
> On Fri, Mar 03, 2017 at 10:20:12PM +0100, Christian Brauner wrote:
>> Hi guys,
>>
>> friendly ping about this patch again. When we last spoke this patch was ready to
>> merge and someone needed to confirm that I actually have signed a copyright
>> agreement. I hope someone is able to confirm this. :) Aside from the personal
>> copyright assignment I signed, Canonical now also has signed a company-wide
>> agreement. I hope this will suffice to get this patch merged.
>>
>> Thanks,
>> Christian
>
> According to https://sourceware.org/glibc/wiki/MAINTAINERS, there are
> 10 maintainers who are responsible for glibc to the GNU Project.
>
> Could one of these honored people consult
> fencepost.gnu.org:/gd/gnuorg/copyright.list file and tell us whether we
> can accept legally significant contributions from Christian, please?
>
>> On Thu, Feb 16, 2017 at 06:00:45PM +0100, Christian Brauner wrote:
>> > Hi guys,
>> >
>> > On Tue, Feb 7, 2017 at 11:38:52AM +0300 Dmitry V. Levin wrote:
>> > > Hi,
>> > >
>> > > On Fri, Jan 27, 2017 at 03:59:58PM +0100, Christian Brauner wrote:
>> > > > Hi,
>> > > >
>> > > > Last we've talked this patches was ready to merge. The only thing left to do was
>> > > > to sign an FSF agreement. I did and it is fully processed.
>> > >
>> > > Could somebody with access to the corresponding file confirm this, please?
>> >
>> > friendly ping about this patch again. As I said, last we spoke this was ready
>> > to merge and all legal barriers have also been removed.
>> > (Btw, aside from the personal copyright assignment I signed Canonical now also
>> > has an agreement with FSFE. So there really shouldn't be any blockers anymore.)
>
> --
> ldv

Re: [PATCH v4] linux ttyname and ttyname_r: do not return wrong results

[Alexandre Oliva]

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

On Feb  7, 2017, "Dmitry V. Levin" <ldv@altlinux.org> wrote:

> On Fri, Jan 27, 2017 at 03:59:58PM +0100, Christian Brauner wrote:
>> Last we've talked this patches was ready to merge. The only thing left to do was
>> to sign an FSF agreement. I did and it is fully processed.

> Could somebody with access to the corresponding file confirm this, please?

Yes, there are assignments on file by both Christian Brauner and
Canonical Limited, covering past and future changes to GLIBC (thanks!)

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

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

Re: [PATCH v4] linux ttyname and ttyname_r: do not return wrong results

[Dmitry V. Levin]

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

On Fri, Jan 27, 2017 at 03:59:59PM +0100, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@canonical.com>
> 
> If a link (say /proc/self/fd/0) pointing to a device, say /dev/pts/2, in a
> parent mount namespace is passed to ttyname, and a /dev/pts/2 exists (in a
> different devpts) in the current namespace, then it returns /dev/pts/2. But
> /dev/pts/2 is NOT the current tty, it is a different file and device.
> 
> Detect this case and return ENODEV. Userspace can choose to take this as a hint
> that the fd points to a tty device but to act on the fd rather than the link.
> 
> Signed-off-by: Serge Hallyn <serge@hallyn.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Applied, thanks!


-- 
ldv

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