[PATCH 1/1] linux ttyname: return link if appropriate

[PATCH 1/1] linux ttyname: return link if appropriate

[Serge Hallyn]

The current ttyname does the wrong thing in two cases:

1. If the passed-in link (say /proc/self/fd/0) points to a
device, say /dev/pts/2, in a parent mount namespace, 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.

2. If the passed-in link (say /proc/self/fd/0) points to
a device, say /dev/pts/2, in a parent mount namespace, and
/dev/pts/2 does not exist in the current namespace, it
returns success but an empty name.  As far as I can tell,
there is no reason for it to not return /proc/self/fd/0.
http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
does not say anything about not returning a link.

Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
 sysdeps/unix/sysv/linux/ttyname.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..b0500a9 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -170,12 +170,21 @@ ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
+	  && st1.st_dev == st.st_dev
 #else
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
 #endif
 	  )
 	return ttyname_buf;
+      /* If the link doesn't exist, then it points to a dvice in another
+       * namespace.  We've already verified it's a tty.  Just return the
+       * link itself. */
+      if (strlen(procname) < buflen - 1) {
+         memcpy(ttyname_buf, procname, strlen(procname));
+         ttyname_buf[strlen(procname)] = '\0';
+         return ttyname_buf;
+      }
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
-- 
2.7.4

Re: [PATCH 1/1] linux ttyname: return link if appropriate

[Florian Weimer]

On 04/15/2016 05:29 PM, Serge Hallyn wrote:
> The current ttyname does the wrong thing in two cases:
>
> 1. If the passed-in link (say /proc/self/fd/0) points to a
> device, say /dev/pts/2, in a parent mount namespace, 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.

Is this the first change?

> 2. If the passed-in link (say /proc/self/fd/0) points to
> a device, say /dev/pts/2, in a parent mount namespace, and
> /dev/pts/2 does not exist in the current namespace, it
> returns success but an empty name.  As far as I can tell,
> there is no reason for it to not return /proc/self/fd/0.
> http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
> does not say anything about not returning a link.

Is it safe to drop the verification that ttyname ordinarily would do?

ttyname_r will need a similar change.

Florian

Re: [PATCH 1/1] linux ttyname: return link if appropriate

[Serge Hallyn]

Quoting Florian Weimer (fweimer@redhat.com):
> On 04/15/2016 05:29 PM, Serge Hallyn wrote:
> >The current ttyname does the wrong thing in two cases:
> >
> >1. If the passed-in link (say /proc/self/fd/0) points to a
> >device, say /dev/pts/2, in a parent mount namespace, 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.
> 
> Is this the first change?

Right, it ensures that the filesystem of the two files is
the same.

> >2. If the passed-in link (say /proc/self/fd/0) points to
> >a device, say /dev/pts/2, in a parent mount namespace, and
> >/dev/pts/2 does not exist in the current namespace, it
> >returns success but an empty name.  As far as I can tell,
> >there is no reason for it to not return /proc/self/fd/0.
> >http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
> >does not say anything about not returning a link.
> 
> Is it safe to drop the verification that ttyname ordinarily would do?

Which verification do you mean exactly?

> ttyname_r will need a similar change.

Oh, yeah, it will.

thanks,
-serge

Re: [PATCH 1/1] linux ttyname: return link if appropriate

[Florian Weimer]

On 04/15/2016 06:46 PM, Serge Hallyn wrote:
> Quoting Florian Weimer (fweimer@redhat.com):
>> On 04/15/2016 05:29 PM, Serge Hallyn wrote:
>>> The current ttyname does the wrong thing in two cases:
>>>
>>> 1. If the passed-in link (say /proc/self/fd/0) points to a
>>> device, say /dev/pts/2, in a parent mount namespace, 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.
>>
>> Is this the first change?
>
> Right, it ensures that the filesystem of the two files is
> the same.
>
>>> 2. If the passed-in link (say /proc/self/fd/0) points to
>>> a device, say /dev/pts/2, in a parent mount namespace, and
>>> /dev/pts/2 does not exist in the current namespace, it
>>> returns success but an empty name.  As far as I can tell,
>>> there is no reason for it to not return /proc/self/fd/0.
>>> http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
>>> does not say anything about not returning a link.
>>
>> Is it safe to drop the verification that ttyname ordinarily would do?
>
> Which verification do you mean exactly?

That the file descriptor actually belongs to a PTY device listed under 
/dev/pts.

>> ttyname_r will need a similar change.
>
> Oh, yeah, it will.

Please also fix the stylistic issues (GNU style requires a space in 
function calls, braces on their own lines etc.).

But I don't think we can make the change, considering the security 
implications.

Florian

Re: [PATCH 1/1] linux ttyname: return link if appropriate

[Serge Hallyn]

Quoting Florian Weimer (fweimer@redhat.com):
> On 04/15/2016 06:46 PM, Serge Hallyn wrote:
> >Quoting Florian Weimer (fweimer@redhat.com):
> >>On 04/15/2016 05:29 PM, Serge Hallyn wrote:
> >>>The current ttyname does the wrong thing in two cases:
> >>>
> >>>1. If the passed-in link (say /proc/self/fd/0) points to a
> >>>device, say /dev/pts/2, in a parent mount namespace, 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.
> >>
> >>Is this the first change?
> >
> >Right, it ensures that the filesystem of the two files is
> >the same.
> >
> >>>2. If the passed-in link (say /proc/self/fd/0) points to
> >>>a device, say /dev/pts/2, in a parent mount namespace, and
> >>>/dev/pts/2 does not exist in the current namespace, it
> >>>returns success but an empty name.  As far as I can tell,
> >>>there is no reason for it to not return /proc/self/fd/0.
> >>>http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
> >>>does not say anything about not returning a link.
> >>
> >>Is it safe to drop the verification that ttyname ordinarily would do?
> >
> >Which verification do you mean exactly?
> 
> That the file descriptor actually belongs to a PTY device listed
> under /dev/pts.

Oh, yeah.  I think that adding a chck that this is a pts (using st_rdev)
before returning "/proc/self/fd/N" (in my newly added block) would be good.

> >>ttyname_r will need a similar change.
> >
> >Oh, yeah, it will.
> 
> Please also fix the stylistic issues (GNU style requires a space in
> function calls, braces on their own lines etc.).

