public inbox for cygwin-developers@cygwin.com
 help / color / mirror / Atom feed
* fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
@ 2021-02-20 23:32 Ken Brown
  2021-02-20 23:51 ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2021-02-20 23:32 UTC (permalink / raw)
  To: cygwin-devel

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.

Ken

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-20 23:32 fstat and similar methods in fhandler_socket_local and fhandler_socket_unix Ken Brown
@ 2021-02-20 23:51 ` Ken Brown
  2021-02-21 18:04   ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2021-02-20 23:51 UTC (permalink / raw)
  To: cygwin-developers

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.

Ken

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-20 23:51 ` Ken Brown
@ 2021-02-21 18:04   ` Ken Brown
  2021-02-22  9:44     ` Corinna Vinschen
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2021-02-21 18:04 UTC (permalink / raw)
  To: cygwin-developers

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-21 18:04   ` Ken Brown
@ 2021-02-22  9:44     ` Corinna Vinschen
  2021-02-22 20:01       ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Corinna Vinschen @ 2021-02-22  9:44 UTC (permalink / raw)
  To: cygwin-developers

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.

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

Yeah, I think you're right.  Thanks!


Corinna

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-22  9:44     ` Corinna Vinschen
@ 2021-02-22 20:01       ` Ken Brown
  2021-02-24  9:06         ` Corinna Vinschen
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2021-02-22 20:01 UTC (permalink / raw)
  To: cygwin-developers

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-22 20:01       ` Ken Brown
@ 2021-02-24  9:06         ` Corinna Vinschen
  2021-02-24 15:32           ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Corinna Vinschen @ 2021-02-24  9:06 UTC (permalink / raw)
  To: cygwin-developers

On Feb 22 15:01, Ken Brown via Cygwin-developers wrote:
> 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);
> -}

Hmm.  What about linkat (AT_EMPTY_PATH, socket, ...)?

This could be called for inet sockets as well as unnamed or abstract
unix sockets, in theory...


Corinna

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-24  9:06         ` Corinna Vinschen
@ 2021-02-24 15:32           ` Ken Brown
  2021-02-24 17:06             ` Corinna Vinschen
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2021-02-24 15:32 UTC (permalink / raw)
  To: cygwin-developers

On 2/24/2021 4:06 AM, Corinna Vinschen via Cygwin-developers wrote:
> On Feb 22 15:01, Ken Brown via Cygwin-developers wrote:
>> 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);
>> -}
> 
> Hmm.  What about linkat (AT_EMPTY_PATH, socket, ...)?
> 
> This could be called for inet sockets as well as unnamed or abstract
> unix sockets, in theory...

Thanks, I missed that case.  So we do need fhandler_socket::link.  The only 
question in my mind is what errno it should set.  I just tried the following:

$ cat socket_linkat.c
#define _GNU_SOURCE
#include <sys/socket.h>
#include <sys/un.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>

#define NEW_PATH "/tmp/newpath"

int
main ()
{
   struct sockaddr_un addr;
   int fd;

   if (unlink (NEW_PATH) == -1  && errno != ENOENT)
     {
       perror ("unlink");
       exit (1);
     }
   fd = socket (AF_UNIX, SOCK_STREAM, 0);
   if (fd == -1)
     {
       perror ("socket");
       exit (1);
     }
  if (linkat (fd, "", AT_FDCWD, NEW_PATH, AT_EMPTY_PATH) == -1)
    perror ("linkat");
}

On Linux I get

   linkat: Invalid cross-device link

So I think fhandler_socket::link should fail with EXDEV.  Probably it should 
continue to call fhandler_base::link, and the latter should fail with EXDEV 
instead of EPERM.  Or is there some case where EPERM is more appropriate?

Ken

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-24 15:32           ` Ken Brown
@ 2021-02-24 17:06             ` Corinna Vinschen
  2021-02-24 17:27               ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Corinna Vinschen @ 2021-02-24 17:06 UTC (permalink / raw)
  To: cygwin-developers

On Feb 24 10:32, Ken Brown via Cygwin-developers wrote:
> On 2/24/2021 4:06 AM, Corinna Vinschen via Cygwin-developers wrote:
> > On Feb 22 15:01, Ken Brown via Cygwin-developers wrote:
> > > 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);
> > > -}
> > 
> > Hmm.  What about linkat (AT_EMPTY_PATH, socket, ...)?
> > 
> > This could be called for inet sockets as well as unnamed or abstract
> > unix sockets, in theory...
> 
> Thanks, I missed that case.  So we do need fhandler_socket::link.  The only
> question in my mind is what errno it should set.  I just tried the
> following:
> [...]
> On Linux I get
> 
>   linkat: Invalid cross-device link
> 
> So I think fhandler_socket::link should fail with EXDEV.  Probably it should
> continue to call fhandler_base::link, and the latter should fail with EXDEV
> instead of EPERM.  Or is there some case where EPERM is more appropriate?

