[rfc][icedtea-web] Use File for cache directory instead of String
Jacob Wisor
gitne at gmx.de
Fri Jan 16 14:29:55 UTC 2015
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.
> @@ -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.
> @@ -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.
> […]
> 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.
Regards
Jacob
More information about the distro-pkg-dev
mailing list