fstat and similar methods in fhandler_socket_local and fhandler_socket_unix

[Ken Brown]

On 2/22/2021 4:44 AM, Corinna Vinschen via Cygwin-developers wrote:
> On Feb 21 13:04, Ken Brown via Cygwin-developers wrote:
>> On 2/20/2021 6:51 PM, Ken Brown via Cygwin-developers wrote:
>>> On 2/20/2021 6:32 PM, Ken Brown via Cygwin-developers wrote:
>>>> fhandler_socket_local::fstat and many similar methods seem to assume
>>>> that the fhandler is based on a socket *file*.  When this is not the
>>>> case, they end up using handles inappropriately.  Consider the
>>>> following test case, for example.
>>>>
>>>> $ cat socket_stat.c
>>>> [...]
>>>> I haven't been able to find any documentation about what fstat(2)
>>>> should do on a socket descriptor, so maybe the difference between
>>>> Cygwin and Linux is unimportant.  But the code path followed on
>>>> Cygwin definitely seems wrong. fhandler_socket_local::fstat calls
>>>> fstat_fs which ends up using the io_handle as though it were a file
>>>> handle.
> 
> I see what you mean.  This was clearly fat-fingered, probably after
> trying to add abstract sockets.
> 
>> It seems to me that fstat_fs should only be called if we
>>>> have a path_conv handle (which is the case if the fstat call
>>>> resulted from a stat call) or if the socket descriptor came from
>>>> open with O_PATH set.
>>>>
>>>> Similar remarks apply to fstatfvs, fchmod,....  In some of these
>>>> cases, like fchmod, POSIX explicitly states that the behavior is
>>>> undefined, so maybe we should just fail when there's no underlying
>>>> socket file.
>>>
>>> Maybe the answer in all these cases is just to revert to the
>>> fhandler_socket methods when our fhandler_socket_local is not based on a
>>> socket file.
>>
>> I think the following should do the job for fstat:
>>
>> --- a/winsup/cygwin/fhandler_socket_local.cc
>> +++ b/winsup/cygwin/fhandler_socket_local.cc
>> @@ -673,11 +673,11 @@ fhandler_socket_local::fcntl (int cmd, intptr_t arg)
>>   int __reg2
>>   fhandler_socket_local::fstat (struct stat *buf)
>>   {
>> -  int res;
>> +  if (!pc.handle () && !(get_flags () & O_PATH))
>> +    /* fstat is not being called on a socket file. */
> 
> The comment should probably read "fstat called on a socket"...
> 
>> +    return fhandler_socket::fstat (buf);
> 
> ...plus adding a comment right here:
> 
>       /* stat/lstat on a socket file or fstat on a socket opened w/ O_PATH */
> 
>> -  if (get_sun_path () && get_sun_path ()[0] == '\0')
>> -    return fhandler_socket_wsock::fstat (buf);
>> -  res = fhandler_base::fstat_fs (buf);
>> +  int res = fhandler_base::fstat_fs (buf);
>>     if (!res)
>>       {
>>         buf->st_mode = (buf->st_mode & ~S_IFMT) | S_IFSOCK;
>>
>> With this patch, the output of my testcase looks like this:
>>
>> File type:                socket
>> I-node number:            6
>> dev:                      1966080
>> rdev:                     1966200
>> Mode:                     140777 (octal)
>> Link count:               1
>> Ownership:                UID=197609   GID=197121
>> Preferred I/O block size: 65536 bytes
>> File size:                0 bytes
>> Blocks allocated:         0
>> Last status change:       Thu Nov 30 19:00:00 2006
>> Last file access:         Sun Feb 21 13:03:03 2021
>> Last file modification:   Sun Feb 21 13:03:03 2021
> 
> Might be a good idea to check the output twice in your testcase, adding
> a second fstat call after calling bind, to see if it's still the same
> after binding the socket to a file.

Done.

>> If this patch looks right, I'll look at the other system calls too.
> 
> Yeah, I think you're right.  Thanks!

OK, I've got patches ready to go for fstat, fstatvfs, fchmod, fchown, facl, and 
link.  I have to test them, and then I'll send them to cygwin-patches.  In the 
case of link, I think the only way an fhandler_socket's link method can be 
called is through link(2) being called on a socket file.  So the following 
should do the job:

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 21e1df172..f4aa12956 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -611,7 +611,6 @@ class fhandler_socket: public fhandler_base
    int __reg1 fchmod (mode_t newmode);
    int __reg2 fchown (uid_t newuid, gid_t newgid);
    int __reg3 facl (int, int, struct acl *);
-  int __reg2 link (const char *);
    off_t lseek (off_t, int)
    {
      set_errno (ESPIPE);
diff --git a/winsup/cygwin/fhandler_socket.cc b/winsup/cygwin/fhandler_socket.cc
index f22412650..08870cc6b 100644
--- a/winsup/cygwin/fhandler_socket.cc
+++ b/winsup/cygwin/fhandler_socket.cc
@@ -357,9 +357,3 @@ fhandler_socket::facl (int cmd, int nentries, aclent_t *aclbufp)
    set_errno (EOPNOTSUPP);
    return -1;
  }
-
-int
-fhandler_socket::link (const char *newpath)
-{
-  return fhandler_base::link (newpath);
-}
diff --git a/winsup/cygwin/fhandler_socket_local.cc 
b/winsup/cygwin/fhandler_socket_local.cc
index 2c66a7fd4..176d64b9b 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -747,8 +747,6 @@ 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')
-    return fhandler_socket_wsock::link (newpath);
    fhandler_disk_file fh (pc);
    return fh.link (newpath);
  }
diff --git a/winsup/cygwin/fhandler_socket_unix.cc 
b/winsup/cygwin/fhandler_socket_unix.cc
index bd5e1b854..3c713e025 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -2394,10 +2394,6 @@ 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'))
-    return fhandler_socket::link (newpath);
    fhandler_disk_file fh (pc);
    return fh.link (newpath);
  }

Let me know if I've missed something and we really do need fhandler_socket::link.

Ken