Add internal AppExecLink support to link APIs #58233

carlossanlop · 2021-08-27T01:50:37Z

This is a continuation of #57996

AppExecLinks, also known as Application Execution Aliases, are a Windows-specific kind of symbolic link for Windows Apps. It's a scenario we should support when resolving links with our FileSystemInfo APIs or the static File/Directory APIs.

AppExecLinks are an explicitly supported scenario by PowerShell, along with mount points. Our APIs should handle these types of links as well, to facilitate the task of the PowerShell team to port their code to use these new link APIs.

ghost · 2021-08-27T01:50:44Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

DRAFT: This is a continuation of #57996 , which is still open, so this PR will contain those changes as well, until it gets merged.

AppExecLinks, also known as Application Execution Aliases, are a Windows-specific kind of symbolic link for Windows Apps. It's a scenario we should support when resolving links with our FileSystemInfo APIs or the static File/Directory APIs.

AppExecLinks are an explicitly supported scenario by PowerShell, along with mount points. Our APIs should handle these types of links as well, to facilitate the task of the PowerShell team to port their code to use these new link APIs.

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-System.IO`
Milestone:	6.0.0

src/libraries/Common/src/System/IO/FileSystem.Attributes.Windows.cs

jozkee · 2021-08-27T02:28:50Z

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

+                        string windowsAppsDir = Path.Join(localAppData, "Microsoft", "WindowsApps");
+                        if (Directory.Exists(windowsAppsDir))
+                        {
+                            return Directory.GetFiles(windowsAppsDir, "*.exe", new EnumerationOptions() { RecurseSubdirectories = true, MaxRecursionDepth = 3 }).Length > 0;


It is granted that %LOCALAPPDATA%/Microsoft/WindowsApps will always contain an AppExecLink?
For extra safety we could add a filter and ensure that at least one of the files is a ReparsePoint.

Suggested change

return Directory.GetFiles(windowsAppsDir, "*.exe", new EnumerationOptions() { RecurseSubdirectories = true, MaxRecursionDepth = 3 }).Length > 0;

new FileSystemEnumerable<string?>(

windowsAppsDir,

(ref FileSystemEntry entry) => null,

new EnumerationOptions { RecurseSubdirectories = true })

{

ShouldIncludePredicate = (ref FileSystemEntry entry) =>

FileSystemName.MatchesWin32Expression("*.exe", entry.FileName) &&

(entry.Attributes & FileAttributes.ReparsePoint) != 0

}.Any();

It's not guaranteed, hence why I added this platform detection check.
The extra ReparsePoint check makes sense. I'll add it.

Let's hope this PlatformDetection change does not cause build failure if the used APIs are not available in netstandard.

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs

jozkee · 2021-08-27T02:44:22Z

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs

@@ -557,7 +594,8 @@ internal static void CreateSymbolicLink(string path, string pathToTarget, bool i
            if ((data.dwFileAttributes & (uint)FileAttributes.ReparsePoint) == 0 ||
                // Only symbolic links and mount points are supported at the moment.


Suggested change

// Only symbolic links and mount points are supported at the moment.

// Only symbolic links, mount points and windows apps are supported at the moment.

jozkee · 2021-08-27T03:01:39Z

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs

+                    Debug.Assert(success);
+
+                    // The target file is at index 2
+                    if (rbAppExecLink.StringCount >= 3)


This SO answer says that this field is actually named Version https://stackoverflow.com/a/65583702/4231460. It may be a good idea to ask the Windows team what's the exact definition of this struct and validate if we are using it correctly.

I based this on the PowerShell code:
https://github.com/PowerShell/PowerShell/blob/56d22bc3865fca445145fd685d6d6f6d63194701/src/System.Management.Automation/namespaces/FileSystemProvider.cs#L8512

From our conversation with the Windows team, this code is doing the correct job.

Maybe show an example in a comment here, to explain the mysterious magic number.
We want to only use documented Windows API - but I see this is documented https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/c3a420cb-8a72-4adf-87e8-eee95379d78f

or just "see documentation for REPARSE_DATA_BUFFER"

PowerShell team found the const in Windows SDK so it is public and the fact opened a way to use the const.

@iSazonov can you share a link to the public Windows SDK location where you found it?

I'm not sure such a link would qualify as "public documentation" though.

@carlossanlop If I remember right @SteveL-MSFT found this in Windows SDK files (I guess in *.cpp).

I don't remember where exactly I got the original buffer struct from, but given the name, I think most likely from the SO post. Since I can't find any public docs on the struct, it should probably not be relied upon as it could change. I'll start an internal thread with the AppX team to see about getting it documented publicly.

I found where we discussed this PowerShell/PowerShell#10331 (comment) - there is a link to official Microsoft example code :-)
https://github.com/microsoft/Windows-driver-samples/blob/82bd522bb3b893e145d6d73f930edfc04977e4de/filesys/miniFilter/minispy/user/mspyLog.c#L70

… an AppExecLinks directory.

danmoseley · 2021-08-27T21:57:09Z

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs

@@ -539,13 +539,59 @@ internal static void CreateSymbolicLink(string path, string pathToTarget, bool i
                    Debug.Assert(!PathInternal.IsPartiallyQualified(targetPath));
                    return targetPath.ToString();
                }
+                else if (rbSymlink.ReparseTag == Interop.Kernel32.IOReparseOptions.IO_REPARSE_TAG_APPEXECLINK)


there's a long list of IO_REPARSE_TAG_* .. how is this one special?
https://docs.microsoft.com/en-us/windows/win32/fileio/reparse-point-tags

It is one of the reparse tags proven to point to another file/folder and it is also one of the three tags covered by PowerShell: https://github.com/PowerShell/PowerShell/blob/008f4b057fdc1482d3200de0a61d23570b4d30e4/src/System.Management.Automation/namespaces/FileSystemProvider.cs#L8217-L8230

I am uncertain if all the reparse tags in the link you posted indicate that the reparse point contains a reference to another file/folder, but having these 3 tags (Symlinks, MountPoints and AppExecLinks) seems to me like a good start.

Yup fair enough, if Powershell has had this support for a while, that's good evidence those are the ones that matter.

how is this one special?

It is unnormal reparse point :-) so we had to add it explicitly in PowerShell.

In common, reparse point list is "open" and if new reparse points will be added in future perhaps we will have to add its explicitly too.

I'd remind about OneDrive. .Net API should be tested for OneDrive paths/links too (at list manually).

I manually tested enumeration of the OneDrive directory (ensuring there were cloud files) and the behavior was the same in 3.1, 5.0 and 6.0 (including the changes in this PR): I can read the file name and the file attributes, and the files are not getting unexpectedly downloaded - their cloud icon stays next to the files when viewing them in the File Explorer.

@iSazonov by any chance, do you know any specific scenarios in which the OneDrive files get unexpectedly downloaded when manipulating them with .NET? I am not sure if you've mentioned this strange behavior in the past, or if it was someone else. It would be a good chance for me to verify it.

@carlossanlop These links are related OneDrive and could be interesting for you:
PowerShell/PowerShell#9895
PowerShell/PowerShell#9509
PowerShell/PowerShell#8745

Also for removing PowerShell/PowerShell#15571

Feel free to ask me if you need additional comments.

src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs

danmoseley · 2021-08-27T22:27:21Z

This is presumably a Windows 8 or Windows 10 feature. Will there be any problems on Windows 7? I assume not, it just won't hit any.

carlossanlop · 2021-08-30T17:36:09Z

Had a conversation with Dan about this issue and it doesn't meet the bar for a 6.0 backport.

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

Add CANT_ACCESS_FILE inside IsPathUnreachableError. Add comment to that method to avoid catching ACCESS_DENIED, SHARING_VIOLATION, SEM_TIMEOUT.

carlossanlop · 2021-08-31T00:13:04Z

This is presumably a Windows 8 or Windows 10 feature. Will there be any problems on Windows 7? I assume not, it just won't hit any.

Execution Aliases for UWP applications were introduced in Windows 10 Fall Creators Update (1709/RS3, build 16299). Windows versions before that should not have appexeclink files in their system. If they do, the scenario would be unsupported on the Windows side, so either OpenSafeFileHandle or DeviceIoControl would fail and we would handle the error.

iSazonov · 2021-08-31T03:28:48Z

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.REPARSE_DATA_BUFFER.cs

+        [StructLayout(LayoutKind.Sequential)]
+        internal struct AppExecLinkReparseBuffer
+        {
+            public uint ReparseTag;
+            public ushort ReparseDataLength;
+            public ushort Reserved;
+            public uint StringCount;
+        }


It is not related the PR but I wonder why do such structs returned from P/Invokes would not be readonly structs?

iSazonov · 2021-08-31T03:30:23Z

src/libraries/Common/src/System/IO/FileSystem.Attributes.Windows.cs

@@ -132,6 +134,7 @@ internal static bool IsPathUnreachableError(int errorCode)
                case Interop.Errors.ERROR_NETWORK_ACCESS_DENIED:
                case Interop.Errors.ERROR_INVALID_HANDLE:           // eg from \\.\CON
                case Interop.Errors.ERROR_FILENAME_EXCED_RANGE:     // Path is too long
+                case Interop.Errors.ERROR_CANT_ACCESS_FILE:         // Broken AppExecLink files may cause this


The error looks very generic. No false positives could be for the error in the context?

I'm not sure I understand the question. Would you mind rephrasing that?

Broken AppExecLink files may cause this

My concern is about this comment. Could there be other files that cause the same error?

iSazonov · 2021-08-31T03:36:57Z

src/libraries/System.IO.FileSystem/tests/AppExecLinks.Windows.cs

+    [PlatformSpecific(TestPlatforms.Windows)]
+    public class AppExecLinks : BaseSymbolicLinks
+    {
+        [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.HasUsableAppExecLinksDirectory))]


Sorry if I skipped this - will this throw if HasUsableAppExecLinksDirectory return false or the test could be skipped always silently in CIs?

It should return false and cause the test to be skipped due to the absence of a WindowsApps dir.

The property this attribute is pointing to, should not throw. If it throws, it's a bug and needs to be fixed.

My concern is that in the case the tests can be never really evaluated on CIs.

adamsitnik · 2021-08-31T05:42:46Z

Had a conversation with Dan about this issue and it doesn't meet the bar for a 6.0 backport.

The bar for approving changes for .NET 6 is:

Broken or significantly incomplete 6.0 scenario

I would not say that it's significantly incomplete, but since it blocks one of our 1st party customers from switching to the new APIs (PowerShell/PowerShell#15960) we should rather include it in 6.0. Just my two cents.

danmoseley · 2021-08-31T22:50:23Z

I would not say that it's significantly incomplete, but since it blocks one of our 1st party customers from switching to the new APIs (PowerShell/PowerShell#15960) we should rather include it in 6.0. Just my two cents.

It does, but I figured this customer already has code that works fine. So it would really just be code cleanup for them. If there were other customers that want to adopt the API and are missing support for "AppExecLink" that would be a little different.

That being said, @iSazonov if we took this change, would it be the last piece allowing significant code cleanup from Powershell? Would Powershell be able to adopt it promptly? Is that a signficant win for you guys?

iSazonov · 2021-09-01T03:43:44Z

That being said, @iSazonov if we took this change, would it be the last piece allowing significant code cleanup from Powershell? Would Powershell be able to adopt it promptly? Is that a signficant win for you guys?

For me, it is not critical to have the API today since we are near RC too (and it is LTS version too!) and I guess PowerShell team doesn't want to catch a regression or bug. It is better to ask @SteveL-MSFT directly.

Really there are some API proposal PowerShell can benefit from (specially path enumeration API) and I hope to see them implemented in next milestone early so that we can migrate and test early too.

carlossanlop · 2021-09-02T19:02:31Z

We had a conversation with the Windows team that owns AppExecLinks. They explained that we are not supposed to care about the target path. AppExecLinks are meant to work seamlessly as any other executable. Hence why they never exposed the APIs that we were adding in this PR, and which PowerShell is also calling.

Having learned that, we decided won't be treating AppExecLinks like symlinks and junctions. We will still return true for Attributes.HasFlag(FileAttributes.ReparsePoint), but the new APIs LinkTarget and ResolveLinkTarget will return null (this scenario already works, thanks to @jozkee 's PR).

We also had a conversation with @SteveL-MSFT , @rjmholt, @daxian-dbw from the PowerShell team to let them know what the Windows people told us, so they make adjustments to their code if needed.

With that said, since we are already providing support to symlinks, junctions and appexeclinks as expected, the PowerShell scenario will be complete in 6.0 after we merge this backport to RC2 (@danmoseley).

@iSazonov

carlossanlop added the area-System.IO label Aug 27, 2021

carlossanlop added this to the 6.0.0 milestone Aug 27, 2021

carlossanlop requested review from adamsitnik and jozkee August 27, 2021 01:50

carlossanlop self-assigned this Aug 27, 2021

jozkee reviewed Aug 27, 2021

View reviewed changes

carlossanlop added 4 commits August 27, 2021 11:38

Add internal AppExecLink support to link APIs

58f0b4e

PlatformDetection property to use as condition if Windows machine has…

69c3d78

… an AppExecLinks directory.

AppExecLink unit test.

6f756c8

Address suggestions.

f150fad

carlossanlop force-pushed the appexeclink branch from 8d95f20 to f150fad Compare August 27, 2021 21:31

carlossanlop marked this pull request as ready for review August 27, 2021 21:33

danmoseley reviewed Aug 27, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj Outdated Show resolved Hide resolved

danmoseley reviewed Aug 27, 2021

View reviewed changes

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs Outdated Show resolved Hide resolved

danmoseley reviewed Aug 27, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs Outdated Show resolved Hide resolved

danmoseley reviewed Aug 27, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs Outdated Show resolved Hide resolved

carlossanlop modified the milestones: 6.0.0, 7.0.0 Aug 30, 2021

carlossanlop commented Aug 30, 2021

View reviewed changes

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs Outdated Show resolved Hide resolved

carlossanlop added 3 commits August 30, 2021 11:41

Remove big try catch in PlatformDetection property.

4237aad

Fix csproj alphabetized ordering.

ccfd2dd

Extra checks in for loop to find null chars of appexeclink stringlist.

4c425d1

Add CANT_ACCESS_FILE inside IsPathUnreachableError. Add comment to that method to avoid catching ACCESS_DENIED, SHARING_VIOLATION, SEM_TIMEOUT.

iSazonov reviewed Aug 31, 2021

View reviewed changes

carlossanlop closed this Sep 2, 2021

carlossanlop deleted the appexeclink branch September 2, 2021 19:02

iSazonov mentioned this pull request Sep 3, 2021

Remove support for AppExeCLinks to retrieve target PowerShell/PowerShell#16044

Merged

22 tasks

ghost locked as resolved and limited conversation to collaborators Oct 2, 2021

-                            return Directory.GetFiles(windowsAppsDir, "*.exe", new EnumerationOptions() { RecurseSubdirectories = true, MaxRecursionDepth = 3 }).Length > 0;
+                            new FileSystemEnumerable<string?>(
+                                windowsAppsDir,
+                                (ref FileSystemEntry entry) => null,
+                                new EnumerationOptions { RecurseSubdirectories = true })
+                                {
+                                    ShouldIncludePredicate = (ref FileSystemEntry entry) =>
+                                        FileSystemName.MatchesWin32Expression("*.exe", entry.FileName) &&
+                                        (entry.Attributes & FileAttributes.ReparsePoint) != 0
+                                }.Any();

		@@ -557,7 +594,8 @@ internal static void CreateSymbolicLink(string path, string pathToTarget, bool i
		if ((data.dwFileAttributes & (uint)FileAttributes.ReparsePoint) == 0 \|\|
		// Only symbolic links and mount points are supported at the moment.

	// Only symbolic links and mount points are supported at the moment.
	// Only symbolic links, mount points and windows apps are supported at the moment.

Add internal AppExecLink support to link APIs #58233

Add internal AppExecLink support to link APIs #58233

Conversation

carlossanlop commented Aug 27, 2021 • edited Loading

ghost commented Aug 27, 2021

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

Choose a reason for hiding this comment

iSazonov Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Aug 27, 2021

carlossanlop commented Aug 30, 2021

carlossanlop commented Aug 31, 2021

Choose a reason for hiding this comment

iSazonov Aug 31, 2021 • edited Loading

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

adamsitnik commented Aug 31, 2021

danmoseley commented Aug 31, 2021

iSazonov commented Sep 1, 2021

carlossanlop commented Sep 2, 2021 • edited Loading

carlossanlop commented Aug 27, 2021 •

edited

Loading

iSazonov Sep 1, 2021 •

edited

Loading

jozkee Aug 27, 2021 •

edited

Loading

carlossanlop Aug 31, 2021 •

edited

Loading

iSazonov Aug 31, 2021 •

edited

Loading

carlossanlop commented Sep 2, 2021 •

edited

Loading