[PATCH] Cygwin: allow opening an AF_LOCAL/AF_UNIX socket with O_PATH

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Cygwin: allow opening an AF_LOCAL/AF_UNIX socket with O_PATH

Ken Brown-6
If that flag is not set, or if an attempt is made to open a different
type of socket, the errno is now EOPNOTSUPP instead of ENXIO.  This is
consistent with POSIX, starting with the 2016 edition.  Earlier
editions were silent on this issue.
---
 winsup/cygwin/fhandler.h               |  2 ++
 winsup/cygwin/fhandler_socket.cc       |  2 +-
 winsup/cygwin/fhandler_socket_local.cc | 16 ++++++++++++++++
 winsup/cygwin/fhandler_socket_unix.cc  | 16 ++++++++++++++++
 winsup/cygwin/release/3.1.3            |  7 +++++++
 winsup/doc/new-features.xml            |  6 ++++++
 6 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index c0d56b4da..71b549d38 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -822,6 +822,7 @@ class fhandler_socket_local: public fhandler_socket_wsock
   int getsockopt (int level, int optname, const void *optval,
   __socklen_t *optlen);
 
+  int open (int flags, mode_t mode = 0);
   int __reg2 fstat (struct stat *buf);
   int __reg2 fstatvfs (struct statvfs *buf);
   int __reg1 fchmod (mode_t newmode);
@@ -1103,6 +1104,7 @@ class fhandler_socket_unix : public fhandler_socket
   virtual int ioctl (unsigned int cmd, void *);
   virtual int fcntl (int cmd, intptr_t);
 
+  int open (int flags, mode_t mode = 0);
   int __reg2 fstat (struct stat *buf);
   int __reg2 fstatvfs (struct statvfs *buf);
   int __reg1 fchmod (mode_t newmode);
diff --git a/winsup/cygwin/fhandler_socket.cc b/winsup/cygwin/fhandler_socket.cc
index 9f33d8087..227004b43 100644
--- a/winsup/cygwin/fhandler_socket.cc
+++ b/winsup/cygwin/fhandler_socket.cc
@@ -269,7 +269,7 @@ fhandler_socket::fcntl (int cmd, intptr_t arg)
 int
 fhandler_socket::open (int flags, mode_t mode)
 {
-  set_errno (ENXIO);
+  set_errno (EOPNOTSUPP);
   return 0;
 }
 
diff --git a/winsup/cygwin/fhandler_socket_local.cc b/winsup/cygwin/fhandler_socket_local.cc
index f88ced22d..dbca74702 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -634,6 +634,22 @@ fhandler_socket_local::dup (fhandler_base *child, int flags)
   return fhandler_socket_wsock::dup (child, flags);
 }
 
+int
+fhandler_socket_local::open (int flags, mode_t mode)
+{
+  /* We don't support opening sockets unless O_PATH is specified. */
+  if (!(flags & O_PATH))
+    {
+      set_errno (EOPNOTSUPP);
+      return 0;
+    }
+
+  query_open (query_read_attributes);
+  nohandle (true);
+  set_flags (flags);
+  return 1;
+}
+
 int __reg2
 fhandler_socket_local::fstat (struct stat *buf)
 {
diff --git a/winsup/cygwin/fhandler_socket_unix.cc b/winsup/cygwin/fhandler_socket_unix.cc
index eea7e76b3..b3d4da49a 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -2296,6 +2296,22 @@ fhandler_socket_unix::fcntl (int cmd, intptr_t arg)
   return ret;
 }
 