fhandler_base::link returns EPERM because that covers "filesystem does
not support the creation of hard links", which is the most probable
case.  If we want to return EXDEV by default, the non-disk_file error
case will have to check if oldpath and newpath are on the same
filesystem.  I wonder if this is really worth the effort.


Corinna

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-24 17:06             ` Corinna Vinschen
@ 2021-02-24 17:27               ` Ken Brown
  2021-02-25 13:18                 ` Ken Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2021-02-24 17:27 UTC (permalink / raw)
  To: cygwin-developers

On 2/24/2021 12:06 PM, Corinna Vinschen via Cygwin-developers wrote:
> On Feb 24 10:32, Ken Brown via Cygwin-developers wrote:
>> On 2/24/2021 4:06 AM, Corinna Vinschen via Cygwin-developers wrote:
>>> On Feb 22 15:01, Ken Brown via Cygwin-developers wrote:
>>>> 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);
>>>> -}
>>>
>>> Hmm.  What about linkat (AT_EMPTY_PATH, socket, ...)?
>>>
>>> This could be called for inet sockets as well as unnamed or abstract
>>> unix sockets, in theory...
>>
>> Thanks, I missed that case.  So we do need fhandler_socket::link.  The only
>> question in my mind is what errno it should set.  I just tried the
>> following:
>> [...]
>> On Linux I get
>>
>>    linkat: Invalid cross-device link
>>
>> So I think fhandler_socket::link should fail with EXDEV.  Probably it should
>> continue to call fhandler_base::link, and the latter should fail with EXDEV
>> instead of EPERM.  Or is there some case where EPERM is more appropriate?
> 
> fhandler_base::link returns EPERM because that covers "filesystem does
> not support the creation of hard links", which is the most probable
> case.  If we want to return EXDEV by default, the non-disk_file error
> case will have to check if oldpath and newpath are on the same
> filesystem.  I wonder if this is really worth the effort.

Not if it's my effort.  So I guess I should just make fhandler_socket::link 
return EXDEV.

Ken

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-24 17:27               ` Ken Brown
@ 2021-02-25 13:18                 ` Ken Brown
  2021-02-25 17:03                   ` Corinna Vinschen
  0 siblings, 1 reply; 11+ messages in thread
From: Ken Brown @ 2021-02-25 13:18 UTC (permalink / raw)
  To: cygwin-developers

On 2/24/2021 12:27 PM, Ken Brown via Cygwin-developers wrote:
> On 2/24/2021 12:06 PM, Corinna Vinschen via Cygwin-developers wrote:
>> On Feb 24 10:32, Ken Brown via Cygwin-developers wrote:
>>> On 2/24/2021 4:06 AM, Corinna Vinschen via Cygwin-developers wrote:
>>>> On Feb 22 15:01, Ken Brown via Cygwin-developers wrote:
>>>>> 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);
>>>>> -}
>>>>
>>>> Hmm.  What about linkat (AT_EMPTY_PATH, socket, ...)?

While working on this, I discovered that linkat(AT_EMPTY_PATH) doesn't currently 
work on socket files opened with O_PATH.  I started debugging this (not finished 
yet), and it occurred to me that the implementation of linkat(AT_EMPTY_PATH) 
could be simplified like this:

--- 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,19 @@ 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. */
           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;
+         fh = cfd;
+         flags = 0;
         }
        else if (gen_full_path_at (oldpath, olddirfd, oldpathname))
         __leave;
@@ -5003,6 +5003,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) {}

This seems to work fine on regular files and on socket files opened with O_PATH, 
after I tweak fhandler_socket_local::link (see below).  I haven't tested other 
file types yet.

$ cat linkat_test.c
#define _GNU_SOURCE
#include <sys/types.h>
#include <time.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>

#define FILE "/tmp/myfile"
#define NEW_FILE "/tmp/mynewfile"
#define SOCK_PATH "/tmp/mysocket"
#define NEW_SOCK_PATH "/tmp/mynewsocket"