Oh, yes I can update those.

> But I don't think we can make the change, considering the security
> implications.

What security implications exactly?  Does ensuring that it is a
device on a devpts filesystem address them?

-serge

Re: [PATCH 1/1] linux ttyname: return link if appropriate

[Serge Hallyn]

Quoting Serge Hallyn (serge.hallyn@ubuntu.com):
> Quoting Florian Weimer (fweimer@redhat.com):
> > On 04/15/2016 06:46 PM, Serge Hallyn wrote:
> > >Quoting Florian Weimer (fweimer@redhat.com):
> > >>On 04/15/2016 05:29 PM, Serge Hallyn wrote:
> > >>>The current ttyname does the wrong thing in two cases:
> > >>>
> > >>>1. If the passed-in link (say /proc/self/fd/0) points to a
> > >>>device, say /dev/pts/2, in a parent mount namespace, 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.
> > >>
> > >>Is this the first change?
> > >
> > >Right, it ensures that the filesystem of the two files is
> > >the same.
> > >
> > >>>2. If the passed-in link (say /proc/self/fd/0) points to
> > >>>a device, say /dev/pts/2, in a parent mount namespace, and
> > >>>/dev/pts/2 does not exist in the current namespace, it
> > >>>returns success but an empty name.  As far as I can tell,
> > >>>there is no reason for it to not return /proc/self/fd/0.
> > >>>http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
> > >>>does not say anything about not returning a link.
> > >>
> > >>Is it safe to drop the verification that ttyname ordinarily would do?
> > >
> > >Which verification do you mean exactly?
> > 
> > That the file descriptor actually belongs to a PTY device listed
> > under /dev/pts.
> 
> Oh, yeah.  I think that adding a chck that this is a pts (using st_rdev)
> before returning "/proc/self/fd/N" (in my newly added block) would be good.

Something like:

From ba0dc51e90d884b107145924821a1e8caf43a468 Mon Sep 17 00:00:00 2001
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Date: Fri, 15 Apr 2016 10:21:07 -0500
Subject: [PATCH 1/1] linux ttyname: return link if appropriate

The current ttyname does the wrong thing in two cases:

1. If the passed-in link (say /proc/self/fd/0) points to a
device, say /dev/pts/2, in a parent mount namespace, 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.

2. If the passed-in link (say /proc/self/fd/0) points to
a device, say /dev/pts/2, in a parent mount namespace, and
/dev/pts/2 does not exist in the current namespace, it
returns success but an empty name.  As far as I can tell,
there is no reason for it to not return /proc/self/fd/0.
http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
does not say anything about not returning a link.

Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
 sysdeps/unix/sysv/linux/ttyname.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..54d0e6b 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -33,6 +33,21 @@
 char *__ttyname;
 #endif
 
+/*
+ * Return true if this is a UNIX98 pty device, as defined in
+ * linux/Documentation/devices.txt
+ */
+static int
+is_pty (struct stat *sb)
+{
+#ifdef _STATBUF_ST_RDEV
+  int m = major (sb.st_rdev);
+  return (136 <= m && m <= 143);
+#else
+  return false;
+#endif
+}
+
 static char *getttyname (const char *dev, dev_t mydev,
 			 ino64_t myino, int save, int *dostat)
      internal_function;
@@ -170,12 +185,22 @@ ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
+	  && st1.st_dev == st.st_dev
 #else
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
 #endif
 	  )
 	return ttyname_buf;
+      /* If the link doesn't exist, then it points to a dvice in another
+       * namespace.  If it is a UNIX98 pty, then return the /proc/self
+       * fd, as it points to a name unreachable in our namespace */
+      if (is_pty (st) && strlen (procname) < buflen - 1)
+        {
+          memcpy (ttyname_buf, procname, strlen (procname));
+          ttyname_buf[strlen (procname)] = '\0';
+          return ttyname_buf;
+        }
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
-- 
2.7.4

Re: [PATCH 1/1] linux ttyname: return link if appropriate

[Mike Frysinger]

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

On 15 Apr 2016 18:47, Serge Hallyn wrote:
> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>

we don't use s-o-b tags

> +/*
> + * Return true if this is a UNIX98 pty device, as defined in
> + * linux/Documentation/devices.txt
> + */

GNU style is:

/* Return true if this is a UNIX98 pty device, as defined in
   linux/Documentation/devices.txt.  */

this applies to comments below too

> +      if (is_pty (st) && strlen (procname) < buflen - 1)
> +        {
> +          memcpy (ttyname_buf, procname, strlen (procname));
> +          ttyname_buf[strlen (procname)] = '\0';

since you already verified buflen, why not use strcpy ?

also, GNU style says 8 spaces -> 1 tab
-mike

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

Re: [PATCH 1/1] linux ttyname: return link if appropriate

[Serge Hallyn]

Quoting Mike Frysinger (vapier@gentoo.org):
> On 15 Apr 2016 18:47, Serge Hallyn wrote:
> > Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
> 
> we don't use s-o-b tags
> 
> > +/*
> > + * Return true if this is a UNIX98 pty device, as defined in
> > + * linux/Documentation/devices.txt
> > + */
> 
> GNU style is:
> 
> /* Return true if this is a UNIX98 pty device, as defined in
>    linux/Documentation/devices.txt.  */
> 
> this applies to comments below too
> 
> > +      if (is_pty (st) && strlen (procname) < buflen - 1)
> > +        {
> > +          memcpy (ttyname_buf, procname, strlen (procname));
> > +          ttyname_buf[strlen (procname)] = '\0';
> 
> since you already verified buflen, why not use strcpy ?

That actually had been my first inclination, not sure why I
switched it.

> also, GNU style says 8 spaces -> 1 tab
> -mike

There were a few other bugs as well.  Replying with a new patch.

thanks,
-serge

[PATCH 1/2] linux ttyname: return link if appropriate

[Serge Hallyn]

The current ttyname does the wrong thing in two cases:

1. If the passed-in link (say /proc/self/fd/0) points to a
device, say /dev/pts/2, in a parent mount namespace, 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.

2. If the passed-in link (say /proc/self/fd/0) points to
a device, say /dev/pts/2, in a parent mount namespace, and
/dev/pts/2 does not exist in the current namespace, it
returns success but an empty name.  As far as I can tell,
there is no reason for it to not return /proc/self/fd/0.
http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
does not say anything about not returning a link.
---
 sysdeps/unix/sysv/linux/ttyname.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..430fb48 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -33,6 +33,19 @@
 char *__ttyname;
 #endif
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt */
+static 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
+}
+
 static char *getttyname (const char *dev, dev_t mydev,
 			 ino64_t myino, int save, int *dostat)
      internal_function;
@@ -170,12 +183,22 @@ ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
+	  && st1.st_dev == st.st_dev
 #else
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
 #endif
 	  )
 	return ttyname_buf;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+        {
+	  strcpy (ttyname_buf, procname);
+	  return ttyname_buf;
+        }
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
-- 
2.7.4