+int
+fhandler_socket_unix::open (int flags, mode_t mode)
+{
+  /* We don't support opening sockets unless O_PATH is specified. */
+  if (!(flags & O_PATH))
+    {
+      set_errno (EOPNOTSUPP);
+      return 0;
+    }
+
+  query_open (query_read_attributes);
+  nohandle (true);
+  set_flags (flags);
+  return 1;
+}
+
 int __reg2
 fhandler_socket_unix::fstat (struct stat *buf)
 {
diff --git a/winsup/cygwin/release/3.1.3 b/winsup/cygwin/release/3.1.3
index 489741136..af9e0f38e 100644
--- a/winsup/cygwin/release/3.1.3
+++ b/winsup/cygwin/release/3.1.3
@@ -1,3 +1,10 @@
+What's new:
+-----------
+
+- AF_LOCAL/AF_UNIX sockets can now be opened with the O_PATH flag.  If
+  that flag is not set, or if an attempt is made to open a different
+  type of socket, the errno is now EOPNOTSUPP instead of ENXIO.
+
 Bug Fixes
 ---------
 
diff --git a/winsup/doc/new-features.xml b/winsup/doc/new-features.xml
index 65bdc17ab..bf481bd04 100644
--- a/winsup/doc/new-features.xml
+++ b/winsup/doc/new-features.xml
@@ -54,6 +54,12 @@ Allow times(2) to have a NULL argument, as on Linux.
 Improve /proc/cpuinfo output and align more closely with Linux.
 </para></listitem>
 
+<listitem><para>
+AF_LOCAL/AF_UNIX sockets can now be opened with the O_PATH flag.  If
+that flag is not set, or if an attempt is made to open a different
+type of socket, the errno is now EOPNOTSUPP instead of ENXIO.
+</para></listitem>
+
 </itemizedlist>
 
 </sect2>
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: allow opening an AF_LOCAL/AF_UNIX socket with O_PATH

Corinna Vinschen-2
On Jan 16 18:34, Ken Brown wrote:

> If that flag is not set, or if an attempt is made to open a different
> type of socket, the errno is now EOPNOTSUPP instead of ENXIO.  This is
> consistent with POSIX, starting with the 2016 edition.  Earlier
> editions were silent on this issue.
> ---
>  winsup/cygwin/fhandler.h               |  2 ++
>  winsup/cygwin/fhandler_socket.cc       |  2 +-
>  winsup/cygwin/fhandler_socket_local.cc | 16 ++++++++++++++++
>  winsup/cygwin/fhandler_socket_unix.cc  | 16 ++++++++++++++++
>  winsup/cygwin/release/3.1.3            |  7 +++++++
>  winsup/doc/new-features.xml            |  6 ++++++
>  6 files changed, 48 insertions(+), 1 deletion(-)
I'm a bit concerned here that some function calls might succeed
accidentally or even crash, given that the original socket code doesn't
cope with the nohandle flag.  Did you perform some basic testing?


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: allow opening an AF_LOCAL/AF_UNIX socket with O_PATH

Corinna Vinschen-2
On Jan 17 10:48, Corinna Vinschen wrote:

> On Jan 16 18:34, Ken Brown wrote:
> > If that flag is not set, or if an attempt is made to open a different
> > type of socket, the errno is now EOPNOTSUPP instead of ENXIO.  This is
> > consistent with POSIX, starting with the 2016 edition.  Earlier
> > editions were silent on this issue.
> > ---
> >  winsup/cygwin/fhandler.h               |  2 ++
> >  winsup/cygwin/fhandler_socket.cc       |  2 +-
> >  winsup/cygwin/fhandler_socket_local.cc | 16 ++++++++++++++++
> >  winsup/cygwin/fhandler_socket_unix.cc  | 16 ++++++++++++++++
> >  winsup/cygwin/release/3.1.3            |  7 +++++++
> >  winsup/doc/new-features.xml            |  6 ++++++
> >  6 files changed, 48 insertions(+), 1 deletion(-)
>
> I'm a bit concerned here that some function calls might succeed
> accidentally or even crash, given that the original socket code doesn't
> cope with the nohandle flag.  Did you perform some basic testing?
Iow, do the usual socket calls on a fhandler_socket_local return EBADF
now?  Ignoring fhandler_socket_unix for now.


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: allow opening an AF_LOCAL/AF_UNIX socket with O_PATH

Ken Brown-6
On 1/17/2020 4:51 AM, Corinna Vinschen wrote:

> On Jan 17 10:48, Corinna Vinschen wrote:
>> On Jan 16 18:34, Ken Brown wrote:
>>> If that flag is not set, or if an attempt is made to open a different
>>> type of socket, the errno is now EOPNOTSUPP instead of ENXIO.  This is
>>> consistent with POSIX, starting with the 2016 edition.  Earlier
>>> editions were silent on this issue.
>>> ---
>>>   winsup/cygwin/fhandler.h               |  2 ++
>>>   winsup/cygwin/fhandler_socket.cc       |  2 +-
>>>   winsup/cygwin/fhandler_socket_local.cc | 16 ++++++++++++++++
>>>   winsup/cygwin/fhandler_socket_unix.cc  | 16 ++++++++++++++++
>>>   winsup/cygwin/release/3.1.3            |  7 +++++++
>>>   winsup/doc/new-features.xml            |  6 ++++++
>>>   6 files changed, 48 insertions(+), 1 deletion(-)
>>
>> I'm a bit concerned here that some function calls might succeed
>> accidentally or even crash, given that the original socket code doesn't
>> cope with the nohandle flag.  Did you perform some basic testing?
>
> Iow, do the usual socket calls on a fhandler_socket_local return EBADF
> now?  Ignoring fhandler_socket_unix for now.

I really hadn't thought this through very well.  I think the following
additional patch should do the job:

--- a/winsup/cygwin/net.cc
+++ b/winsup/cygwin/net.cc
@@ -67,6 +67,11 @@ get (const int fd)

    if (!fh)
      set_errno (ENOTSOCK);
+  else if (fh->get_flags () & O_PATH)
+    {
+      set_errno (EBADF);
+      fh = NULL;
+    }

    return fh;
  }

