Relax LinkTarget so it always returns null when steps on an error #55668

jozkee · 2021-07-14T18:37:49Z

Motivated by #55664. When I was testing that PR, I noticed that I was getting ACCESS DENIED errors sporadically, here's the callstack:

  Unhandled exception. System.UnauthorizedAccessException: Access to the path 'C:\Users\david\AppData\Local\Temp\tapmbdam.rgw\4d2sb45x.hix_FOO_True' is denied.
     at System.IO.FileSystem.GetImmediateLinkTarget(String linkPath, Boolean isDirectory, Boolean throwOnUnreachable, Boolean returnFullPath) in System.Private.CoreLib.dll:token 0x6005ec9+0x3b
     at System.IO.FileSystem.GetLinkTarget(String linkPath, Boolean isDirectory) in System.Private.CoreLib.dll:token 0x6005ec8+0x0
     at System.IO.FileSystemInfo.get_LinkTarget() in System.Private.CoreLib.dll:token 0x6005ee2+0xf
     at Microsoft.Extensions.FileProviders.Physical.FileSystemInfoHelper.ResolveFileLinkTarget(FileInfo fileInfo) in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\Internal\FileSystemInfoHelper.cs:line 44
     at Microsoft.Extensions.FileProviders.Physical.FileSystemInfoHelper.ResolveFileLinkTarget(String filePath) in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\Internal\FileSystemInfoHelper.cs:line 34
     at Microsoft.Extensions.FileProviders.Physical.PollingWildCardChangeToken.GetLastWriteUtc(String path) in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\PollingWildCardChangeToken.cs:line 147
     at Microsoft.Extensions.FileProviders.Physical.PollingWildCardChangeToken.CalculateChanges() in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\PollingWildCardChangeToken.cs:line 115
     at Microsoft.Extensions.FileProviders.Physical.PollingWildCardChangeToken.get_HasChanged() in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\PollingWildCardChangeToken.cs:line 98
     at Microsoft.Extensions.FileProviders.Physical.PhysicalFilesWatcher.RaiseChangeEvents(Object state) in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\PhysicalFilesWatcher.cs:line 437
     at System.Threading.TimerQueueTimer.CallCallback(Boolean isThreadPool) in System.Private.CoreLib.dll:token 0x600303a+0x2b
     at System.Threading.TimerQueueTimer.Fire(Boolean isThreadPool) in System.Private.CoreLib.dll:token 0x6003038+0x3b
     at System.Threading.TimerQueue.FireNextTimers() in System.Private.CoreLib.dll:token 0x6002d9e+0x1a9
     at System.Threading.TimerQueue.AppDomainTimerCallback(Int32 id) in System.Private.CoreLib.dll:token 0x6002d95+0x0

I suspect is because of the following (even though I am getting the error when calling CreateFileW):

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

Lines 73 to 76 in 44f7103

    
           // Files that are marked for deletion will not let you GetFileAttributes, 
        
           // ERROR_ACCESS_DENIED is given back without filling out the data struct. 
        
           // FindFirstFile, however, will. Historically we always gave back attributes 
        
           // for marked-for-deletion files.

I wasn't able to create a unit case that consistently could hit this issue but by always returning null when the LinkTarget property hits an error, we can mitigate the issue.
In addition, this can make developer's life easier by not being afraid of such property breaking their app.

This also grants us parity with the Unix implementation.

ghost · 2021-07-14T18:37:58Z

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

Issue Details

