[PATCH 0/7] Fix some system calls on sockets

[PATCH 0/7] Fix some system calls on sockets

[Ken Brown]

Several of the fhandler_socket_local and fhandler_socket_unix methods
that support system calls are written as though they are operating on
socket files unless the socket is an abstract socket.  This patchset
(except for the last patch) attempts to fix this by checking whether
the fhandler is associated with a socket file.  If not, we call an
fhandler_socket_wsock or fhandler_socket method instead of an
fhandler_disk_file method.

The last patch is just a code simplification that arose while I was
working on fhandler_socket_local::link.

Ken Brown (7):
  Cygwin: fix fstat on sockets that are not socket files
  Cygwin: fix fstatvfs on sockets that are not socket files
  Cygwin: fix fchmod on sockets that are not socket files
  Cygwin: fix fchown on sockets that are not socket files
  Cygwin: fix facl on sockets that are not socket files
  Cygwin: fix linkat(2) on sockets that are not socket files
  Cygwin: simplify linkat with AT_EMPTY_PATH

 winsup/cygwin/fhandler_socket_local.cc | 39 +++++++++++++-----
 winsup/cygwin/fhandler_socket_unix.cc  | 56 ++++++++++++++++----------
 winsup/cygwin/syscalls.cc              | 24 +++++++----
 3 files changed, 81 insertions(+), 38 deletions(-)

-- 
2.30.0

[PATCH 1/7] Cygwin: fix fstat on sockets that are not socket files

[Ken Brown]

If fstat(2) is called on an AF_LOCAL or AF_UNIX socket that is not a
socket file, the current code calls fstat_fs.  The latter expects to
be operating on a disk file and uses the socket's io_handle, which is
not a file handle.

Fix this by calling fstat_fs only if the fhandler_socket object is a
file (determined by testing dev().isfs()).
---
 winsup/cygwin/fhandler_socket_local.cc |  9 +++++----
 winsup/cygwin/fhandler_socket_unix.cc  | 11 +++++------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_local.cc b/winsup/cygwin/fhandler_socket_local.cc
