diff mbox

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

Message ID 20161122022953.12513-2-christian.brauner@canonical.com
State New, archived
Headers

Commit Message

Christian Brauner Nov. 22, 2016, 2:29 a.m. UTC 
  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@canonical.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
---
 sysdeps/unix/sysv/linux/ttyname.c   | 16 ++++++++++++----
 sysdeps/unix/sysv/linux/ttyname.h   | 35 +++++++++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/ttyname_r.c | 17 +++++++++++++----
 3 files changed, 60 insertions(+), 8 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/ttyname.h

Comments

Dmitry V. Levin Nov. 24, 2016, 12:48 a.m. UTC | #1 
On Tue, Nov 22, 2016 at 03:29:53AM +0100, Christian Brauner wrote:
> 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@canonical.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
> ---
>  sysdeps/unix/sysv/linux/ttyname.c   | 16 ++++++++++++----
>  sysdeps/unix/sysv/linux/ttyname.h   | 35 +++++++++++++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/ttyname_r.c | 17 +++++++++++++----
>  3 files changed, 60 insertions(+), 8 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/ttyname.h

Could you add a properly formatted GNU-style ChangeLog entry for this
change, please? (https://sourceware.org/glibc/wiki/Contribution%20checklist#Properly_Formatted_GNU_ChangeLog)

[...]
> diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
> new file mode 100644
> index 0000000..19af6e6
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/ttyname.h
> @@ -0,0 +1,35 @@
> +/* Copyright (C) 2004-2016 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.

This file looks like a new one, so could you explain where does this
year range come from?

BTW, the added copyright notice means that the change is legally
significant.  If this is the case, could you explain your current
FSF copyright assignment status, please?
Christian Brauner Nov. 24, 2016, 1:17 a.m. UTC | #2 
On Thu, Nov 24, 2016 at 03:48:58AM +0300, Dmitry V. Levin wrote:
> On Tue, Nov 22, 2016 at 03:29:53AM +0100, Christian Brauner wrote:
> > 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@canonical.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
> > ---
> >  sysdeps/unix/sysv/linux/ttyname.c   | 16 ++++++++++++----
> >  sysdeps/unix/sysv/linux/ttyname.h   | 35 +++++++++++++++++++++++++++++++++++
> >  sysdeps/unix/sysv/linux/ttyname_r.c | 17 +++++++++++++----
> >  3 files changed, 60 insertions(+), 8 deletions(-)
> >  create mode 100644 sysdeps/unix/sysv/linux/ttyname.h
> 
> Could you add a properly formatted GNU-style ChangeLog entry for this
> change, please? (https://sourceware.org/glibc/wiki/Contribution%20checklist#Properly_Formatted_GNU_ChangeLog)

Will send a new version with the requested changelog in a minute.

> 
> [...]
> > diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
> > new file mode 100644
> > index 0000000..19af6e6
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/ttyname.h
> > @@ -0,0 +1,35 @@
> > +/* Copyright (C) 2004-2016 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> 
> This file looks like a new one, so could you explain where does this
> year range come from?

Sorry, the header file was copied over from another header file. I corrected the
year in the patch I'm going to send out in a minute.

> 
> BTW, the added copyright notice means that the change is legally
> significant.  If this is the case, could you explain your current
> FSF copyright assignment status, please?

So far I have not signed any FSF copyright assignment form. If this is
absolutely necessary, I will do so but since we were requested to add a new
simple header file it would be much appreciated if we could avoid this. At least
you could maybe apply the patch and then send me the form.
Dmitry V. Levin Nov. 24, 2016, 10:20 a.m. UTC | #3 
On Thu, Nov 24, 2016 at 02:17:13AM +0100, Christian Brauner wrote:
> On Thu, Nov 24, 2016 at 03:48:58AM +0300, Dmitry V. Levin wrote:
> > On Tue, Nov 22, 2016 at 03:29:53AM +0100, Christian Brauner wrote:
[...]
> > > diff --git a/sysdeps/unix/sysv/linux/ttyname.h b/sysdeps/unix/sysv/linux/ttyname.h
> > > new file mode 100644
> > > index 0000000..19af6e6
> > > --- /dev/null
> > > +++ b/sysdeps/unix/sysv/linux/ttyname.h
> > > @@ -0,0 +1,35 @@
> > > +/* Copyright (C) 2004-2016 Free Software Foundation, Inc.
> > > +   This file is part of the GNU C Library.
> > 
> > This file looks like a new one, so could you explain where does this
> > year range come from?
> 
> Sorry, the header file was copied over from another header file. I corrected the
> year in the patch I'm going to send out in a minute.
> 
> > BTW, the added copyright notice means that the change is legally
> > significant.  If this is the case, could you explain your current
> > FSF copyright assignment status, please?
> 
> So far I have not signed any FSF copyright assignment form. If this is
> absolutely necessary, I will do so but since we were requested to add a new
> simple header file it would be much appreciated if we could avoid this. At least
> you could maybe apply the patch and then send me the form.

Assuming that this is your first contribution to glibc, and the change is
below the threshold of 15 lines of code that is legally significant for
copyright purposes, you don't have to sign a copyright assignment yet.

By adding a copyright notice to the new file, however, you state that
the contribution is legally significant.  If this is the case, then
a copyright assignment must be signed before this change could be merged.

This is just my understanding of
https://www.gnu.org/prep/maintain/maintain.html#Legally-Significant,
there are people responsible for glibc to the GNU Project
(https://sourceware.org/glibc/wiki/MAINTAINERS#Project_stewards_.28GNU_package_maintainers.29)
who have the final say on these matters.
Florian Weimer Nov. 24, 2016, 1:04 p.m. UTC | #4 
On 11/24/2016 02:17 AM, Christian Brauner wrote:

> So far I have not signed any FSF copyright assignment form. If this is
> absolutely necessary, I will do so but since we were requested to add a new
> simple header file it would be much appreciated if we could avoid this. At least
> you could maybe apply the patch and then send me the form.

You need to check with Canonical if they claim copyright over your 
contribution anyway.  I suppose we have a blanket assignment from 
Canonical on file for glibc (at least), but one of the official GNU 
maintainers can check this.

Thanks,
Florian
Joseph Myers Nov. 24, 2016, 1:45 p.m. UTC | #5 
On Thu, 24 Nov 2016, Florian Weimer wrote:

> You need to check with Canonical if they claim copyright over your
> contribution anyway.  I suppose we have a blanket assignment from Canonical on
> file for glibc (at least), but one of the official GNU maintainers can check
> this.

From Canonical, I only see an assignment for GNU-ARCH and a disclaimer for 
one person's changes to WGET.
Christian Brauner Nov. 24, 2016, 8:20 p.m. UTC | #6 
On Thu, Nov 24, 2016 at 02:04:01PM +0100, Florian Weimer wrote:
> On 11/24/2016 02:17 AM, Christian Brauner wrote:
> 
> > So far I have not signed any FSF copyright assignment form. If this is
> > absolutely necessary, I will do so but since we were requested to add a new
> > simple header file it would be much appreciated if we could avoid this. At least
> > you could maybe apply the patch and then send me the form.
> 
> You need to check with Canonical if they claim copyright over your
> contribution anyway.  I suppose we have a blanket assignment from Canonical
> on file for glibc (at least), but one of the official GNU maintainers can
> check this.

This is not a problem.
Ok, on the glibc contribution page it states that a maintainer will point me in
the right direction regarding what steps are required to sign a copyright
agreement with the FSF. Could someone please help me out with this? I suspect I
will contribute in the future as well so something longer lasting would be much
appreciated.

Christian
Christian Brauner Nov. 29, 2016, 2:50 a.m. UTC | #7 
On Thu, Nov 24, 2016 at 09:20:34PM +0100, Christian Brauner wrote:
> On Thu, Nov 24, 2016 at 02:04:01PM +0100, Florian Weimer wrote:
> > On 11/24/2016 02:17 AM, Christian Brauner wrote:
> > 
> > > So far I have not signed any FSF copyright assignment form. If this is
> > > absolutely necessary, I will do so but since we were requested to add a new
> > > simple header file it would be much appreciated if we could avoid this. At least
> > > you could maybe apply the patch and then send me the form.
> > 
> > You need to check with Canonical if they claim copyright over your
> > contribution anyway.  I suppose we have a blanket assignment from Canonical
> > on file for glibc (at least), but one of the official GNU maintainers can
> > check this.
> 
> This is not a problem.
> Ok, on the glibc contribution page it states that a maintainer will point me in
> the right direction regarding what steps are required to sign a copyright
> agreement with the FSF. Could someone please help me out with this? I suspect I
> will contribute in the future as well so something longer lasting would be much
> appreciated.

Hey guys, could someone please send me the correct form or instruct me about the
procedure as advertised on the glibc homepage. I'd like to get this sorted
before the end of the year. :)

Christian
Joseph Myers Nov. 29, 2016, 5:44 p.m. UTC | #8 
On Tue, 29 Nov 2016, Christian Brauner wrote:

> Hey guys, could someone please send me the correct form or instruct me about the
> procedure as advertised on the glibc homepage. I'd like to get this sorted
> before the end of the year. :)

Please see 
<http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future>.
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..0941e2c 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 0000000..19af6e6
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/ttyname.h
@@ -0,0 +1,35 @@ 
+/* Copyright (C) 2004-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 d15bc74..8b3fe53 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.  */