Re: [PATCH 1/2] linux ttyname: return link if appropriate

[Mike Frysinger]

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

On 18 Apr 2016 19:53, Serge Hallyn wrote:
> +	  strcpy (ttyname_buf, procname);
> +	  return ttyname_buf;

at this point you could just write:
  return strcpy (ttyname_buf, procname);
-mike

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

Re: [PATCH 1/2] linux ttyname: return link if appropriate

[Serge Hallyn]

Quoting Mike Frysinger (vapier@gentoo.org):
> On 18 Apr 2016 19:53, Serge Hallyn wrote:
> > +	  strcpy (ttyname_buf, procname);
> > +	  return ttyname_buf;
> 
> at this point you could just write:
>   return strcpy (ttyname_buf, procname);

Yup.

I can send another patch (squashed), but I'm sort of waiting
for Florian's answer about security concerns.

[PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge Hallyn]

The current ttyname does the wrong thing in two cases:

1. If the passed-in link (say /proc/self/fd/0) points to a
device, say /dev/pts/2, in a parent mount namespace, 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.

2. If the passed-in link (say /proc/self/fd/0) points to
a device, say /dev/pts/2, in a parent mount namespace, and
/dev/pts/2 does not exist in the current namespace, it
returns success but an empty name.  As far as I can tell,
there is no reason for it to not return /proc/self/fd/0.
http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
does not say anything about not returning a link.
---
 sysdeps/unix/sysv/linux/ttyname.c   | 20 ++++++++++++++++++++
 sysdeps/unix/sysv/linux/ttyname_r.c | 22 ++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..932140a 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -33,6 +33,19 @@
 char *__ttyname;
 #endif
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt */
+static 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
+}
+
 static char *getttyname (const char *dev, dev_t mydev,
 			 ino64_t myino, int save, int *dostat)
      internal_function;
@@ -170,12 +183,19 @@ ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
+	  && st1.st_dev == st.st_dev
 #else
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
 #endif
 	  )
 	return ttyname_buf;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	return strcpy (ttyname_buf, procname);
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index d15bc74..e3d0c8b 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -32,6 +32,19 @@ static int getttyname_r (char *buf, size_t buflen,
 			 dev_t mydev, ino64_t myino, int save,
 			 int *dostat) internal_function;
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt */
+static 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
+}
+
 static int
 internal_function attribute_compat_text_section
 getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
@@ -158,6 +171,15 @@ __ttyname_r (int fd, char *buf, size_t buflen)
 #endif
 	  )
 	return 0;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	{
+	  strcpy (buf, procname);
+	  return 0;
+	}
     }
 
   /* Prepare the result buffer.  */
-- 
2.7.4

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Mike Frysinger]

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

On 20 Apr 2016 02:10, Serge Hallyn wrote:
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
>
> +   linux/Documentation/devices.txt */

should be a period at the end followed by two spaces

> +  int m = major (sb->st_rdev);

you'll need to include sys/sysmacros.h in this file since you're using
major now.

> +      /* If the link doesn't exist, then it points to a device in another
> +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> +	 fd, as it points to a name unreachable in our namespace */

should be "namespace.  */" at the end ;)

> --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> +++ b/sysdeps/unix/sysv/linux/ttyname_r.c

same comments here
-mike

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

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge Hallyn]

Quoting Mike Frysinger (vapier@gentoo.org):
> On 20 Apr 2016 02:10, Serge Hallyn wrote:
> > --- a/sysdeps/unix/sysv/linux/ttyname.c
> > +++ b/sysdeps/unix/sysv/linux/ttyname.c
> >
> > +   linux/Documentation/devices.txt */
> 
> should be a period at the end followed by two spaces
> 
> > +  int m = major (sb->st_rdev);
> 
> you'll need to include sys/sysmacros.h in this file since you're using
> major now.

Hm, odd - it did build here fine.  But presumably that's due to
something in my local environment, so added it.  New version below,
thanks.

> > +      /* If the link doesn't exist, then it points to a device in another
> > +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> > +	 fd, as it points to a name unreachable in our namespace */
> 
> should be "namespace.  */" at the end ;)
> 
> > --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> > +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> 
> same comments here
> -mike

Subject: linux ttyname and ttyname_r: return link if appropriate

The current ttyname does the wrong thing in two cases:

1. If the passed-in link (say /proc/self/fd/0) points to a
device, say /dev/pts/2, in a parent mount namespace, 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.

2. If the passed-in link (say /proc/self/fd/0) points to
a device, say /dev/pts/2, in a parent mount namespace, and
/dev/pts/2 does not exist in the current namespace, it
returns success but an empty name.  As far as I can tell,
there is no reason for it to not return /proc/self/fd/0.
http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
does not say anything about not returning a link.
---
 sysdeps/unix/sysv/linux/ttyname.c   | 21 +++++++++++++++++++++
 sysdeps/unix/sysv/linux/ttyname_r.c | 23 +++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..d0d8d2c 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -33,6 +34,19 @@
 char *__ttyname;
 #endif
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static char *getttyname (const char *dev, dev_t mydev,
 			 ino64_t myino, int save, int *dostat)
      internal_function;
@@ -170,12 +184,19 @@ ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
+	  && st1.st_dev == st.st_dev
 #else
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
 #endif
 	  )
 	return ttyname_buf;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	return strcpy (ttyname_buf, procname);
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index d15bc74..d5f6f05 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -32,6 +33,19 @@ static int getttyname_r (char *buf, size_t buflen,
 			 dev_t mydev, ino64_t myino, int save,
 			 int *dostat) internal_function;
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static int
 internal_function attribute_compat_text_section
 getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
