Bug 573456 - [performance] Avoid File.getCanonicalPathY20210810-0820Y20210810-0500Y20210805-0800Y20210804-1030I20210811-1800I20210810-1800I20210809-1800I20210808-1800I20210807-1800I20210806-1800I20210805-1800I20210804-1800I20210804-0930

File.getCanonicalPath is much slower then URI.normalize under windows
Also avoid normalization if no ".." is involved.

Change-Id: I641a64d48f8acb9cf793c948d08c51ace45aec50
Signed-off-by: Joerg Kubitz <jkubitz-eclipse@gmx.de>
Reviewed-on: https://git.eclipse.org/r/c/equinox/rt.equinox.framework/+/180404
Tested-by: Equinox Bot <equinox-bot@eclipse.org>
Reviewed-by: Thomas Watson <tjwatson@us.ibm.com>

4 files changed, 108 insertions, 10 deletions

diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/util/AllTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/util/AllTests.java
index beaa87133..9ac4293dd 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/util/AllTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/util/AllTests.java
@@ -30,6 +30,7 @@ public class AllTests extends TestSuite {
 		addTest(new TestSuite(ObjectPoolTestCase.class));
 		addTest(new JUnit4TestAdapter(ManifestElementTestCase.class));
 		addTest(new TestSuite(NLSTestCase.class));
+		addTest(new TestSuite(StorageUtilTestCase.class));
 		addBidiTests();
 		addLatinTests();
 	}
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/util/StorageUtilTestCase.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/util/StorageUtilTestCase.java
new file mode 100644
index 000000000..749138288
--- /dev/null
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/util/StorageUtilTestCase.java
@@ -0,0 +1,53 @@
+/*******************************************************************************
+ * Copyright (c) 2011, 2013 IBM Corporation and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *     IBM Corporation - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.osgi.tests.util;
+
+import java.io.File;
+import org.eclipse.core.runtime.Platform;
+import org.eclipse.core.tests.harness.CoreTest;
+import org.eclipse.osgi.storage.StorageUtil;
+import org.junit.Test;
+
+public class StorageUtilTestCase extends CoreTest {
+
+	@Test
+	public void testRegularWindowsFileName() {
+		String[] validFilenames = { //
+				"something\\somethingelse", // normal file
+				"something/somethingelse", // normal file
+				"COM1anything", // normal file
+				"COM1/anything", // illegal directory name but normal file
+				".temp", // It is acceptable to specify a period as the first character of a
+							// name. For example, ".temp".
+				"COM56", // there is no predefined NT namespace for COM56.
+		};
+		for (String validFilename : validFilenames) {
+			assertFalse(validFilename, StorageUtil.isReservedFileName(new File(validFilename)));
+		}
+		// test reserved names according to
+		// https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file
+		String[] invalidFilenames = { //
+				"CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9",
+				"LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", // reserved names
+				"COM1.anything", "NUL.txt", // "Also avoid these names followed immediately by an extension; for
+											// example, NUL.txt is not recommended"
+				"COM1", "com1", "coM1"// case insensitive
+		};
+		boolean isWindows = Platform.getOS().equals(Platform.OS_WIN32);
+		for (String invalidFilename : invalidFilenames) {
+			assertTrue(invalidFilename, isWindows == StorageUtil.isReservedFileName(new File(invalidFilename)));
+		}
+	}
+
+}
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
index f7be9b57c..2f6d93984 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
@@ -30,6 +30,7 @@ import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.security.PrivilegedActionException;
@@ -1078,18 +1079,19 @@ public class Storage {
 		}
 
 		// if base is not null then move root to include the base
-		root = new File(root, base);
-		File result = new File(root, path);
-
-		// do the extra check to make sure the path did not escape the root path
-		try {
-			String resultCanonical = result.getCanonicalPath();
-			String rootCanonical = root.getCanonicalPath();
-			if (!resultCanonical.startsWith(rootCanonical + File.separator) && !resultCanonical.equals(rootCanonical)) {
+		File rootBase = new File(root, base);
+		File result = new File(rootBase, path);
+		if (path.contains("..")) { //$NON-NLS-1$
+			// do the extra check to make sure the path did not escape the root path
+			Path resultNormalized = result.toPath().normalize();
+			Path rootBaseNormalized = rootBase.toPath().normalize();
+			if (!resultNormalized.startsWith(rootBaseNormalized)) {
 				throw new StorageException("Invalid path: " + path); //$NON-NLS-1$
 			}
-		} catch (IOException e) {
-			throw new StorageException("Invalid path: " + path, e); //$NON-NLS-1$
+		}
+		// Additional check if it is a special device instead of a regular file.
+		if (StorageUtil.isReservedFileName(result)) {
+			throw new StorageException("Invalid filename: " + path); //$NON-NLS-1$
 		}
 		return result;
 	}
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/StorageUtil.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/StorageUtil.java
index 6af3d74d6..4f4ebc37e 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/StorageUtil.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/StorageUtil.java
@@ -25,7 +25,9 @@ import java.net.MalformedURLException;
 import java.net.URL;
 import java.nio.file.Files;
 import java.nio.file.StandardCopyOption;
+import java.util.Arrays;
 import java.util.Dictionary;
+import java.util.HashSet;
 import java.util.Hashtable;
 import org.eclipse.osgi.internal.debug.Debug;
 import org.osgi.framework.BundleContext;
@@ -256,4 +258,44 @@ public class StorageUtil {
 			Debug.println("Successfully moved file: " + from + " to " + to); //$NON-NLS-1$ //$NON-NLS-2$
 		}
 	}
+
+	private static final boolean IS_WINDOWS = "\\\\.\\NUL" //$NON-NLS-1$
+			.equals(getDeviceName(new File("NUL"))); //$NON-NLS-1$
+
+	private static String getDeviceName(File file) {
+		try {
+			return file.getCanonicalPath();
+		} catch (IOException e) {
+			return null;
+		}
+	}
+
+	// reserved names according to
+	// https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file
+	private static HashSet<String> RESERVED_NAMES = new HashSet<>(
+			Arrays.asList(new String[] { "aux", "com1", "com2", "com3", "com4", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$
+					"com5", "com6", "com7", "com8", "com9", "con", "lpt1", "lpt2", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$ //$NON-NLS-8$
+					"lpt3", "lpt4", "lpt5", "lpt6", "lpt7", "lpt8", "lpt9", "nul", "prn" })); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$ //$NON-NLS-8$ //$NON-NLS-9$
+
+	/** Tests whether the filename can escape path into special device **/
+	public static boolean isReservedFileName(File file) {
+		// Directory names are not checked here because illegal directory names will be
+		// handled by OS.
+		String fileName = file.getName();
+		return isReservedFileName(fileName);
+	}
+
+	private static boolean isReservedFileName(String name) {
+		// Illegal characters are not checked here because they are check by both JDK
+		// and OS. This is only a check against technical allowed but unwanted device
+		// names.
+		if (!IS_WINDOWS) { // only windows has special file names which can escape any path
+			return false;
+		}
+		int dot = name.indexOf('.');
+		// on windows, filename suffixes are not relevant to name validity
+		String basename = dot == -1 ? name : name.substring(0, dot);
+		return RESERVED_NAMES.contains(basename.toLowerCase());
+	}
+
 }