Fix problems with application path and installation directory resolution #951

mjcheetham · 2022-11-07T18:54:14Z

When we introduced the rename warning in #551, there was broken logic for resolving the actual invoked program name on Mac and Linux.

Windows
We continue to use CommandLineToArgvW(GetCommandLine(), ..) to return the absolute, full path to the entry executable.

macOS
Switch from _NSGetArgv() to _NSGetExecutablePath(..) which, like the Windows APIs above, returns the full path to the entry executable (or symlink).

Linux
As there is no equivalent to CommandLineToArgvW or _NSGetExecutablePath here, we instead do some path computation based on the form of argv[0] that we get from /proc/self/cmdline.

If the value is an absolute path, just use that.
If the value is relative to the current directory (./name) then combine this with the current directory.
If the value contains a directory separator (dir/name) then also resolve this from the current directory.
Otherwise, this argv[0] value must have been a file name (name) resolved from the $PATH.

If we still don't manage to resolve from the $PATH, try and resolve the symlink /proc/self/exe that points to the executable image that was loaded from disk. Note that we may miss any intermediate link names here, but it's better than nothing.

In addition, we also never tested the .NET Tool scenario after updating the ICommandContext.ApplicationPath to use the entry executable name, whereas previously this was computed from the .NET assembly path.

Introduce a separate concept, the InstallationDirectory that always points to the home of the core assemblies. This is used for resolving things like in-box UI helpers.

dscho

Looks good!

I just left a bit of feedback that is hopefully helpful and constructive.

src/shared/Core/Interop/Linux/LinuxFileSystem.cs

dscho · 2022-11-07T19:16:56Z

src/shared/Core/Interop/Windows/WindowsFileSystem.cs

+            b = Path.GetFullPath(b);
+
+            // Note: we do not resolve or handle symlinks on Windows
+            // because they require administrator permissions to even create!


That's not true on Windows 10 in Developer Mode, though.

But until there's enough of the world on a version of Windows where you don't need admin to create a symlink, then it's not worth our time supporting symlinks at all in Windows, IMO.

We do not have any reliable indicator in Git for Windows as to how many users run in Developer Mode and use symlinks, but there have been enough complaints that we had to implement patches to support that (git-for-windows/git@9086edc, git-for-windows/build-extra@e6085cb, just to name two).

So I do not believe that the fraction of symlink/Developer Mode users is actually small.

But how many people are going to be installing and configuring standalone GCM via a symlink? :-)

Thanks for the context however on symlink usage for G4W 👍

dscho · 2022-11-07T19:22:49Z

src/shared/Core/PlatformUtils.cs

+            // (how we were launched), so let's now try to extract the
+            // fully resolved executable path from /proc/self/exe.
+            // Note that this means we may miss if we've been invoked
+            // via a symlink, but it's better than nothing at this point!


I'll play devil's advocate here for a moment: why do we need the absolute path? Is the purpose of this functionality not to determine whether the program was called via the git-credential-manager-core symlink?

If that is the case, using /proc/self/exe won't give us what we need because it already points to the resolved path of the actual executable, not to the symlink.

And if that is the case, we actually do not need the absolute path at all, right? If we see that argv[0] is git-credential-manager-core (without any slash, i.e. not an absolute nor a relative path), we know that we want to warn, right?

I'll play devil's advocate here for a moment: why do we need the absolute path? Is the purpose of this functionality not to determine whether the program was called via the git-credential-manager-core symlink?

The path here is used for two things.. one is indeed the -core rename warning, but the other, more important one is for the git-credential-manager configure command.
We want to use the invoked name (absolute) for the credential.helper value.

Okay, that makes sense. I do suspect, though, that we need to look at argv[0] instead of /proc/self/exe for that -core rename warning.

Indeed we will. But since this is just a warning, and there may be some cases where argv[0] is missing/empty, we can fall-back on /proc/self/exe to get a valid(*) path to the real executable for the more important (un)configure command usage.

(*) unless someone deletes us before we read this link of course 🤡

src/shared/Core/PlatformUtils.cs

Replace netstandard2.0 where possible with net6.0, or dual target net6.0 and net472 for projects that are included on Windows.

