Fix muxer handling of symlinks on Windows #87717

agocke · 2023-06-17T01:00:54Z

ghost · 2023-06-17T01:01:19Z

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #83314

Author:	agocke
Assignees:	-
Labels:	`area-Host`
Milestone:	-

vitek-karas

My main concern if perf. In 6.0 we intentionally avoided file existence checks for lot of the files, specifically anything from .deps.json. Could you please confirm that this doesn't effectively bring that penalty back? If we call realpath on the assets from .deps.json, that would effectively add back the file existence check. (I don't remember if we call realpath in that case).

vitek-karas · 2023-06-19T09:21:36Z

src/installer/tests/HostActivation.Tests/PortableAppActivation.cs

+                .CaptureStdOut()
+                .CaptureStdErr()
+                .EnvironmentVariable("DOTNET_SKIP_FIRST_TIME_EXPERIENCE", "1")
+                .EnvironmentVariable("DOTNET_MULTILEVEL_LOOKUP", "0"); // Avoid looking at machine state by default


Nit:

Suggested change

.EnvironmentVariable("DOTNET_MULTILEVEL_LOOKUP", "0"); // Avoid looking at machine state by default

.MultilevelLookup(false); // Avoid looking at machine state by default

vitek-karas · 2023-06-19T09:30:16Z

I'd also really like for @elinor-fung to review this as well.

agocke · 2023-06-19T17:44:56Z

This isn't ready yet, just trying to see what breaks if I change the helper.

My basic principles are:

I think the muxer behavior should resolve through symlinks
I think the realpath function in our PAL should behave like the Unix realpath function does.

More broadly, I see a couple possible uses for realpath, including constructing a canonical path string for internal use and constructing a path that will be used for relative file path resolution. For the first one, I don't think it's important to resolve symlinks. If we use realpath for that, we can use any other canonicalization form and it doesn't matter. For the latter, I think it's important to do relative directory resolution from the path of the destination, not the path of the symlink.

Right now I was trying to just change the realpath behavior to the Unix behavior and see what broke on Windows. More broadly, I think it would be reasonable to audit the uses of realpath in the code to try to separate canonicalization from relative directory analysis. The former would not use realpath, while the latter would.

elinor-fung · 2023-06-27T20:24:12Z

Perf would be my concern as well. I believe we skipped the file existence checks for .deps parsing by default by avoiding pal::realpath and pal::file_exists calls, so I don't think the changes here would affect that specific path. However, the implementation of pal::file_exists on Windows just calls pal::realpath, which is now doing additional work to resolve the target. It may make sense to separate those two as part of this change.

realpath is guaranteed to resolve symlinks. fullpath may resolve symlinks. Moves all existing code to call fullpath, which is the existing contract. On Unix, fullpath calls realpath, meaning it resolves symlinks. On Windows it doesn't. Code which requires symlinks to be resolved should be moved to call realpath. realpath is temporarily renamed realpath2 in this commit to find dangling references.

agocke · 2023-07-11T20:08:45Z

I think this is ready for review. I've added a new fullpath function that has the current behavior on Windows and Unix is unchanged.

The new semantics are: fullpath gives you a canonical path, maybe resolving symlinks (undefined). realpath gives you a resolved path, going through symlinks.

I've walked through each call and added a comment specifying exactly which I think is appropriate and why.

agocke · 2023-07-11T20:16:48Z

Also, I think that none of the realpath calls should have any perf concerns (only hitting a small number of files), but I'd like a double check on that.

vitek-karas

I want to make sure we treat this with the right risk awareness. This change modifies the behavior of hostfxr.dll and dotnet.exe (on Windows only, Linux remains the same).

These are global "effectively unversioned" components (we always use latest version). So for example just installing the latest 8 Preview with this change will change the behavior for all .NET apps/invocation on the machine, even those targeting .NET 7 and other official releases.

I do agree that the risk is relatively low as usage of symlinks on Windows is limitted (especially since it requires switching the machine into a dev mode), but still.

I must admit this makes me nervous since we don't have a "backward compat" behavior at all - if this breaks somebody there's no workaround.

I think we should spend some time on figuring out the real potential impact of this change - and if it seems non-trivial, then probably run it through official triage as a real breaking change. The bar on this should be really high due to the global nature of hostfxr/dotnet/

vitek-karas · 2023-07-17T08:24:43Z

src/native/corehost/corehost.cpp

@@ -139,7 +140,7 @@ int exe_start(const int argc, const pal::char_t* argv[])
    {
        trace::info(_X("Detected Single-File app bundle"));
    }
-    else if (!pal::realpath(&app_path))
+    else if (!pal::realpath(&app_path)) // Use realpath to find the app path through symlinks


To properly test this we would need to setup two symlinks. The executable can be a symlink, we should look for the app.dll next to the resolved path (after resolving the symlink in host_path) and then the app.dll could also be a symlink, which we will resolve here.