@@ -158,6 +172,15 @@ __ttyname_r (int fd, char *buf, size_t buflen)
 #endif
 	  )
 	return 0;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	{
+	  strcpy (buf, procname);
+	  return 0;
+	}
     }
 
   /* Prepare the result buffer.  */
-- 
2.7.4

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

Hi,

back in april we went through some back and forth on this patch to fix a
behavior in ttyname which broke containers in some setups.  It also now
apparently breaks the package manager dnf in some setups.  The last
email appears to be this one

https://sourceware.org/ml/libc-alpha/2016-04/msg00500.html

I just checked, and the patch still applies cleanly to today's git head. If
there are no further objections could this patch be applied?

thanks,
-serge

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Dmitry V. Levin]

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

On Wed, Apr 20, 2016 at 06:51:41PM +0000, Serge Hallyn wrote:
> 1. If the passed-in link (say /proc/self/fd/0) points to a
> device, say /dev/pts/2, in a parent mount namespace, 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.
[...]
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
[...]
> @@ -170,12 +184,19 @@ ttyname (int fd)
>  #ifdef _STATBUF_ST_RDEV
>  	  && S_ISCHR (st1.st_mode)
>  	  && st1.st_rdev == st.st_rdev
> +	  && st1.st_dev == st.st_dev
>  #else
>  	  && st1.st_ino == st.st_ino
>  	  && st1.st_dev == st.st_dev
>  #endif

__ttyname_r also needs this st_dev check.

To be on the safe side, I'd check st_ino in _STATBUF_ST_RDEV case as well.


-- 
ldv

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

[PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

Quoting Dmitry V. Levin (ldv@altlinux.org):
> On Wed, Apr 20, 2016 at 06:51:41PM +0000, Serge Hallyn wrote:
> > 1. If the passed-in link (say /proc/self/fd/0) points to a
> > device, say /dev/pts/2, in a parent mount namespace, 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.
> [...]
> > --- a/sysdeps/unix/sysv/linux/ttyname.c
> > +++ b/sysdeps/unix/sysv/linux/ttyname.c
> [...]
> > @@ -170,12 +184,19 @@ ttyname (int fd)
> >  #ifdef _STATBUF_ST_RDEV
> >  	  && S_ISCHR (st1.st_mode)
> >  	  && st1.st_rdev == st.st_rdev
> > +	  && st1.st_dev == st.st_dev
> >  #else
> >  	  && st1.st_ino == st.st_ino
> >  	  && st1.st_dev == st.st_dev
> >  #endif
> 
> __ttyname_r also needs this st_dev check.
> 
> To be on the safe side, I'd check st_ino in _STATBUF_ST_RDEV case as well.

(new patch follows)

The current ttyname does the wrong thing in two cases:

1. If the passed-in link (say /proc/self/fd/0) points to a
device, say /dev/pts/2, in a parent mount namespace, 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.

2. If the passed-in link (say /proc/self/fd/0) points to
a device, say /dev/pts/2, in a parent mount namespace, and
/dev/pts/2 does not exist in the current namespace, it
returns success but an empty name.  As far as I can tell,
there is no reason for it to not return /proc/self/fd/0.
http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
does not say anything about not returning a link.
---
 sysdeps/unix/sysv/linux/ttyname.c   | 22 ++++++++++++++++++++++
 sysdeps/unix/sysv/linux/ttyname_r.c | 25 +++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..c45a847 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -33,6 +34,19 @@
 char *__ttyname;
 #endif
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static char *getttyname (const char *dev, dev_t mydev,
 			 ino64_t myino, int save, int *dostat)
      internal_function;
@@ -170,12 +184,20 @@ ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
+	  && st1.st_dev == st.st_dev
+	  && st1.st_ino == st.st_ino
 #else
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
 #endif
 	  )
 	return ttyname_buf;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	return strcpy (ttyname_buf, procname);
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index d15bc74..d95d07e 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -32,6 +33,19 @@ static int getttyname_r (char *buf, size_t buflen,
 			 dev_t mydev, ino64_t myino, int save,
 			 int *dostat) internal_function;
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static int
 internal_function attribute_compat_text_section
 getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
@@ -152,12 +166,23 @@ __ttyname_r (int fd, char *buf, size_t buflen)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
+	  && st1.st_dev == st.st_dev
+	  && st1.st_ino == st.st_ino
 #else
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
 #endif
 	  )
 	return 0;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	{
+	  strcpy (buf, procname);
+	  return 0;
+	}
     }
 
   /* Prepare the result buffer.  */
-- 
2.7.4

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Mike Frysinger]

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

On 05 Aug 2016 21:08, Serge E. Hallyn wrote:
>  #ifdef _STATBUF_ST_RDEV
>  	  && S_ISCHR (st1.st_mode)
>  	  && st1.st_rdev == st.st_rdev
> +	  && st1.st_dev == st.st_dev
> +	  && st1.st_ino == st.st_ino
>  #else
>  	  && st1.st_ino == st.st_ino
>  	  && st1.st_dev == st.st_dev
>  #endif

wouldn't it be better to shuffle the ifdef then ?
	#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)
-mike

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

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

Quoting Mike Frysinger (vapier@gentoo.org):
> On 05 Aug 2016 21:08, Serge E. Hallyn wrote:
> >  #ifdef _STATBUF_ST_RDEV
> >  	  && S_ISCHR (st1.st_mode)
> >  	  && st1.st_rdev == st.st_rdev
> > +	  && st1.st_dev == st.st_dev
> > +	  && st1.st_ino == st.st_ino
> >  #else
> >  	  && st1.st_ino == st.st_ino
> >  	  && st1.st_dev == st.st_dev
> >  #endif
> 
> wouldn't it be better to shuffle the ifdef then ?
> 	#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)

That occurred to me when I looked at it again after sending,
seems reasonable.

Subject: [PATCH] linux ttyname and ttyname_r: return link if appropriate

The current ttyname does the wrong thing in two cases:

1. If the passed-in link (say /proc/self/fd/0) points to a
device, say /dev/pts/2, in a parent mount namespace, 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.