Motivated by #55664. When I was testing that PR, I noticed that I was getting ACCESS DENIED errors sporadically, here's the callstack:

  Unhandled exception. System.UnauthorizedAccessException: Access to the path 'C:\Users\david\AppData\Local\Temp\tapmbdam.rgw\4d2sb45x.hix_FOO_True' is denied.
     at System.IO.FileSystem.GetImmediateLinkTarget(String linkPath, Boolean isDirectory, Boolean throwOnUnreachable, Boolean returnFullPath) in System.Private.CoreLib.dll:token 0x6005ec9+0x3b
     at System.IO.FileSystem.GetLinkTarget(String linkPath, Boolean isDirectory) in System.Private.CoreLib.dll:token 0x6005ec8+0x0
     at System.IO.FileSystemInfo.get_LinkTarget() in System.Private.CoreLib.dll:token 0x6005ee2+0xf
     at Microsoft.Extensions.FileProviders.Physical.FileSystemInfoHelper.ResolveFileLinkTarget(FileInfo fileInfo) in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\Internal\FileSystemInfoHelper.cs:line 44
     at Microsoft.Extensions.FileProviders.Physical.FileSystemInfoHelper.ResolveFileLinkTarget(String filePath) in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\Internal\FileSystemInfoHelper.cs:line 34
     at Microsoft.Extensions.FileProviders.Physical.PollingWildCardChangeToken.GetLastWriteUtc(String path) in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\PollingWildCardChangeToken.cs:line 147
     at Microsoft.Extensions.FileProviders.Physical.PollingWildCardChangeToken.CalculateChanges() in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\PollingWildCardChangeToken.cs:line 115
     at Microsoft.Extensions.FileProviders.Physical.PollingWildCardChangeToken.get_HasChanged() in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\PollingWildCardChangeToken.cs:line 98
     at Microsoft.Extensions.FileProviders.Physical.PhysicalFilesWatcher.RaiseChangeEvents(Object state) in C:\repos\runtime\src\libraries\Microsoft.Extensions.FileProviders.Physical\src\PhysicalFilesWatcher.cs:line 437
     at System.Threading.TimerQueueTimer.CallCallback(Boolean isThreadPool) in System.Private.CoreLib.dll:token 0x600303a+0x2b
     at System.Threading.TimerQueueTimer.Fire(Boolean isThreadPool) in System.Private.CoreLib.dll:token 0x6003038+0x3b
     at System.Threading.TimerQueue.FireNextTimers() in System.Private.CoreLib.dll:token 0x6002d9e+0x1a9
     at System.Threading.TimerQueue.AppDomainTimerCallback(Int32 id) in System.Private.CoreLib.dll:token 0x6002d95+0x0

I suspect is because of the following (even though I am getting the error when calling CreateFileW):

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

Lines 73 to 76 in 44f7103

    
           // Files that are marked for deletion will not let you GetFileAttributes, 
        
           // ERROR_ACCESS_DENIED is given back without filling out the data struct. 
        
           // FindFirstFile, however, will. Historically we always gave back attributes 
        
           // for marked-for-deletion files.

I wasn't able to create a unit case that consistently could hit this issue but by always returning null when the LinkTarget property hits an error, we can mitigate the issue.
In addition, this can make developer's life easier by not being afraid of such property breaking their app.

This also grants us parity with the Unix implementation.

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

stephentoub · 2021-07-14T19:40:32Z

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

@@ -600,13 +604,15 @@ uint GetFinalPathNameByHandle(SafeFileHandle handle, char[] buffer)
            {
                // Since all these paths will be passed to CreateFile, which takes a string anyway, it is pointless to use span.
                // I am not sure if it's possible to change CreateFile's param to ROS<char> and avoid all these allocations.


Can we also follow-up to address this comment? If we can save allocations by marshaling a span instead of creating a string to then pass down, we should. You can't today pass a span directly to a DllImport method, but you can manually pin it and pass in the pointer, which is exactly what the runtime does with string.

Will address this in a separate PR in order to get this in before the preview7 snap, thanks.

Relax LinkTarget so it always returns null when steps on an error

f515710

jozkee added this to the 6.0.0 milestone Jul 14, 2021

jozkee requested review from carlossanlop, adamsitnik and bartonjs July 14, 2021 18:37

jozkee self-assigned this Jul 14, 2021

dotnet-issue-labeler bot added the area-System.IO label Jul 14, 2021

jozkee mentioned this pull request Jul 14, 2021

Make polling use the symbolic link target's LastWriteTime #55664

Merged

stephentoub reviewed Jul 14, 2021

View reviewed changes

bartonjs approved these changes Jul 15, 2021

View reviewed changes

jozkee merged commit 71d0d2b into dotnet:main Jul 15, 2021

jozkee deleted the relax_linktarget branch July 15, 2021 16:52

ManickaP mentioned this pull request Jul 20, 2021

[QUIC] Remove AppContext switch from S.N.Quic #56027

Merged

ghost locked as resolved and limited conversation to collaborators Aug 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax LinkTarget so it always returns null when steps on an error #55668

Relax LinkTarget so it always returns null when steps on an error #55668

jozkee commented Jul 14, 2021

ghost commented Jul 14, 2021

stephentoub Jul 14, 2021

jozkee Jul 15, 2021

	// Files that are marked for deletion will not let you GetFileAttributes,
	// ERROR_ACCESS_DENIED is given back without filling out the data struct.
	// FindFirstFile, however, will. Historically we always gave back attributes
	// for marked-for-deletion files.

Relax LinkTarget so it always returns null when steps on an error #55668

Relax LinkTarget so it always returns null when steps on an error #55668

Conversation

jozkee commented Jul 14, 2021

ghost commented Jul 14, 2021

stephentoub Jul 14, 2021

Choose a reason for hiding this comment

jozkee Jul 15, 2021

Choose a reason for hiding this comment