Skip to content

Fix failing symlink tests (9.0) - #130343

Merged
alinpahontu2912 merged 4 commits into
dotnet:release/9.0-stagingfrom
alinpahontu2912:tar_fix_9
Jul 15, 2026
Merged

Fix failing symlink tests (9.0)#130343
alinpahontu2912 merged 4 commits into
dotnet:release/9.0-stagingfrom
alinpahontu2912:tar_fix_9

Conversation

@alinpahontu2912

@alinpahontu2912alinpahontu2912 commented Jul 8, 2026•
edited by karelz
Loading

Copy link
Copy Markdown
Member

Backport of #130342 to .NET 9.

Fixes#129227

Customer Impact

The new tar symlink resolution is not working properly on some platforms.

Found internally via test failures in servicing branches - see #129227.

Corrects a length mismatch that made the Tar traversal guard crash with ArgumentOutOfRangeException (subtracting a symlink-resolved path length from a logical path). Without it, extraction to any symlinked destination (macOS /var , /tmp ) is broken.

Regression

Yes - caught by test failures in servicing branches - see #129227.

Testing

Re-enabled failing CI tests.

Risk

Low.

CopilotAI review requested due to automatic review settings July 8, 2026 09:21
@dotnet-policy-service

dotnet-policy-serviceBot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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

@alinpahontu2912alinpahontu2912 mentioned this pull request Jul 8, 2026
Merged
@alinpahontu2912
alinpahontu2912 requested review from a team and svickJuly 8, 2026 09:23
CopilotAI reviewed Jul 8, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Backport to release/9.0-staging that fixes TarEntry.FilePathEscapesDirectory so extraction correctly detects symlink-based directory escapes without throwing, and re-enables previously quarantined symlink traversal tests.

Changes:

  • Update FilePathEscapesDirectory to separate logical vs. physical path handling and add a prefix guard before computing a relative path.
  • Adjust symlink resolution to avoid treating non-links/dangling links incorrectly during path-walk checks.
  • Remove [ActiveIssue(#129227)] from three symlink-related extraction tests now that the underlying bug is addressed.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.csFixes symlink escape detection logic to avoid Substring OOR and correctly walk/resolve paths.
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.csRe-enables symlink traversal tests by removing the ActiveIssue suppressions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
This was referenced Jul 8, 2026
Open
Open
Closed
Closed
@alinpahontu2912
Guard destination-root path in FilePathEscapesDirectory and re-enable…
bd0a3d2
… symlink tests
When an entry resolves to the destination directory itself, the relative
path computation did an out-of-range Substring (logicalPrefix includes a
trailing separator). Return an empty relative path in that case. This also
fixes the ArgumentOutOfRangeException seen on macOS where the physical
destination path is longer than the logical one.
Remove the [ActiveIssue] dotnet#129227 tags now that the affected tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 9, 2026 07:54
CopilotAI reviewed Jul 9, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs Outdated
@build-analysisbuild-analysisBot mentioned this pull request Jul 9, 2026
Open
CopilotAI review requested due to automatic review settings July 9, 2026 11:32
CopilotAI reviewed Jul 9, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
@mrek-msft

mrek-msft commented Jul 10, 2026

Copy link
Copy Markdown
Member

Hi,

the code complete date for 9.0.19 (the August 2026 release) is Monday 2026-07-13. Make sure to merge this PR on that date at the latest, or it won't make it into that release.

As a reminder, to merge it, you need Tactics approval, the PR needs to be approved by another area co-owner and there can't be any CI failures that could be related to the fix.

@alinpahontu2912
alinpahontu2912 enabled auto-merge (squash) July 15, 2026 08:17
CopilotAI review requested due to automatic review settings July 15, 2026 08:29
CopilotAI reviewed Jul 15, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
This was referenced Aug 22, 2026
Merged
Open
Merged
Merged
Merged
Closed
Open
Merged
Merged
Closed
Merged
Closed
Open
Merged
Merged
Merged
Closed
Closed
Closed
Closed
Merged
Closed
Merged
Closed
Merged
Closed
Closed
Closed
Closed
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 27, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code reviewCopilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@rzikmrzikmrzikm approved these changes
@iremyuxiremyuxiremyux approved these changes
@svicksvickAwaiting requested review from svick

Assignees

@alinpahontu2912alinpahontu2912

Labels

area-System.Formats.TarServicing-approvedApproved for servicing release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alinpahontu2912@mrek-msft@rzikm@iremyux