2. If the passed-in link (say /proc/self/fd/0) points to
a device, say /dev/pts/2, in a parent mount namespace, and
/dev/pts/2 does not exist in the current namespace, it
returns success but an empty name.  As far as I can tell,
there is no reason for it to not return /proc/self/fd/0.
http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
does not say anything about not returning a link.
---
 sysdeps/unix/sysv/linux/ttyname.c   | 25 ++++++++++++++++++++++---
 sysdeps/unix/sysv/linux/ttyname_r.c | 26 ++++++++++++++++++++++++--
 2 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..3065ed4 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -33,6 +34,19 @@
 char *__ttyname;
 #endif
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static char *getttyname (const char *dev, dev_t mydev,
 			 ino64_t myino, int save, int *dostat)
      internal_function;
@@ -170,12 +184,17 @@ 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_dev == st.st_dev
+	  && st1.st_ino == st.st_ino
 	  )
 	return ttyname_buf;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	return strcpy (ttyname_buf, procname);
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index d15bc74..97527c0 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -32,6 +33,19 @@ static int getttyname_r (char *buf, size_t buflen,
 			 dev_t mydev, ino64_t myino, int save,
 			 int *dostat) internal_function;
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static int
 internal_function attribute_compat_text_section
 getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
@@ -152,12 +166,20 @@ __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
+#endif
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
-#endif
 	  )
 	return 0;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	{
+	  strcpy (buf, procname);
+	  return 0;
+	}
     }
 
   /* Prepare the result buffer.  */
-- 
2.7.4

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serge@hallyn.com):
> Quoting Mike Frysinger (vapier@gentoo.org):
> > On 05 Aug 2016 21:08, Serge E. Hallyn wrote:
> > >  #ifdef _STATBUF_ST_RDEV
> > >  	  && S_ISCHR (st1.st_mode)
> > >  	  && st1.st_rdev == st.st_rdev
> > > +	  && st1.st_dev == st.st_dev
> > > +	  && st1.st_ino == st.st_ino
> > >  #else
> > >  	  && st1.st_ino == st.st_ino
> > >  	  && st1.st_dev == st.st_dev
> > >  #endif
> > 
> > wouldn't it be better to shuffle the ifdef then ?
> > 	#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)
> 
> That occurred to me when I looked at it again after sending,
> seems reasonable.

Is this now ready to be applied?  More peopel appear to be running into
it, https://github.com/lxc/lxc/issues/554

> Subject: [PATCH] linux ttyname and ttyname_r: return link if appropriate
> 
> The current ttyname does the wrong thing in two cases:
> 
> 1. If the passed-in link (say /proc/self/fd/0) points to a
> device, say /dev/pts/2, in a parent mount namespace, 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.
> 
> 2. If the passed-in link (say /proc/self/fd/0) points to
> a device, say /dev/pts/2, in a parent mount namespace, and
> /dev/pts/2 does not exist in the current namespace, it
> returns success but an empty name.  As far as I can tell,
> there is no reason for it to not return /proc/self/fd/0.
> http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
> does not say anything about not returning a link.
> ---
>  sysdeps/unix/sysv/linux/ttyname.c   | 25 ++++++++++++++++++++++---
>  sysdeps/unix/sysv/linux/ttyname_r.c | 26 ++++++++++++++++++++++++--
>  2 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
> index 7a001b4..3065ed4 100644
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
> @@ -25,6 +25,7 @@
>  #include <unistd.h>
>  #include <string.h>
>  #include <stdlib.h>
> +#include <sys/sysmacros.h>
>  
>  #include <_itoa.h>
>  
> @@ -33,6 +34,19 @@
>  char *__ttyname;
>  #endif
>  
> +/* Return true if this is a UNIX98 pty device, as defined in
> +   linux/Documentation/devices.txt.  */
> +static 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
> +}
> +
>  static char *getttyname (const char *dev, dev_t mydev,
>  			 ino64_t myino, int save, int *dostat)
>       internal_function;
> @@ -170,12 +184,17 @@ 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_dev == st.st_dev
> +	  && st1.st_ino == st.st_ino
>  	  )
>  	return ttyname_buf;
> +
> +      /* If the link doesn't exist, then it points to a device in another
> +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> +	 fd, as it points to a name unreachable in our namespace.  */
> +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> +	return strcpy (ttyname_buf, procname);
>      }
>  
>    if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
> diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
> index d15bc74..97527c0 100644
> --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> @@ -25,6 +25,7 @@
>  #include <unistd.h>
>  #include <string.h>
>  #include <stdlib.h>
> +#include <sys/sysmacros.h>
>  
>  #include <_itoa.h>
>  
> @@ -32,6 +33,19 @@ static int getttyname_r (char *buf, size_t buflen,
>  			 dev_t mydev, ino64_t myino, int save,
>  			 int *dostat) internal_function;
>  
> +/* Return true if this is a UNIX98 pty device, as defined in
> +   linux/Documentation/devices.txt.  */
> +static 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
> +}
> +
>  static int
>  internal_function attribute_compat_text_section
>  getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
> @@ -152,12 +166,20 @@ __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
> +#endif
>  	  && st1.st_ino == st.st_ino
>  	  && st1.st_dev == st.st_dev
> -#endif
>  	  )
>  	return 0;
> +
> +      /* If the link doesn't exist, then it points to a device in another
> +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> +	 fd, as it points to a name unreachable in our namespace.  */
> +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> +	{
> +	  strcpy (buf, procname);
> +	  return 0;
> +	}
>      }
>  
>    /* Prepare the result buffer.  */
> -- 
> 2.7.4

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Dmitry V. Levin]

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

On Sat, Aug 06, 2016 at 10:00:02AM -0500, Serge E. Hallyn wrote:
[...]
> Subject: [PATCH] linux ttyname and ttyname_r: return link if appropriate

Please compare

> @@ -170,12 +184,17 @@ 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_dev == st.st_dev
> +	  && st1.st_ino == st.st_ino

with

> @@ -152,12 +166,20 @@ __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
> +#endif
>  	  && st1.st_ino == st.st_ino
>  	  && st1.st_dev == st.st_dev
> -#endif

Wouldn't it be better if both functions implemented these comparisons
exactly the same way?


-- 
ldv

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

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

