Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Win32: stat/unlink fails on UNIX sockets #20204

Closed
bram-perl opened this issue Aug 31, 2022 · 6 comments · Fixed by #20271
Closed

Win32: stat/unlink fails on UNIX sockets #20204

bram-perl opened this issue Aug 31, 2022 · 6 comments · Fixed by #20271

Comments

@bram-perl
Copy link

bram-perl commented Aug 31, 2022

dist/IO/t/io_unix.t contains a test that creates a UNIX socket, does some tests and then attempts to unlink the file.
That unlink call fails and that results in a stray tmp file.

A reduced example case:

#!./perl

use IO::Socket;

my $PATH = "sock-$$";

my $listen = IO::Socket::UNIX->new(Local => $PATH, Listen => 0);
close $listen;

my @x = stat($PATH) or warn "stat $PATH failed: $!";
unlink $PATH or die "unlink $PATH failed: $!";

Running on Windows 10:

$ .\perl -I..\lib foo.t
stat sock-6816 failed: Invalid argument at foo.t line 10.
unlink sock-6816 failed: Invalid argument at foo.t line 11.

See @tonycoz comment: #20179 (comment)

I expect this needs to be fixed in the core code, at least in win32_stat() and maybe in Perl_apply() (which does unlink() amongst other ops).

I'll be looking at win32_stat() tomorrow, but being windows, I expect it to be moderately ugly (see golang/go#33357 for example).

Other references:

@bram-perl
Copy link
Author

Using fsutil:

C:\Perl\perl5\t>fsutil reparsepoint query sock-6816
Reparse Tag Value : 0x80000023
Tag value: Microsoft

Reparse Data Length: 0x0

Possibly relevant: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/c8e77b37-3909-4fe6-a4ea-2b9d423b1ee4

Value Meaning
IO_REPARSE_TAG_AF_UNIX 0x80000023 Used by the Windows Subsystem for Linux (WSL) to represent a UNIX domain socket. Server-side interpretation only, not meaningful over the wire.

Looking at that doc there might be some other ones for which something needs to be done in win32_stat:

Value Meaning
IO_REPARSE_TAG_LX_FIFO 0x80000024 Used by the Windows Subsystem for Linux (WSL) to represent a UNIX FIFO (named pipe). Server-side interpretation only, not meaningful over the wire.
IO_REPARSE_TAG_LX_CHR 0x80000025 Used by the Windows Subsystem for Linux (WSL) to represent a UNIX character special file. Server-side interpretation only, not meaningful over the wire.
IO_REPARSE_TAG_LX_BLK 0x80000026 Used by the Windows Subsystem for Linux (WSL) to represent a UNIX block special file. Server-side interpretation only, not meaningful over the wire

@bram-perl
Copy link
Author

A quick & dirty patch to add some debugging info:

