Re
Mark Wielaard wrote:
I propose to check the validity of a File resource by walking through
all the path components and making sure that all intermediate components
are valid (ie File.isDirectory and File.exists are true) and that we
never try to get "out" the root directory.
I only consider ".." as a way to escaping the root directory, it may
be more complex than that ...
".." seems a good heuristic. I assume much more will break on any
platform were ".." isn't "up in the file hierarchy".
ok, fine
I also think that extracting the path components could be improved
from a performance point of view, maybe working on the char[]
representation instead of using charAt and storing the length once for
all. I was thinking about using a StringTokenizer but it doesn't handle
'//' as a single separator and split & cie are 1.4+ additions.
You can use 1.4+ additions as long as we have implemented them. The only
real exceptions are Graphics2D and things that need 1.5 language
features. Our Graphics2D is just not mature enough to let anything
really depend on it and the 1.5 language features can only be used on
the generics branch atm.
The implementation seems correct, but as you say it could be made more
efficient. You could do the walking over the char[] or use
String.indexOf(File.separatorChar, currentIndex) to get more efficiently
at the next token. And I would try to get rid of the recursion by
keeping track of the last separator position you saw.
okey,
here is attached another version of the patch rewrote with
StringTokenizer and an iterative algorithm. It still passes all the
mauve test about resources and looks more efficient.
Cheers,
Thanks for feedback,
take care
Mark
+Olivier
Index: URLClassLoader.java
===================================================================
RCS file: /sources/classpath/classpath/java/net/URLClassLoader.java,v
retrieving revision 1.45
diff -u -r1.45 URLClassLoader.java
--- URLClassLoader.java 5 Mar 2006 17:20:51 -0000 1.45
+++ URLClassLoader.java 20 Mar 2006 19:12:45 -0000
@@ -538,6 +538,11 @@
{
try
{
+
+ // Make sure that all components in name are valid by walking through them
+ if (!checkFileValidity(name))
+ return null;
+
File file = new File(dir, name).getCanonicalFile();
if (file.exists())
return new FileResource(this, file);
@@ -548,6 +553,55 @@
}
return null;
}
+
+ /**
+ * Check all path tokens for validity. At no moment, we are allowed to reach
+ * a directory located "above" the root directory, stored in "dir" property.
+ * We are also not allowed to enter a non existing directory or a non
+ * directory component (plain file, symbolic link, ...). An empty or null
+ * path is valid. Pathnames components are separated by
+ * <code>File.separatorChar</code>
+ *
+ * @param resourceFileName the remaining name to be checked for validity.
+ * @return whether all path components are valid
+ * @see File#separatorChar
+ */
+ private boolean checkFileValidity(String resourceFileName)
+ {
+ StringTokenizer stringTokenizer = new StringTokenizer(resourceFileName, File.separator);
+ File currentFile = dir;
+ int tokenCount = stringTokenizer.countTokens();
+
+ for (int i = 0; i < tokenCount - 1; i++)
+ {
+ String currentToken = stringTokenizer.nextToken();
+
+ // If we are at the root directory and trying to go up, the walking is
+ // finished with an error
+ if ("..".equals(currentToken) && currentFile.equals(dir))
+ return false;
+
+ currentFile = new File(currentFile, currentToken);
+
+ // If the current file doesn't exist or is not a directory, the walking is
+ // finished with an error
+ if (! (currentFile.exists() && currentFile.isDirectory()))
+ return false;
+
+ }
+
+ // Treat the last token differently, if it exists, because it does not need
+ // to be a directory
+ if (tokenCount > 0)
+ {
+ String currentToken = stringTokenizer.nextToken();
+
+ if ("..".equals(currentToken) && currentFile.equals(dir))
+ return false;
+ }
+
+ return true;
+ }
}
static final class FileResource extends Resource