Quoting Dmitry V. Levin (ldv@altlinux.org):
> On Sat, Aug 06, 2016 at 10:00:02AM -0500, Serge E. Hallyn wrote:
> [...]
> > Subject: [PATCH] linux ttyname and ttyname_r: return link if appropriate
> 
> Please compare
> 
> > @@ -170,12 +184,17 @@ 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_dev == st.st_dev
> > +	  && st1.st_ino == st.st_ino
> 
> with
> 
> > @@ -152,12 +166,20 @@ __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
> > +#endif
> >  	  && st1.st_ino == st.st_ino
> >  	  && st1.st_dev == st.st_dev
> > -#endif
> 
> Wouldn't it be better if both functions implemented these comparisons
> exactly the same way?

Subject: [PATCH] linux ttyname and ttyname_r: return link if appropriate

The current ttyname does the wrong thing in two cases:

1. If the passed-in link (say /proc/self/fd/0) points to a
device, say /dev/pts/2, in a parent mount namespace, 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.

2. If the passed-in link (say /proc/self/fd/0) points to
a device, say /dev/pts/2, in a parent mount namespace, and
/dev/pts/2 does not exist in the current namespace, it
returns success but an empty name.  As far as I can tell,
there is no reason for it to not return /proc/self/fd/0.
http://pubs.opengroup.org/onlinepubs/009695399/functions/ttyname.html
does not say anything about not returning a link.
---
 sysdeps/unix/sysv/linux/ttyname.c   | 23 +++++++++++++++++++++--
 sysdeps/unix/sysv/linux/ttyname_r.c | 26 ++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..e28970f 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -33,6 +34,19 @@
 char *__ttyname;
 #endif
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static char *getttyname (const char *dev, dev_t mydev,
 			 ino64_t myino, int save, int *dostat)
      internal_function;
@@ -170,12 +184,17 @@ ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
-#else
+#endif
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
-#endif
 	  )
 	return ttyname_buf;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	return strcpy (ttyname_buf, procname);
     }
 
   if (__xstat64 (_STAT_VER, "/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode))
diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index d15bc74..97527c0 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -32,6 +33,19 @@ static int getttyname_r (char *buf, size_t buflen,
 			 dev_t mydev, ino64_t myino, int save,
 			 int *dostat) internal_function;
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static int
 internal_function attribute_compat_text_section
 getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
@@ -152,12 +166,20 @@ __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
+#endif
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
-#endif
 	  )
 	return 0;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+	{
+	  strcpy (buf, procname);
+	  return 0;
+	}
     }
 
   /* Prepare the result buffer.  */
-- 
2.7.4

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Dmitry V. Levin]

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

On Tue, Aug 09, 2016 at 04:39:37PM -0500, Serge E. Hallyn wrote:
[...]
> --- a/sysdeps/unix/sysv/linux/ttyname.c
> +++ b/sysdeps/unix/sysv/linux/ttyname.c
[...]
> +      /* If the link doesn't exist, then it points to a device in another
> +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> +	 fd, as it points to a name unreachable in our namespace.  */
> +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> +	return strcpy (ttyname_buf, procname);

With buflen == 4095 and sizeof(procname) == 30, this buflen check looks
redundant.  To keep the safe side, I'd rather add a static assert instead,
e.g.

#define TTYNAME_BUFLEN 4095
...
buflen = TTYNAME_BUFLEN;
...
_Static_assert (sizeof (procname) < TTYNAME_BUFLEN, "buflen too small");

[...]
> --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
[...]
> +      /* If the link doesn't exist, then it points to a device in another
> +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> +	 fd, as it points to a name unreachable in our namespace.  */
> +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> +	{
> +	  strcpy (buf, procname);
> +	  return 0;
> +	}

Unlike ttyname.c, here buflen might be quite small, and this code skips
strlen (procname) == buflen - 1.  Shouldn't it rather be
strlen (procname) < buflen ?

If is_pty(&st) is true but buflen is too small for procname, is there any
chance of finding the device using getttyname_r?  Shouldn't ttyname_r fail
with ERANGE instead?


-- 
ldv

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

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

Quoting Dmitry V. Levin (ldv@altlinux.org):
> On Tue, Aug 09, 2016 at 04:39:37PM -0500, Serge E. Hallyn wrote:
> [...]
> > --- a/sysdeps/unix/sysv/linux/ttyname.c
> > +++ b/sysdeps/unix/sysv/linux/ttyname.c
> [...]
> > +      /* If the link doesn't exist, then it points to a device in another
> > +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> > +	 fd, as it points to a name unreachable in our namespace.  */
> > +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> > +	return strcpy (ttyname_buf, procname);
> 
> With buflen == 4095 and sizeof(procname) == 30, this buflen check looks
> redundant.  To keep the safe side, I'd rather add a static assert instead,
> e.g.
> 
> #define TTYNAME_BUFLEN 4095
> ...
> buflen = TTYNAME_BUFLEN;
> ...
> _Static_assert (sizeof (procname) < TTYNAME_BUFLEN, "buflen too small");
> 
> [...]
> > --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> > +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> [...]
> > +      /* If the link doesn't exist, then it points to a device in another
> > +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> > +	 fd, as it points to a name unreachable in our namespace.  */
> > +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> > +	{
> > +	  strcpy (buf, procname);
> > +	  return 0;
> > +	}
> 
> Unlike ttyname.c, here buflen might be quite small, and this code skips
> strlen (procname) == buflen - 1.  Shouldn't it rather be
> strlen (procname) < buflen ?

Hm, yeah that's right.

> If is_pty(&st) is true but buflen is too small for procname, is there any
> chance of finding the device using getttyname_r?  Shouldn't ttyname_r fail
> with ERANGE instead?

So you mean

	if (is_pty (&st)) {
		if (strlen (procname) < buflen) {
			strcpy (buf, procname);
			return 0;
		}
		return -ERANGE;
	}

?

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Dmitry V. Levin]

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

