RE: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions

[Eric DeCosta <edecosta@mathworks.com>]

> -----Original Message-----
> From: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>
> Sent: Monday, April 10, 2023 3:46 PM
> To: git@vger.kernel.org
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Jeff Hostetler
> <git@jeffhostetler.com>; Eric DeCosta <edecosta@mathworks.com>; Eric
> DeCosta <edecosta@mathworks.com>
> Subject: [PATCH v2] fsmonitor: handle differences between Windows named
> pipe functions
> 
> From: Eric DeCosta <edecosta@mathworks.com>
> 
> 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 <edecosta@mathworks.com>
> ---
> 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 <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: 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 <edecosta@mathworks.com>
> 
> ## 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(-)
> 
> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
> index 997f6144344..632b12e1ab5 100644
> --- a/compat/simple-ipc/ipc-win32.c
> +++ b/compat/simple-ipc/ipc-win32.c
> @@ -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
> --
> 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