finish implementation of "win,fs: get fstat when no permission"

[vtjnash]

Replaces #4504, addressing all of my comments there

TODO: if I force it to use this in all cases, there are a couple test failures that I see:

• stat on "." is broken (returns ENOENT)

  not ok 148 - getters_setters
  # exit code 3
  # Output from process `getters_setters`:
  # Assertion failed in /home/jameson/libuv/test/test-getters-setters.c on line 101: `uv_fs_get_result(fs) == 0` (-4058 == 0
  

• fs_write_alotof_bufs_with_offset with add_flags == 0 shows the st_size value is stale since the kernel hasn't flushed this info yet (until file close). This stale info is warned about in the NT kernel docs, so is expected.

  not ok 127 - fs_write_alotof_bufs_with_offset
  # exit code 3
  # Output from process `fs_write_alotof_bufs_with_offset`:
  # Assertion failed in /home/jameson/libuv/test/test-fs.c on line 3639: `(int64_t)((uv_stat_t*)stat_req.ptr)->st_size == offset + (int64_t)write_req.result` (0 == 706183
  

• fs_stat_root fails (returns ENOENT), since the query (dirname = "/", filename = "") makes no sense. This might be an implementation issue for handling trailing slashes in general though.
  # Assertion failed in /home/jameson/libuv/test/test-fs.c on line 2760: `r == 0` (-4058 == 0)
• fs_symlink_junction fails, which seems like possibly a general failing of lstat vs stat?

  not ok 121 - fs_symlink_junction
  # exit code 3
  # Output from process `fs_symlink_junction`:
  # r == 0
  # Assertion failed in /home/jameson/libuv/test/test-fs.c on line 2415: `((uv_stat_t*)req.ptr)->st_size == strlen(test_dir + 4)` (0 == 40)
  

• fs_readdir_symlink fails, which is a EPERM issue with my user account and not related to this change

  not ok 106 - fs_readdir_symlink
  # exit code 3
  # Output from process `fs_readdir_symlink`:
  # Assertion failed in /home/jameson/libuv/test/test-fs-readdir.c on line 536: `r == 0` (-4048 == 0)
  

• fs_poll_getpath segfaults (jumps to NULL), which may not even be related, but is currently failing on my machine

  ok 95 - fs_poll_getpath
  not ok 96 - fs_poll_ref
  # exit code -1073741819
  # Output from process `fs_poll_ref`: (no output)
  

[vtjnash]

Local test with

