Tar: Only treat reparse points marked as junctions or symlinks as actual tar symlinks #89102

carlossanlop · 2023-07-18T16:22:15Z

The current code only checks if it's a reparse point and adds the filesystem object as a symlink, which is wrong for many types of reparse points. Only those marked as junctions or symlinks should be treated as tar symlinks.

I added a test to confirm that a junction is stored as symlink. I am unsure how to create other types of reparse points so I didn't add tests for that, but since those are rarer cases, I would prefer to tackle them separately if requested.

cc @billfreist, thanks for reporting this.

Fixes #82949

…stFileEx.cs for shared consumption.

… point is a junction or a symbolic link, in which case the filesystem object will be treated as a tar symbolic link. All other reparse points will be treated as regular files.

…specified format and consume it in TarWriter.Unix.cs and TarWriter.Windows.cs.

…um value for the specified format.

…ve, verify they are added as symbolic links entries.

ghost · 2023-07-18T16:22:29Z

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

Issue Details

The current code only checks if it's a reparse point and adds the filesystem object as a symlink, which is wrong for many types of reparse points. Only those marked as junctions or symlinks should be treated as tar symlinks.

I added a test to confirm that a junction is stored as symlink. I am unsure how to create other types of reparse points so I didn't add tests for that, but since those are rarer cases, I would prefer to tackle them separately if requested.

cc @billfreist, thanks for reporting this.

