Simplify DropPathRoot and fix out of bounds issue #593
Conversation
| @@ -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> | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The results are in!
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 _?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
It's used in WindowsNameTransform (for zip -> windows file name), PathNameTransform and ZipFileTransform for ZipOutputStream and FastZip/ZipEntryFactory respectively.
testing for UNC paths on *nix does not really make sense
Fixes #56
This did grow a bit, since a lot of stuff was discovered while working on this.
Prior to this, all tests (and indeed the behaviour) for handling paths just presumed that the platform was Windows.
The core problem method (DropPathRoot) has now been changed to use the framework/platforms path handling methods, but with some special exceptions for .NET < 4.6.2.
This does feel a bit hackish, but I think it's better than the previous method, which was to implement custom code for determining what a root path is. If that code fails to notice something that the OS would consider a absolute path it could lead to vulnerabilities. If this implementation fails to notice it, it would instead throw, which could then be fixed.
And also, since this only happens in older frameworks, in most cases it would just work as intended.
If creating from a windows OS, it will strip what the OS considers to be a root path from the file name, which ensures "correct" entries in our archive, and on reading entries from an archive, it will strip them as appropriate for the OS.
This PR also includes updates to tests which now have separated versions for windows and non-windows platforms, and also pipes the output of
7zto the test context, so that it will only be displayed if a test fails.I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.