diff --git a/win32/win32.c b/win32/win32.c
index e861ed39e1..42f8c976c8 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -1497,6 +1497,8 @@ win32_stat_low(HANDLE handle, const char *path, STRLEN len, Stat_t *sbuf) {

     type &= ~FILE_TYPE_REMOTE;

+    fprintf(stderr, "in win32_stat_low, path = %s, type = %d\n", path, type);
+
     switch (type) {
     case FILE_TYPE_DISK:
         if (GetFileInformationByHandle(handle, &bhi)) {
@@ -1559,6 +1561,7 @@ win32_stat_low(HANDLE handle, const char *path, STRLEN len, Stat_t *sbuf) {
             }
         }
         else {
+            fprintf(stderr, "win32_stat_low: %s: GetFileInformationByHandle failed? --> return -1\n", path);
             translate_to_errno();
             return -1;
         }
@@ -1575,6 +1578,7 @@ win32_stat_low(HANDLE handle, const char *path, STRLEN len, Stat_t *sbuf) {
         break;

     default:
+        fprintf(stderr, "win32_stat_low: %s: default case, type: %d --> return -1\n", path, type);
         return -1;
     }

@@ -1595,15 +1599,24 @@ win32_stat(const char *path, Stat_t *sbuf)

     path = PerlDir_mapA(path);

+    fprintf(stderr, "in win32_stat, path = %s\n", path);
     handle =
-        CreateFileA(path, FILE_READ_ATTRIBUTES,
-                    FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE,
-                    NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
+        CreateFileA(
+            path,                                                       /* lpFileName */
+            FILE_READ_ATTRIBUTES,                                       /* dwDesiredAccess */
+            FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE,     /* dwShareMode */
+            NULL,                                                       /* lpSecurityAttribute */
+            OPEN_EXISTING,                                              /* dwCreationDisposition */
+            FILE_FLAG_BACKUP_SEMANTICS,                                 /* dwFlagsAndAttributes  */
+            NULL                                                        /* hTemplateFile */
+        );
     if (handle != INVALID_HANDLE_VALUE) {
+        fprintf(stderr, "in win32_stat --> valid handle\n");
         result = win32_stat_low(handle, path, strlen(path), sbuf);
         CloseHandle(handle);
     }
     else {
+        fprintf(stderr, "in win32_stat --> invalid handle\n");
         translate_to_errno();
         result = -1;
     }

Running:

About to stat

in win32_stat, path = C:\Perl\perl5\sock-4232
in win32_stat --> invalid handle
stat sock-4232 failed: Invalid argument at t\foo.t line 18.

About to unlink

in win32_stat, path = C:\Perl\perl5\sock-4232
in win32_stat --> invalid handle
unlink sock-4232 failed: Invalid argument at t\foo.t line 23.

A quick & dirty fix (on top of the quick & dirty debug patch):

diff --git a/win32/win32.c b/win32/win32.c
index 42f8c976c8..981215ba20 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -1607,7 +1607,7 @@ win32_stat(const char *path, Stat_t *sbuf)
             FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE,     /* dwShareMode */
             NULL,                                                       /* lpSecurityAttribute */
             OPEN_EXISTING,                                              /* dwCreationDisposition */
-            FILE_FLAG_BACKUP_SEMANTICS,                                 /* dwFlagsAndAttributes  */
+            FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT,  /* dwFlagsAndAttributes  */
             NULL                                                        /* hTemplateFile */
         );
     if (handle != INVALID_HANDLE_VALUE) {

Running:

About to stat

in win32_stat, path = C:\Perl\perl5\sock-248
in win32_stat --> valid handle
in win32_stat_low, path = C:\Perl\perl5\sock-248, type = 1

About to unlink

in win32_stat, path = C:\Perl\perl5\sock-248
in win32_stat --> valid handle
in win32_stat_low, path = C:\Perl\perl5\sock-248, type = 1

=> The stat and unlink succeeded.

Some more relevant information: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea

Flag Meaning
FILE_FLAG_OPEN_REPARSE_POINT 0x00200000 Normal reparse point processing will not occur; CreateFile will attempt to open the reparse point. When a file is opened, a file handle is returned, whether or not the filter that controls the reparse point is operational. This flag cannot be used with the CREATE_ALWAYS flag. If the file is not a reparse point, then this flag is ignored. For more information, see the Remarks section.

What I don't know if this makes sense/is good enough.. Relevant part of the Remarks section: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#symbolic-link-behavior
Reading that I assume it may alter the behavior with symbolic links.. (and I don't know what behavior is wanted)

@tonycoz
Copy link
Contributor

tonycoz commented Sep 1, 2022

The main difficulty I see is with symlinks - opening with FILE_FLAG_OPEN_REPARSE_POINT means that if the name is a symlink() it won't be followed, so we'll need to follow it ourselves, including handling relative pathnames and loop detection.

@bram-perl
Copy link
Author

What about first doing a stat without FILE_FLAG_OPEN_REPARSE_POINT and if that fails retrying it with the flag set?
It's a race condition tho.. but if we start to follow the symlinks ourselves then that may also be a race condition..
And it might be more tricky then that.. if the file is a symlink that points to a non-existing file then the stat call should fail..
So I guess it should then be something like:

  • call without FILE_FLAG_OPEN_REPARSE_POINT
  • if it fails: redo with FILE_FLAG_OPEN_REPARSE_POINT
    • if it succeeds: check if it's a symlink: if it is fail the stat (symlink to a non-existing file)

Also: what about the (perl) lstat function? How/where is that implemented? (Is there a "special" implementation for Windows?)

@tonycoz
Copy link
Contributor

tonycoz commented Sep 1, 2022

I have some work in progress code, just still working on it

@tonycoz
Copy link
Contributor

tonycoz commented Sep 1, 2022

What about first doing a stat without FILE_FLAG_OPEN_REPARSE_POINT and if that fails retrying it with the flag set? It's a race condition tho.. but if we start to follow the symlinks ourselves then that may also be a race condition..

I don't think we can avoid the race, luckily it's only an issue for symlink chains to other reparse points.

And it might be more tricky then that.. if the file is a symlink that points to a non-existing file then the stat call should fail.. So I guess it should then be something like:

* call without `FILE_FLAG_OPEN_REPARSE_POINT`

* if it fails: redo with `FILE_FLAG_OPEN_REPARSE_POINT`
  
  * if it succeeds: check if it's a symlink: if it is fail the stat (symlink to a non-existing file)

That's what I'm doing.

Also: what about the (perl) lstat function? How/where is that implemented? (Is there a "special" implementation for Windows?)

All of stat(), lstat() and unlink() have special implementations in win32/win32.c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants