Breaking FilePermission changes in JDK 9 b140+ and performance impact

[ywelsch]

A new change introduced in JDK 9 b140 removes pathname canonicalization when FilePermission objects are constructed. From the official announcement:

We do this mainly for performance enhancement so that there is no need to consult the file system every time a FilePermisson is created.

This means FilePermissions on the following pathnames will be unrelated:

1. On "./file" and "/path/to/current/directory/file".
2. On symlink and its target.
3. On "C:\Program Files" and "C:\PROGRA~1" on Windows.

and any other name change that a canonicalization can make.

This possibly breaks file permissions for Elasticsearch. A concrete example is the default temp directory java.io.tmpdir on my Mac under /var/... which is symlinked to /private/var/..., which breaks running the tests from the IDE. Running them with Gradle also breaks as java.io.tmpdir is specified as ./temp by the test task.

The JDK 9 change implements a compatibility layer which covers permissions defined in policy files:

That said, this changeset also adds a compatibility layer at the permission check level to translate between an absolute pathname and a relative one. So, even if FilePermission on a relative path does not imply one on an absolute path pointing to the same file, when you grant a permission in a relative pathname, you are still allowed to read the file with an absolute pathname.

This compatibility layer covers these cases:

1. When a permission is granted in a policy file.

2. When a permission is automatically granted because it's on a path where the class files are stored.

3. When a permission is requested in a doPrivileged-with-permissions call

We do hope that whenever you want to access a file, the permission you granted in a policy would be better to match the pathname you use to access the file. For example, if you plan to call 'new FileInputStream("/home/me/x")', please also grant FilePermission on "/home/me/x" instead of just "x".

Please note the compatibility layer does NOT cover:

1. A user-defined security manager or policy implementation, because we cannot control it. (Not always, but you need to test).

2. The translation between a symlink and its target, because it needs to read the file system.

3. The translation between a Windows long file name and its DOS-8.3 shorted name, because it needs to read the file system.

Also, "/-" does not imply "./file" even if it's a Unix. Please use "<>" instead.

Elasticsearch does not only rely on policy files though (e.g. adding permissions for temp directory is done programmatically).

Alternatively the JDK 9 also introduces a new system property that brings back the old behavior:

Finally, if you cannot live with this new behavior and like the pre-jdk9 style, you can always set the system property jdk.io.permissionsUseCanonicalPath back to true.

I've tried that one and everything runs again without any modifications. I wonder if we should treat this as a temporary fix for now. As a more permanent fix I've tried patching the ES security code so that it resolves symlinks / resolves relative to absolute paths before add file permissions by replacing the last two lines in org.elasticsearch.bootstrap.security.Security#addPath(...):

            policy.add(new FilePermission(path.toString(), permissions));
            policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", permissions));

with

        Path realPath;
        try {
            realPath = path.normalize().toRealPath();
        } catch (IOException e) {
            throw new IllegalStateException("Unable to convert to real path '" + configurationName + "' (" + path + ")", e);
        }
        if (path.toString().equals(realPath.toString()) == false) {
            System.out.println("NEED REALPATH " + path + " real: " + realPath);
            policy.add(new FilePermission(realPath.toString(), permissions));
            policy.add(new FilePermission(realPath.toString() + path.getFileSystem().getSeparator() + "-", permissions));
        } else {
            policy.add(new FilePermission(path.toString(), permissions));
            policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", permissions));
        }

I observed however a big performance hit with this compared to JDK 8 (which goes as far as to make some tests fail due to timeouts that are too low):

Test that I've run (with fixed seed):

• org.elasticsearch.cluster.allocation.ClusterRerouteIT#testDelayWithALargeAmountOfShards
• JDK8 b111 with and without the patch above: ~7 seconds
• JDK9 b140/b144 with and without the patch above and with -Djdk.io.permissionsUseCanonicalPath=true (simulating JDK8 behavior): ~7 seconds
• JDK9 b140/b144 with the patch above and the new behavior -Djdk.io.permissionsUseCanonicalPath=false which corresponds to not setting the system property at all: ~32 seconds

I've looked at the runs using a profiler but couldn't notice anything beside the obvious that extra time is spent in java.security.Permissions#implies when using the new JDK9 behavior. Thoughts?

[rjernst]

I wonder if it is worthwhile to try doing a simple comparison first, and only if that fails normalize the requested path?

[ywelsch]

We have agreed on Fixit Friday to revisit this issue once JDK 9 goes GA / when we release ES v6.0.0. Leaving it open and marking it as blocker for v6.0.0.

[uschindler]

Hi,
there were some changes in build 158 because of bug reports by Lucene (so they added some symlink-handling special cases - e.g. the proposal was that if a file permission was created using a symlink, 2 permissions will be added to the policy - one for the symlink and one for the real path. This is still work in progress.

Here is the actual state, sent yesterday:

-----Original Message-----
From: Weijun Wang [mailto:[email protected]]
Sent: Friday, February 24, 2017 9:03 AM
To: [email protected]
Cc: Uwe Schindler [email protected]; Rory O'Donnell
[email protected]
Subject: More change in jdk-9+158 (was: FilePermission change in jdk-9+140)

Hi Again

The following changeset is included in jdk-9+158:

http://hg.openjdk.java.net/jdk9/dev/jdk/rev/8b0d55e02f54

As said in the parent mail below:

Please note the compatibility layer does NOT cover:

1. A user-defined security manager or policy implementation, because
   we cannot control it. (Not always, but you need to test).

In fact, we did try to cover 3rd-party Policy implementations last time,
but it turns out not working perfectly, and we've disabled it in
JDK-8168410.

If you have a user-defined Policy implementation that grants
FilePermission on ${user.dir}/-, reading a file in the current directory
using its base name will fail.

Still the same solution: Ensure that the path used in permission
granting has the same style as the one how you access the file.

Setting -Djdk.security.filePermCompat=true will take you back to the
jdk-9+140 behavior. Setting -Djdk.io.permissionsUseCanonicalPath=true
will take you back to the jdk8 behavior.

Any feedback is welcome.

Thanks
Max

On 10/14/2016 10:03 PM, Wang Weijun wrote:

Hi All

If you use a security manager and file permissions then read on.

The following changeset is included in jdk-9+140:

8164705: Remove pathname canonicalization from FilePermission
http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/4251b451be17

This code change removes pathname canonicalization from FilePermission
creation, thus calculations of the equals() and implies() methods will be
based on the raw path string one provides in "new FilePermission(path,
action)".

The new behavior is described as @implNote at various places in

http://download.java.net/java/jdk9/docs/api/java/io/FilePermission.html

We do this mainly for performance enhancement so that there is no need
to consult the file system every time a FilePermisson is created.

This means FilePermissions on the following pathnames will be unrelated:

1. On "./file" and "/path/to/current/directory/file".

2. On symlink and its target.

3. On "C:\Program Files" and "C:\PROGRA~1" on Windows.

and any other name change that a canonicalization can make.

Please note that the new FilePermission implementation is based on NIO
Path, and Path happens to understand case-sensitiveness and various other
things, so there is no need to worry about "C:\PATH" and "c:\path" on
Windows or abundant ./down/.. on all platforms.

That said, this changeset also adds a compatibility layer at the permission
check level to translate between an absolute pathname and a relative one.
So, even if FilePermission on a relative path does not imply one on an
absolute path pointing to the same file, when you grant a permission in a
relative pathname, you are still allowed to read the file with an absolute
pathname.

This compatibility layer covers these cases:

1. When a permission is granted in a policy file.

2. When a permission is automatically granted because it's on a path where
   the class files are stored.

3. When a permission is requested in a doPrivileged-with-permissions call

We do hope that whenever you want to access a file, the permission you
granted in a policy would be better to match the pathname you use to access
the file. For example, if you plan to call 'new
FileInputStream("/home/me/x")', please also grant FilePermission on
"/home/me/x" instead of just "x".

Please note the compatibility layer does NOT cover:

1. A user-defined security manager or policy implementation, because we
   cannot control it. (Not always, but you need to test).

2. The translation between a symlink and its target, because it needs to
   read the file system.

3. The translation between a Windows long file name and its DOS-8.3
   shorted name, because it needs to read the file system.

Also, "/-" does not imply "./file" even if it's a Unix. Please use "<>" instead.

Finally, if you cannot live with this new behavior and like the pre-jdk9 style,
you can always set the system property jdk.io.permissionsUseCanonicalPath
back to true.

Thanks
Max

[wangweij]

I'm the author of the new FilePermission behavior.

In our benchmarking, the new behavior is much faster than the old one if every FilePermission on the right hand side of implies() is created from a brand new path, and much slower if it is not always new (Precisely, if the number of all different paths is <= 200).