On Wed, Aug 10, 2016 at 06:24:30PM -0500, Serge E. Hallyn wrote:
> Quoting Dmitry V. Levin (ldv@altlinux.org):
> > On Tue, Aug 09, 2016 at 04:39:37PM -0500, Serge E. Hallyn wrote:
> > [...]
> > > --- a/sysdeps/unix/sysv/linux/ttyname.c
> > > +++ b/sysdeps/unix/sysv/linux/ttyname.c
> > [...]
> > > +      /* If the link doesn't exist, then it points to a device in another
> > > +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> > > +	 fd, as it points to a name unreachable in our namespace.  */
> > > +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> > > +	return strcpy (ttyname_buf, procname);
> > 
> > With buflen == 4095 and sizeof(procname) == 30, this buflen check looks
> > redundant.  To keep the safe side, I'd rather add a static assert instead,
> > e.g.
> > 
> > #define TTYNAME_BUFLEN 4095
> > ...
> > buflen = TTYNAME_BUFLEN;
> > ...
> > _Static_assert (sizeof (procname) < TTYNAME_BUFLEN, "buflen too small");
> > 
> > [...]
> > > --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> > > +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> > [...]
> > > +      /* If the link doesn't exist, then it points to a device in another
> > > +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> > > +	 fd, as it points to a name unreachable in our namespace.  */
> > > +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> > > +	{
> > > +	  strcpy (buf, procname);
> > > +	  return 0;
> > > +	}
> > 
> > Unlike ttyname.c, here buflen might be quite small, and this code skips
> > strlen (procname) == buflen - 1.  Shouldn't it rather be
> > strlen (procname) < buflen ?
> 
> Hm, yeah that's right.
> 
> > If is_pty(&st) is true but buflen is too small for procname, is there any
> > chance of finding the device using getttyname_r?  Shouldn't ttyname_r fail
> > with ERANGE instead?
> 
> So you mean
> 
> 	if (is_pty (&st)) {
> 		if (strlen (procname) < buflen) {
> 			strcpy (buf, procname);
> 			return 0;
> 		}
> 		return -ERANGE;
> 	}
> 
> ?

Yes, something like that.  In fact, it has to be

	__set_errno (ERANGE);
	return ERANGE;


-- 
ldv

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

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Florian Weimer]

On 08/09/2016 11:39 PM, Serge E. Hallyn wrote:
> 2. If the passed-in link (say /proc/self/fd/0) points to
> a device, say /dev/pts/2, in a parent mount namespace, and
> /dev/pts/2 does not exist in the current namespace, it
> returns success but an empty name.  As far as I can tell,
> there is no reason for it to not return /proc/self/fd/0.

I'm still concerned that returning something which is not a stable 
reference to a fixed PTY could introduce security vulnerabilities 
because some applications may assume that if ttyname succeeds, the 
returned name is safe to chmod/chown (even if the descriptor is closed 
beforehand).

Maybe this is small risk and is worth the change.  It seems rather 
unlikely that the process would close the descriptor and open a new one 
in its place before the file system change.

Florian

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

Quoting Florian Weimer (fweimer@redhat.com):
> On 08/09/2016 11:39 PM, Serge E. Hallyn wrote:
> >2. If the passed-in link (say /proc/self/fd/0) points to
> >a device, say /dev/pts/2, in a parent mount namespace, and
> >/dev/pts/2 does not exist in the current namespace, it
> >returns success but an empty name.  As far as I can tell,
> >there is no reason for it to not return /proc/self/fd/0.
> 
> I'm still concerned that returning something which is not a stable
> reference to a fixed PTY could introduce security vulnerabilities
> because some applications may assume that if ttyname succeeds, the
> returned name is safe to chmod/chown

which (I think you know, but just to state it) it can be, but

> (even if the descriptor is
> closed beforehand).

Indeed, not, good point.

> Maybe this is small risk and is worth the change.  It seems rather
> unlikely that the process would close the descriptor and open a new
> one in its place before the file system change.

... not inconceivable though, but i would expect most cases of ttyname
plus chown would be using 0/1/2, not a newly opened fd.  This might be
worth a code search.

But, even if we decide that part is dangerous, the part where we do not
return /dev/pts/N when /proc/self/fd/M is from a different devpts mount
than the /dev/pts/N in caller's namespace is I think very important, and
should at least be separately applied.

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Dmitry V. Levin]

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

On Wed, Aug 10, 2016 at 06:03:51PM -0500, Serge E. Hallyn wrote:
[...]
> But, even if we decide that part is dangerous, the part where we do not
> return /dev/pts/N when /proc/self/fd/M is from a different devpts mount
> than the /dev/pts/N in caller's namespace is I think very important, and
> should at least be separately applied.

I agree.
In that case, what should ttyname/ttyname_r set errno to? ENOTTY?


-- 
ldv

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

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

Quoting Dmitry V. Levin (ldv@altlinux.org):
> On Wed, Aug 10, 2016 at 06:03:51PM -0500, Serge E. Hallyn wrote:
> [...]
> > But, even if we decide that part is dangerous, the part where we do not
> > return /dev/pts/N when /proc/self/fd/M is from a different devpts mount
> > than the /dev/pts/N in caller's namespace is I think very important, and
> > should at least be separately applied.
> 
> I agree.
> In that case, what should ttyname/ttyname_r set errno to? ENOTTY?

Ideally something like EXDEV, which updated userspace could recognize
as "shucks i'll just use /proc/self/fd/N", but that probably isn't
realistically do-able?

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

On Thu, Aug 11, 2016 at 02:18:18AM +0300, Dmitry V. Levin wrote:
> On Wed, Aug 10, 2016 at 06:03:51PM -0500, Serge E. Hallyn wrote:
> [...]
> > But, even if we decide that part is dangerous, the part where we do not
> > return /dev/pts/N when /proc/self/fd/M is from a different devpts mount
> > than the /dev/pts/N in caller's namespace is I think very important, and
> > should at least be separately applied.
> 
> I agree.
> In that case, what should ttyname/ttyname_r set errno to? ENOTTY?

I chose ENODEV below.  I like that as it is more meaningful to uptodate
userspace.  Is it ok with you?

thanks,
-serge

From 72de5a6616dde09c2851554b917a07dd7ebc1449 Mon Sep 17 00:00:00 2001
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Date: Fri, 15 Apr 2016 10:21:07 -0500
Subject: [PATCH 1/1] linux ttyname and ttyname_r: do not return wrong results

If a link (say /proc/self/fd/0) pointint 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.
---
 sysdeps/unix/sysv/linux/ttyname.c   | 25 +++++++++++++++++++++++--
 sysdeps/unix/sysv/linux/ttyname_r.c | 26 ++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..a9e7e20 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -33,6 +34,19 @@
 char *__ttyname;
 #endif
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static char *getttyname (const char *dev, dev_t mydev,
 			 ino64_t myino, int save, int *dostat)
      internal_function;
