GVFSService: platforms should use appropriate pipe name by jamill · Pull Request #1155 · microsoft/VFSForGit

jamill · 2019-05-14T14:03:26Z

On POSIX platforms the pipe name should be a rooted path so that we have
full control over the pipe path. If a relative path is used, then the
pipe is created in a temporary dictory, which can be a different
location depending on the user's context (i.e. running as root vs normal
user). This can result in an process running as root not being able to
communicate with the GVFS Service.

This change enables the different platforms to create a pipe path that
is suitable for the platform, and encapsulates the logic for creating
the pipe name for client / server in a single location.

alameenshah · 2019-05-14T14:26:57Z


+        public override string GetGVFSServiceNamedPipeName()
+        {
+            return GVFSConstants.Service.ServiceName + ".pipe";


Question - On POSIXPlatform.cs we are dynamically getting the root directory path for Service and constructing the pipe name by appending .pipe string to it. But here on Windows, we are using a fixed <service_name>.pipe string. Why is this different between platforms - where does the pipe file get created on Windows.

where does the pipe file get created on Windows.

On Windows pipes are not files, but exist in a special namespace for Pipes: \\.\pipe\<NAME>

https://docs.microsoft.com/en-us/windows/desktop/ipc/pipe-names

Since in *NIX pipes exist in the unified file system namespace they need to go somewhere under /.
A pipe with a simple name gets created directly under Path.GetTempPath() with "CoreFXPipe_" prefix.

See here for more information.

alameenshah · 2019-05-14T14:48:00Z

+        public GVFSService(
+            ITracer tracer,
+            string serviceName,
+            IRepoRegistry repoRegistry)


Are these changes caused by File Encoding differences between Windows and Mac? This has become a common issue while editing cross-plat files.

yes... I have wondered if we could help solve this with a setting in editorconfig. I think the expectation is that all lines are crlf terminated, but that gets mixed up...

Setting up the .editorconfig to enforce line endings and normalizing the files that are currently mixed has been on my backlog of things I wanted to do for a long time. I think the difficult part there is validating that it behaves as expected in all the IDEs we use.

wilbaker · 2019-05-14T16:39:07Z

-            {
-                return this.ServiceName + ".Pipe";
+            {
+                return GVFSPlatform.Instance.GetGVFSServiceNamedPipeName();


I suspect the change from this.ServiceName + ".Pipe" to GVFSPlatform.Instance.GetGVFSServiceNamedPipeName will break some things on Windows because during the functionals tests the test service's pipe is named Test.GVFS.Service.

Perhaps GetGVFSServiceNamedPipeName should take a service name as a parameter?

I missed the test service! thanks. I will update to take in the service name.

wilbaker · 2019-05-14T16:43:04Z

+        {
+            // Pipes are stored as files on POSIX, use a rooted pipe name
+            // in the same location as the service to keep full control of the location of the file
+            return this.GetDataRootForGVFSComponent(GVFSConstants.Service.ServiceName) + ".pipe";


Not your change, and looking for your opinion.

I've had some trouble remembering what GetDataRootForGVFSComponent returns (and if it's a global path).

Do you think this should be renamed (perhaps to something like GetGlobalDataRootForGVFSComponent)?

It looks like in the git world "global" means per-user, and so that would seem to be consistent.

It looks like in the git world "global" means per-user, and so that would seem to be consistent.

I guess except on Windows GetDataRootForGVFSComponent returns something system wide ... maybe there is a better name.

I wonder if GetDataDirectoryForService, GetDataDirectoryForFeature would be easier... but I am not sure... Also - what is global on Windows is not currently global on macOS...

On POSIX platforms the pipe name should be a rooted path so that we have full control over the pipe path. If a relative path is used, then the pipe is created in a temporary dictory, which can be a different location depending on the user's context (i.e. running as root vs normal user). This can result in an process running as root not being able to communicate with the GVFS Service. This change enables the different platforms to create a pipe path that is suitable for the platform, and encapsulates the logic for creating the pipe name for client / server in a single location.

chrisd8088 · 2019-05-14T18:06:59Z

This is not immediately relevant to this PR, but the changes here reminded me to note that the current code which generates named piped names on POSIX platforms (Mac, Linux) does not constrain them to the limits imposed on domain sockets of about 100 characters.

One can generate a stack trace like the following by simply mixing up the order of arguments on the gvfs clone command line:

Unhandled Exception: System.ArgumentOutOfRangeException: The path '.../.gvfs/GVFS_NetCorePipe' is of an invalid length for use with domain sockets on this platform.  The length must be between 1 and 108 characters, inclusive.
...
   at System.IO.Pipes.NamedPipeClientStream.Connect(Int32 timeout)
   at GVFS.Common.NamedPipes.NamedPipeClient.Connect(Int32 timeoutMilliseconds) in .../VFSForGit/GVFS/GVFS.Common/NamedPipes/NamedPipeClient.cs:line 29
   at GVFS.CommandLine.GVFSVerb.IsExistingPipeListening(String enlistmentRoot) in .../VFSForGit/GVFS/GVFS/CommandLine/GVFSVerb.cs:line 360
   at GVFS.CommandLine.CloneVerb.CheckNotInsideExistingRepo(String normalizedEnlistmentRootPath)
...

I could put this into an issue if it's not something you'd like to address in this PR, but I thought I'd mention it here because creating pipe names using absolute paths may also sometimes trigger this exception, depending on the path.

jamill · 2019-05-14T18:24:38Z

I could put this into an issue if it's not something you'd like to address in this PR, but I thought I'd mention it here because creating pipe names using absolute paths may also sometimes trigger this exception, depending on the path.

@chrisd8088 - Thanks for pointing this out! If I understand the issue correctly, I think we are most susceptible to this in the pipe for the mount process, which depends on the enlistment path. I think the paths for the GVFS Service's named pipes are more constrained / controlled, as they do not depend on a mount's directory.

Let's file a separate issue for this - would you mind filing it? If not, I can go ahead and file it to make sure we track it.

nickgra · 2019-05-15T00:05:51Z

+        public GVFSService(
+            ITracer tracer,
+            string serviceName,
+            IRepoRegistry repoRegistry)


Setting up the .editorconfig to enforce line endings and normalizing the files that are currently mixed has been on my backlog of things I wanted to do for a long time. I think the difficult part there is validating that it behaves as expected in all the IDEs we use.

jamill requested review from alameenshah, mjcheetham and wilbaker May 14, 2019 14:03

jamill mentioned this pull request May 14, 2019

Upgrader for macOS MVP #1129

Closed

15 tasks

alameenshah reviewed May 14, 2019

View reviewed changes

wilbaker reviewed May 14, 2019

View reviewed changes

jamill force-pushed the upgrader_tasks/rooted_pipe branch from 67849c3 to 2bb6048 Compare May 14, 2019 17:44

nickgra approved these changes May 15, 2019

View reviewed changes

wilbaker approved these changes May 15, 2019

View reviewed changes

jamill merged commit 4e5a493 into microsoft:master May 16, 2019

wilbaker added platform: linux platform: macOS labels May 20, 2019

jrbriggs added this to the M153 milestone May 24, 2019

Conversation

jamill commented May 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjcheetham May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisd8088 commented May 14, 2019

Uh oh!

jamill commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mjcheetham May 14, 2019 •

edited

Loading

jamill commented May 14, 2019 •

edited

Loading