int
main ()
{
   struct sockaddr_un addr;
   int fd;

   /* Regular file. */
   if (unlink (FILE) < 0  && errno != ENOENT)
     {
       perror ("unlink");
       exit (1);
     }
   if (unlink (NEW_FILE) < 0  && errno != ENOENT)
     {
       perror ("unlink");
       exit (1);
     }
   fd = open (FILE, O_RDONLY | O_CREAT, S_IRUSR | S_IWUSR);
   if (fd < 0)
     {
       perror ("open");
       exit (1);
     }
   printf ("Testing linkat (AT_EMPTY_PATH) on a regular file...\n");
   if (linkat (fd, "", AT_FDCWD, NEW_FILE, AT_EMPTY_PATH) < 0)
     perror ("linkat");
   else
     printf ("success\n\n");
   if (close (fd) < 0)
     {
       perror ("close");
       exit (1);
     }

   /* Socket file. */
   if (unlink (SOCK_PATH) < 0  && errno != ENOENT)
     {
       perror ("unlink");
       exit (1);
     }
   if (unlink (NEW_SOCK_PATH) < 0  && errno != ENOENT)
     {
       perror ("unlink");
       exit (1);
     }
   fd = socket (AF_UNIX, SOCK_STREAM, 0);
   if (fd < 0)
     {
       perror ("socket");
       exit (1);
     }
   memset (&addr, 0, sizeof (struct sockaddr_un));
   addr.sun_family = AF_UNIX;
   strcpy (addr.sun_path, SOCK_PATH);
   if (bind (fd, (struct sockaddr *) &addr, sizeof(struct sockaddr_un)) < 0)
     {
       perror ("bind");
       exit (1);
     }
   if (close (fd) < 0)
     {
       perror ("close");
       exit (1);
     }

   fd = open (SOCK_PATH, O_PATH);
   if (fd < 0)
     {
       perror ("open");
       exit (1);
     }

   printf ("Testing linkat (AT_EMPTY_PATH) on a socket file...\n");
   if (linkat (fd, "", AT_FDCWD, NEW_SOCK_PATH, AT_EMPTY_PATH) < 0)
     perror ("linkat");
   else
     printf ("success\n");
   if (close (fd) < 0)
     {
       perror ("close");
       exit (1);
     }
}

$ gcc -g -O0 -o linkat_test linkat_test.c

$ ./linkat_test.exe
Testing linkat (AT_EMPTY_PATH) on a regular file...
success

Testing linkat (AT_EMPTY_PATH) on a socket file...
success

$ ls -l /tmp/my*
-rw------- 2 kbrown None 0 2021-02-25 07:59 /tmp/myfile
-rw------- 2 kbrown None 0 2021-02-25 07:59 /tmp/mynewfile
srw-r--r-- 2 kbrown None 0 2021-02-25 07:59 /tmp/mynewsocket=
srw-r--r-- 2 kbrown None 0 2021-02-25 07:59 /tmp/mysocket=

And here's the tweak to fhandler_socket_local::link:

--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -749,9 +749,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);
  }

I'll put all this together in a patchset, along with the tweaks to fstat, etc., 
but first I wanted to get your reaction to the proposed simplification of 
linkat(AT_EMPTY_PATH).

Thanks.

Ken

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fstat and similar methods in fhandler_socket_local and fhandler_socket_unix
  2021-02-25 13:18                 ` Ken Brown
@ 2021-02-25 17:03                   ` Corinna Vinschen
  0 siblings, 0 replies; 11+ messages in thread
From: Corinna Vinschen @ 2021-02-25 17:03 UTC (permalink / raw)
  To: cygwin-developers

On Feb 25 08:18, Ken Brown via Cygwin-developers wrote:
> While working on this, I discovered that linkat(AT_EMPTY_PATH) doesn't
> currently work on socket files opened with O_PATH.  I started debugging this
> (not finished yet), and it occurred to me that the implementation of
> linkat(AT_EMPTY_PATH) could be simplified like this:
> 
> --- 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,19 @@ 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. */
>           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;
> +         fh = cfd;
> +         flags = 0;
>         }
>        else if (gen_full_path_at (oldpath, olddirfd, oldpathname))
>         __leave;
> @@ -5003,6 +5003,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) {}

Good idea.  Just a small comment why you set flags to 0 might be nice.

> This seems to work fine on regular files and on socket files opened with
> O_PATH, after I tweak fhandler_socket_local::link (see below).  I haven't
> tested other file types yet.

For most of them that should end up in fhandler_base:link anyway.


Thanks,
Corinna

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-02-25 17:03 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-20 23:32 fstat and similar methods in fhandler_socket_local and fhandler_socket_unix Ken Brown
2021-02-20 23:51 ` Ken Brown
2021-02-21 18:04   ` Ken Brown
2021-02-22  9:44     ` Corinna Vinschen
2021-02-22 20:01       ` Ken Brown
2021-02-24  9:06         ` Corinna Vinschen
2021-02-24 15:32           ` Ken Brown
2021-02-24 17:06             ` Corinna Vinschen
2021-02-24 17:27               ` Ken Brown
2021-02-25 13:18                 ` Ken Brown
2021-02-25 17:03                   ` Corinna Vinschen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).