This is because although the File.getCanonicalPath() call (which is used by the old behavior) is quite slow, it has an internal cache that can hold 200 results. If you keep calling the method on these 200 paths, it will be super fast.

Is your test also using a limited number of paths?

[rjernst]

@wangweij Each test uses it's own randomly generated path(s) (unique to that test), and we run many tests within the same JVM (close to 1000 unit tests classes). However, the paths which we add permissions for (when the test jvm starts up) is indeed limited to a few paths in the security manager policy. All of the relevant paths to the performance difference we see would be using this one root file permission: https://github.com/elastic/elasticsearch/blob/master/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java#L116. But the paths compared against that would be mostly unique (subpaths).

[wangweij]

@rjernst So you granted FilePermission on /tmp/- and call checkRead on /tmp/random-sub-path? Are the 1000 tests running in parallel? It will be nice if you can create a standalone test or please tell me how I can run your test. Sorry I am not familiar with ElasticSearch.

[rjernst]

@wangweij You can run a single test as follows (just using the same one that @ywelsch used to benchmark this in the original issue description). You will need gradle 3.3.

1. In ~/.gradle/gradle.properties set org.gradle.java.home=path/to/java/8. This is needed because gradle does not yet work on java 9 correctly.
2. Set JAVA_HOME to your jdk9 path.
3. Set GRADLE_OPTS to --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
4. Run the following gradle command from the root of an elasticsearch checkout:

gradle :core:integTest -Dtests.class=org.elasticsearch.cluster.allocation.ClusterRerouteIT -Dtests.method=testDelayWithALargeAmountOfShards -Dtests.seed=4CCF7644FDF3724D

Note that we have the canonical path option turned on. To run without that option, and with the originally suggested change here (adding explicit permissions for canonical paths), I created a branch. You can check it out from my fork, and you can see the diff.

On my laptop and jdk9 b159, that test runs in about 15 seconds on master (which has -Djdk.io.permissionsUseCanonicalPath=true), and almost 60 seconds with the branch I linked there.

[wangweij]

@rjernst I can reproduce the problem here. It's 50s vs 92s on my laptop (jdk9+161). I will try to look into it. I see your branch added 2 extra permissions. When a file is accessed, are you usually using the original path or the real path?

[rjernst]

@wangweij

When a file is accessed, are you usually using the original path or the real path?

I believe we would be using the original path. For example, with java.io.tmpdir set to ./temp (which we use in tests, each test runner creates a cwd for the jvm that is unique), we would be using paths that start with ./temp, not the equivalent $PWD/temp.

[wangweij]

Dumped all FilePermission checks of a test run into a log. The total number is 882642 with 29275 uniq ones. I'll write a test that simulate these 882642 checks tomorrow and see how it performs.

[wangweij]

The simulation on these 882642 checks shows the same behavior, i.e. jdk9 spends about 100% more time than jdk8. Of all the checks, 881303 are in the same directory ("core/build/testrun/integTest/J0/temp"). For jdk8 with a canonicalized path cache of size 200, 830696 checks (94.26%) are hits and 50607 (5.74%) are misses. The sub-directory names inside "tempDir-002/d?/nodes/?/indices" look like randomly generated strings (For example, "cdJ1TZYqRFCMpEn44235hg"), but there are only 25 of them.

So the performance degradation can be explained now, which is because in jdk8 most canonicalized paths are cached so the most time consuming part is not a problem. I will think about if it is possible to make some similar optimization in jdk9. Thanks for reporting this issue about FilePermission checks in the real world.

[ywelsch]

@wangweij thank you for investigating the issue and looking into possible solutions. This is much appreciated.

[wangweij]

https://bugs.openjdk.java.net/browse/JDK-8177969 filed including a link to the suggested fix. After code review, it will be pushed into jdk9.

[rjernst]

This is great news @wangweij. Thank you for your time investigating and fixing!

[wangweij]

Fixed now in jdk9+166. Please take a try.

[ywelsch]

Initial testing shows very promising results (performance on-par with "Java 8" mode). Thank you for this fix @wangweij!

[rjernst]

@ywelsch Do you want to create a PR with your original change suggested in this issue and remove the canonicalpath override flag from jvm.options? I still see this as a blocker for 6.0, so that we can work with jdk9 on release.

[ywelsch]

Closed by #26302