diff --git a/src/win/fs.c b/src/win/fs.c
index f476181a..be81b3aa 100644
--- a/src/win/fs.c
+++ b/src/win/fs.c
@@ -2116,13 +2116,15 @@ INLINE static DWORD fs__stat_impl_from_path(WCHAR* path,
   if (do_lstat)
     flags |= FILE_FLAG_OPEN_REPARSE_POINT;
 
-  handle = CreateFileW(path,
-                       FILE_READ_ATTRIBUTES,
-                       FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-                       NULL,
-                       OPEN_EXISTING,
-                       flags,
-                       NULL);
+  handle = INVALID_HANDLE_VALUE;
+  SetLastError(ERROR_ACCESS_DENIED);
+  //handle = CreateFileW(path,
+  //                     FILE_READ_ATTRIBUTES,
+  //                     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+  //                     NULL,
+  //                     OPEN_EXISTING,
+  //                     flags,
+  //                     NULL);
 
   if (handle == INVALID_HANDLE_VALUE) {
     DWORD error = GetLastError();
 

[huseyinacacak-janea]

Hey @vtjnash, thank you for the changes to my PR. If there is anything I can do to help you move this forward, I would be happy to help.
Please note that this PR also fixes this issue nodejs/node#36790.

[vtjnash]

If you'd like to take this over and investigate those possible test failures to make sure the new code path handles them correctly if necessary, that would help get this finished

[huseyinacacak-janea]

I’d be happy to take this over and investigate the test failures to ensure the new code path handles them correctly if needed. I’ll start working on it and will keep you updated on my progress.

[huseyinacacak-janea]

I've implemented the fix below and tested it with the patch you provided. Only 2 tests (fs_symlink_dir and fs_symlink_junction) are failing which is expected because of symlinks. If you encounter any problems, you can let me know and I can try to fix them too.

diff --git a/src/win/fs.c b/src/win/fs.c
index f476181a..1811772b 100644
--- a/src/win/fs.c
+++ b/src/win/fs.c
@@ -1970,18 +1970,41 @@ INLINE static int fs__stat_directory(WCHAR* path, uv_stat_t* statbuf,
   size_t len;
   size_t split;
   WCHAR splitchar;
+  int is_long_path;
+  int includes_name;
 
   /* aka strtok or wcscspn, in reverse */
   len = wcslen(path);
   split = len;
-  while (split > 0 && path[split - 1] != L'\\' && path[split - 1] != L'/')
+  is_long_path = wcsncmp(path, LONG_PATH_PREFIX, LONG_PATH_PREFIX_LEN) == 0;
+
+  includes_name = 0;
+  while (split > 0 && path[split - 1] != L'\\' && path[split - 1] != L'/' &&
+                      path[split - 1] != L':') {
+    // check if the path contains a character other than /,\,:,.
+    if (path[split-1] != '.') {
+      includes_name = 1;
+    }
     split--;
-  if (split == 0) {
+  }
+  // if the path is a relative path with a file name or a folder name
+  if (split == 0 && includes_name) {
     path_dirpath = L".";
+  // if there is a slash or a backslash
+  } else if (path[split - 1] == L'\\' || path[split - 1] == L'/') {
+    path_dirpath = path;
+    // if there is no filename, consider it as a relative folder path
+    if (!includes_name) {
+      split = len;
+    // if it is not a long path, split it
+    } else if (!is_long_path) {
+      splitchar = path[split - 1];
+      path[split - 1] = L'\0';
+    }
+  // e.g. "..", "c:"
   } else {
     path_dirpath = path;
-    splitchar = path[split - 1];
-    path[split - 1] = L'\0';
+    split = len;
   }
   path_filename = &path[split];
 

[vtjnash]

Thanks. I did some testing on the remaining failing test (fs_symlink_junction) and I think that it appears that this function behaves like lstat rather than stat (although without the ability to set st_size correctly as required by posix) so I added some logic to turn stat into an error in that edge case and kept lstat returning the wrong answer (similar to IBM PASE I suppose). I think this PR is good to merge now.

[vtjnash]

This is ready to merge, if we could get at least one other libuv reviewer to check for any obvious issues or style guideline nits.

[huseyinacacak-janea]

I noticed that this PR no longer fixes nodejs/node#36790. I've implemented a fix and attached below. This patch splits long paths correctly as far as I can see in local tests. However, when I apply this patch and get the stats of the file provided in the referenced issue, I get an unknown error. After further investigation, I found that this code causes this error. What can be done to resolve this problem?

diff --git a/src/win/fs.c b/src/win/fs.c
index d4afe513..071fd681 100644
--- a/src/win/fs.c
+++ b/src/win/fs.c
@@ -1968,13 +1968,11 @@ INLINE static DWORD fs__stat_directory(WCHAR* path, uv_stat_t* statbuf,
   size_t len;
   size_t split;
   WCHAR splitchar;
-  int is_long_path;
   int includes_name;
 
   /* aka strtok or wcscspn, in reverse */
   len = wcslen(path);
   split = len;
-  is_long_path = wcsncmp(path, LONG_PATH_PREFIX, LONG_PATH_PREFIX_LEN) == 0;
 
   includes_name = 0;
   while (split > 0 && path[split - 1] != L'\\' && path[split - 1] != L'/' &&
@@ -1994,8 +1992,8 @@ INLINE static DWORD fs__stat_directory(WCHAR* path, uv_stat_t* statbuf,
     /* if there is no filename, consider it as a relative folder path */
     if (!includes_name) {
       split = len;
-    /* if it is not a long path, split it */
-    } else if (!is_long_path) {
+    /* else, split it */
+    } else {
       splitchar = path[split - 1];
       path[split - 1] = L'\0';
     }

[vtjnash]

Did it work before (return correct answers) or merely not error?

We can get the ReparseTag (as I wrote in the comment on that line of code) to see that it is APPEXECLINK, but I don't think we'd have a way to read the correct stat info from it, as I think we have lstat info only?

libuv/src/win/fs.c

Line 279 in 652ce8c

} else if (reparse_data->ReparseTag == IO_REPARSE_TAG_APPEXECLINK) {

[vtjnash]

What can be done to resolve this problem?

Looking closer at this behavior on my machine, it appears that CreateFile does not operate reliably without the FILE_FLAG_OPEN_REPARSE_POINT being passed since the meaning of the reparse point tags are defined by userspace, while only some of them have kernel filters to make them transparently succeed to naive code. This failure of the kernel to handle most of these edge cases, requiring userspace workarounds, is documented as being expected:

When the file system opens a file with a reparse point, it attempts to find the file system filter associated with the data format identified by the reparse tag. If a file system filter is found, the filter processes the file as directed by the reparse data. If a file system filter is not found, the file open operation fails.

Unfortunately, this also means there is no completely reliable way to handle all symlinks, since Microsoft does not provide that ability. We however could hack around this OS failing in libuv by handling all CreateFile failures with manual FILE_FLAG_OPEN_REPARSE_POINT & readlink & retry to emulate what Win32 also was supposed to do internally.

[huseyinacacak-janea]

I've implemented the fix below. However, I'm still getting an error (no 4390) when trying to get the file size using readlink.

diff --git a/src/win/fs.c b/src/win/fs.c
index a4742aa2..3a5af286 100644
--- a/src/win/fs.c
+++ b/src/win/fs.c
@@ -2072,8 +2072,28 @@ INLINE static DWORD fs__stat_directory(WCHAR* path, uv_stat_t* statbuf,
      * ReparsePointTag by querying FILE_ID_EXTD_DIR_INFORMATION instead to make
      * sure this really is a link before giving up here on the uv_fs_stat call,
      * but that doesn't seem essential. */
-    if (!do_lstat)
+
+    /* Get directory handle */
+    handle = CreateFileW(path,
+                         0,
+                         0,
+                         NULL,
+                         OPEN_EXISTING,
+                         FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT,
+                         NULL);
+
+    if (handle == INVALID_HANDLE_VALUE) {
+      ret_error = GetLastError();
+      goto cleanup;
+    }
+
+    size_t target_length = 0;
+    if (fs__readlink_handle(handle, NULL, &target_length) != 0) {
+      printf("fs__readlink_handle error: %d\n", GetLastError());
       goto cleanup;
+    }
+    stat_info.EndOfFile.QuadPart = target_length;
+
     stat_info.EndOfFile.QuadPart = 0;
     stat_info.AllocationSize.QuadPart = 0;
   } else {

[vtjnash]

Are you okay if we go ahead and merge this, so we can separate out working on that fix?

I believe with that patch, you'd still be doing an lstat query instead of correctly doing stat on that path. The actual handling required seems to be to call fs__readlink_handle whenever any CreateFileW in fs.c fails on any file (not the directory) and then retry the CreateFileW on the resulting target read from it, repeating up to 256 times

[huseyinacacak-janea]

Are you okay if we go ahead and merge this, so we can separate out working on that fix?

Sure, we can merge this PR. I can open a new PR for the new fix.