Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix

[Ken Brown <kbrown@cornell.edu>]

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
>> #include <time.h>
>> #include <sys/socket.h>
>> #include <sys/un.h>
>> #include <sys/stat.h>
>> #include <errno.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <unistd.h>
>>
>> void print_stat (struct stat);
>>
>> int
>> main ()
>> {
>>    struct stat st;
>>    int fd;
>>
>>    fd = socket (AF_UNIX, SOCK_STREAM, 0);
>>    if (fd == -1)
>>      {
>>        perror ("socket");
>>        exit (1);
>>      }
>>    if (fstat (fd, &st) < 0)
>>      {
>>        perror ("fstat");
>>        exit (1);
>>      }
>>    print_stat (st);
>> }
>>
>> /* https://man7.org/linux/man-pages/man2/lstat.2.html */
>> void
>> print_stat (struct stat st)
>> {
>>    printf("File type:                ");
>>
>>    switch (st.st_mode & S_IFMT) {
>>    case S_IFBLK:  printf("block device\n");            break;
>>    case S_IFCHR:  printf("character device\n");        break;
>>    case S_IFDIR:  printf("directory\n");               break;
>>    case S_IFIFO:  printf("FIFO/pipe\n");               break;
>>    case S_IFLNK:  printf("symlink\n");                 break;
>>    case S_IFREG:  printf("regular file\n");            break;
>>    case S_IFSOCK: printf("socket\n");                  break;
>>    default:       printf("unknown?\n");                break;
>>    }
>>
>>    printf("I-node number:            %ld\n", (long) st.st_ino);
>>    printf("dev:                      %lu\n", st.st_dev);
>>    printf("rdev:                     %lu\n", st.st_rdev);
>>    printf("Mode:                     %lo (octal)\n",
>>           (unsigned long) st.st_mode);
>>
>>    printf("Link count:               %ld\n", (long) st.st_nlink);
>>    printf("Ownership:                UID=%ld   GID=%ld\n",
>>           (long) st.st_uid, (long) st.st_gid);
>>
>>    printf("Preferred I/O block size: %ld bytes\n",
>>           (long) st.st_blksize);
>>    printf("File size:                %lld bytes\n",
>>           (long long) st.st_size);
>>    printf("Blocks allocated:         %lld\n",
>>           (long long) st.st_blocks);
>>
>>    printf("Last status change:       %s", ctime(&st.st_ctime));
>>    printf("Last file access:         %s", ctime(&st.st_atime));
>>    printf("Last file modification:   %s", ctime(&st.st_mtime));
>>    printf("\n");
>> }
>>
>> $ gcc -g -O0 -o socket_stat socket_stat.c
>>
>> $ ./socket_stat.exe
>> File type:                socket
>> I-node number:            4
>> dev:                      1966200
>> rdev:                     1966200
>> Mode:                     140644 (octal)
>> Link count:               0
>> Ownership:                UID=197609   GID=197121
>> Preferred I/O block size: 65536 bytes
>> File size:                0 bytes
>> Blocks allocated:         0
>> Last status change:       Wed Dec 31 19:00:00 1969
>> Last file access:         Wed Dec 31 19:00:00 1969
>> Last file modification:   Wed Dec 31 19:00:00 1969
>>
>> On Linux the output is:
>>
>> File type:                socket
>> I-node number:            117
>> dev:                      0
>> rdev:                     0
>> Mode:                     140777 (octal)
>> Link count:               1
>> Ownership:                UID=1000   GID=1000
>> Preferred I/O block size: 512 bytes
>> File size:                0 bytes
>> Blocks allocated:         0
>> Last status change:       Sat Feb 20 18:13:50 2021
>> Last file access:         Sat Feb 20 18:13:50 2021
>> Last file modification:   Sat Feb 20 18:13:50 2021
>>
>> 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.  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. */
+    return fhandler_socket::fstat (buf);

-  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


If this patch looks right, I'll look at the other system calls too.

Ken