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

Windows: fs.readdir() includes "." and ".." when running over Sharepoint connection #4002

Closed
bpasero opened this issue Nov 24, 2015 · 18 comments · Fixed by libuv/libuv#636 or #6796
Closed
Labels
fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform.

Comments

@bpasero
Copy link
Contributor

bpasero commented Nov 24, 2015

I see this in Electron 0.34.1 which is using node.js 4.1.1. I have mapped a Sharepoint connection as a drive on Windows 10 and notice that fs.readdir() includes "." and "..". I have never seen this in any other OS or environment. I dont think "." and ".." should be included in the call.

@bnoordhuis bnoordhuis added fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform. labels Nov 24, 2015
@bnoordhuis
Copy link
Member

Here is the code that filters out the "." and ".." directory entries. I speculate that the SharePoint driver returns something that looks like but is not exactly those strings.

What does filename.split('').map(c => c.charCodeAt(0)) print for those entries?

@bpasero
Copy link
Contributor Author

bpasero commented Nov 24, 2015

@bnoordhuis 46 for "." and 46, 46 for "..". I can also see that the same code is running in the patched node version of Electron (https://github.com/atom/node/blob/1445826ca73cc79bc57d503dd11d4ffaf695625c/deps/uv/src/win/fs.c#L890).

While the char code in JS is 46, maybe it is different in fs.c at that point in time actually?

@bpasero
Copy link
Contributor Author

bpasero commented Nov 24, 2015

Actually this reproduces from vanilla node.js 4.1.1 and 5.x:

C:\Users\benjpas\Downloads>node

process.version
'v5.1.0'
require("fs").readdirSync("Z:")
[ '.',
'..',
'Presentations',
........
'Vision',
'Research' ]

@bnoordhuis
Copy link
Member

Does this patch help?

diff --git a/deps/uv/src/win/fs.c b/deps/uv/src/win/fs.c
index 4a17573..cade6e3 100644
--- a/deps/uv/src/win/fs.c
+++ b/deps/uv/src/win/fs.c
@@ -921,8 +921,6 @@ void fs__scandir(uv_fs_t* req) {
       if (dirent == NULL)
         goto out_of_memory_error;

-      dirents[dirents_used++] = dirent;
-
       /* Convert file name to UTF-8. */
       if (WideCharToMultiByte(CP_UTF8,
                               0,
@@ -934,6 +932,19 @@ void fs__scandir(uv_fs_t* req) {
                               NULL) == 0)
         goto win32_error;

+      /* Skip over '.' and '..' entries. */
+      if (utf8_len == 1 && dirent->d_name[0] == '.') {
+        uv__free(dirent);
+        continue;
+      }
+      if (utf8_len == 2 && dirent->d_name[0] == '.' &&
+          dirent->d_name[1] == '.') {
+        uv__free(dirent);
+        continue;
+      }
+
+      dirents[dirents_used++] = dirent;
+
       /* Add a null terminator to the filename. */
       dirent->d_name[utf8_len] = '\0';

@bpasero
Copy link
Contributor Author

bpasero commented Nov 24, 2015

@bnoordhuis unfortunately not, can we somehow console.log more information to find out what the string really is from the C code?

@bnoordhuis
Copy link
Member

I'd start by adding printf statements to the code in deps/uv/src/win/fs.c.

@bpasero
Copy link
Contributor Author

bpasero commented Nov 24, 2015

@bnoordhuis anything special I have to do to see the output from printf() in my compiled node.exe?

@bnoordhuis
Copy link
Member

You have to recompile it after every change but I assume you know that. Apart from that, there's nothing you need to do; the printfs should show up next time you run the binary.

@bpasero
Copy link
Contributor Author

bpasero commented Nov 26, 2015

@bnoordhuis both utf8_len and wchar_len are 2 (for the case of '.') and thats why those checks fail.

@bpasero
Copy link
Contributor Author

bpasero commented Nov 26, 2015

@bnoordhuis so if I take out the length check the filter function works and "." and ".." are not returned.

@bnoordhuis
Copy link
Member

wchar_len == 2 for the "." case? What's the value of the second character?

@bpasero
Copy link
Contributor Author

bpasero commented Dec 1, 2015

@bnoordhuis it is the null character, looks like the string we get back is a null-terminated string maybe?

@bnoordhuis
Copy link
Member

Can you try this patch?

diff --git a/deps/uv/src/win/fs.c b/deps/uv/src/win/fs.c
index 4a17573..7947c3b 100644
--- a/deps/uv/src/win/fs.c
+++ b/deps/uv/src/win/fs.c
@@ -886,12 +886,26 @@ void fs__scandir(uv_fs_t* req) {
       /* Compute the length of the filename in WCHARs. */
       wchar_len = info->FileNameLength / sizeof info->FileName[0];

-      /* Skip over '.' and '..' entries. */
-      if (wchar_len == 1 && info->FileName[0] == L'.')
+      /* Skip over '.' and '..' entries.  It has been reported that
+       * the SharePoint driver includes the terminating zero byte in
+       * the filename length.
+       */
+      if (wchar_len == 1 && info->FileName[0] == L'.') {
         continue;
-      if (wchar_len == 2 && info->FileName[0] == L'.' &&
-          info->FileName[1] == L'.')
+      }
+
+      if (wchar_len == 2 &&
+          info->FileName[0] == L'.' &&
+          (info->FileName[1] == L'.' || info->FileName[1] == L'\0')) {
         continue;
+      }
+
+      if (wchar_len == 3 &&
+          info->FileName[0] == L'.' &&
+          info->FileName[1] == L'.' &&
+          info->FileName[2] == L'\0') {
+        continue;
+      }

       /* Compute the space required to store the filename as UTF-8. */
       utf8_len = WideCharToMultiByte(

@bpasero
Copy link
Contributor Author

bpasero commented Dec 1, 2015

@bnoordhuis yeah looks like that one works!

bnoordhuis added a commit to bnoordhuis/libuv that referenced this issue Dec 1, 2015
It has been reported that for SharePoint connections mapped as a drive,
uv_fs_scandir() returns "." and ".." entries when the expectation is
that they should be filtered out.

After some investigation it looks like the driver returns ".\0" and
"..\0" for those entries, that is, it includes the zero byte in the
filename length.  Rewrite the filter to catch those entries as well.

Fixes: nodejs/node#4002
@bnoordhuis
Copy link
Member

libuv/libuv#636

@bpasero
Copy link
Contributor Author

bpasero commented Dec 2, 2015

Thanks!

bnoordhuis added a commit to bnoordhuis/libuv that referenced this issue Apr 15, 2016
It has been reported that for SharePoint connections mapped as a drive,
uv_fs_scandir() returns "." and ".." entries when the expectation is
that they should be filtered out.

After some investigation it looks like the driver returns ".\0" and
"..\0" for those entries, that is, it includes the zero byte in the
filename length.  Rewrite the filter to catch those entries as well.

Fixes: nodejs/node#4002
@targos
Copy link
Member

targos commented Apr 16, 2016

I didn't know you could close issues with commit in a different repo !
Should we reopen until we get a libuv update ?

@bnoordhuis
Copy link
Member

Only if you have commit access to both. I'll reopen. =)

kthelgason pushed a commit to kthelgason/libuv that referenced this issue May 7, 2016
It has been reported that for SharePoint connections mapped as a drive,
uv_fs_scandir() returns "." and ".." entries when the expectation is
that they should be filtered out.

After some investigation it looks like the driver returns ".\0" and
"..\0" for those entries, that is, it includes the zero byte in the
filename length.  Rewrite the filter to catch those entries as well.

Fixes: nodejs/node#4002
PR-URL: libuv#636
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
saghul added a commit to saghul/node that referenced this issue May 17, 2016
Fixes: nodejs#4002
Fixes: nodejs#5384
Fixes: nodejs#6563
Refs: nodejs#2680 (comment)
PR-URL: nodejs#6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
evanlucas pushed a commit that referenced this issue May 17, 2016
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
saghul added a commit to saghul/node that referenced this issue Jul 11, 2016
Fixes: nodejs#4002
Fixes: nodejs#5384
Fixes: nodejs#6563
Refs: nodejs#2680 (comment)
PR-URL: nodejs#6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 11, 2016
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 11, 2016
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 12, 2016
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
Fixes: #4002
Fixes: #5384
Fixes: #6563
Refs: #2680 (comment)
PR-URL: #6796
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform.
Projects
None yet
3 participants