Fixes #82949

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-System.Formats.Tar`
Milestone:	-

danmoseley · 2023-07-18T18:40:51Z

src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.File.Tests.Windows.cs

+    [InlineData(TarEntryFormat.Ustar)]
+    [InlineData(TarEntryFormat.Pax)]
+    [InlineData(TarEntryFormat.Gnu)]
+    public void Add_Junction_As_SymbolicLink(TarEntryFormat format)


Can you run these tests without elevation? I think it's missing a can create symlinks condition. I've added them a few times when tests failed locally.

Starting with Windows 10 Insiders build 14972, symlinks can be created without needing to elevate the console as administrator. https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/

If we still have tests running older Windows versions, they should break in the CI.

Junctions are ancient, they don't require elevation AFAIK. They ran without problem in my local machine. The CI should also cry if something goes wrong.

This new test does not create symlinks in the disk by the way, it adds an entry to the tar archive as a symlink. The only thing that gets created in disk is the junction.

jozkee · 2023-07-18T20:05:50Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Windows.cs

+            bool isDirectory = (attributes & FileAttributes.Directory) != 0;
+
+            Interop.Kernel32.WIN32_FIND_DATA data = default;
+            Interop.Kernel32.GetFindData(fullPath, isDirectory, ignoreAccessDenied: false, ref data);


Shouldn't this be called only when the file is a reparse point?

You're right.

jozkee · 2023-07-18T20:09:05Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Windows.cs

+                if (data.dwReserved0 is Interop.Kernel32.IOReparseOptions.IO_REPARSE_TAG_SYMLINK or
+                                        Interop.Kernel32.IOReparseOptions.IO_REPARSE_TAG_MOUNT_POINT)


This looks like another data point in favor of having an API for ReparseTags #1908.

jozkee · 2023-07-18T20:19:14Z

I am unsure how to create other types of reparse points so I didn't add tests for that

You can test IO_REPARSE_TAG_APPEXECLINK by trying to archive one file with such ReparseTag:

runtime/src/libraries/System.IO.FileSystem/tests/File/SymbolicLinks.cs

Lines 49 to 61 in b8653d9

    
           [Fact] 
        
           [PlatformSpecific(TestPlatforms.Windows)] 
        
           public void UnsupportedLink_ReturnsNull() 
        
           { 
        
               string unsupportedLinkPath = GetAppExecLinkPath(); 
        
               if (unsupportedLinkPath is null) 
        
               { 
        
                   return; 
        
               } 
        
               Assert.Null(File.ResolveLinkTarget(unsupportedLinkPath, false)); 
        
               Assert.Null(File.ResolveLinkTarget(unsupportedLinkPath, true)); 
        
           }

carlossanlop · 2023-07-18T21:58:23Z

You can test IO_REPARSE_TAG_APPEXECLINK by trying to archive one file with such ReparseTag:

Thanks @jozkee, I forgot about that.

I wrote a test that attempts to verify what happens when adding an AppExecLink. I reused the method that we have to retrieve the first AppExecLink from the WindowsApps folder. In my case, it's accesschk.exe, which is a binary that is installed by the Sysinternals Suite. I confirmed it's an actual AppExecLink because it's 0KB in length.

It seems that my current code would throw an IOException when FileStream attempts to create a file handle to it (see the callkstack below). Which means we can't treat AppExecLinks as regular files.

I think for now, we should just throw an IOException on the Tar code when we encounter an unsupported ReparsePoint. There might be a way to crack open an AppExecLink, retrieve some internal information it holds, and then store it in the DataStream, but we don't have an actual entry type to indicate this is an AppExecLink file, which differs from a regular file and from a symlink. So if the user generates a tar archive containing one of these, then tries to extract it, it will be extracted as something that it is not.

Thoughts?

      System.IO.IOException : The file cannot be accessed by the system. : 'C:\Users\calope\AppData\Local\Microsoft\WindowsApps\accesschk.exe'
        Stack Trace:
          C:\Users\calope\source\repos\runtime\src\libraries\System.Private.CoreLib\src\Microsoft\Win32\SafeHandles\SafeFileHandle.Windows.cs(114,0): at Microsoft.Win32.SafeHandles.SafeFileHandle.CreateFile(String fullPath, FileMode mode, FileAccess access, FileShare share
  , FileOptions options)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Private.CoreLib\src\Microsoft\Win32\SafeHandles\SafeFileHandle.Windows.cs(47,0): at Microsoft.Win32.SafeHandles.SafeFileHandle.Open(String fullPath, FileMode mode, FileAccess access, FileShare share, FileO
  ptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\Strategies\OSFileStreamStrategy.cs(46,0): at System.IO.Strategies.OSFileStreamStrategy..ctor(String path, FileMode mode, FileAccess access, FileShare share, FileOptions option
  s, Int64 preallocationSize, Nullable`1 unixCreateMode)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\Strategies\SyncWindowsFileStreamStrategy.cs(15,0): at System.IO.Strategies.SyncWindowsFileStreamStrategy..ctor(String path, FileMode mode, FileAccess access, FileShare share,
  FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\Strategies\FileStreamHelpers.Windows.cs(35,0): at System.IO.Strategies.FileStreamHelpers.ChooseStrategyCore(String path, FileMode mode, FileAccess access, FileShare share, Fil
  eOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\Strategies\FileStreamHelpers.cs(24,0): at System.IO.Strategies.FileStreamHelpers.ChooseStrategy(FileStream fileStream, String path, FileMode mode, FileAccess access, FileShare
   share, Int32 bufferSize, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\FileStream.cs(222,0): at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options, Int64 preallocationS
  ize)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\FileStream.cs(146,0): at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Formats.Tar\src\System\Formats\Tar\TarWriter.Windows.cs(79,0): at System.Formats.Tar.TarWriter.ConstructEntryForWriting(String fullPath, String entryName, FileOptions fileOptions)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Formats.Tar\src\System\Formats\Tar\TarWriter.cs(166,0): at System.Formats.Tar.TarWriter.ReadFileFromDiskAndWriteToArchiveStreamAsEntry(String fullPath, String entryName)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Formats.Tar\src\System\Formats\Tar\TarWriter.cs(139,0): at System.Formats.Tar.TarWriter.WriteEntry(String fileName, String entryName)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Formats.Tar\tests\TarWriter\TarWriter.WriteEntry.File.Tests.Windows.cs(72,0): at System.Formats.Tar.Tests.TarWriter_WriteEntry_File_Tests.Add_AppExecLink_As_RegularFile(TarEntryFormat format)
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          C:\Users\calope\source\repos\runtime\src\coreclr\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.CoreCLR.cs(36,0): at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
          C:\Users\calope\source\repos\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs(177,0): at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
      System.Formats.Tar.Tests.TarWriter_WriteEntry_File_Tests.Add_AppExecLink_As_RegularFile(format: Ustar) [FAIL]

…k file to an archive.

src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.File.Tests.Windows.cs

...libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.File.Tests.Windows.cs

ericstj · 2023-09-25T15:09:01Z

@carlossanlop what's the next step to move this one forward?

carlossanlop · 2023-09-25T16:57:37Z

@carlossanlop what's the next step to move this one forward?

Make sure we have tests that confirm my changes are working, haven't been able to get back to it yet. Need to find a way to test adding a OneDrive cloud file. Maybe I could add manual tests.

ghost · 2023-11-15T03:01:16Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

ghost · 2023-11-29T06:01:14Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

carlossanlop added 6 commits July 18, 2023 08:42

Move GetFindData method from FileSystem.Windows.cs to Interop.FindFir…

507f169

…stFileEx.cs for shared consumption.

Consume GetFindData in TarWriter.Windows.cs to determine if a reparse…

326846a

… point is a junction or a symbolic link, in which case the filesystem object will be treated as a tar symbolic link. All other reparse points will be treated as regular files.

Remove unused using directives.

689064c

Create method that chooses the right regular file enum value for the …

417585a

…specified format and consume it in TarWriter.Unix.cs and TarWriter.Windows.cs.

Also add a helper method in tests to choose the right regular file en…

5348435

…um value for the specified format.

Add windows-only tests that create junctions and add them to an archi…

ddd8c8f

…ve, verify they are added as symbolic links entries.

carlossanlop added the area-System.Formats.Tar label Jul 18, 2023

carlossanlop requested review from tmds, adamsitnik, ViktorHofer, ericstj and jozkee July 18, 2023 16:22

carlossanlop self-assigned this Jul 18, 2023

danmoseley reviewed Jul 18, 2023

View reviewed changes

This was referenced Jul 18, 2023

Timeout in System.Net.Quic.Functional.Tests #86019

Closed

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

ViktorHofer approved these changes Jul 18, 2023

View reviewed changes

jozkee reviewed Jul 18, 2023

View reviewed changes

Move tag retrieval into if

53e2ad8

carlossanlop added 2 commits July 18, 2023 16:01

Throw IOException for all other reparse points.

9d17d9c

Add test that throws when attempting to add an unsupported appexeclin…

e934e76

…k file to an archive.

jozkee approved these changes Aug 23, 2023

View reviewed changes

Apply suggestions from code review

375f1b8

This was referenced Aug 24, 2023

Tracking issue for CI build timeouts #76454

Closed

[8.0] [mono] Test process gets killed #90834

Closed

Http2_MultipleConnectionsEnabled test failures #91075

Closed

carlossanlop marked this pull request as draft October 30, 2023 14:25

jozkee added the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 31, 2023

ghost added the no-recent-activity label Nov 15, 2023

ghost closed this Nov 29, 2023

github-actions bot locked and limited conversation to collaborators Dec 29, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tar: Only treat reparse points marked as junctions or symlinks as actual tar symlinks #89102

Tar: Only treat reparse points marked as junctions or symlinks as actual tar symlinks #89102

carlossanlop commented Jul 18, 2023

ghost commented Jul 18, 2023

danmoseley Jul 18, 2023

carlossanlop Jul 18, 2023

carlossanlop Jul 18, 2023

jozkee Jul 18, 2023

carlossanlop Jul 18, 2023

jozkee Jul 18, 2023

carlossanlop Jul 18, 2023

jozkee commented Jul 18, 2023

carlossanlop commented Jul 18, 2023

ericstj commented Sep 25, 2023

carlossanlop commented Sep 25, 2023

ghost commented Nov 15, 2023

ghost commented Nov 29, 2023

		if (data.dwReserved0 is Interop.Kernel32.IOReparseOptions.IO_REPARSE_TAG_SYMLINK or
		Interop.Kernel32.IOReparseOptions.IO_REPARSE_TAG_MOUNT_POINT)

Tar: Only treat reparse points marked as junctions or symlinks as actual tar symlinks #89102

Tar: Only treat reparse points marked as junctions or symlinks as actual tar symlinks #89102

Conversation

carlossanlop commented Jul 18, 2023

ghost commented Jul 18, 2023

danmoseley Jul 18, 2023

Choose a reason for hiding this comment

carlossanlop Jul 18, 2023

Choose a reason for hiding this comment

carlossanlop Jul 18, 2023

Choose a reason for hiding this comment

jozkee Jul 18, 2023

Choose a reason for hiding this comment

carlossanlop Jul 18, 2023

Choose a reason for hiding this comment

jozkee Jul 18, 2023

Choose a reason for hiding this comment

carlossanlop Jul 18, 2023

Choose a reason for hiding this comment

jozkee commented Jul 18, 2023

carlossanlop commented Jul 18, 2023

ericstj commented Sep 25, 2023

carlossanlop commented Sep 25, 2023

ghost commented Nov 15, 2023

ghost commented Nov 29, 2023