Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TarEntry: link target path checks #78695

Closed
tmds opened this issue Nov 22, 2022 · 4 comments · Fixed by #85378
Closed

TarEntry: link target path checks #78695

tmds opened this issue Nov 22, 2022 · 4 comments · Fixed by #85378

Comments

@tmds
Copy link
Member

tmds commented Nov 22, 2022

@carlossanlop @jozkee do we need these checks:

if (!string.IsNullOrEmpty(linkTargetPath))
{
string? targetDirectoryPath = Path.GetDirectoryName(linkTargetPath);
// If the destination target contains a directory segment, need to check that it exists
if (!string.IsNullOrEmpty(targetDirectoryPath) && !Path.Exists(targetDirectoryPath))
{
throw new IOException(SR.Format(SR.TarSymbolicLinkTargetNotExists, filePath, linkTargetPath));
}
if (EntryType is TarEntryType.HardLink)
{
if (!Path.Exists(linkTargetPath))
{
throw new IOException(SR.Format(SR.TarHardLinkTargetNotExists, filePath, linkTargetPath));
}
else if (Directory.Exists(linkTargetPath))
{
throw new IOException(SR.Format(SR.TarHardLinkToDirectoryNotAllowed, filePath, linkTargetPath));
}
}

I'm not sure what the Path.Exists is about?

For the hard link, it seems they check conditions on which creating the hard link will fail. So we might just as well fail when the link gets created.

If we can get rid of the checks, we can simplify the code a little because there is no need to pass around linkTargetPath to this method.

I tried leaving them out, and there were no test failures.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 22, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Nov 22, 2022

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

Issue Details

@carlossanlop @jozkee do we need these checks:

if (!string.IsNullOrEmpty(linkTargetPath))
{
string? targetDirectoryPath = Path.GetDirectoryName(linkTargetPath);
// If the destination target contains a directory segment, need to check that it exists
if (!string.IsNullOrEmpty(targetDirectoryPath) && !Path.Exists(targetDirectoryPath))
{
throw new IOException(SR.Format(SR.TarSymbolicLinkTargetNotExists, filePath, linkTargetPath));
}
if (EntryType is TarEntryType.HardLink)
{
if (!Path.Exists(linkTargetPath))
{
throw new IOException(SR.Format(SR.TarHardLinkTargetNotExists, filePath, linkTargetPath));
}
else if (Directory.Exists(linkTargetPath))
{
throw new IOException(SR.Format(SR.TarHardLinkToDirectoryNotAllowed, filePath, linkTargetPath));
}
}

I'm not sure what the Path.Exists is about?

For the hard link, it seems they check conditions on which creating the hard link will fail. So we might just as well fail when the link gets created.

If we can get rid of the checks, we can simplify the code a little because there is no need to pass around linkTargetPath to this method.

I tried leaving them out, and there were no test failures.

Author: tmds
Assignees: -
Labels:

area-System.IO.Compression, untriaged

Milestone: -

@tmds tmds removed the untriaged New issue has not been triaged by the area owner label Nov 22, 2022
@jozkee jozkee added area-System.Formats.Tar untriaged New issue has not been triaged by the area owner and removed area-System.IO.Compression labels Nov 22, 2022
@tmds tmds removed the untriaged New issue has not been triaged by the area owner label Nov 30, 2022
@tmds
Copy link
Member Author

tmds commented Nov 30, 2022

@carlossanlop @jozkee do you recall why the first condition is there?

@tmds
Copy link
Member Author

tmds commented Dec 13, 2022

ping @carlossanlop @jozkee

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 26, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 27, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants