You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2017/09/25 23:20:56 UTC
svn commit: r1809669 - in /tomcat/trunk:
java/org/apache/catalina/webresources/AbstractFileResourceSet.java
java/org/apache/tomcat/util/compat/JrePlatform.java
test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java
Author: markt
Date: Mon Sep 25 23:20:56 2017
New Revision: 1809669
URL: http://svn.apache.org/viewvc?rev=1809669&view=rev
Log:
Add some additional checks required on Windows to keep all the checks in one place and to avoid exceptions later in the processing.
Includes utility class to determine if platform is Windows and performance test case for alternative implementations.
Added:
tomcat/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java (with props)
tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java (with props)
Modified:
tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java
Modified: tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java?rev=1809669&r1=1809668&r2=1809669&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java (original)
+++ tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java Mon Sep 25 23:20:56 2017
@@ -22,6 +22,7 @@ import java.net.MalformedURLException;
import java.net.URL;
import org.apache.catalina.LifecycleException;
+import org.apache.tomcat.util.compat.JrePlatform;
import org.apache.tomcat.util.http.RequestUtil;
public abstract class AbstractFileResourceSet extends AbstractResourceSet {
@@ -77,6 +78,12 @@ public abstract class AbstractFileResour
return file;
}
+ // Additional Windows specific checks to handle known problems with
+ // File.getCanonicalPath()
+ if (JrePlatform.IS_WINDOWS && isInvalidWindowsFilename(name)) {
+ return null;
+ }
+
// Check that this file is located under the WebResourceSet's base
String canPath = null;
try {
@@ -127,6 +134,34 @@ public abstract class AbstractFileResour
return file;
}
+
+ private boolean isInvalidWindowsFilename(String name) {
+ // For typical length file names, this is 2-3 times faster than the
+ // equivalent regular expression. The cut-over point is file names (not
+ // full paths) of ~65 characters.
+ char[] chars = name.toCharArray();
+ for (char c : chars) {
+ if (c == '\"' || c == '<' || c == '>') {
+ // These characters are disallowed in Windows file names and
+ // there are known problems for file names with these characters
+ // when using File#getCanonicalPath().
+ // Note: There are additional characters that are disallowed in
+ // Windows file names but these are not known to cause
+ // problems when using File#getCanonicalPath().
+ return true;
+ }
+ }
+ // Windows does allow file names to end in ' ' unless specific low level
+ // APIs are used to create the files that bypass various checks. File
+ // names that end in ' ' are known to cause problems when using
+ // File#getCanonicalPath().
+ if (chars[chars.length -1] == ' ') {
+ return true;
+ }
+ return false;
+ }
+
+
/**
* Return a context-relative path, beginning with a "/", that represents
* the canonical version of the specified path after ".." and "." elements
Added: tomcat/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java?rev=1809669&view=auto
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java (added)
+++ tomcat/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java Mon Sep 25 23:20:56 2017
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tomcat.util.compat;
+
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+
+public class JrePlatform {
+
+ private static final String OS_NAME_PROPERTY = "os.name";
+ private static final String OS_NAME_WINDOWS_PREFIX = "Windows";
+
+ static {
+ /*
+ * There are a few places where a) the behaviour of the Java API depends
+ * on the underlying platform and b) those behavioural differences have
+ * an impact on Tomcat.
+ *
+ * Tomcat therefore needs to be able to determine the platform it is
+ * running on to account for those differences.
+ *
+ * In an ideal world this code would not exist.
+ */
+
+ // This check is derived from the check in Apache Commons Lang
+ String osName;
+ if (System.getSecurityManager() == null) {
+ osName = System.getProperty(OS_NAME_PROPERTY);
+ } else {
+ osName = AccessController.doPrivileged(
+ new PrivilegedAction<String>() {
+
+ @Override
+ public String run() {
+ return System.getProperty(OS_NAME_PROPERTY);
+ }
+ });
+ }
+
+ IS_WINDOWS = osName.startsWith(OS_NAME_WINDOWS_PREFIX);
+ }
+
+
+ public static final boolean IS_WINDOWS;
+}
Propchange: tomcat/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java
------------------------------------------------------------------------------
svn:eol-style = native
Added: tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java?rev=1809669&view=auto
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java (added)
+++ tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java Mon Sep 25 23:20:56 2017
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.webresources;
+
+import java.util.regex.Pattern;
+
+import org.junit.Test;
+
+public class TestAbstractFileResourceSetPerformance {
+
+ private static final Pattern UNSAFE_WINDOWS_FILENAME_PATTERN = Pattern.compile(" $|[\"<>]");
+
+ private static final int LOOPS = 10_000_000;
+
+ /*
+ * Checking individual characters is about 3 times faster on markt's dev
+ * PC for typical length file names. The file names need to get to ~65
+ * characters before the Pattern matching is faster.
+ */
+ @Test
+ public void testFileNameFiltering() {
+
+ long start = System.nanoTime();
+ for (int i = 0; i < LOOPS; i++) {
+ UNSAFE_WINDOWS_FILENAME_PATTERN.matcher("testfile.jsp ").matches();
+ }
+ long end = System.nanoTime();
+ System.out.println("Regular expression took " + (end - start) + "ns or " +
+ (end-start)/LOOPS + "ns per iteration");
+
+ start = System.nanoTime();
+ for (int i = 0; i < LOOPS; i++) {
+ checkForBadChars("testfile.jsp ");
+ }
+ end = System.nanoTime();
+ System.out.println("char[] check took " + (end - start) + "ns or " +
+ (end-start)/LOOPS + "ns per iteration");
+ }
+
+ private boolean checkForBadChars(String filename) {
+ char[] chars = filename.toCharArray();
+ for (char c : chars) {
+ if (c == '\"' || c == '<' || c == '>') {
+ return false;
+ }
+ }
+ if (chars[chars.length -1] == ' ') {
+ return false;
+ }
+ return true;
+ }
+}
Propchange: tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java
------------------------------------------------------------------------------
svn:eol-style = native
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1809669 - in /tomcat/trunk: java/org/apache/catalina/webresources/AbstractFileResourceSet.java
java/org/apache/tomcat/util/compat/JrePlatform.java test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java
Posted by Konstantin Kolinko <kn...@gmail.com>.
2017-09-26 2:20 GMT+03:00 <ma...@apache.org>:
> Author: markt
> Date: Mon Sep 25 23:20:56 2017
> New Revision: 1809669
>
> URL: http://svn.apache.org/viewvc?rev=1809669&view=rev
> Log:
> Add some additional checks required on Windows to keep all the checks in one place and to avoid exceptions later in the processing.
> Includes utility class to determine if platform is Windows and performance test case for alternative implementations.
>
> Added:
> tomcat/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java (with props)
> tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java (with props)
> Modified:
> tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java
>
> Modified: tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java?rev=1809669&r1=1809668&r2=1809669&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java Mon Sep 25 23:20:56 2017
> @@ -22,6 +22,7 @@ import java.net.MalformedURLException;
> import java.net.URL;
>
> import org.apache.catalina.LifecycleException;
> +import org.apache.tomcat.util.compat.JrePlatform;
> import org.apache.tomcat.util.http.RequestUtil;
>
> public abstract class AbstractFileResourceSet extends AbstractResourceSet {
> @@ -77,6 +78,12 @@ public abstract class AbstractFileResour
> return file;
> }
>
> + // Additional Windows specific checks to handle known problems with
> + // File.getCanonicalPath()
> + if (JrePlatform.IS_WINDOWS && isInvalidWindowsFilename(name)) {
> + return null;
> + }
> +
> // Check that this file is located under the WebResourceSet's base
> String canPath = null;
> try {
> @@ -127,6 +134,34 @@ public abstract class AbstractFileResour
> return file;
> }
>
> +
> + private boolean isInvalidWindowsFilename(String name) {
> + // For typical length file names, this is 2-3 times faster than the
> + // equivalent regular expression. The cut-over point is file names (not
> + // full paths) of ~65 characters.
> + char[] chars = name.toCharArray();
> + for (char c : chars) {
> + if (c == '\"' || c == '<' || c == '>') {
> + // These characters are disallowed in Windows file names and
> + // there are known problems for file names with these characters
> + // when using File#getCanonicalPath().
> + // Note: There are additional characters that are disallowed in
> + // Windows file names but these are not known to cause
> + // problems when using File#getCanonicalPath().
> + return true;
> + }
> + }
> + // Windows does allow file names to end in ' ' unless specific low level
I think it was meant "does not"
> + // APIs are used to create the files that bypass various checks. File
> + // names that end in ' ' are known to cause problems when using
> + // File#getCanonicalPath().
> + if (chars[chars.length -1] == ' ') {
I hope that the name is not a zero-length string is already checked
somewhere. Otherwise a ArrayIndexOutOfBoundsException might happen
here.
> + return true;
> + }
> + return false;
> + }
> +
> +
> /**
> * Return a context-relative path, beginning with a "/", that represents
> * the canonical version of the specified path after ".." and "." elements
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1809669 - in /tomcat/trunk: java/org/apache/catalina/webresources/AbstractFileResourceSet.java
java/org/apache/tomcat/util/compat/JrePlatform.java test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java
Posted by Konstantin Kolinko <kn...@gmail.com>.
2017-09-26 2:20 GMT+03:00 <ma...@apache.org>:
> Author: markt
> Date: Mon Sep 25 23:20:56 2017
> New Revision: 1809669
>
> URL: http://svn.apache.org/viewvc?rev=1809669&view=rev
> Log:
> Add some additional checks required on Windows to keep all the checks in one place and to avoid exceptions later in the processing.
> Includes utility class to determine if platform is Windows and performance test case for alternative implementations.
>
> Added:
> tomcat/trunk/java/org/apache/tomcat/util/compat/JrePlatform.java (with props)
> tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java (with props)
> Modified:
> tomcat/trunk/java/org/apache/catalina/webresources/AbstractFileResourceSet.java
> Added: tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java?rev=1809669&view=auto
> ==============================================================================
> --- tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java (added)
> +++ tomcat/trunk/test/org/apache/catalina/webresources/TestAbstractFileResourceSetPerformance.java Mon Sep 25 23:20:56 2017
> +package org.apache.catalina.webresources;
> +
> +import java.util.regex.Pattern;
> +
> +import org.junit.Test;
> +
> +public class TestAbstractFileResourceSetPerformance {
> +
> + private static final Pattern UNSAFE_WINDOWS_FILENAME_PATTERN = Pattern.compile(" $|[\"<>]");
> +
> + private static final int LOOPS = 10_000_000;
> +
> + /*
> + * Checking individual characters is about 3 times faster on markt's dev
> + * PC for typical length file names. The file names need to get to ~65
> + * characters before the Pattern matching is faster.
> + */
> + @Test
> + public void testFileNameFiltering() {
> +
> + long start = System.nanoTime();
> + for (int i = 0; i < LOOPS; i++) {
> + UNSAFE_WINDOWS_FILENAME_PATTERN.matcher("testfile.jsp ").matches();
This matches() here is wrong. With the pattern above it should be ".find()".
Using find() makes the result even worse,
find(): 1641ns per iteration.
matches(): 352ns per iteration
> + }
> + long end = System.nanoTime();
> + System.out.println("Regular expression took " + (end - start) + "ns or " +
> + (end-start)/LOOPS + "ns per iteration");
> +
> + start = System.nanoTime();
> + for (int i = 0; i < LOOPS; i++) {
> + checkForBadChars("testfile.jsp ");
> + }
> + end = System.nanoTime();
> + System.out.println("char[] check took " + (end - start) + "ns or " +
> + (end-start)/LOOPS + "ns per iteration");
start = System.nanoTime();
for (int i = 0; i < LOOPS; i++) {
checkForBadChars2("testfile.jsp ");
}
end = System.nanoTime();
System.out.println("charAt() check took " + (end - start) + "ns or " +
(end-start)/LOOPS + "ns per iteration");
> + }
> +
> + private boolean checkForBadChars(String filename) {
> + char[] chars = filename.toCharArray();
> + for (char c : chars) {
> + if (c == '\"' || c == '<' || c == '>') {
> + return false;
> + }
> + }
> + if (chars[chars.length -1] == ' ') {
> + return false;
> + }
> + return true;
> + }
Second variant, without array copying:
private boolean checkForBadChars2(String filename) {
final int len = filename.length();
for (int i = 0; i < len; i++) {
char c = filename.charAt(i);
if (c == '\"' || c == '<' || c == '>') {
return false;
}
}
if (filename.charAt(len - 1) == ' ') {
return false;
}
return true;
}
My result:
[junit] char[] check took 1019605894ns or 101ns per iteration
[junit] charAt() check took 773188173ns or 77ns per iteration
[junit] char[] check took 978054736ns or 97ns per iteration
[junit] charAt() check took 842951721ns or 84ns per iteration
> +}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org