I'll do some testing and then report back with a revised patch.  Thanks for
catching my mistakes.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: allow opening an AF_LOCAL/AF_UNIX socket with O_PATH

Corinna Vinschen-2
On Jan 19 20:25, Ken Brown wrote:

> On 1/17/2020 4:51 AM, Corinna Vinschen wrote:
> > On Jan 17 10:48, Corinna Vinschen wrote:
> >> On Jan 16 18:34, Ken Brown wrote:
> >>> If that flag is not set, or if an attempt is made to open a different
> >>> type of socket, the errno is now EOPNOTSUPP instead of ENXIO.  This is
> >>> consistent with POSIX, starting with the 2016 edition.  Earlier
> >>> editions were silent on this issue.
> >>> ---
> >>>   winsup/cygwin/fhandler.h               |  2 ++
> >>>   winsup/cygwin/fhandler_socket.cc       |  2 +-
> >>>   winsup/cygwin/fhandler_socket_local.cc | 16 ++++++++++++++++
> >>>   winsup/cygwin/fhandler_socket_unix.cc  | 16 ++++++++++++++++
> >>>   winsup/cygwin/release/3.1.3            |  7 +++++++
> >>>   winsup/doc/new-features.xml            |  6 ++++++
> >>>   6 files changed, 48 insertions(+), 1 deletion(-)
> >>
> >> I'm a bit concerned here that some function calls might succeed
> >> accidentally or even crash, given that the original socket code doesn't
> >> cope with the nohandle flag.  Did you perform some basic testing?
> >
> > Iow, do the usual socket calls on a fhandler_socket_local return EBADF
> > now?  Ignoring fhandler_socket_unix for now.
>
> I really hadn't thought this through very well.  I think the following
> additional patch should do the job:
>
> --- a/winsup/cygwin/net.cc
> +++ b/winsup/cygwin/net.cc
> @@ -67,6 +67,11 @@ get (const int fd)
>
>     if (!fh)
>       set_errno (ENOTSOCK);
> +  else if (fh->get_flags () & O_PATH)
> +    {
> +      set_errno (EBADF);
> +      fh = NULL;
> +    }
>
>     return fh;
>   }
Looks like the easiest solution indeed.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: allow opening an AF_LOCAL/AF_UNIX socket with O_PATH

Ken Brown-6
On 1/20/2020 4:35 AM, Corinna Vinschen wrote:

> On Jan 19 20:25, Ken Brown wrote:
>> On 1/17/2020 4:51 AM, Corinna Vinschen wrote:
>>> On Jan 17 10:48, Corinna Vinschen wrote:
>>>> On Jan 16 18:34, Ken Brown wrote:
>>>>> If that flag is not set, or if an attempt is made to open a different
>>>>> type of socket, the errno is now EOPNOTSUPP instead of ENXIO.  This is
>>>>> consistent with POSIX, starting with the 2016 edition.  Earlier
>>>>> editions were silent on this issue.
>>>>> ---
>>>>>    winsup/cygwin/fhandler.h               |  2 ++
>>>>>    winsup/cygwin/fhandler_socket.cc       |  2 +-
>>>>>    winsup/cygwin/fhandler_socket_local.cc | 16 ++++++++++++++++
>>>>>    winsup/cygwin/fhandler_socket_unix.cc  | 16 ++++++++++++++++
>>>>>    winsup/cygwin/release/3.1.3            |  7 +++++++
>>>>>    winsup/doc/new-features.xml            |  6 ++++++
>>>>>    6 files changed, 48 insertions(+), 1 deletion(-)
>>>>
>>>> I'm a bit concerned here that some function calls might succeed
>>>> accidentally or even crash, given that the original socket code doesn't
>>>> cope with the nohandle flag.  Did you perform some basic testing?
>>>
>>> Iow, do the usual socket calls on a fhandler_socket_local return EBADF
>>> now?  Ignoring fhandler_socket_unix for now.
>>
>> I really hadn't thought this through very well.  I think the following
>> additional patch should do the job:
>>
>> --- a/winsup/cygwin/net.cc
>> +++ b/winsup/cygwin/net.cc
>> @@ -67,6 +67,11 @@ get (const int fd)
>>
>>      if (!fh)
>>        set_errno (ENOTSOCK);
>> +  else if (fh->get_flags () & O_PATH)
>> +    {
>> +      set_errno (EBADF);
>> +      fh = NULL;
>> +    }
>>
>>      return fh;
>>    }
>
> Looks like the easiest solution indeed.

It turns out that some further tweaks are needed, so it may be a while before a
finish this.  And in the course of working on it, I discovered that I was
careless when I attempted to support O_PATH for FIFOs in commit aa55d22c.  So
I'll be sending a fix for that shortly, along with a test program, and then I'll
return to the socket case.

I *think* what I did for symlinks is OK as it stands, but I'll recheck that too
with a suitable test program.

Ken