index 964f3e819..f8adf6c46 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -673,11 +673,12 @@ fhandler_socket_local::fcntl (int cmd, intptr_t arg)
 int __reg2
 fhandler_socket_local::fstat (struct stat *buf)
 {
-  int res;
-
-  if (get_sun_path () && get_sun_path ()[0] == '\0')
+  if (!dev ().isfs ())
+    /* fstat called on a socket. */
     return fhandler_socket_wsock::fstat (buf);
-  res = fhandler_base::fstat_fs (buf);
+
+  /* stat/lstat on a socket file or fstat on a socket opened w/ O_PATH. */
+  int res = fhandler_base::fstat_fs (buf);
   if (!res)
     {
       buf->st_mode = (buf->st_mode & ~S_IFMT) | S_IFSOCK;
diff --git a/winsup/cygwin/fhandler_socket_unix.cc b/winsup/cygwin/fhandler_socket_unix.cc
index eedb0847e..8091fa820 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -2337,13 +2337,12 @@ fhandler_socket_unix::fcntl (int cmd, intptr_t arg)
 int __reg2
 fhandler_socket_unix::fstat (struct stat *buf)
 {
-  int ret = 0;
-
-  if (sun_path ()
-      && (sun_path ()->un_len <= (socklen_t) sizeof (sa_family_t)
-	  || sun_path ()->un.sun_path[0] == '\0'))
+  if (!dev ().isfs ())
+    /* fstat called on a socket. */
     return fhandler_socket::fstat (buf);
-  ret = fhandler_base::fstat_fs (buf);
+
+  /* stat/lstat on a socket file or fstat on a socket opened w/ O_PATH. */
+  int ret = fhandler_base::fstat_fs (buf);
   if (!ret)
     {
       buf->st_mode = (buf->st_mode & ~S_IFMT) | S_IFSOCK;
-- 
2.30.0

[PATCH 2/7] Cygwin: fix fstatvfs on sockets that are not socket files

[Ken Brown]

If fstatvfs(2) is called on an AF_LOCAL or AF_UNIX socket that is not
a socket file, the current code calls fhandler_disk_file::fstatvfs in
most cases.  The latter expects to be operating on a disk file and
uses the socket's io_handle, which is not a file handle.

Fix this by calling fhandler_disk_file::fstatvfs only if the
fhandler_socket object is a socket file (determined by testing
dev().isfs()).
---
 winsup/cygwin/fhandler_socket_local.cc |  5 ++++-
 winsup/cygwin/fhandler_socket_unix.cc  | 14 +++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_local.cc b/winsup/cygwin/fhandler_socket_local.cc
index f8adf6c46..5ca6d8550 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -690,8 +690,11 @@ fhandler_socket_local::fstat (struct stat *buf)
 int __reg2
 fhandler_socket_local::fstatvfs (struct statvfs *sfs)
 {
-  if (get_sun_path () && get_sun_path ()[0] == '\0')
+  if (!dev ().isfs ())
+    /* fstatvfs called on a socket. */
     return fhandler_socket_wsock::fstatvfs (sfs);
+
+  /* statvfs on a socket file or fstatvfs on a socket opened w/ O_PATH. */
   if (get_flags () & O_PATH)
     /* We already have a handle. */
     {
diff --git a/winsup/cygwin/fhandler_socket_unix.cc b/winsup/cygwin/fhandler_socket_unix.cc
index 8091fa820..06db929ed 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -2354,10 +2354,18 @@ fhandler_socket_unix::fstat (struct stat *buf)
 int __reg2
 fhandler_socket_unix::fstatvfs (struct statvfs *sfs)
 {
-  if (sun_path ()
-      && (sun_path ()->un_len <= (socklen_t) sizeof (sa_family_t)
-	  || sun_path ()->un.sun_path[0] == '\0'))
+  if (!dev ().isfs ())
+    /* fstatvfs called on a socket. */
     return fhandler_socket::fstatvfs (sfs);
+
+  /* statvfs on a socket file or fstatvfs on a socket opened w/ O_PATH. */
+  if (get_flags () & O_PATH)
+    /* We already have a handle. */
+    {
+      HANDLE h = get_handle ();
+      if (h)
+	return fstatvfs_by_handle (h, sfs);
+    }
   fhandler_disk_file fh (pc);
   fh.get_device () = FH_FS;
   return fh.fstatvfs (sfs);
-- 
2.30.0

[PATCH 3/7] Cygwin: fix fchmod on sockets that are not socket files

[Ken Brown]

If fchmod(2) is called on an AF_LOCAL or AF_UNIX socket that is not a
socket file, the current code calls fhandler_disk_file::fchmod in most
cases.  The latter expects to be operating on a disk file and uses the
socket's io_handle, which is not a file handle.

Fix this by calling fhandler_disk_file::fchmod only if the
fhandler_socket object is a file (determined by testing dev().isfs()).
---
 winsup/cygwin/fhandler_socket_local.cc | 6 +++++-
 winsup/cygwin/fhandler_socket_unix.cc  | 8 +++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_local.cc b/winsup/cygwin/fhandler_socket_local.cc
index 5ca6d8550..d1faa079a 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -710,8 +710,12 @@ fhandler_socket_local::fstatvfs (struct statvfs *sfs)
 int
 fhandler_socket_local::fchmod (mode_t newmode)
 {
-  if (get_sun_path () && get_sun_path ()[0] == '\0')
+  if (!dev ().isfs ())
+    /* fchmod called on a socket. */
     return fhandler_socket_wsock::fchmod (newmode);
+
+  /* chmod on a socket file.  [We won't get here if fchmod is called
+     on a socket opened w/ O_PATH.] */
   fhandler_disk_file fh (pc);
   fh.get_device () = FH_FS;
   return fh.fchmod (S_IFSOCK | adjust_socket_file_mode (newmode));
diff --git a/winsup/cygwin/fhandler_socket_unix.cc b/winsup/cygwin/fhandler_socket_unix.cc
index 06db929ed..e08e9bdd9 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -2374,10 +2374,12 @@ fhandler_socket_unix::fstatvfs (struct statvfs *sfs)
 int
 fhandler_socket_unix::fchmod (mode_t newmode)
 {
-  if (sun_path ()
-      && (sun_path ()->un_len <= (socklen_t) sizeof (sa_family_t)
-	  || sun_path ()->un.sun_path[0] == '\0'))
+  if (!dev ().isfs ())
+    /* fchmod called on a socket. */
     return fhandler_socket::fchmod (newmode);
+
+  /* chmod on a socket file.  [We won't get here if fchmod is called
+     on a socket opened w/ O_PATH.] */
   fhandler_disk_file fh (pc);
   fh.get_device () = FH_FS;
   /* Kludge: Don't allow to remove read bit on socket files for
-- 
2.30.0

[PATCH 4/7] Cygwin: fix fchown on sockets that are not socket files

[Ken Brown]

If fchown(2) is called on an AF_LOCAL or AF_UNIX socket that is not a
socket file, the current code calls fhandler_disk_file::fchown in most
cases.  The latter expects to be operating on a disk file and uses the
socket's io_handle, which is not a file handle.

Fix this by calling fhandler_disk_file::fchown only if the
fhandler_socket object is a file (determined by testing dev().isfs()).
---
 winsup/cygwin/fhandler_socket_local.cc | 6 +++++-
 winsup/cygwin/fhandler_socket_unix.cc  | 8 +++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_local.cc b/winsup/cygwin/fhandler_socket_local.cc
index d1faa079a..349ade897 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -724,8 +724,12 @@ fhandler_socket_local::fchmod (mode_t newmode)
 int
 fhandler_socket_local::fchown (uid_t uid, gid_t gid)
 {
-  if (get_sun_path () && get_sun_path ()[0] == '\0')
+  if (!dev ().isfs ())
+    /* fchown called on a socket. */
     return fhandler_socket_wsock::fchown (uid, gid);
+
+  /* chown/lchown on a socket file.  [We won't get here if fchown is
+     called on a socket opened w/ O_PATH.] */
   fhandler_disk_file fh (pc);
   return fh.fchown (uid, gid);
 }
diff --git a/winsup/cygwin/fhandler_socket_unix.cc b/winsup/cygwin/fhandler_socket_unix.cc
index e08e9bdd9..573864b9f 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -2395,10 +2395,12 @@ fhandler_socket_unix::fchmod (mode_t newmode)
 int
 fhandler_socket_unix::fchown (uid_t uid, gid_t gid)
 {
-  if (sun_path ()
-      && (sun_path ()->un_len <= (socklen_t) sizeof (sa_family_t)
-	  || sun_path ()->un.sun_path[0] == '\0'))
+  if (!dev ().isfs ())
+    /* fchown called on a socket. */
     return fhandler_socket::fchown (uid, gid);
+
+  /* chown/lchown on a socket file.  [We won't get here if fchown is
+     called on a socket opened w/ O_PATH.] */
   fhandler_disk_file fh (pc);
   return fh.fchown (uid, gid);
 }
-- 
2.30.0

[PATCH 5/7] Cygwin: fix facl on sockets that are not socket files

[Ken Brown]

If facl(2) is called on an AF_LOCAL or AF_UNIX socket that is not a
socket file, the current code calls fhandler_disk_file::facl in most
cases.  The latter expects to be operating on a disk file and uses the
socket's io_handle, which is not a file handle.

Fix this by calling fhandler_disk_file::facl only if the
fhandler_socket object is a file (determined by testing dev().isfs()).
---
 winsup/cygwin/fhandler_socket_local.cc | 6 +++++-
 winsup/cygwin/fhandler_socket_unix.cc  | 8 +++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_local.cc b/winsup/cygwin/fhandler_socket_local.cc
index 349ade897..22586c0dd 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -737,8 +737,12 @@ fhandler_socket_local::fchown (uid_t uid, gid_t gid)
 int
 fhandler_socket_local::facl (int cmd, int nentries, aclent_t *aclbufp)
 {
-  if (get_sun_path () && get_sun_path ()[0] == '\0')
+  if (!dev ().isfs ())
+    /* facl called on a socket. */
     return fhandler_socket_wsock::facl (cmd, nentries, aclbufp);
+
+  /* facl on a socket file.  [We won't get here if facl is called on a
+     socket opened w/ O_PATH.] */
   fhandler_disk_file fh (pc);
   return fh.facl (cmd, nentries, aclbufp);
 }
diff --git a/winsup/cygwin/fhandler_socket_unix.cc b/winsup/cygwin/fhandler_socket_unix.cc
index 573864b9f..fae07367d 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -2408,10 +2408,12 @@ fhandler_socket_unix::fchown (uid_t uid, gid_t gid)
 int
 fhandler_socket_unix::facl (int cmd, int nentries, aclent_t *aclbufp)
 {
-  if (sun_path ()
-      && (sun_path ()->un_len <= (socklen_t) sizeof (sa_family_t)
-	  || sun_path ()->un.sun_path[0] == '\0'))
+  if (!dev ().isfs ())
+    /* facl called on a socket. */
     return fhandler_socket::facl (cmd, nentries, aclbufp);
+
+  /* facl on a socket file.  [We won't get here if facl is called on a
+     socket opened w/ O_PATH.] */
   fhandler_disk_file fh (pc);
   return fh.facl (cmd, nentries, aclbufp);
 }
-- 
2.30.0

[PATCH 6/7] Cygwin: fix linkat(2) on sockets that are not socket files

[Ken Brown]

If linkat(2) is called with AT_EMPTY_PATH on an AF_LOCAL or
AF_UNIX socket that is not a socket file, the current code calls
fhandler_disk_file::link in most cases.  The latter expects to be
operating on a disk file and uses the socket's io_handle, which
is not a file handle.

Fix this by calling fhandler_disk_file::link only if the
fhandler_socket object is a file (determined by testing
dev().isfs()).

Also fix the case of a socket file opened with O_PATH by setting
the fhandler_disk_file's io_handle.
---
 winsup/cygwin/fhandler_socket_local.cc | 7 ++++++-
 winsup/cygwin/fhandler_socket_unix.cc  | 9 ++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_local.cc b/winsup/cygwin/fhandler_socket_local.cc
index 22586c0dd..ad7dd0a98 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -750,9 +750,14 @@ fhandler_socket_local::facl (int cmd, int nentries, aclent_t *aclbufp)
 int
 fhandler_socket_local::link (const char *newpath)
 {
-  if (get_sun_path () && get_sun_path ()[0] == '\0')
+  if (!dev ().isfs ())
+    /* linkat w/ AT_EMPTY_PATH called on a socket not opened w/ O_PATH. */
     return fhandler_socket_wsock::link (newpath);
+  /* link on a socket file or linkat w/ AT_EMPTY_PATH called on a
+     socket opened w/ O_PATH. */
   fhandler_disk_file fh (pc);
+  if (get_flags () & O_PATH)
+    fh.set_handle (get_handle ());
   return fh.link (newpath);
 }
 
diff --git a/winsup/cygwin/fhandler_socket_unix.cc b/winsup/cygwin/fhandler_socket_unix.cc
index fae07367d..252bcd9a9 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -2421,11 +2421,14 @@ fhandler_socket_unix::facl (int cmd, int nentries, aclent_t *aclbufp)
 int
 fhandler_socket_unix::link (const char *newpath)
 {
-  if (sun_path ()
-      && (sun_path ()->un_len <= (socklen_t) sizeof (sa_family_t)
-	  || sun_path ()->un.sun_path[0] == '\0'))
+  if (!dev ().isfs ())
+    /* linkat w/ AT_EMPTY_PATH called on a socket not opened w/ O_PATH. */
     return fhandler_socket::link (newpath);
+  /* link on a socket file or linkat w/ AT_EMPTY_PATH called on a
+     socket opened w/ O_PATH. */
   fhandler_disk_file fh (pc);
+  if (get_flags () & O_PATH)
+    fh.set_handle (get_handle ());
   return fh.link (newpath);
 }
 
-- 
2.30.0

[PATCH 7/7] Cygwin: simplify linkat with AT_EMPTY_PATH

[Ken Brown]

linkat(olddirfd, oldpath, oldname, newdirfd, newname, AT_EMPTY_PATH)
is supposed to create a link to the file referenced by olddirfd if
oldname is the empty string.  Currently this is done via the /proc
filesystem by converting the call to

  linkat(AT_FDCWD, "/proc/self/fd/<olddirfd>", newdirfd, newname,
         AT_SYMLINK_FOLLOW),

which ultimately leads to a call to the appropriate fhandler's link
method.  Simplify this by using cygheap_fdget to obtain the fhandler
directly.
---
 winsup/cygwin/syscalls.cc | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 460fe6801..6ba4f10f7 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -4962,6 +4962,8 @@ linkat (int olddirfd, const char *oldpathname,
 	int flags)
 {
   tmp_pathbuf tp;
+  fhandler_base *fh = NULL;
+
   __try
     {
       if (flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH))
@@ -4970,21 +4972,25 @@ linkat (int olddirfd, const char *oldpathname,
 	  __leave;
 	}
       char *oldpath = tp.c_get ();
-      /* AT_EMPTY_PATH with an empty oldpathname is equivalent to
-
-	   linkat(AT_FDCWD, "/proc/self/fd/<olddirfd>", newdirfd,
-		  newname, AT_SYMLINK_FOLLOW);
-
-	 Convert the request accordingly. */
       if ((flags & AT_EMPTY_PATH) && oldpathname && oldpathname[0] == '\0')
 	{
+	  /* Operate directly on olddirfd, which can be anything
+	     except a directory. */
 	  if (olddirfd == AT_FDCWD)
 	    {
 	      set_errno (EPERM);
 	      __leave;
 	    }
-	  __small_sprintf (oldpath, "/proc/%d/fd/%d", myself->pid, olddirfd);
-	  flags = AT_SYMLINK_FOLLOW;
+	  cygheap_fdget cfd (olddirfd);
+	  if (cfd < 0)
+	    __leave;
+	  if (cfd->pc.isdir ())
+	    {
+	      set_errno (EPERM);
+	      __leave;
+	    }
+	  fh = cfd;
+	  flags = 0;		/* In case AT_SYMLINK_FOLLOW was set. */
 	}
       else if (gen_full_path_at (oldpath, olddirfd, oldpathname))
 	__leave;
@@ -5003,6 +5009,8 @@ linkat (int olddirfd, const char *oldpathname,
 	    }
 	  strcpy (oldpath, old_name.get_posix ());
 	}
+      if (fh)
+	return fh->link (newpath);
       return link (oldpath, newpath);
     }
   __except (EFAULT) {}
-- 
2.30.0

Re: [PATCH 0/7] Fix some system calls on sockets

[Corinna Vinschen]

On Feb 25 17:48, Ken Brown via Cygwin-patches wrote:
> Several of the fhandler_socket_local and fhandler_socket_unix methods
> that support system calls are written as though they are operating on
> socket files unless the socket is an abstract socket.  This patchset
> (except for the last patch) attempts to fix this by checking whether
> the fhandler is associated with a socket file.  If not, we call an
> fhandler_socket_wsock or fhandler_socket method instead of an
> fhandler_disk_file method.
> 
> The last patch is just a code simplification that arose while I was
> working on fhandler_socket_local::link.
> 
> Ken Brown (7):
>   Cygwin: fix fstat on sockets that are not socket files
>   Cygwin: fix fstatvfs on sockets that are not socket files
>   Cygwin: fix fchmod on sockets that are not socket files
>   Cygwin: fix fchown on sockets that are not socket files
>   Cygwin: fix facl on sockets that are not socket files
>   Cygwin: fix linkat(2) on sockets that are not socket files
>   Cygwin: simplify linkat with AT_EMPTY_PATH
> 
>  winsup/cygwin/fhandler_socket_local.cc | 39 +++++++++++++-----
>  winsup/cygwin/fhandler_socket_unix.cc  | 56 ++++++++++++++++----------
>  winsup/cygwin/syscalls.cc              | 24 +++++++----
>  3 files changed, 81 insertions(+), 38 deletions(-)
> 
> -- 
> 2.30.0

LGTM, please push.


Thanks,
Corinna