| Message ID | pull.1503.git.1679678090412.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fsmonitor: handle differences between Windows named pipe functions | expand |
Hi Eric, On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote: > From: Eric DeCosta <edecosta@mathworks.com> > > 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. 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).
On 3/27/23 7:37 AM, Johannes Schindelin wrote: > Hi Eric, > > On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote: > >> From: Eric DeCosta <edecosta@mathworks.com> >> >> 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. > > 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). Agreed. Please use forward slashes. Thanks, Jeff
Jeff Hostetler <git@jeffhostetler.com> writes: >> 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.
> -----Original Message----- > From: Jeff Hostetler <git@jeffhostetler.com> > Sent: Monday, March 27, 2023 11:02 AM > To: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Eric DeCosta via > GitGitGadget <gitgitgadget@gmail.com> > Cc: git@vger.kernel.org; Eric DeCosta <edecosta@mathworks.com> > Subject: Re: [PATCH] fsmonitor: handle differences between Windows > named pipe functions > > > > On 3/27/23 7:37 AM, Johannes Schindelin wrote: > > Hi Eric, > > > > On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote: > > > >> From: Eric DeCosta <edecosta@mathworks.com> > >> > >> 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. > > > > 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). > > 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
On 4/6/23 3:08 PM, Eric DeCosta wrote: > > >> -----Original Message----- >> From: Jeff Hostetler <git@jeffhostetler.com> >> Sent: Monday, March 27, 2023 11:02 AM >> To: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Eric DeCosta via >> GitGitGadget <gitgitgadget@gmail.com> >> Cc: git@vger.kernel.org; Eric DeCosta <edecosta@mathworks.com> >> Subject: Re: [PATCH] fsmonitor: handle differences between Windows >> named pipe functions >> >> >> >> On 3/27/23 7:37 AM, Johannes Schindelin wrote: >>> Hi Eric, >>> >>> On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote: >>> >>>> From: Eric DeCosta <edecosta@mathworks.com> >>>> >>>> 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. >>> >>> 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). >> >> 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 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
diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c index 20ea7b65e0b..867590abd10 100644 --- a/compat/simple-ipc/ipc-win32.c +++ b/compat/simple-ipc/ipc-win32.c @@ -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)