[rfc][icedtea-web] Use File for cache directory instead of String
Jie Kang
jkang at redhat.com
Mon Jan 19 16:55:10 UTC 2015
----- Original Message -----
> Hello there!
>
> On 01/15/2015 at 08:17 PM Jie Kang wrote:
> > Hello,
> >
> > This patch changes the type of cacheDir from String to File.
> >
> > This patch is a changeset on top of the patch "Allow cache directory to be
> > changed during runtime" discussed here[1]. Both patches have been attached
> > for
> > convenience.
> >
> > [1]
> > http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2015-January/030316.html
> >
> > Patch order: HEAD -->> itw-change-cache-location -->>
> > itw-cache-string-to-file
> >
> >
> > How does it look?
>
> A few things:
>
> > diff --git a/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
> > b/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
> > --- a/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
> > +++ b/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
> > […]
> > @@ -80,11 +80,11 @@
> >
> > private CacheLRUWrapper() {
> > String setCachePath =
> > JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR);
> > - String cacheDir = new File(setCachePath != null ? setCachePath :
> > System.getProperty("java.io.tmpdir")).getPath();
> > + File cacheDir = new File(setCachePath != null ? setCachePath :
> > System.getProperty("java.io.tmpdir"));
> > setupCacheDir(cacheDir);
> > }
> >
> > - private void setupCacheDir(String cacheDir) {
> > + private void setupCacheDir(File cacheDir) {
> > this.cacheDir = cacheDir;
> > cacheOrder = new PropertiesFile(
> > new File(cacheDir + File.separator +
> > CACHE_INDEX_FILE_NAME));
>
> Warning! "cacheDir + File.separator + CACHE_INDEX_FILE_NAME" evaluates to
> "cacheDir.toString() + File.separator + CACHE_INDEX_FILE_NAME".
> You should carefully check other locations where CacheLRUWrapper.cacheDir
> might
> evaluate to File.toString() instead of File.getPath(). Or, where rather
> File.toPath() should be used.
File.toString() == File.getPath() [1]. But I have added .getPath() to all locations where it is unclear to be more explicit.
[1] http://docs.oracle.com/javase/7/docs/api/java/io/File.html#toString()
>
> > @@ -152,7 +152,7 @@
> >
> > // 2. check path format - does the path look correct?
> > if (path != null) {
> > - if (path.indexOf(cacheDir) < 0) {
> > + if (path.indexOf(cacheDir.getPath()) < 0) {
> > it.remove();
> > modified = true;
> > }
>
> Is this piece of code still required after CacheLRUWrapper.cacheDir has been
> changed to java.io.File? java.io.File already does proper system specific
> path
> syntax checking internally upon construction.
Yes it's still required. This code has nothing to do with 'proper system specific path syntax checking'. It is validating all the entries in the LRU file to make sure they seem sane. In this case, it is making sure the entry in the LRU is actually in the cache directory.
Whether or not this is the 'best' way to do this is up for debate, but it isn't in the scope of this patch.
>
> > @@ -206,7 +206,7 @@
> > }
> >
> > private String getIdForCacheFolder(String folder) {
> > - int len = cacheDir.length();
> > + int len = cacheDir.getPath().length();
> > int index = folder.indexOf(File.separatorChar, len + 1);
> > return folder.substring(len + 1, index);
> > }
>
> Err, I do not want to sound like a smart ass but this whole ID collocation in
> a
> file path scheme looks bogous and error prone to me.
> Warning! folder.length() may be smaller than cacheDir.length() + 1! So,
> getting
> the ID by analyzing a File object's String path instead of using
> File.toPath()
> since JDK 1.7 is probably not the safest thing to do.
>
> > […]
> > diff --git a/netx/net/sourceforge/jnlp/cache/CacheUtil.java
> > b/netx/net/sourceforge/jnlp/cache/CacheUtil.java
> > --- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java
> > +++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java
> > […]
> > @@ -319,7 +319,7 @@
> > * Get the path to file minus the cache directory and indexed folder.
> > */
> > private static String pathToURLPath(String path) {
> > - int len = lruHandler.getCacheDir().length();
> > + int len = lruHandler.getCacheDir().getPath().length();
> > int index = path.indexOf(File.separatorChar, len + 1);
> > return path.substring(index);
> > }
>
> Again, this is probably not the safest thing to do since we have
> java.nio.file.Path since JDK 1.7 at our disposal. Beware though that
> java.nio.file.Path is a system dependent representation of a path. If you
> really want to be system independent you probably gonna have to stick to
> URIs.
> However, only URIs having the "file:" scheme should be or rather must be
> allowed for cached resources in order to prevent malicious code from loading
> supposedly cached resources from arbitrary network locations.
>
> > @@ -332,7 +332,7 @@
> > String path = filePath;
> > String tempPath = "";
> >
> > - String cacheDir = lruHandler.getCacheDir();
> > + String cacheDir = lruHandler.getCacheDir().getPath();
> >
> > while(path.startsWith(cacheDir) && !path.equals(cacheDir)){
> > tempPath = new File(path).getParent();
>
> Same goes here for parsing path Strings. Strangely, the last line seems to be
> doing it the safe way, yet does not use the even safer way by calling
> File.getParentFile().
>
> > @@ -567,7 +567,7 @@
> > * rStr first becomes:
> > /0/http/www.example.com/subdir/a.jar
> > * then rstr becomes: /home/user1/.icedtea/cache/0
> > */
> > - String rStr =
> > file.getPath().substring(lruHandler.getCacheDir().length());
> > + String rStr =
> > file.getPath().substring(lruHandler.getCacheDir().getPath().length());
> > rStr = lruHandler.getCacheDir() + rStr.substring(0,
> > rStr.indexOf(File.separatorChar, 1));
> > long len = file.length();
>
> This is very difficult to read and understand. Besides, there are multiple
> unneccessary calls to CacheLRUWrapper.getCacheDir() and multiple assignments
> to
> rStr.
The three comments above have been noted, however they aren't in the scope of this patch.
At the moment the caching code makes a huge number of assumptions on the correctness of the cache on the file-system. This is obviously a bad idea but surprisingly has led to only a few bugs, afaik. Work can be done in this area and your comments are reasonable, but the changes will definitely not be in this patch.
>
> > […]
> > public class CacheLRUWrapperTest {
> >
> > private static final CacheLRUWrapper clw =
> > CacheLRUWrapper.getInstance();
> > - private static String cacheDirBackup;
> > + private static File cacheDirBackup;
> > private static PropertiesFile cacheOrderBackup;
> > // does no DeploymentConfiguration exist for this file name?
> > private static final String cacheIndexFileName =
> > CacheLRUWrapper.CACHE_INDEX_FILE_NAME + "_testing";
> > @@ -72,7 +72,7 @@
> > static public void setupJNLPRuntimeConfig() {
> > cacheDirBackup = clw.cacheDir;
> > cacheOrderBackup = clw.cacheOrder;
> > - clw.cacheDir=System.getProperty("java.io.tmpdir");
> > + clw.cacheDir = new File(System.getProperty("java.io.tmpdir"));
> > clw.cacheOrder = new PropertiesFile( new File(clw.cacheDir +
> > File.separator + cacheIndexFileName));
>
> Watch it here!
>
> clw.cacheDir + File.separator + cacheIndexFileName
>
> evaluates to
>
> clw.cacheDir.toString() + File.separator + cacheIndexFileName
>
> What you probably have meant was
>
> clw.cacheDir.getPath() + File.separator + cacheIndexFileName
>
> > […]
> > diff --git
> > a/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTrackerTest.java
> > b/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTrackerTest.java
> > --- a/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTrackerTest.java
> > +++ b/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTrackerTest.java
> > @@ -445,8 +445,6 @@
> > Assert.assertTrue(u.getQuery().contains("version-id=1.0"));
> > }
> >
> > - private static String cacheDir;
> > -
> > @BeforeClass
> > public static void setupDownloadServer() throws IOException {
> > File dir = new File(System.getProperty("java.io.tmpdir"),
> > "itw-down");
> > @@ -456,8 +454,8 @@
> > downloadServer =
> > ServerAccess.getIndependentInstance(dir.getAbsolutePath(),
> > ServerAccess.findFreePort());
> > redirectErrBack();
> >
> > - String tempCache = System.getProperty("java.io.tmpdir") +
> > File.separator + "tempcache";
> > - CacheUtil.setCacheDir(tempCache);
> > + File tempCacheDir = new File(System.getProperty("java.io.tmpdir")
> > +
> > File.separator + "tempcache");
> > + CacheUtil.setCacheDir(tempCacheDir);
> > }
> >
> > @AfterClass
> > @@ -465,7 +463,8 @@
> > downloadServer.stop();
> >
> > CacheUtil.clearCache();
> > -
> > CacheUtil.setCacheDir(JNLPRuntime.getConfiguration().getProperty(Dep
> > loymentConfiguration.KEY_USER_CACHE_DIR));
> > + File defaultCacheDir = new
> > File(JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_U
> > SER_CACHE_DIR));
> > + CacheUtil.setCacheDir(defaultCacheDir);
>
> I am bit confused here. CacheUtil.setCacheDir(File) does not seem to exist.
I am confused too since it exists for me in 'itw-cache-string-to-file-1.patch'.
@@ -622,11 +622,11 @@
}
}
- protected static void setCacheDir(String cacheDir) {
+ protected static void setCacheDir(File cacheDir) {
lruHandler.setCacheDir(cacheDir);
}
Attached is an updated patch that replaces all the uses of File.toString() with File.getPath(). Good catch with that one as it's more explicit now.
How does it look?
Regards,
>
> Regards
>
> Jacob
>
--
Jie Kang
OpenJDK Team - Software Engineering Intern
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-cache-string-to-file-2.patch
Type: text/x-patch
Size: 8009 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150119/b19877f9/itw-cache-string-to-file-2-0001.patch>
More information about the distro-pkg-dev
mailing list