@@ -170,12 +184,19 @@ ttyname (int fd)
 #ifdef _STATBUF_ST_RDEV
 	  && S_ISCHR (st1.st_mode)
 	  && st1.st_rdev == st.st_rdev
-#else
+#endif
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
-#endif
 	  )
 	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_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index d15bc74..e4a2ac6 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -32,6 +33,19 @@ static int getttyname_r (char *buf, size_t buflen,
 			 dev_t mydev, ino64_t myino, int save,
 			 int *dostat) internal_function;
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static int
 internal_function attribute_compat_text_section
 getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
@@ -152,12 +166,20 @@ __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
+#endif
 	  && st1.st_ino == st.st_ino
 	  && st1.st_dev == st.st_dev
-#endif
 	  )
 	return 0;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st))
+	{
+	  __set_errno (ENODEV);
+	  return ENODEV;
+	}
     }
 
   /* Prepare the result buffer.  */
-- 
2.7.4

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Andreas Schwab]

On Okt 03 2016, "Serge E. Hallyn" <serge@hallyn.com> wrote:

> @@ -170,12 +184,19 @@ ttyname (int fd)
>  #ifdef _STATBUF_ST_RDEV
>  	  && S_ISCHR (st1.st_mode)
>  	  && st1.st_rdev == st.st_rdev
> -#else
> +#endif
>  	  && st1.st_ino == st.st_ino
>  	  && st1.st_dev == st.st_dev
> -#endif
>  	  )

Please unfold the paren.

> @@ -152,12 +166,20 @@ __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
> +#endif
>  	  && st1.st_ino == st.st_ino
>  	  && st1.st_dev == st.st_dev
> -#endif
>  	  )

Likewise.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Serge E. Hallyn]

If a link (say /proc/self/fd/0) pointint 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.
---
 sysdeps/unix/sysv/linux/ttyname.c   | 28 ++++++++++++++++++++++++----
 sysdeps/unix/sysv/linux/ttyname_r.c | 29 +++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c
index 7a001b4..798f396 100644
--- a/sysdeps/unix/sysv/linux/ttyname.c
+++ b/sysdeps/unix/sysv/linux/ttyname.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -33,6 +34,19 @@
 char *__ttyname;
 #endif
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static char *getttyname (const char *dev, dev_t mydev,
 			 ino64_t myino, int save, int *dostat)
      internal_function;
@@ -170,12 +184,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_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index d15bc74..2573729 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -25,6 +25,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <stdlib.h>
+#include <sys/sysmacros.h>
 
 #include <_itoa.h>
 
@@ -32,6 +33,19 @@ static int getttyname_r (char *buf, size_t buflen,
 			 dev_t mydev, ino64_t myino, int save,
 			 int *dostat) internal_function;
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt.  */
+static 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
+}
+
 static int
 internal_function attribute_compat_text_section
 getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
@@ -152,12 +166,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 it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace.  */
+      if (is_pty (&st))
+	{
+	  __set_errno (ENODEV);
+	  return ENODEV;
+	}
     }
 
   /* Prepare the result buffer.  */
-- 
2.7.4

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Florian Weimer]

On 10/03/2016 08:16 AM, Serge E. Hallyn wrote:

> +/* Return true if this is a UNIX98 pty device, as defined in
> +   linux/Documentation/devices.txt.  */
> +static 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
> +}
> +

Ideally, this function should go into a separate header file which is 
included.  A static inline function would be fine for this.

> +      /* If the link doesn't exist, then it points to a device in another
> +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> +	 fd, as it points to a name unreachable in our namespace.  */

This comment does not appear to be correct (the /proc/self part).

Thanks,
Florian

Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

[Serge E. Hallyn]

On Tue, Oct 04, 2016 at 11:53:50AM +0200, Florian Weimer wrote:
> On 10/03/2016 08:16 AM, Serge E. Hallyn wrote:
> 
> >+/* Return true if this is a UNIX98 pty device, as defined in
> >+   linux/Documentation/devices.txt.  */
> >+static 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
> >+}
> >+
> 
> Ideally, this function should go into a separate header file which
> is included.  A static inline function would be fine for this.

Yeah that would be nicer.

> >+      /* If the link doesn't exist, then it points to a device in another
> >+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> >+	 fd, as it points to a name unreachable in our namespace.  */
> 
> This comment does not appear to be correct (the /proc/self part).

D'oh, yeah that's no longer true.

[PATCH 2/2] linux ttyname_r: return link if appropriate

[Serge Hallyn]

Same as previous commit for ttyname
---
 sysdeps/unix/sysv/linux/ttyname_r.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/ttyname_r.c b/sysdeps/unix/sysv/linux/ttyname_r.c
index d15bc74..e3d0c8b 100644
--- a/sysdeps/unix/sysv/linux/ttyname_r.c
+++ b/sysdeps/unix/sysv/linux/ttyname_r.c
@@ -32,6 +32,19 @@ static int getttyname_r (char *buf, size_t buflen,
 			 dev_t mydev, ino64_t myino, int save,
 			 int *dostat) internal_function;
 
+/* Return true if this is a UNIX98 pty device, as defined in
+   linux/Documentation/devices.txt */
+static 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
+}
+
 static int
 internal_function attribute_compat_text_section
 getttyname_r (char *buf, size_t buflen, dev_t mydev, ino64_t myino,
@@ -158,6 +171,15 @@ __ttyname_r (int fd, char *buf, size_t buflen)
 #endif
 	  )
 	return 0;
+
+      /* If the link doesn't exist, then it points to a device in another
+	 namespace.  If it is a UNIX98 pty, then return the /proc/self
+	 fd, as it points to a name unreachable in our namespace */
+      if (is_pty (&st) && strlen (procname) < buflen - 1)
+        {
+	  strcpy (buf, procname);
+	  return 0;
+        }
     }
 
   /* Prepare the result buffer.  */
-- 
2.7.4

Re: [PATCH 2/2] linux ttyname_r: return link if appropriate

[Mike Frysinger]

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

On 18 Apr 2016 19:53, Serge Hallyn wrote:
> Same as previous commit for ttyname

i think it's preferable to squash it into the previous commit.
it's not like we'd merge one w/out the other.
-mike

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