From 1b5266786faccc58fbf54e84655c22cc9b7ab007 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sun, 7 Mar 2021 18:12:54 +0100 Subject: [PATCH 1/4] Simplify DropPathRoot and fix out of bounds issue --- src/ICSharpCode.SharpZipLib/Core/PathUtils.cs | 45 ++----------------- .../Core/CoreTests.cs | 14 ++++++ 2 files changed, 17 insertions(+), 42 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs b/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs index 2d6121786..8e6497c9e 100644 --- a/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs +++ b/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs @@ -15,48 +15,9 @@ public static class PathUtils /// Unlike the class the path isn't otherwise checked for validity. 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); } /// diff --git a/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs b/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs index 985718fb2..04ee1d800 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs @@ -1,3 +1,4 @@ +using System; using ICSharpCode.SharpZipLib.Core; using NUnit.Framework; @@ -54,5 +55,18 @@ public void ValidFilter() Assert.IsFalse(NameFilter.IsValidFilterExpression(@"\,)")); Assert.IsFalse(NameFilter.IsValidFilterExpression(@"[]")); } + + [Test] + [Category("Core")] + public void DropPathRoot() + { + Func 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")); + } } } From 0e522c056d41278e7477fe1577eddfa5bf07fcea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sun, 14 Mar 2021 10:03:27 +0100 Subject: [PATCH 2/4] prevent throwing in DropPathRoot and add tests --- src/ICSharpCode.SharpZipLib/Core/PathUtils.cs | 20 ++++++++++--- .../Core/CoreTests.cs | 30 ++++++++++++++----- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs b/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs index 8e6497c9e..8f8e5121a 100644 --- a/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs +++ b/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs @@ -1,4 +1,6 @@ +using System; using System.IO; +using System.Linq; namespace ICSharpCode.SharpZipLib.Core { @@ -8,14 +10,24 @@ namespace ICSharpCode.SharpZipLib.Core public static class PathUtils { /// - /// Remove any path root present in the path + /// Remove any path root present in the path and optionally replaces invalid path chars, + /// as indicated by , with '_' /// /// A containing path information. + /// Replaces any invalid path chars /// The path with the root removed if it was present; path otherwise. - /// Unlike the class the path isn't otherwise checked for validity. - public static string DropPathRoot(string path) + public static string DropPathRoot(string path, bool replaceInvalidChars = false) { - var stripLength = Path.GetPathRoot(path).Length; + // Replace any invalid path characters with '_' to prevent Path.GetPathRoot throwing + var invalidChars = Path.GetInvalidPathChars(); + var cleanPath = new string(path.Select(c => invalidChars.Contains(c) ? '_' : c).ToArray()); + + if (replaceInvalidChars) + { + path = cleanPath; + } + + var stripLength = Path.GetPathRoot(cleanPath).Length; while (path.Length > stripLength && (path[stripLength] == '/' || path[stripLength] == '\\')) stripLength++; return path.Substring(stripLength); } diff --git a/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs b/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs index 04ee1d800..555cfb48c 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs @@ -1,4 +1,5 @@ using System; +using System.IO; using ICSharpCode.SharpZipLib.Core; using NUnit.Framework; @@ -60,13 +61,28 @@ public void ValidFilter() [Category("Core")] public void DropPathRoot() { - Func 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")); + string TestPath(string s) => PathUtils.DropPathRoot(s, replaceInvalidChars: false); + 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")); + Assert.DoesNotThrow(() => Console.WriteLine(TestPath(@"c:\file:+/"))); + Assert.DoesNotThrow(() => Console.WriteLine(TestPath(@"c:\file*?"))); + Assert.DoesNotThrow(() => Console.WriteLine(TestPath("c:\\file|\""))); + Assert.DoesNotThrow(() => Console.WriteLine(TestPath(@"c:\file<>"))); + } + + [Test] + [TestCase(@"c:\file:+/")] + [TestCase(@"c:\file*?")] + [TestCase("c:\\file|\"")] + [TestCase(@"c:\file<>")] + [Category("Core")] + public void DropPathRoot_DoesNotThrowForInvalidPath(string path) + { + Assert.DoesNotThrow(() => Console.WriteLine(PathUtils.DropPathRoot(path))); } } } From cc766e504076100358b623ecbbda8f48823c19f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sun, 14 Mar 2021 13:01:33 +0100 Subject: [PATCH 3/4] make path tests platform-specific testing for UNC paths on *nix does not really make sense --- .../Core/CoreTests.cs | 37 +++++++---- .../TestSupport/SevenZip.cs | 17 ++++- .../TestSupport/Utils.cs | 7 ++ .../Zip/ZipFileHandling.cs | 9 ++- .../Zip/ZipNameTransformHandling.cs | 66 ++++++++++++------- 5 files changed, 97 insertions(+), 39 deletions(-) diff --git a/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs b/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs index 555cfb48c..757e3b84c 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs @@ -1,5 +1,4 @@ using System; -using System.IO; using ICSharpCode.SharpZipLib.Core; using NUnit.Framework; @@ -56,22 +55,34 @@ public void ValidFilter() Assert.IsFalse(NameFilter.IsValidFilterExpression(@"\,)")); Assert.IsFalse(NameFilter.IsValidFilterExpression(@"[]")); } + + private static string DropRoot(string s) => PathUtils.DropPathRoot(s, replaceInvalidChars: false); [Test] [Category("Core")] - public void DropPathRoot() + [Platform("Win")] + public void DropPathRoot_Windows() { - string TestPath(string s) => PathUtils.DropPathRoot(s, replaceInvalidChars: false); - 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")); - Assert.DoesNotThrow(() => Console.WriteLine(TestPath(@"c:\file:+/"))); - Assert.DoesNotThrow(() => Console.WriteLine(TestPath(@"c:\file*?"))); - Assert.DoesNotThrow(() => Console.WriteLine(TestPath("c:\\file|\""))); - Assert.DoesNotThrow(() => Console.WriteLine(TestPath(@"c:\file<>"))); + Assert.AreEqual("file.txt", DropRoot(@"\\server\share\file.txt")); + Assert.AreEqual("file.txt", DropRoot(@"c:\file.txt")); + Assert.AreEqual(@"subdir with spaces\file.txt", DropRoot(@"z:\subdir with spaces\file.txt")); + Assert.AreEqual("", DropRoot(@"\\server\share\")); + Assert.AreEqual(@"server\share\file.txt", DropRoot(@"\server\share\file.txt")); + Assert.AreEqual(@"path\file.txt", DropRoot(@"\\server\share\\path\file.txt")); + } + + [Test] + [Category("Core")] + [Platform(Exclude="Win")] + public void DropPathRoot_Posix() + { + Assert.AreEqual("file.txt", DropRoot("/file.txt")); + Assert.AreEqual(@"tmp/file.txt", DropRoot(@"/tmp/file.txt")); + Assert.AreEqual(@"tmp\file.txt", DropRoot(@"\tmp\file.txt")); + Assert.AreEqual(@"tmp/file.txt", DropRoot(@"\tmp/file.txt")); + Assert.AreEqual(@"tmp\file.txt", DropRoot(@"/tmp\file.txt")); + Assert.AreEqual("", DropRoot("/")); + } [Test] diff --git a/test/ICSharpCode.SharpZipLib.Tests/TestSupport/SevenZip.cs b/test/ICSharpCode.SharpZipLib.Tests/TestSupport/SevenZip.cs index c312ec401..e9887172a 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/TestSupport/SevenZip.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/TestSupport/SevenZip.cs @@ -17,7 +17,7 @@ internal static class SevenZipHelper Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ProgramFilesX86), "7-Zip", "7z.exe"), }; - public static bool TryGet7zBinPath(out string path7z) + private static bool TryGet7zBinPath(out string path7z) { var runTimeLimit = TimeSpan.FromSeconds(3); @@ -77,12 +77,25 @@ internal static void VerifyZipWith7Zip(Stream zipStream, string password) zipStream.CopyTo(fs); } - var p = Process.Start(path7z, $"t -p{password} \"{fileName}\""); + var p = Process.Start(new ProcessStartInfo(path7z, $"t -p{password} \"{fileName}\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + }); + + if (p == null) + { + Assert.Inconclusive("Failed to start 7z process. Skipping!"); + } if (!p.WaitForExit(2000)) { Assert.Warn("Timed out verifying zip file!"); } + TestContext.Out.Write(p.StandardOutput.ReadToEnd()); + var errors = p.StandardError.ReadToEnd(); + Assert.IsEmpty(errors, "7z reported errors"); Assert.AreEqual(0, p.ExitCode, "Archive verification failed"); } finally diff --git a/test/ICSharpCode.SharpZipLib.Tests/TestSupport/Utils.cs b/test/ICSharpCode.SharpZipLib.Tests/TestSupport/Utils.cs index 9c582daa6..33d6e3e9b 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/TestSupport/Utils.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/TestSupport/Utils.cs @@ -13,9 +13,16 @@ public static class Utils public static int DummyContentLength = 16; private static Random random = new Random(); + + /// + /// Returns the system root for the current platform (usually c:\ for windows and / for others) + /// + public static string SystemRoot { get; } = + Path.GetPathRoot(Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData)); private static void Compare(byte[] a, byte[] b) { + if (a == null) { throw new ArgumentNullException(nameof(a)); diff --git a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs index 9d50fb844..a9a7583fc 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs @@ -23,6 +23,7 @@ public void NullStreamDetected() try { + // ReSharper disable once ExpressionIsAlwaysNull bad = new ZipFile(nullStream); } catch @@ -439,17 +440,21 @@ public void AddAndDeleteEntriesMemory() f.IsStreamOwner = false; f.BeginUpdate(new MemoryArchiveStorage()); - f.Add(new StringMemoryDataSource("Hello world"), @"z:\a\a.dat"); + f.Add(new StringMemoryDataSource("Hello world"), Utils.SystemRoot + @"a\a.dat"); f.Add(new StringMemoryDataSource("Another"), @"\b\b.dat"); f.Add(new StringMemoryDataSource("Mr C"), @"c\c.dat"); f.Add(new StringMemoryDataSource("Mrs D was a star"), @"d\d.dat"); f.CommitUpdate(); Assert.IsTrue(f.TestArchive(true)); + foreach (ZipEntry entry in f) + { + Console.WriteLine($" - {entry.Name}"); + } } byte[] master = memStream.ToArray(); - TryDeleting(master, 4, 1, @"z:\a\a.dat"); + TryDeleting(master, 4, 1, Utils.SystemRoot + @"a\a.dat"); TryDeleting(master, 4, 1, @"\a\a.dat"); TryDeleting(master, 4, 1, @"a/a.dat"); diff --git a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipNameTransformHandling.cs b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipNameTransformHandling.cs index 498a8f723..e51b8f19d 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipNameTransformHandling.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipNameTransformHandling.cs @@ -3,6 +3,7 @@ using NUnit.Framework; using System; using System.IO; +using ICSharpCode.SharpZipLib.Tests.TestSupport; namespace ICSharpCode.SharpZipLib.Tests.Zip { @@ -16,8 +17,6 @@ public void Basic() var t = new ZipNameTransform(); TestFile(t, "abcdef", "abcdef"); - TestFile(t, @"\\uncpath\d1\file1", "file1"); - TestFile(t, @"C:\absolute\file2", "absolute/file2"); // This is ignored but could be converted to 'file3' TestFile(t, @"./file3", "./file3"); @@ -28,50 +27,73 @@ public void Basic() // Trick filenames. TestFile(t, @".....file3", ".....file3"); + } + + [Test] + [Category("Zip")] + [Platform("Win")] + public void Basic_Windows() + { + var t = new ZipNameTransform(); + TestFile(t, @"\\uncpath\d1\file1", "file1"); + TestFile(t, @"C:\absolute\file2", "absolute/file2"); + TestFile(t, @"c::file", "_file"); } + + [Test] + [Category("Zip")] + [Platform(Exclude="Win")] + public void Basic_Posix() + { + var t = new ZipNameTransform(); + TestFile(t, @"backslash_path\file1", "backslash_path/file1"); + TestFile(t, "/absolute/file2", "absolute/file2"); + + TestFile(t, @"////////:file", "_file"); + } [Test] public void TooLong() { var zt = new ZipNameTransform(); - var veryLong = new string('x', 65536); - try - { - zt.TransformDirectory(veryLong); - Assert.Fail("Expected an exception"); - } - catch (PathTooLongException) - { - } + var tooLong = new string('x', 65536); + Assert.Throws(() => zt.TransformDirectory(tooLong)); } [Test] public void LengthBoundaryOk() { var zt = new ZipNameTransform(); - string veryLong = "c:\\" + new string('x', 65535); - try - { - zt.TransformDirectory(veryLong); - } - catch - { - Assert.Fail("Expected no exception"); - } + var tooLongWithRoot = Utils.SystemRoot + new string('x', 65535); + Assert.DoesNotThrow(() => zt.TransformDirectory(tooLongWithRoot)); } [Test] [Category("Zip")] - public void NameTransforms() + [Platform("Win")] + public void NameTransforms_Windows() { INameTransform t = new ZipNameTransform(@"C:\Slippery"); Assert.AreEqual("Pongo/Directory/", t.TransformDirectory(@"C:\Slippery\Pongo\Directory"), "Value should be trimmed and converted"); Assert.AreEqual("PoNgo/Directory/", t.TransformDirectory(@"c:\slipperY\PoNgo\Directory"), "Trimming should be case insensitive"); - Assert.AreEqual("slippery/Pongo/Directory/", t.TransformDirectory(@"d:\slippery\Pongo\Directory"), "Trimming should be case insensitive"); + Assert.AreEqual("slippery/Pongo/Directory/", t.TransformDirectory(@"d:\slippery\Pongo\Directory"), "Trimming should account for root"); Assert.AreEqual("Pongo/File", t.TransformFile(@"C:\Slippery\Pongo\File"), "Value should be trimmed and converted"); } + + [Test] + [Category("Zip")] + [Platform(Exclude="Win")] + public void NameTransforms_Posix() + { + INameTransform t = new ZipNameTransform(@"/Slippery"); + Assert.AreEqual("Pongo/Directory/", t.TransformDirectory(@"/Slippery\Pongo\Directory"), "Value should be trimmed and converted"); + Assert.AreEqual("PoNgo/Directory/", t.TransformDirectory(@"/slipperY\PoNgo\Directory"), "Trimming should be case insensitive"); + Assert.AreEqual("slippery/Pongo/Directory/", t.TransformDirectory(@"/slippery/slippery/Pongo/Directory"), "Trimming should account for root"); + + Assert.AreEqual("Pongo/File", t.TransformFile(@"/Slippery/Pongo/File"), "Value should be trimmed and converted"); + } /// /// Test ZipEntry static file name cleaning methods From b9287309c189cfb9e081ca8add5207c6c2e101c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sun, 14 Mar 2021 14:06:17 +0100 Subject: [PATCH 4/4] handle .NET < 4.6.2 special cases --- src/ICSharpCode.SharpZipLib/Core/PathUtils.cs | 21 +++++++++---------- .../Core/CoreTests.cs | 8 ++++++- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs b/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs index 8f8e5121a..77c4b07fc 100644 --- a/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs +++ b/src/ICSharpCode.SharpZipLib/Core/PathUtils.cs @@ -10,23 +10,22 @@ namespace ICSharpCode.SharpZipLib.Core public static class PathUtils { /// - /// Remove any path root present in the path and optionally replaces invalid path chars, - /// as indicated by , with '_' + /// Remove any path root present in the path /// /// A containing path information. - /// Replaces any invalid path chars /// The path with the root removed if it was present; path otherwise. - public static string DropPathRoot(string path, bool replaceInvalidChars = false) + public static string DropPathRoot(string path) { - // Replace any invalid path characters with '_' to prevent Path.GetPathRoot throwing var invalidChars = Path.GetInvalidPathChars(); - var cleanPath = new string(path.Select(c => invalidChars.Contains(c) ? '_' : c).ToArray()); - - if (replaceInvalidChars) - { - path = cleanPath; - } + // If the first character after the root is a ':', .NET < 4.6.2 throws + var cleanRootSep = path.Length >= 3 && path[1] == ':' && path[2] == ':'; + // Replace any invalid path characters with '_' to prevent Path.GetPathRoot from throwing. + // Only pass the first 258 (should be 260, but that still throws for some reason) characters + // as .NET < 4.6.2 throws on longer paths + var cleanPath = new string(path.Take(258) + .Select( (c, i) => invalidChars.Contains(c) || (i == 2 && cleanRootSep) ? '_' : c).ToArray()); + var stripLength = Path.GetPathRoot(cleanPath).Length; while (path.Length > stripLength && (path[stripLength] == '/' || path[stripLength] == '\\')) stripLength++; return path.Substring(stripLength); diff --git a/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs b/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs index 757e3b84c..1b43c79ae 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs @@ -56,7 +56,8 @@ public void ValidFilter() Assert.IsFalse(NameFilter.IsValidFilterExpression(@"[]")); } - private static string DropRoot(string s) => PathUtils.DropPathRoot(s, replaceInvalidChars: false); + // Use a shorter name wrapper to make tests more legible + private static string DropRoot(string s) => PathUtils.DropPathRoot(s); [Test] [Category("Core")] @@ -90,6 +91,11 @@ public void DropPathRoot_Posix() [TestCase(@"c:\file*?")] [TestCase("c:\\file|\"")] [TestCase(@"c:\file<>")] + [TestCase(@"c:file")] + [TestCase(@"c::file")] + [TestCase(@"c:?file")] + [TestCase(@"c:+file")] + [TestCase(@"cc:file")] [Category("Core")] public void DropPathRoot_DoesNotThrowForInvalidPath(string path) {