It's an interesting question if we should actually behave like this. We will use the app_path for everything else (.deps.json, .runtimeconfig.json, app base path, ...).
I guess we want to keep working this way since we've done it forever on Linux - and so making Windows match Linux in this case makes a lot of sense (since previous to this change Windows symlink support was messy).

vitek-karas · 2023-07-17T09:13:15Z

src/native/corehost/fxr/hostfxr.cpp

@@ -545,6 +545,7 @@ namespace

        if (startup_info.host_path.empty())
        {
+            // Use realpath to find the host_path behind symlinks


Should we also "realpath" the host_path if it is specified by the caller? This comes from custom host code (so external input).
I think for consistency it would make sense to follow symlinks everywhere, but it is technically a breaking change - and in hostfxr :-(

vitek-karas · 2023-07-17T09:16:36Z

src/native/corehost/hostmisc/pal.windows.cpp

-bool pal::realpath(string_t* path, bool skip_error_logging)
+typedef std::unique_ptr<std::remove_pointer<HANDLE>::type, decltype(&::CloseHandle)> SmartHandle;
+
+// Like realpath, but resolves symlinks.


Should it be?

Suggested change

// Like realpath, but resolves symlinks.

// Like fullpath, but resolves symlinks.

vitek-karas · 2023-07-17T09:31:23Z

src/native/corehost/hostmisc/pal.windows.cpp

+            // Remove the \\?\ prefix, unless it is necessary or was already there
+            if (LongFile::IsExtended(str) && !LongFile::IsExtended(*path) &&
+                !LongFile::ShouldNormalize(str.substr(LongFile::ExtendedPrefix.size())))
+            {
+                str.erase(0, LongFile::ExtendedPrefix.size());
+            }


Just an observation:
According to the docs the first check (LongFile::IsExtended(str)) will pretty much always return true, since the GetFinalPathNameByHandleW is supposed to return the \\?\ prefixed path. The second check that the original path is not extended is also going to be true almost always. So we will end up calling ShouldNormalize and very likely erase below as well.

Can we try to make this somehow cheaper? (or maybe it's not worth it) - this will likely allocate twice (substr and erase) which makes this method allocate at least 3 times (the last is the str.assign above).

Maybe I'm just too careful and it's not worth the complexity to optimize this.

Might be worth adding a comment that the Win32 API call will typically return \\?\ prefixed path.

vitek-karas · 2023-07-17T09:34:05Z

src/native/corehost/hostpolicy/args.cpp

+    // We use realpath here because we want the final path through symlinks for the app_root.
+    if (pal::fullpath(&args.managed_application))


The comment and code disagree here.
I think we want realpath in code as well.

Apparently we're missing a test for this case.

vitek-karas · 2023-07-17T09:35:50Z

src/installer/tests/HostActivation.Tests/PortableAppActivation.cs

@@ -20,6 +20,31 @@ public PortableAppActivation(SharedTestState fixture)
            sharedTestState = fixture;
        }

+        [Fact]


Ideally we would add tests for all use cases of realpath in the code.

vitek-karas · 2023-07-17T09:38:42Z

src/native/corehost/fxr/command_line.cpp

            app_candidate = host_info.app_path;
+            // Use realpath since we want the path to the app through symlinks
            doesAppExist = bundle::info_t::is_single_file_bundle() || pal::realpath(&app_candidate);


Maybe not in this PR, but it would be worth looking into this a bit higher level. For example in this case, the host_info.app_path might already be after symlink resolution (in fact it typically will be, since if you run apphost.exe we will populate this as resolved via the code in corehost which also uses realpath).

But there are other places which initialize the host_info.app_path to paths which are unresolved yet.

Would be nice to have a principle that it either is guaranteed resolved, or not (in which case the callers don't try to resolve at all).

vitek-karas · 2023-07-17T09:39:58Z

src/native/corehost/hostmisc/pal.unix.cpp

+bool pal::fullpath(pal::string_t* path, bool skip_error_logging)
+{
+    return realpath(path, skip_error_logging);
+}
+
 bool pal::realpath(pal::string_t* path, bool skip_error_logging)


I think this deserves a comment - especially since the implementation is "the other way round" from Windows.

vitek-karas · 2023-07-17T09:40:39Z

src/native/corehost/hostmisc/pal.h

    bool realpath(string_t* path, bool skip_error_logging = false);
+    bool fullpath(string_t* path, bool skip_error_logging = false);


Please add a comment what is the difference between the two - the naming is Unix based, but Unix doesn't have fullpath, so it's unclear what that does.

agocke · 2023-07-18T02:52:23Z

Agreed on testing and impact -- I'll add more testing to validate these changes.

agocke · 2023-07-18T02:55:44Z

@vitek-karas Do you think it's worth scoping this change down to just the muxer, because of the risk?

vitek-karas · 2023-07-18T07:27:34Z

@vitek-karas Do you think it's worth scoping this change down to just the muxer, because of the risk?

That would help only somewhat. I guess we could try to avoid doing the change in hostfxr, so that would cut the impact by more than half. But muxer is also global/non-versioned and so any change there will impact everything on the machine (which uses the muxer).

In general I'm in favor of this change. If nothing else, it brings Windows and Linux closer in behavior. I just want us to be careful, that's all.

Maybe we should keep this for now and merge it first thing into .NET 9, so that we have an entire year of previews to validate (and also .NET 8 is LTS...). The global nature of muxer/hostfxr means that if somebody wants these changes, installing .NET 9 Preview 1 will be enough to "fix it" for everything.

agocke · 2023-07-18T21:29:25Z

Sounds good to me.

stephentoub · 2024-07-22T13:17:53Z

@agocke, this hasn't been touched in a year. Should it be closed? Are you still working on it?

agocke · 2024-08-02T21:24:08Z

Closing out, I don't think we need to take this until we get customer feedback on other parts that don't work behind symlinks.

dotnet-issue-labeler bot added the area-Host label Jun 17, 2023

ghost assigned agocke Jun 17, 2023

This was referenced Jun 18, 2023

Could not load file or assembly 'Microsoft.CodeAnalysis.NetAnalyzers #84995

Closed

Unable to load Analyzer assembly .../Microsoft.CodeAnalysis.Analyzers.dll : Not a valid assembly #85082

Closed

Work item tests crash on OSX x64 Debug #87287

Closed

vitek-karas reviewed Jun 19, 2023

View reviewed changes

runfoapp bot mentioned this pull request Jun 19, 2023

Infra improvements for Helix #68176

Closed

Add test and copy implementation to realpath2

c57c6fb

agocke force-pushed the muxer-symlink branch from 9a2b79c to c57c6fb Compare July 7, 2023 22:11

agocke and others added 4 commits July 7, 2023 15:34

Remove realpath2 from pal

2ec2e6b

pal prefix

392ab46

Examine each use of fullpath and assess whether it should be realpath

cb0a3d9

build-analysis bot mentioned this pull request Jul 11, 2023

simpleruntimeeventvalidation test failing in CI #88499

Closed

Use fullpath for directories

6bd2cf8

This was referenced Jul 11, 2023

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

Test failure readytorun/HardwareIntrinsics/X86/CpuId_R2R_Avx/CpuId_R2R_Avx.sh #88582

Closed

agocke marked this pull request as ready for review July 11, 2023 20:08

vitek-karas reviewed Jul 17, 2023

View reviewed changes

agocke mentioned this pull request Mar 12, 2024

Simplified muxer fix #99576

Merged

agocke closed this Aug 2, 2024

agocke deleted the muxer-symlink branch August 2, 2024 21:24

github-actions bot locked and limited conversation to collaborators Sep 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix muxer handling of symlinks on Windows #87717

Fix muxer handling of symlinks on Windows #87717

agocke commented Jun 17, 2023

ghost commented Jun 17, 2023

vitek-karas left a comment

vitek-karas Jun 19, 2023

vitek-karas commented Jun 19, 2023

agocke commented Jun 19, 2023

elinor-fung commented Jun 27, 2023

agocke commented Jul 11, 2023

agocke commented Jul 11, 2023

vitek-karas left a comment

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

vitek-karas Jul 17, 2023

agocke commented Jul 18, 2023

agocke commented Jul 18, 2023

vitek-karas commented Jul 18, 2023

agocke commented Jul 18, 2023

stephentoub commented Jul 22, 2024

agocke commented Aug 2, 2024

	.EnvironmentVariable("DOTNET_MULTILEVEL_LOOKUP", "0"); // Avoid looking at machine state by default
	.MultilevelLookup(false); // Avoid looking at machine state by default

	// Like realpath, but resolves symlinks.
	// Like fullpath, but resolves symlinks.

		// We use realpath here because we want the final path through symlinks for the app_root.
		if (pal::fullpath(&args.managed_application))

		bool realpath(string_t* path, bool skip_error_logging = false);
		bool fullpath(string_t* path, bool skip_error_logging = false);

Fix muxer handling of symlinks on Windows #87717

Fix muxer handling of symlinks on Windows #87717

Conversation

agocke commented Jun 17, 2023

ghost commented Jun 17, 2023

vitek-karas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitek-karas commented Jun 19, 2023

agocke commented Jun 19, 2023

elinor-fung commented Jun 27, 2023

agocke commented Jul 11, 2023

agocke commented Jul 11, 2023

vitek-karas 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented Jul 18, 2023

agocke commented Jul 18, 2023

vitek-karas commented Jul 18, 2023

agocke commented Jul 18, 2023

stephentoub commented Jul 22, 2024

agocke commented Aug 2, 2024