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.

Sign up for GitHub

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

Jump to bottom

Simplify DropPathRoot and fix out of bounds issue #593

Merged
merged 4 commits into from Apr 27, 2021
Merged

Simplify DropPathRoot and fix out of bounds issue #593

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b52667
Simplify DropPathRoot and fix out of bounds issue
piksel Mar 7, 2021
0e522c0
prevent throwing in DropPathRoot and add tests
piksel Mar 14, 2021
cc766e5
make path tests platform-specific
piksel Mar 14, 2021
b928730
handle .NET < 4.6.2 special cases
piksel Mar 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 3 additions & 42 deletions src/ICSharpCode.SharpZipLib/Core/PathUtils.cs
View file
Open in desktop
Expand Up @@ -15,48 +15,9 @@ public static class PathUtils
/// <remarks>Unlike the <see cref="System.IO.Path"/> class the path isn't otherwise checked for validity.</remarks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Referring to the comment about not checking for validity - I can't recall if this is ever called on reading a file or just creating? - are there any situations where GetPathRoot might throw on .NET 4.x if it thinks something is invalid (e.g. same sort of situation as #341)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you are probably right. I'll add a test for this and fix it if it triggers anything.
If it does, it is probably better to use another code path for cleaning "input" file names, rather than sharing it with the "output" ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

The results are in!
image
The usual reserved characters for file names in windows are \ / : * ? " < > |, but out of those, only < > | " are listed in Path.GetInvalidPathChars() and throws on net46.
I guess the easiest way to solve it is to just replace those chars with _?

Copy link
Member Author

@piksel piksel Mar 14, 2021 •
edited

Choose a reason for hiding this comment

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

Added an optional replaceInvalidChars to the arguments as well, since the work is already done, but per default it returns the stripped path, including any invalid path characters.

Scratch that since it won't be consistent across all platforms. This will only strip the root from the path, nothing else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've forgotten all the details offhand about how DropPathRoot interacts with the other name transform/cleaning stuff, but looking at the comment about long paths reminded me of the \\?\ long path syntax on Windows, the handling of which might be another reason to leave it up to the built in functions instead of doing things manually

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used in WindowsNameTransform (for zip -> windows file name), PathNameTransform and ZipFileTransform for ZipOutputStream and FastZip/ZipEntryFactory respectively.

public static string DropPathRoot(string path)
{
string result = path;

if (!string.IsNullOrEmpty(path))
{
if ((path[0] == '\\') || (path[0] == '/'))
{
// UNC name ?
if ((path.Length > 1) && ((path[1] == '\\') || (path[1] == '/')))
{
int index = 2;
int elements = 2;

// Scan for two separate elements \\machine\share\restofpath
while ((index <= path.Length) &&
(((path[index] != '\\') && (path[index] != '/')) || (--elements > 0)))
{
index++;
}

index++;

if (index < path.Length)
{
result = path.Substring(index);
}
else
{
result = "";
}
}
}
else if ((path.Length > 1) && (path[1] == ':'))
{
int dropCount = 2;
if ((path.Length > 2) && ((path[2] == '\\') || (path[2] == '/')))
{
dropCount = 3;
}
result = result.Remove(0, dropCount);
}
}
return result;
var stripLength = Path.GetPathRoot(path).Length;
while (path.Length > stripLength && (path[stripLength] == '/' || path[stripLength] == '\\')) stripLength++;
return path.Substring(stripLength);
}

/// <summary>
Expand Down
14 changes: 14 additions & 0 deletions test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs
View file
Open in desktop
@@ -1,3 +1,4 @@
using System;
using ICSharpCode.SharpZipLib.Core;
using NUnit.Framework;

Expand Down Expand Up @@ -54,5 +55,18 @@ public void ValidFilter()
Assert.IsFalse(NameFilter.IsValidFilterExpression(@"\,)"));
Assert.IsFalse(NameFilter.IsValidFilterExpression(@"[]"));
}

[Test]
[Category("Core")]
public void DropPathRoot()
{
Func<string, string> testPath = PathUtils.DropPathRoot;
Assert.AreEqual("file.txt", testPath(@"\\server\share\file.txt"));
Assert.AreEqual("file.txt", testPath(@"c:\file.txt"));
Assert.AreEqual(@"subdir with spaces\file.txt", testPath(@"z:\subdir with spaces\file.txt"));
Assert.AreEqual("", testPath(@"\\server\share\"));
Assert.AreEqual(@"server\share\file.txt", testPath(@"\server\share\file.txt"));
Assert.AreEqual(@"path\file.txt", testPath(@"\\server\share\\path\file.txt"));
}
}
}