PR #593: Simplify DropPathRoot and fix out of bounds issue

* Simplify DropPathRoot and fix out of bounds issue

* prevent throwing in DropPathRoot and add tests

* make path tests platform-specific

testing for UNC paths on *nix does not really make sense

* handle .NET < 4.6.2 special cases

Author: piksel

src/ICSharpCode.SharpZipLib/Core/PathUtils.cs

@@ -1,4 +1,6 @@
+using System;
 using System.IO;
+using System.Linq;
 
 namespace ICSharpCode.SharpZipLib.Core
 {
@@ -12,51 +14,21 @@ public static class PathUtils
 		/// </summary>
 		/// <param name="path">A <see cref="string"/> containing path information.</param>
 		/// <returns>The path with the root removed if it was present; path otherwise.</returns>
-		/// <remarks>Unlike the <see cref="System.IO.Path"/> class the path isn't otherwise checked for validity.</remarks>
 		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 invalidChars = Path.GetInvalidPathChars();
+			// 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);
 		}
 
 		/// <summary>

test/ICSharpCode.SharpZipLib.Tests/Core/CoreTests.cs

@@ -1,3 +1,4 @@
+using System;
 using ICSharpCode.SharpZipLib.Core;
 using NUnit.Framework;
 
@@ -54,5 +55,51 @@ public void ValidFilter()
 			Assert.IsFalse(NameFilter.IsValidFilterExpression(@"\,)"));
 			Assert.IsFalse(NameFilter.IsValidFilterExpression(@"[]"));
 		}
+
+		// Use a shorter name wrapper to make tests more legible
+		private static string DropRoot(string s) => PathUtils.DropPathRoot(s);
+		
+		[Test]
+		[Category("Core")]
+		[Platform("Win")]
+		public void DropPathRoot_Windows()
+		{
+			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]
+		[TestCase(@"c:\file:+/")]
+		[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)
+		{
+			Assert.DoesNotThrow(() => Console.WriteLine(PathUtils.DropPathRoot(path)));
+		}
 	}
 }

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

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();
+		
+		/// <summary>
+		/// Returns the system root for the current platform (usually c:\ for windows and / for others)
+		/// </summary>
+		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));

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");
 

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<PathTooLongException>(() => 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");
+		}
 
 		/// <summary>
 		/// Test ZipEntry static file name cleaning methods