ldennington

Great work! ⭐ This was really nice to review - the code is very clean and easy to follow.

src/shared/Core.Tests/Interop/Windows/WindowsFileSystemTests.cs

src/shared/Core/Interop/Posix/Native/Unistd.cs

There is a systematic bug in all the IFileSystem implementations in that we were incorrectly calling `Path.GetFileName` rather than `Path.GetFullPath`, meaning we're only comparing the file _names_ of two paths for equality! Fix this. For Mac and Linux we also handle symlinks (or firmlinks) by now using the .NET APIs to resolve links. On .NET Framework we don't do anything.

Introduce a new property on the ICommandContext that represents the GCM installation directory, separate from the application executable path. The app executable is not always found in the installation directory if the app exe path is a symlink (or we're a .NET tool where the AppHost is outside of the tool store).

Our previous implementation of getting the native argv[0] was flawed on Linux and Mac. The Mac `_NSArgv` and Linux `/proc/self/cmdline` function/proc-file return the raw argv[0] which may not be an absolute or rooted path, but we expected this to be the same. Additionally, the logic of appending the CWD to the argv[0] value when the latter wasn't rooted was also just wrong! Update to now use `_NSExecutablePath` on macOS, which always returns the full absolute path, and add correct handling of unrooted argv[0] on Linux. Furthermore, on Linux, if we are unable to handle the value from /proc/self/cmdline, we fall back to resolving the /proc/self/exe symlink instead.

Moving to target .NET 6 directly with the core library has unearthed a bug/warning in the secret service implementation. IntPtr is a value type, so it can never be null.. we should have been comparing this pointer to `IntPtr.Zero`.

Changes: - Convert issue templates to YAML (24ae90a) - Replace netstandard2.0 with dotnet6.0 (65cead2) - Ensure correct installation directory resolution for all OSes and distribution methods (#951) - Align with dotnet-supported Linux distributions (#953)

mjcheetham added the bug A bug in Git Credential Manager label Nov 7, 2022

mjcheetham requested a review from ldennington November 7, 2022 18:54

dscho approved these changes Nov 7, 2022

View reviewed changes

build: replace netstandard2.0 TFM with net6.0

65cead2

Replace netstandard2.0 where possible with net6.0, or dual target net6.0 and net472 for projects that are included on Windows.

mjcheetham force-pushed the fix-apppath branch 3 times, most recently from fac3f47 to b375ff6 Compare November 7, 2022 21:00

ldennington approved these changes Nov 8, 2022

View reviewed changes

src/shared/Core.Tests/Interop/Windows/WindowsFileSystemTests.cs Show resolved Hide resolved

src/shared/Core/Interop/Posix/Native/Unistd.cs Outdated Show resolved Hide resolved

mjcheetham added 4 commits November 8, 2022 09:54

libsecret: fix warning with move to net6.0 TFM

322ef98

Moving to target .NET 6 directly with the core library has unearthed a bug/warning in the secret service implementation. IntPtr is a value type, so it can never be null.. we should have been comparing this pointer to `IntPtr.Zero`.

mjcheetham force-pushed the fix-apppath branch from b375ff6 to 322ef98 Compare November 8, 2022 17:54

mjcheetham merged commit df9a165 into git-ecosystem:main Nov 8, 2022

mjcheetham deleted the fix-apppath branch November 8, 2022 19:35

ldennington mentioned this pull request Nov 16, 2022

Release 2.0.877 #961

Merged

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix problems with application path and installation directory resolution #951

Fix problems with application path and installation directory resolution #951

mjcheetham commented Nov 7, 2022

dscho left a comment

dscho Nov 7, 2022

mjcheetham Nov 7, 2022

dscho Nov 8, 2022

mjcheetham Nov 8, 2022

mjcheetham Nov 8, 2022

dscho Nov 7, 2022

mjcheetham Nov 7, 2022

dscho Nov 8, 2022

mjcheetham Nov 8, 2022

ldennington left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Fix problems with application path and installation directory resolution #951

Fix problems with application path and installation directory resolution #951

Conversation

mjcheetham commented Nov 7, 2022

dscho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldennington left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.