Re: ptsname_r

Re: ptsname_r

[Eric Blake]

On 11/07/2011 01:46 PM, Eric Blake wrote:
>> Thanks. Also, even with your patches of today, ptsname() is still not
>> thread-safe; should we be sticking that in a thread-local buffer rather
>> than in static storage, similar to how other functions like strerror()
>> are thread-safe?

I didn't tackle that,

>
> Also, should we have an efault handler in syscalls.cc ptsname_r(),
> similar to ttyname_r(), so as to gracefully reject invalid buffers
> rather than faulting?

but I had this additional code in my sandbox right before your commit 
hit CVS; should I add a ChangeLog and make it a formal patch submission?

diff --git a/winsup/cygwin/include/cygwin/stdlib.h 
b/winsup/cygwin/include/cygwin/stdlib.h
index d2dfe4c..20358ef 100644
--- a/winsup/cygwin/include/cygwin/stdlib.h
+++ b/winsup/cygwin/include/cygwin/stdlib.h
@@ -1,6 +1,6 @@
  /* stdlib.h

-   Copyright 2005, 2006, 2007, 2008, 2009 Red Hat Inc.
+   Copyright 2005, 2006, 2007, 2008, 2009, 2011 Red Hat Inc.

  This file is part of Cygwin.

diff --git a/winsup/cygwin/libc/bsdlib.cc b/winsup/cygwin/libc/bsdlib.cc
index 3b6e7e4..c4398d3 100644
--- a/winsup/cygwin/libc/bsdlib.cc
+++ b/winsup/cygwin/libc/bsdlib.cc
@@ -108,7 +108,7 @@ openpty (int *amaster, int *aslave, char *name, 
const struct termios *termp,
      {
        grantpt (master);
        unlockpt (master);
-      strcpy (pts, ptsname (master));
+      ptsname_r (master, pts, sizeof pts);
        revoke (pts);
        if ((slave = open (pts, O_RDWR | O_NOCTTY)) >= 0)
  	{
diff --git a/winsup/cygwin/posix.sgml b/winsup/cygwin/posix.sgml
index ef27fde..5c510ed 100644
--- a/winsup/cygwin/posix.sgml
+++ b/winsup/cygwin/posix.sgml
@@ -1131,6 +1131,7 @@ also IEEE Std 1003.1-2008 (POSIX.1-2008).</para>
      pow10f
      ppoll
      pthread_getattr_np
+    ptsname_r
      removexattr
      setxattr
      strchrnul
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 514d458..68dec59 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -2913,16 +2913,24 @@ ptsname (int fd)
  extern "C" int
  ptsname_r (int fd, char *buf, size_t buflen)
  {
-  if (!buf)
+  int ret = 0;
+  myfault efault;
+  if (efault.faulted ())
+    ret = EFAULT;
+  else
      {
-      set_errno (EINVAL);
-      return EINVAL;
+      cygheap_fdget cfd (fd, true);
+      if (cfd < 0)
+	ret = EBADF;
+      else if (!buf)
+        ret = EINVAL;
+      else
+	ret = cfd->ptsname_r (buf, buflen);
      }
-
-  cygheap_fdget cfd (fd);
-  if (cfd < 0)
-    return 0;
-  return cfd->ptsname_r (buf, buflen);
+  if (ret)
+    set_errno (ret);
+  debug_printf ("returning %d pts: %s", ret, ret ? "NULL" : buf);
+  return ret;
  }

  static int __stdcall

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Re: ptsname_r

[Christopher Faylor]

On Mon, Nov 07, 2011 at 01:59:40PM -0700, Eric Blake wrote:
>On 11/07/2011 01:46 PM, Eric Blake wrote:
>>> Thanks. Also, even with your patches of today, ptsname() is still not
>>> thread-safe; should we be sticking that in a thread-local buffer rather
>>> than in static storage, similar to how other functions like strerror()
>>> are thread-safe?
>
>I didn't tackle that,
>
>>
>> Also, should we have an efault handler in syscalls.cc ptsname_r(),
>> similar to ttyname_r(), so as to gracefully reject invalid buffers
>> rather than faulting?
>
>but I had this additional code in my sandbox right before your commit 
>hit CVS; should I add a ChangeLog and make it a formal patch submission?

No thanks.  Except for the Copyright change in stdlib.h, there is nothing that
I think should go in.  On inspection, openpty() needs some work.

cgf

Re: ptsname_r

[Christopher Faylor]

On Tue, Nov 08, 2011 at 01:04:18AM -0500, Christopher Faylor wrote:
>On Mon, Nov 07, 2011 at 01:59:40PM -0700, Eric Blake wrote:
>>On 11/07/2011 01:46 PM, Eric Blake wrote:
>>>> Thanks. Also, even with your patches of today, ptsname() is still not
>>>> thread-safe; should we be sticking that in a thread-local buffer rather
>>>> than in static storage, similar to how other functions like strerror()
>>>> are thread-safe?
>>
>>I didn't tackle that,
>>
>>>
>>> Also, should we have an efault handler in syscalls.cc ptsname_r(),
>>> similar to ttyname_r(), so as to gracefully reject invalid buffers
>>> rather than faulting?
>>
>>but I had this additional code in my sandbox right before your commit 
>>hit CVS; should I add a ChangeLog and make it a formal patch submission?
>
>No thanks.  Except for the Copyright change in stdlib.h, there is nothing that
>I think should go in.  On inspection, openpty() needs some work.

Actually, Corinna has added your patch to posix.sgml.  I forgot that gnu
functions were mentioned there too.

cgf