Tar: Only treat reparse points marked as junctions or symlinks as actual tar symlinks#124753
Tar: Only treat reparse points marked as junctions or symlinks as actual tar symlinks#124753rzikm merged 3 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where TarWriter on Windows incorrectly classified all reparse points (including data deduplication, OneDrive placeholders, and AppExecLinks) as symbolic links. The fix leverages the FileSystemInfo.LinkTarget property which only returns a non-null value for true symbolic links (IO_REPARSE_TAG_SYMLINK) and junctions (IO_REPARSE_TAG_MOUNT_POINT), allowing other reparse point types to be correctly classified as regular files or directories.
Changes:
- Modified
TarWriter.Windows.csto checkLinkTargetproperty instead of just the ReparsePoint attribute - Extracted
GetRegularFileEntryTypeForFormathelper toTarHelpers.csto eliminate duplication - Added comprehensive tests for junctions and non-symlink reparse points (using AppExecLinks)
- Moved
GetAppExecLinkPathutility to sharedReparsePointUtilities.csfor reuse across test projects
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| TarWriter.Windows.cs | Core fix: checks LinkTarget is not null to distinguish symlinks/junctions from other reparse points; caches LinkTarget value to avoid redundant I/O |
| TarHelpers.cs | Adds GetRegularFileEntryTypeForFormat helper method to centralize V7 vs regular file type selection logic |
| TarWriter.Unix.cs | Refactored to use new helper method; removed unused using directives |
| TarWriter.WriteEntry.File.Tests.Windows.cs | New test file with junction test and non-symlink reparse point test (sync version) |
| TarWriter.WriteEntryAsync.File.Tests.Windows.cs | New test file with junction test and non-symlink reparse point test (async version) |
| System.Formats.Tar.Tests.csproj | Registers new Windows-specific test files |
| TarTestsBase.cs | Adds GetRegularFileEntryTypeForFormat helper for test code reuse |
| Various test files | Replace inline ternary expressions with GetRegularFileEntryTypeForFormat helper for consistency |
| ReparsePointUtilities.cs | Adds GetAppExecLinkPath method migrated from FileSystem tests for cross-project reuse |
| File/FileInfo SymbolicLinks.cs | Updated to use MountHelper.GetAppExecLinkPath() instead of local method |
| BaseSymbolicLinks.FileSystem.cs | Removed GetAppExecLinkPath (migrated to shared utilities); removed unused using directive |
a3c53f0 to
fff18c6
Compare
fff18c6 to
e9aac36
Compare
aaa64b5 to
926b2fd
Compare
…n Windows Only treat reparse points as symbolic links when FileSystemInfo.LinkTarget returns a non-null value, indicating the reparse point is a true symlink or junction. Other reparse points (e.g., deduplication, OneDrive) now fall through to be handled as regular files or directories. Added GetRegularFileEntryTypeForFormat helper to reduce duplication of the V7/regular file entry type check pattern across source and test files. Moved GetAppExecLinkPath to shared ReparsePointUtilities so it can be used by both FileSystem and Tar tests. Added tests verifying junctions are correctly written as symbolic link entries and that non-symlink reparse points are not misidentified. Fixes dotnet#82949 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
926b2fd to
1e1492f
Compare
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Windows.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Windows.cs
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.FindFirstFileEx.cs
Outdated
Show resolved
Hide resolved
Remove FindFirstFile/GetFindData interop and use FileSystemInfo.LinkTarget to detect symlinks and junctions. Non-symlink reparse points with file attributes (e.g., dedup, OneDrive) fall through to be classified as regular files or directories based on their attributes. Opaque reparse points (e.g., AppExecLinks) that cannot be read are caught at FileStream open and rethrown with TarUnsupportedFile message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot run the code review skill |
ericstj
left a comment
There was a problem hiding this comment.
This change looks good.
I think this will make us take existing archives which might have excluded some files and automatically include them. While most people might want this behavior it could be seen as breaking behavior to some and we should be sure to document.
If instead we'd always throw before then it's less breaking to make that work and I wouldn't recommend documenting as breaking change.
This change affects only creating new tar archives Before, we would throw on any reparse point, which is not a symlink/junction. (because for these, With this change, we will attempt to include non-symlink reparse points as regular files, for some types, where opening the file fails (such as AppExecLink reparse point), we throw IOException with message that given file is not supported. An alternative would be to throw for all non-symlink reparse points (including file deduplication reparse point mentioned in the original issue), and only "dereference" these if we implement some sort of "DereferenceSymlinks" tar creation options. I will update the PR description to make this more clear |
|
Sorry, yes I understand it will only effect new archives. I meant to say "existing code paths creating archives". Trying to evaluate compat behavior. Making an exception path successful is something we do consider non-breaking. So long as all the cases this unblocks are that way I'm good with us not marking as breaking. |
Fixes #82949
Before, we would throw on any reparse point, which is not a symlink/junction. (because for these,
LinkTargetis null) and the exception would be something cryptic likeWith this change, we will attempt to include non-symlink reparse points as regular files,. For some types, where opening the file fails (such as AppExecLink reparse point), we throw IOException with message that given file is not supported.
An alternative would be to throw for all non-symlink reparse points (including file deduplication reparse point mentioned in the original issue), and only "dereference" these if we implement some sort of "DereferenceSymlinks" tar creation options.