fsmonitor: handle differences between Windows named pipe functions
PATCH 1/1 fsmonitor: handle differences between Windows named pipe functions
From: Eric DeCosta <[email protected]>
CreateNamedPipeW is perfectly happy accepting pipe names with seemingly embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created with CreateNamedPipeW, exists.
For example, this network path is problemmatic:
\\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
In order to work around this issue, rather than using the path to the worktree directly as the name of the pipe, instead use the hash of the worktree path.
---
fsmonitor: handle differences between Windows named pipe functions
CreateNamedPipeW is perfectly happy accepting pipe names with embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly returns ERROR_FILE_NOT_FOUND when clearly a named pipe with the given name exists.
For example, this path is problemmatic:
\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
In order to work around this issue, rather than using the path to the worktree directly as the name of the pipe, instead use the hash of the worktree path.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1503%2Fedecosta-mw%2Ffsmonitor_windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1503/edecosta-mw/fsmonitor_windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1503
compat/simple-ipc/ipc-win32.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
compat/simple-ipc/ipc-win32.c +17 -16
@@ -1,4 +1,5 @@#include "cache.h" +#include "hex.h" #include "simple-ipc.h" #include "strbuf.h" #include "pkt-line.h"
@@ -17,27 +18,27 @@static int initialize_pipe_name (const char *path,wchar_t *wpath,size_t alloc) {int off =0 ; -struct strbuf realpath =STRBUF_INIT ; - -if (!strbuf_realpath (&realpath, path,0 )) -return -1 ; +int ret =0 ; +git_SHA_CTX sha1ctx; +struct strbuf real_path =STRBUF_INIT ; +struct strbuf pipe_name =STRBUF_INIT ; +unsigned char hash[GIT_MAX_RAWSZ ]; - off =swprintf (wpath, alloc,L"\\\\.\\pipe\\" ); -if (xutftowcs (wpath + off, realpath.buf, alloc - off) <0 ) +if (!strbuf_realpath (&real_path, path,0 ))return -1 ; -/* Handle drive prefix */ -if (wpath[off] && wpath[off +1 ] ==L':' ) { - wpath[off +1 ] =L'_' ; - off +=2 ; - } +git_SHA1_Init (&sha1ctx); +git_SHA1_Update (&sha1ctx, real_path.buf, real_path.len); +git_SHA1_Final (hash, &sha1ctx); +strbuf_release (&real_path); -for (; wpath[off]; off++) -if (wpath[off] ==L'/' ) - wpath[off] =L'\\' ; +strbuf_addf (&pipe_name,"git-fsmonitor-%s" ,hash_to_hex (hash)); + off =swprintf (wpath, alloc,L"\\\\.\\pipe\\" ); +if (xutftowcs (wpath + off, pipe_name.buf, alloc - off) <0 ) + ret =-1 ; -strbuf_release (&realpath); -return 0 ; +strbuf_release (&pipe_name); +return ret; }static enum ipc_active_state get_active_state (wchar_t *pipe_path)
base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e
Signature · 1 line
gitgitgadget
Hi Eric,
Quoted 7 lines · Eric DeCosta via GitGitGadget
From: Eric DeCosta <[email protected]>
CreateNamedPipeW is perfectly happy accepting pipe names with seemingly embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly returns
ERROR_FILE_NOT_FOUNDwhen clearly a named pipe, succesfully created with CreateNamedPipeW, exists.
Quoted 6 lines
For example, this network path is problemmatic:
\\batfs-sb29-cifs\vmgr\sbs29\my_git_repoIn order to work around this issue, rather than using the path to the worktree directly as the name of the pipe, instead use the hash of the worktree path.
This is a rather large deviation from the other platforms, and it has an unwanted side effect: Git for Windows' installer currently enumerates the named pipes to figure out which FSMonitor instances need to be stopped before upgrading. It has to do that because it would otherwise be unable to overwrite the Git executable. And it needs to know the paths [*1*] so that it can stop the FSMonitors gracefully (as opposed to terminating them and risk interrupting them while they serve a reply to a Git client).
A much less intrusive change (that would not break Git for Windows' installer) would be to replace backslashes by forward slashes in the path.
Please do that instead.
Ciao,
Johannes
Footnote *1*: If you think that the Git for Windows installer could simply enumerate the process IDs of the FSMonitor instances and then look for their working directories: That is not a viable option. Not only does the Windows-based FSMonitor specifically switch to the parent directory (to avoid blocking the removal of a Git directory merely by running the process in said directory), even worse: there is no officially-sanctioned way to query a running process' current working directory (the only way I know of involves injecting a remote thread! Which will of course risk being labeled as malware by current anti-malware solutions).
Quoted 34 lines · Johannes Schindelin
Hi Eric,
Quoted 7 lines · Eric DeCosta via GitGitGadget
From: Eric DeCosta <[email protected]>
CreateNamedPipeW is perfectly happy accepting pipe names with seemingly embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly returns
ERROR_FILE_NOT_FOUNDwhen clearly a named pipe, succesfully created with CreateNamedPipeW, exists.Quoted 6 lines
For example, this network path is problemmatic:
\\batfs-sb29-cifs\vmgr\sbs29\my_git_repoIn order to work around this issue, rather than using the path to the worktree directly as the name of the pipe, instead use the hash of the worktree path.
This is a rather large deviation from the other platforms, and it has an unwanted side effect: Git for Windows' installer currently enumerates the named pipes to figure out which FSMonitor instances need to be stopped before upgrading. It has to do that because it would otherwise be unable to overwrite the Git executable. And it needs to know the paths [*1*] so that it can stop the FSMonitors gracefully (as opposed to terminating them and risk interrupting them while they serve a reply to a Git client).
A much less intrusive change (that would not break Git for Windows' installer) would be to replace backslashes by forward slashes in the path.
Please do that instead.
Ciao,
Johannes
Quoted 9 lines
Footnote *1*: If you think that the Git for Windows installer could simply enumerate the process IDs of the FSMonitor instances and then look for their working directories: That is not a viable option. Not only does the Windows-based FSMonitor specifically switch to the parent directory (to avoid blocking the removal of a Git directory merely by running the process in said directory), even worse: there is no officially-sanctioned way to query a running process' current working directory (the only way I know of involves injecting a remote thread! Which will of course risk being labeled as malware by current anti-malware solutions).
Agreed. Please use forward slashes.
Thanks,
Jeff
Quoted 5 lines · Jeff Hostetler
Quoted 3 lines
This is a rather large deviation from the other platforms, and it has an
unwanted side effect: Git for Windows' installer currently enumerates the...
Agreed. Please use forward slashes.
Thanks, both.
I'll mark the topic to be expecting a reroll, then.
Quoted 58 lines
-----Original Message-----
From: Jeff Hostetler <[email protected]>
Sent: Monday, March 27, 2023 11:02 AM
To: Johannes Schindelin <[email protected]>; Eric DeCosta via GitGitGadget <[email protected]>
Cc: [email protected]; Eric DeCosta <[email protected]>
Subject: Re: [PATCH] fsmonitor: handle differences between Windows named pipe functionsQuoted 18 lines · Johannes Schindelin
Hi Eric,
Quoted 7 lines · Eric DeCosta via GitGitGadget
From: Eric DeCosta <[email protected]>
CreateNamedPipeW is perfectly happy accepting pipe names with seemingly embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly returns
ERROR_FILE_NOT_FOUNDwhen clearly a named pipe, succesfully created with CreateNamedPipeW, exists.Quoted 6 lines
For example, this network path is problemmatic:
\\batfs-sb29-cifs\vmgr\sbs29\my_git_repoIn order to work around this issue, rather than using the path to the worktree directly as the name of the pipe, instead use the hash of the worktree path.
Quoted 7 lines
This is a rather large deviation from the other platforms, and it has an unwanted side effect: Git for Windows' installer currently enumerates the named pipes to figure out which FSMonitor instances need to be stopped before upgrading. It has to do that because it would otherwise be unable to overwrite the Git executable. And it needs to know the paths [*1*] so that it can stop the FSMonitors gracefully (as opposed to terminating them and risk interrupting them
while they serve a reply to a Git client).
Quoted 9 lines
A much less intrusive change (that would not break Git for Windows' installer) would be to replace backslashes by forward slashes in the path.
Please do that instead.
Ciao,
JohannesQuoted 9 lines
Footnote *1*: If you think that the Git for Windows installer could simply enumerate the process IDs of the FSMonitor instances and then look for their working directories: That is not a viable option. Not only does the Windows-based FSMonitor specifically switch to the parent directory (to avoid blocking the removal of a Git directory merely by running the process in said directory), even worse: there is no officially-sanctioned way to query a running process' current working directory (the only way I know of involves injecting a remote thread! Which will of course risk being labeled as malware by current anti-
Quoted 7 lines
malware solutions).
Agreed. Please use forward slashes.
Thanks,
Jeff
I have misdiagnosed the problem. Here are my most recent findings:
The problem is the leading double-slashes for repos that resolve to remote filesystems. i.e. if S:\myrepo resolves to \\some-server\some-dir\myrepo then the path passed to initialize_pipe_name is //some-server/some-dir/myrepo
Regardless of what type or how many slashes appear after \\.\pipe\ the pipe name, as reported from PowerShell, is always \\.\\pipe\\some-server\some-dir\myrepo and WaitNamedPipeW returns ERROR_FILE_NOT_FOUND
If I skip over the first leading slash an use /some-server/some-dir/myrepo I get the same pipe name as before, WaitNamedPipeW is happy and commands like git fsmonitor--daemon status correctly report that the daemon is watching the repo.
-Eric
Quoted 68 lines · Eric DeCosta
Quoted 58 lines
-----Original Message-----
From: Jeff Hostetler <[email protected]>
Sent: Monday, March 27, 2023 11:02 AM
To: Johannes Schindelin <[email protected]>; Eric DeCosta via GitGitGadget <[email protected]>
Cc: [email protected]; Eric DeCosta <[email protected]>
Subject: Re: [PATCH] fsmonitor: handle differences between Windows named pipe functionsQuoted 18 lines · Johannes Schindelin
Hi Eric,
Quoted 7 lines · Eric DeCosta via GitGitGadget
From: Eric DeCosta <[email protected]>
CreateNamedPipeW is perfectly happy accepting pipe names with seemingly embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly returns
ERROR_FILE_NOT_FOUNDwhen clearly a named pipe, succesfully created with CreateNamedPipeW, exists.Quoted 6 lines
For example, this network path is problemmatic:
\\batfs-sb29-cifs\vmgr\sbs29\my_git_repoIn order to work around this issue, rather than using the path to the worktree directly as the name of the pipe, instead use the hash of the worktree path.
Quoted 7 lines
This is a rather large deviation from the other platforms, and it has an unwanted side effect: Git for Windows' installer currently enumerates the named pipes to figure out which FSMonitor instances need to be stopped before upgrading. It has to do that because it would otherwise be unable to overwrite the Git executable. And it needs to know the paths [*1*] so that it can stop the FSMonitors gracefully (as opposed to terminating them and risk interrupting them
while they serve a reply to a Git client).
Quoted 9 lines
A much less intrusive change (that would not break Git for Windows' installer) would be to replace backslashes by forward slashes in the path.
Please do that instead.
Ciao,
JohannesQuoted 9 lines
Footnote *1*: If you think that the Git for Windows installer could simply enumerate the process IDs of the FSMonitor instances and then look for their working directories: That is not a viable option. Not only does the Windows-based FSMonitor specifically switch to the parent directory (to avoid blocking the removal of a Git directory merely by running the process in said directory), even worse: there is no officially-sanctioned way to query a running process' current working directory (the only way I know of involves injecting a remote thread! Which will of course risk being labeled as malware by current anti-
Quoted 7 lines
malware solutions).
Agreed. Please use forward slashes.
Thanks,
JeffI have misdiagnosed the problem. Here are my most recent findings:
Quoted 7 lines
The problem is the leading double-slashes for repos that resolve to remote filesystems. i.e. if S:\myrepo resolves to \\some-server\some-dir\myrepo then the path passed to
initialize_pipe_nameis //some-server/some-dir/myrepoRegardless of what type or how many slashes appear after \\.\pipe\ the pipe name, as reported from PowerShell, is always \\.\\pipe\\some-server\some-dir\myrepo and WaitNamedPipeW returns
ERROR_FILE_NOT_FOUNDIf I skip over the first leading slash an use /some-server/some-dir/myrepo I get the same pipe name as before, WaitNamedPipeW is happy and commands like git fsmonitor--daemon status correctly report that the daemon is watching the repo.
-Eric
The named pipe file system (NPFS) is a little "special". It is a flat namespace and not hierarchical and not subject to the usual Win32 and/or NTFS limitations/quirks (such as restricted characters or legacy filename suffixes). It is a single level dictionary, in a sense.
The local form is "\\.\pipe\<name>" and according to [1], the only restriction is that <name> portion may not contain backslashes[1], but I'm seeing lots of named pipes of the form "\\.\pipe\Winsock2\..." on my Windows 10 system, so that restriction may have been lifted since the documentation was last updated.
[1] https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createnamedpipew
Forward slashes (and now it seems backslashes) are not directory separators -- they are just another character in the allowed char[256]. We tend to think of them as directory separators, but that is an illusion. For example, in a CMD prompt:
dir \\.\pipe\\ /b
shows a simple list of all the named pipes on the system, including some "Winsock2\CatalogChangeListener..." ones. However, any attempt to list the contents of the "Winsock2" directory:
dir \\.\pipe\\Winsock2\\ /b
fails with a file not found error.
However, a simple wildcard lists them:
dir \\.\pipe\\Winsock2* /b
From PowerShell, we can see a complete list of pipes with:
(get-childitem \\.\pipe\).FullName
But we get a path does not exist with:
(get-childitem \\.\pipe\Winsock2\).FullName
However "get-childitem" is confused and reports "Winsock2" as a directory multiple times, each with one item, when we do:
(get-childitem \\.\pipe)
(BTW There's also the "Pipelist" tool from SysInternals that shows them as a simple list of names (some with the embedded backslashes).
In [1], it also says that CreateNamedPipeW() can only create a local "\\.\pipe\<something>" pipe, so I wonder if CreateNamedPipeW() is silently prefixing "\\.\pipe\" if necessary... I haven't had time to try this.
Then WaitNamedPipeW() and/or CreateFileW() allows fully general "\\<host>\<share>\<pathnames>", so the OS cannot do any implicit fixup -- and these calls actually try to access the (intended) network file.
I'm guessing here that this is the problem you've found. If that is the case, we need to think about how to fix it mainly because of what Johannes said about the installer needing to properly shutdown running daemons during an upgrade. Or rather, we will need to coordinate with the GFW installer.
Please let me know if any of this makes sense.
Thanks,
Jeff
PATCH v2 1/1 fsmonitor: handle differences between Windows named pipe functions
In reply to Eric DeCosta via GitGitGadgetFrom: Eric DeCosta <[email protected]>
The two functions involved with creating and checking for the existance of the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear to handle names with leading slashes or backslashes a bit differently.
If a repo resolves to a remote file system with the UNC path of //some-server/some-dir/some-repo, CreateNamedPipeW accepts this name and creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
However, when the same UNC path is passed to WaitNamedPipeW, it fails with ERROR_FILE_NOT_FOUND.
Skipping the leading slash for UNC paths works for both CreateNamedPipeW and WaitNamedPipeW. Resulting in a named pipe with the same name as above that WaitNamedPipeW is able to correctly find.
---
fsmonitor: handle differences between Windows named pipe functions
The two functions involved with creating and checking for the existance of the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear to handle names with leading slashes or backslashes a bit differently.
If a repo resolves to a remote file system with the UNC path of //some-server/some-dir/some-repo, CreateNamedPipeW accepts this name and creates this named pipe: .\pipe\some-server\some-dir\some-repo
However, when the same UNC path is passed to WaitNamedPipeW, it fails with ERROR_FILE_NOT_FOUND.
Skipping the leading slash for UNC paths works for both CreateNamedPipeW and WaitNamedPipeW. Resulting in a named pipe with the same name as above that WaitNamedPipeW is able to correctly find.
v1 differs from v0:
* Abandon hashing repo path in favor of simpler solution where the leading slash is simply dropped for UNC paths
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1503%2Fedecosta-mw%2Ffsmonitor_windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1503/edecosta-mw/fsmonitor_windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1503
Range-diff vs v1:
1: c5307928cd7 ! 1: b41f9bd2e96 fsmonitor: handle differences between Windows named pipe functions
@@ Metadata
## Commit message ##
fsmonitor: handle differences between Windows named pipe functions
- CreateNamedPipeW is perfectly happy accepting pipe names with seemingly
- embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly
- returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created
- with CreateNamedPipeW, exists.
+ The two functions involved with creating and checking for the existance of
+ the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear
+ to handle names with leading slashes or backslashes a bit differently.
- For example, this network path is problemmatic:
- \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
+ If a repo resolves to a remote file system with the UNC path of
+ //some-server/some-dir/some-repo, CreateNamedPipeW accepts this name and
+ creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
- In order to work around this issue, rather than using the path to the
- worktree directly as the name of the pipe, instead use the hash of the
- worktree path.
+ However, when the same UNC path is passed to WaitNamedPipeW, it fails with
+ ERROR_FILE_NOT_FOUND.
+
+ Skipping the leading slash for UNC paths works for both CreateNamedPipeW and
+ WaitNamedPipeW. Resulting in a named pipe with the same name as above that
+ WaitNamedPipeW is able to correctly find.
Signed-off-by: Eric DeCosta <[email protected]>
## compat/simple-ipc/ipc-win32.c ##
-@@
- #include "cache.h"
-+#include "hex.h"
- #include "simple-ipc.h"
- #include "strbuf.h"
- #include "pkt-line.h"
@@
static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)
{
int off = 0;
-- struct strbuf realpath = STRBUF_INIT;
--
-- if (!strbuf_realpath(&realpath, path, 0))
-- return -1;
-+ int ret = 0;
-+ git_SHA_CTX sha1ctx;
-+ struct strbuf real_path = STRBUF_INIT;
-+ struct strbuf pipe_name = STRBUF_INIT;
-+ unsigned char hash[GIT_MAX_RAWSZ];
++ int real_off = 0;
+ struct strbuf realpath = STRBUF_INIT;
-- off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
-- if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
-+ if (!strbuf_realpath(&real_path, path, 0))
+ if (!strbuf_realpath(&realpath, path, 0))
return -1;
-- /* Handle drive prefix */
-- if (wpath[off] && wpath[off + 1] == L':') {
-- wpath[off + 1] = L'_';
-- off += 2;
-- }
-+ git_SHA1_Init(&sha1ctx);
-+ git_SHA1_Update(&sha1ctx, real_path.buf, real_path.len);
-+ git_SHA1_Final(hash, &sha1ctx);
-+ strbuf_release(&real_path);
-
-- for (; wpath[off]; off++)
-- if (wpath[off] == L'/')
-- wpath[off] = L'\\';
-+ strbuf_addf(&pipe_name, "git-fsmonitor-%s", hash_to_hex(hash));
-+ off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
-+ if (xutftowcs(wpath + off, pipe_name.buf, alloc - off) < 0)
-+ ret = -1;
-
-- strbuf_release(&realpath);
-- return 0;
-+ strbuf_release(&pipe_name);
-+ return ret;
- }
++ /* UNC Path, skip leading slash */
++ if (realpath.buf[0] == '/' && realpath.buf[1] == '/')
++ real_off = 1;
++
+ off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
+- if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
++ if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
+ return -1;
- static enum ipc_active_state get_active_state(wchar_t *pipe_path)
+ /* Handle drive prefix */compat/simple-ipc/ipc-win32.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
compat/simple-ipc/ipc-win32.c +6 -1
@@ -19,13 +19,18 @@static int initialize_pipe_name (const char *path,wchar_t *wpath,size_t alloc) {int off =0 ; +int real_off =0 ;struct strbuf realpath =STRBUF_INIT ;if (!strbuf_realpath (&realpath, path,0 ))return -1 ; +/* UNC Path, skip leading slash */ +if (realpath.buf[0 ] =='/' && realpath.buf[1 ] =='/' ) + real_off =1 ; + off =swprintf (wpath, alloc,L"\\\\.\\pipe\\" ); -if (xutftowcs (wpath + off, realpath.buf, alloc - off) <0 ) +if (xutftowcs (wpath + off, realpath.buf + real_off, alloc - off) <0 )return -1 ;/* Handle drive prefix */
base-commit: f285f68a132109c234d93490671c00218066ace9
Signature · 1 line
gitgitgadget
Quoted 181 lines
-----Original Message-----
From: Eric DeCosta via GitGitGadget <[email protected]>
Sent: Monday, April 10, 2023 3:46 PM
To: [email protected]
Cc: Johannes Schindelin <[email protected]>; Jeff Hostetler <[email protected]>; Eric DeCosta <[email protected]>; Eric DeCosta <[email protected]>
Subject: [PATCH v2] fsmonitor: handle differences between Windows named pipe functionsFrom: Eric DeCosta <[email protected]>
The two functions involved with creating and checking for the existance of the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear to handle names with leading slashes or backslashes a bit differently.
If a repo resolves to a remote file system with the UNC path of //some- server/some-dir/some-repo, CreateNamedPipeW accepts this name and creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
However, when the same UNC path is passed to WaitNamedPipeW, it fails with
ERROR_FILE_NOT_FOUND.Skipping the leading slash for UNC paths works for both CreateNamedPipeW and WaitNamedPipeW. Resulting in a named pipe with the same name as above that WaitNamedPipeW is able to correctly find.
Signed-off-by Eric DeCosta <[email protected]>---
fsmonitor: handle differences between Windows named pipe functionsThe two functions involved with creating and checking for the existance of the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear to handle names with leading slashes or backslashes a bit differently.
If a repo resolves to a remote file system with the UNC path of //some- server/some-dir/some-repo, CreateNamedPipeW accepts this name and creates this named pipe: .\pipe\some-server\some-dir\some-repo
However, when the same UNC path is passed to WaitNamedPipeW, it fails with
ERROR_FILE_NOT_FOUND.Skipping the leading slash for UNC paths works for both CreateNamedPipeW and WaitNamedPipeW. Resulting in a named pipe with the same name as above that WaitNamedPipeW is able to correctly find.
v1 differs from v0:
* Abandon hashing repo path in favor of simpler solution where the leading slash is simply dropped for UNC paths
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-
1503%2Fedecosta-mw%2Ffsmonitor_windows-v2 <https://protect-
us.mimecast.com/s/WUMKCqxoXGIDMzRYtE7lUM?domain=github.com>
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git <https://protect-
us.mimecast.com/s/meLwCrkpNJSpB16QhjvzbL?domain=github.com> pr- 1503/edecosta-mw/fsmonitor_windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1503 <https://protect-
us.mimecast.com/s/0_e0Cv2w7NFGk24wH5_RA3?domain=github.com>Range-diff vs v1:
1:
c5307928cd7! 1:b41f9bd2e96fsmonitor: handle differences between Windows named pipe functions @@ Metadata ## Commit message ##
fsmonitor: handle differences between Windows named pipe functions- CreateNamedPipeW is perfectly happy accepting pipe names with seemingly
- embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly
- returnsERROR_FILE_NOT_FOUNDwhen clearly a named pipe, succesfully created
- with CreateNamedPipeW, exists.
+ The two functions involved with creating and checking for the
+ existance of the local fsmonitor named pipe, CratedNamedPipeW and
+ WaitNamedPipeW appear to handle names with leading slashes or backslashes a bit differently.- For example, this network path is problemmatic:
- \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
+ If a repo resolves to a remote file system with the UNC path of
+ //some-server/some-dir/some-repo, CreateNamedPipeW accepts this name
+ and creates this named pipe: \\.\pipe\some-server\some-dir\some-repo- In order to work around this issue, rather than using the path to the
- worktree directly as the name of the pipe, instead use the hash of the
- worktree path.
+ However, when the same UNC path is passed to WaitNamedPipeW, it fails
+ withERROR_FILE_NOT_FOUND.
+
+ Skipping the leading slash for UNC paths works for both
+ CreateNamedPipeW and WaitNamedPipeW. Resulting in a named pipe with
+ the same name as above that WaitNamedPipeW is able to correctly find.Signed-off-by Eric DeCosta <[email protected]>##
compat/simple-ipc/ipc-win32.c##
-@@
- #include "cache.h"
-+#include "hex.h"
- #include "simple-ipc.h"
- #include "strbuf.h"
- #include "pkt-line.h"
@@
static intinitialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc){ int off = 0;
-- struct strbuf realpath =STRBUF_INIT;
--
-- if (!strbuf_realpath(&realpath, path, 0))
-- return -1;
-+ int ret = 0;
-+git_SHA_CTXsha1ctx;
-+ struct strbufreal_path=STRBUF_INIT; struct strbufpipe_name= -+STRBUF_INIT; unsigned char hash[GIT_MAX_RAWSZ];
++ intreal_off= 0;
+ struct strbuf realpath =STRBUF_INIT;-- off =
swprintf(wpath, alloc, L"\\\\.\\pipe\\"); -- if (xutftowcs(wpath + off, realpath.buf, alloc - off)< 0) -+ if (!strbuf_realpath(&real_path, path, 0))
+ if (!strbuf_realpath(&realpath, path, 0))
return -1;-- /* Handle drive prefix */
-- if (wpath[off] && wpath[off + 1] == L':') {
-- wpath[off + 1] = L'_';
-- off += 2;
-- }
-+git_SHA1_Init(&sha1ctx);
-+git_SHA1_Update(&sha1ctx, real_path.buf, real_path.len); -+git_SHA1_Final(hash, &sha1ctx);strbuf_release(&real_path);(quoted diff) +8
-14- -- for (; wpath[off]; off++) -- if (wpath[off] == L'/') -- wpath[off] = L'\\'; -+ strbuf_addf(&pipe_name, "git-fsmonitor-%s", hash_to_hex(hash)); off = -+ swprintf(wpath, alloc, L"\\\\.\\pipe\\"); if (xutftowcs(wpath + off, -+ pipe_name.buf, alloc - off) < 0) ret = -1; - -- strbuf_release(&realpath); -- return 0; -+ strbuf_release(&pipe_name); -+ return ret; - } ++ /* UNC Path, skip leading slash */ ++ if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1; ++ + off = swprintf(wpath, alloc, L"\\\\.\\pipe\\"); +- if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0) ++ if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0) + return -1; - static enum ipc_active_state get_active_state(wchar_t *pipe_path) + /* Handle drive prefix */
compat/simple-ipc/ipc-win32.c| 7 ++++++-1 file changed, 6 insertions(+), 1 deletion(-)compat/simple-ipc/ipc-win32.c +0
-0@@ -19,13 +19,18 @@static int
initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc){ int off = 0;
+ intreal_off= 0;
struct strbuf realpath =STRBUF_INIT;if (!strbuf_realpath(&realpath, path, 0)) return -1;
+ /* UNC Path, skip leading slash */
+ if (realpath.buf[0] == '/' && realpath.buf[1] == '/')real_off= 1; +
off =swprintf(wpath, alloc, L"\\\\.\\pipe\\");
- if (xutftowcs(wpath + off, realpath.buf, alloc - off)< 0)
+ if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off)< 0) return -1;
Quoted 5 lines
/* Handle drive prefix */
base-commit:
f285f68a132109c234d93490671c00218066ace9--
gitgitgadget
Are there any other thoughts about this?
I believe that this is the simplest change possible that will ensure that fsmonitor correctly handles network repos.
-Eric
Quoted 29 lines · Eric DeCosta
Quoted 29 lines
-----Original Message-----
From: Eric DeCosta via GitGitGadget <[email protected]>
Sent: Monday, April 10, 2023 3:46 PM
To: [email protected]
Cc: Johannes Schindelin <[email protected]>; Jeff Hostetler <[email protected]>; Eric DeCosta <[email protected]>; Eric DeCosta <[email protected]>
Subject: [PATCH v2] fsmonitor: handle differences between Windows named pipe functionsFrom: Eric DeCosta <[email protected]>
The two functions involved with creating and checking for the existance of the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear to handle names with leading slashes or backslashes a bit differently.
If a repo resolves to a remote file system with the UNC path of //some- server/some-dir/some-repo, CreateNamedPipeW accepts this name and creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
However, when the same UNC path is passed to WaitNamedPipeW, it fails with
ERROR_FILE_NOT_FOUND.Skipping the leading slash for UNC paths works for both CreateNamedPipeW and WaitNamedPipeW. Resulting in a named pipe with the same name as above that WaitNamedPipeW is able to correctly find.
Signed-off-by Eric DeCosta <[email protected]>
[...]
Quoted 17 lines
Quoted 17 lines
compat/simple-ipc/ipc-win32.c| 7 ++++++-1 file changed, 6 insertions(+), 1 deletion(-)compat/simple-ipc/ipc-win32.c +0
-0@@ -19,13 +19,18 @@static int
initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc){ int off = 0;
+ intreal_off= 0;
struct strbuf realpath =STRBUF_INIT;if (!strbuf_realpath(&realpath, path, 0)) return -1;
Quoted 7 lines
Quoted 7 lines
+ /* UNC Path, skip leading slash */
+ if (realpath.buf[0] == '/' && realpath.buf[1] == '/')real_off= 1; +
off =swprintf(wpath, alloc, L"\\\\.\\pipe\\");
- if (xutftowcs(wpath + off, realpath.buf, alloc - off)< 0)
+ if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off)< 0) return -1;
I haven't had a chance to test this, but this does look like a minimal solution for the pathname confusion in the MSFT APIs.
Do you need to test for realpath.buf[0] and [1] being a forward OR a backslash ?
Should we set real_off to 2 rather than 1 because we already appended a trailing backslash in the swprintf() ?
You should run one of those NPFS directory listing tools to confirm the exact spelling of the pipe matches your expectation here. Yes, if both functions now work, we should be good, but it would be good to confirm your real_off choice, right?
If would be good to (at least interactively) test that the git-for-windows installer can find the path and stop the daemon on an upgrade or uninstall. See Johannes' earlier point.
We should state somewhere that we are running the fsmonitor daemon locally and it is watching a remote file system.
You should run a few stress tests to ensure that the MAX_RDCW_BUF_FALLBACK throttling works and that the daemon doesn't fall behind on a very busy remote file system. (There are SMB/CIFS wire protocol limits. See the source.) (I did test this between the combination of systems that I had, but YMMV.)
During the stress test, it would also be good to test that IO generated by a client process on your local machine to the remote file system is reported, but also that random IO from remote processes on the remote system are seen in the event stream. Again, I tested the combinations of machines that I had available at the time.
Hope this helps,
Jeff
Quoted 13 lines
Quoted 6 lines
/* Handle drive prefix */
base-commit:
f285f68a132109c234d93490671c00218066ace9--
gitgitgadgetAre there any other thoughts about this?
I believe that this is the simplest change possible that will ensure that fsmonitor correctly handles network repos.
-Eric
On 4/26/23 4:33 PM, Jeff Hostetler wrote:
...
Quoted 13 lines
Quoted 8 lines
Quoted 8 lines
+ /* UNC Path, skip leading slash */
+ if (realpath.buf[0] == '/' && realpath.buf[1] == '/')real_off= 1; +
off =swprintf(wpath, alloc, L"\\\\.\\pipe\\");
- if (xutftowcs(wpath + off, realpath.buf, alloc - off)< 0)
+ if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off)< 0) return -1;I haven't had a chance to test this, but this does look like a minimal solution for the pathname confusion in the MSFT APIs.
Quoted 5 lines
Do you need to test for realpath.buf[0] and [1] being a forward OR a backslash ?
Should we set
real_offto 2 rather than 1 because we already appended a trailing backslash in theswprintf()?
On second thought, you might actually need the second slash in //./pipe//server/mount/whatever so that the GfW installer can find remote repo path to CWD and stop the daemon.
Testing will tell here.
Jeff
Quoted 44 lines
You should run one of those NPFS directory listing tools to confirm the exact spelling of the pipe matches your expectation here. Yes, if both functions now work, we should be good, but it would be good to confirm your
real_offchoice, right?If would be good to (at least interactively) test that the git-for-windows installer can find the path and stop the daemon on an upgrade or uninstall. See Johannes' earlier point.
We should state somewhere that we are running the fsmonitor daemon locally and it is watching a remote file system.
You should run a few stress tests to ensure that the
MAX_RDCW_BUF_FALLBACKthrottling works and that the daemon doesn't fall behind on a very busy remote file system. (There are SMB/CIFS wire protocol limits. See the source.) (I did test this between the combination of systems that I had, but YMMV.)During the stress test, it would also be good to test that IO generated by a client process on your local machine to the remote file system is reported, but also that random IO from remote processes on the remote system are seen in the event stream. Again, I tested the combinations of machines that I had available at the time.
Hope this helps,
JeffQuoted 13 lines
Quoted 6 lines
/* Handle drive prefix */
base-commit:
f285f68a132109c234d93490671c00218066ace9Signature · 1 line
gitgitgadgetAre there any other thoughts about this?
I believe that this is the simplest change possible that will ensure that fsmonitor correctly handles network repos.
-Eric
Quoted 77 lines
-----Original Message-----
From: Jeff Hostetler <[email protected]>
Sent: Wednesday, April 26, 2023 4:34 PM
To: Eric DeCosta <[email protected]>; Eric DeCosta via GitGitGadget <[email protected]>; [email protected]
Cc: Johannes Schindelin <[email protected]>
Subject: Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functionsQuoted 9 lines · Eric DeCosta
Quoted 9 lines
-----Original Message-----
From: Eric DeCosta via GitGitGadget <[email protected]>
Sent: Monday, April 10, 2023 3:46 PM
To: [email protected]
Cc: Johannes Schindelin <[email protected]>; Jeff Hostetler <[email protected]>; Eric DeCosta <[email protected]>; Eric DeCosta <[email protected]>
Subject: [PATCH v2] fsmonitor: handle differences between Windowsnamed
Quoted 6 lines
Quoted 6 lines
pipe functions
From: Eric DeCosta <[email protected]>
The two functions involved with creating and checking for the existance of the local fsmonitor named pipe, CratedNamedPipeW and
WaitNamedPipeW
Quoted 1 line
Quoted 1 line
appear to handle names with leading slashes or backslashes a bit
differently.
Quoted 9 lines
Quoted 9 lines
If a repo resolves to a remote file system with the UNC path of //some- server/some-dir/some-repo, CreateNamedPipeW accepts this name and creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
However, when the same UNC path is passed to WaitNamedPipeW, it fails with
ERROR_FILE_NOT_FOUND.Skipping the leading slash for UNC paths works for both
CreateNamedPipeW
Quoted 4 lines
Quoted 4 lines
and WaitNamedPipeW. Resulting in a named pipe with the same name as above that WaitNamedPipeW is able to correctly find.
Signed-off-by Eric DeCosta <[email protected]>[...]
Quoted 6 lines
compat/simple-ipc/ipc-
Quoted 6 lines
compat/simple-ipc/ipc-win32.c| 7 ++++++-1 file changed, 6 insertions(+), 1 deletion(-)compat/simple-ipc/ipc- renamed from compat/simple-ipc/ipc-win32.c +0
-0
win32.cQuoted 11 lines
Quoted 11 lines
index
997f6144344..632b12e1ab5100644compat/simple-ipc/ipc-win32.c +0
-0@@ -19,13 +19,18 @@static int
initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc){ int off = 0;
+ intreal_off= 0;
struct strbuf realpath =STRBUF_INIT;if (!strbuf_realpath(&realpath, path, 0)) return -1;
Quoted 7 lines
Quoted 7 lines
+ /* UNC Path, skip leading slash */
+ if (realpath.buf[0] == '/' && realpath.buf[1] == '/')real_off= 1; +
off =swprintf(wpath, alloc, L"\\\\.\\pipe\\");
- if (xutftowcs(wpath + off, realpath.buf, alloc - off)< 0)
+ if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off)< 0) return -1;I haven't had a chance to test this, but this does look like a minimal solution for the pathname confusion in the MSFT APIs.
Quoted 6 lines
Do you need to test for realpath.buf[0] and [1] being a forward OR a backslash ?
Should we set
real_offto 2 rather than 1 because we already appended a trailing backslash in theswprintf()?
Attempts to add additional backslashes after \\.\pipe\\ are apparently ignored. The name of the local pipe always ends up looking like this:
for UNC paths:
\\.\pipe\\some-server\some-dir\some-repo
and for local paths:
\\.\pipe\\C_\some-dir\some-repo
Thus for a UNC path of //some-server/some-dir/some-repo the simplest thing to do is to skip the first slash.
Quoted 5 lines
You should run one of those NPFS directory listing tools to confirm the exact spelling of the pipe matches your expectation here. Yes, if both functions now work, we should be good, but it would be good to confirm your
real_offchoice, right?
I have used both PowerShell and Process Explorer and see similar results.
Quoted 4 lines
If would be good to (at least interactively) test that the git-for-windows installer can find the path and stop the daemon on an upgrade or uninstall. See Johannes' earlier point.
In regards to the GfW installer, if the daemon is running against a network mounted repo it reports the following:
Could not stop FSMonitor daemon in some-server\some-dir\some-repo (output: , errors: fatal cannot change to 'some-server\some-dir\some-repo':
No such file or directory)
It looks like the installer may have to do something like:
look for "<drive letter>_" immediately after "\\.\pipe\\" and if it does not find it, assume a UNC path.
Quoted 10 lines
We should state somewhere that we are running the fsmonitor daemon locally and it is watching a remote file system.
You should run a few stress tests to ensure that the
MAX_RDCW_BUF_FALLBACKthrottling works and that the daemon doesn't fall behind on a very busy remote file system. (There are SMB/CIFS wire protocol limits. See the source.) (I did test this between the combination of systems that I had, but YMMV.)
Quoted 7 lines
During the stress test, it would also be good to test that IO generated by a client process on your local machine to the remote file system is reported, but also that random IO from remote processes on the remote system are seen in the event stream. Again, I tested the combinations of machines that I had available at the time.
I hear what you are saying, however reporting that information increases the scope of this change. As this stands right now the advertised feature of allowing fsmonitor to work on network-mounted sandboxes for Windows is not working as expected.
-Eric
Quoted 17 lines
Hope this helps,
JeffQuoted 13 lines
Quoted 6 lines
/* Handle drive prefix */
base-commit:
f285f68a132109c234d93490671c00218066ace9--
gitgitgadgetAre there any other thoughts about this?
I believe that this is the simplest change possible that will ensure that fsmonitor correctly handles network repos.
-Eric
Sorry this got lost in my inbox.
Quoted 86 lines · Eric DeCosta
Quoted 77 lines
-----Original Message-----
From: Jeff Hostetler <[email protected]>
Sent: Wednesday, April 26, 2023 4:34 PM
To: Eric DeCosta <[email protected]>; Eric DeCosta via GitGitGadget <[email protected]>; [email protected]
Cc: Johannes Schindelin <[email protected]>
Subject: Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functionsQuoted 9 lines · Eric DeCosta
Quoted 9 lines
-----Original Message-----
From: Eric DeCosta via GitGitGadget <[email protected]>
Sent: Monday, April 10, 2023 3:46 PM
To: [email protected]
Cc: Johannes Schindelin <[email protected]>; Jeff Hostetler <[email protected]>; Eric DeCosta <[email protected]>; Eric DeCosta <[email protected]>
Subject: [PATCH v2] fsmonitor: handle differences between Windowsnamed
Quoted 6 lines
Quoted 6 lines
pipe functions
From: Eric DeCosta <[email protected]>
The two functions involved with creating and checking for the existance of the local fsmonitor named pipe, CratedNamedPipeW and
WaitNamedPipeW
Quoted 1 line
Quoted 1 line
appear to handle names with leading slashes or backslashes a bit
differently.
Quoted 9 lines
Quoted 9 lines
If a repo resolves to a remote file system with the UNC path of //some- server/some-dir/some-repo, CreateNamedPipeW accepts this name and creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
However, when the same UNC path is passed to WaitNamedPipeW, it fails with
ERROR_FILE_NOT_FOUND.Skipping the leading slash for UNC paths works for both
CreateNamedPipeW
Quoted 4 lines
Quoted 4 lines
and WaitNamedPipeW. Resulting in a named pipe with the same name as above that WaitNamedPipeW is able to correctly find.
Signed-off-by Eric DeCosta <[email protected]>[...]
Quoted 6 lines
compat/simple-ipc/ipc-
Quoted 6 lines
compat/simple-ipc/ipc-win32.c| 7 ++++++-1 file changed, 6 insertions(+), 1 deletion(-)compat/simple-ipc/ipc- renamed from compat/simple-ipc/ipc-win32.c +0
-0
win32.cQuoted 11 lines
Quoted 11 lines
index
997f6144344..632b12e1ab5100644compat/simple-ipc/ipc-win32.c +0
-0@@ -19,13 +19,18 @@static int
initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc){ int off = 0;
+ intreal_off= 0;
struct strbuf realpath =STRBUF_INIT;if (!strbuf_realpath(&realpath, path, 0)) return -1;
Quoted 7 lines
Quoted 7 lines
+ /* UNC Path, skip leading slash */
+ if (realpath.buf[0] == '/' && realpath.buf[1] == '/')real_off= 1; +
off =swprintf(wpath, alloc, L"\\\\.\\pipe\\");
- if (xutftowcs(wpath + off, realpath.buf, alloc - off)< 0)
+ if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off)< 0) return -1;I haven't had a chance to test this, but this does look like a minimal solution for the pathname confusion in the MSFT APIs.
Quoted 6 lines
Do you need to test for realpath.buf[0] and [1] being a forward OR a backslash ?
Should we set
real_offto 2 rather than 1 because we already appended a trailing backslash in theswprintf()?Attempts to add additional backslashes after \\.\pipe\\ are apparently ignored. The name of the local pipe always ends up looking like this:
Quoted 8 lines
for UNC paths:
\\.\pipe\\some-server\some-dir\some-repoand for local paths:
\\.\pipe\\C_\some-dir\some-repoThus for a UNC path of //some-server/some-dir/some-repo the simplest thing to do is to skip the first slash.
Ok thanks. Just being paranoid...
Quoted 8 lines
Quoted 6 lines
You should run one of those NPFS directory listing tools to confirm the exact spelling of the pipe matches your expectation here. Yes, if both functions now work, we should be good, but it would be good to confirm your
real_offchoice, right?I have used both PowerShell and Process Explorer and see similar results.
good. thanks.
Quoted 7 lines
Quoted 4 lines
If would be good to (at least interactively) test that the git-for-windows installer can find the path and stop the daemon on an upgrade or uninstall. See Johannes' earlier point.
In regards to the GfW installer, if the daemon is running against a network mounted repo it reports the following:
Quoted 8 lines
Could not stop FSMonitor daemon in some-server\some-dir\some-repo (output: , errors: fatal cannot change to 'some-server\some-dir\some-repo':
No such file or directory)It looks like the installer may have to do something like:
look for "<drive letter>_" immediately after "\\.\pipe\\" and if it does not find it, assume a UNC path.
ok. perhaps you could log an issue on github.com/git-for-windows/git.git to describe this.
Quoted 17 lines
Quoted 10 lines
We should state somewhere that we are running the fsmonitor daemon locally and it is watching a remote file system.
You should run a few stress tests to ensure that the
MAX_RDCW_BUF_FALLBACKthrottling works and that the daemon doesn't fall behind on a very busy remote file system. (There are SMB/CIFS wire protocol limits. See the source.) (I did test this between the combination of systems that I had, but YMMV.)Quoted 7 lines
During the stress test, it would also be good to test that IO generated by a client process on your local machine to the remote file system is reported, but also that random IO from remote processes on the remote system are seen in the event stream. Again, I tested the combinations of machines that I had available at the time.
Quoted 4 lines
I hear what you are saying, however reporting that information increases the scope of this change. As this stands right now the advertised feature of allowing fsmonitor to work on network-mounted sandboxes for Windows is not working as expected.
understood. my only concern was that remotely mounted file systems didn't get a lot of testing. it worked, but i didn't have resources to really stress it. but then again, it if falls behind it will force a resync, so you *shouldn't* get a wrong answer.
Thanks for all your work (and patience with me) on this. Jeff
Quoted 21 lines
-Eric
Quoted 17 lines
Hope this helps,
JeffQuoted 13 lines
Quoted 6 lines
/* Handle drive prefix */
base-commit:
f285f68a132109c234d93490671c00218066ace9--
gitgitgadgetAre there any other thoughts about this?
I believe that this is the simplest change possible that will ensure that fsmonitor correctly handles network repos.
-Eric